Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 26 Aug 2020 19:03:46 +0300 | Subject | Re: [PATCH v2] iio: dac: ad5593r: Dynamically set AD5593R channel modes |
| |
On Tue, Aug 25, 2020 at 1:19 PM William Sung <william.sung@advantech.com.tw> wrote: > > To use ad5593r more flexibly, we use the module parameter to setting the > channel modes dynamically whenever the module probe up. Users can pass > the channel modes to the module parameter for allocating the > functionality of channels as desired. > > For example: > * Use in the kernel command line: > Users can add the module parameter in the kernel command line such as > > "ad5593r.ch_mode=88001122" > > "88001122" means the channel mode setting for each channel. The most > left side indicates the mode of channel 7, and the most right side > indicates the mode of channel 0. > > * Use when manually probe the module: > Similar to the kernel command line usage, users can enter > > "modprobe ad5593r ch_mode=88001122" > > to start the ad5593r module with the desired channel mode setting.
> v2: Fix the patch description and remove redundant for loop
This should go after the cutter '---' line below.
> Signed-off-by: William Sung <william.sung@advantech.com.tw> > ---
...
> +/* Parameters for dynamic channel mode setting */ > +static u8 update_channel_mode; > +static u8 new_channel_modes[AD559XR_CHANNEL_NR];
Huh?! Global variables?!
...
> +static void ad5592r_set_def_channel_modes(struct ad5592r_state *st) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(st->channel_modes); i++) > + st->channel_modes[i] = new_channel_modes[i]; > +}
NIH of memcpy() ?
...
> +void ad5592r_update_default_channel_modes(u8 *new_modes) > +{ > + int idx = 0; > + > + update_channel_mode = 1; > + for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) > + new_channel_modes[idx] = new_modes[idx];
> +
Redundant blank line.
> +}
Ditto (see memcpy() above).
> +EXPORT_SYMBOL_GPL(ad5592r_update_default_channel_modes);
What?!
...
> +/* Parameters for dynamic channel mode setting */ > +static char *ch_mode = ""; > +module_param(ch_mode, charp, 0400);
We have sysfs ABI, what's wrong with it?
...
> +static void ad5593r_check_new_channel_mode(void) > +{ > + char *new_mode = NULL, tmp[2]; > + u8 new_ch_modes[AD559XR_CHANNEL_NR];
> + int idx = 0;
Redundant assignment. Also for new_mode.
> +
> + if (strlen(ch_mode) != AD559XR_CHANNEL_NR)
This is interesting...
> + return; > + > + new_mode = ch_mode; > + > + /* Check if all channel modes are valid */ > + for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) { > + switch (new_mode[idx]) { > + case '0': > + case '1': > + case '2': > + case '3': > + case '8': > + continue; > + default: > + /* There is invalid mode exist, ignore the settings */
> + pr_err("%s: invalid(%c) in index(%d)\n", > + __func__, new_mode[idx], idx);
Oh...
> + return; > + } > + } > + > + /* Set the new modes to ad5592r-base driver to setup the new channel modes */ > + memset(tmp, 0, 2); > + for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) { > + tmp[0] = new_mode[idx];
> + if (kstrtou8(tmp, 10, &new_ch_modes[AD559XR_CHANNEL_NR - idx - 1])) {
Shadowing errors?
> + /* Something error when converting the string to integer */ > + /* Ignore this settings */
Wrong style of comment.
> + pr_err("%s: kstr error idx(%d)\n", __func__, idx); > + return; > + } > + } > + > + ad5592r_update_default_channel_modes(new_ch_modes); > +}
-- With Best Regards, Andy Shevchenko
| |