On Tue, Aug 25, 2020 at 1:19 PM William Sung
<william.sung@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> ---
...
> +/* 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
