| Message ID | 20200825101614.2462-1-william.sung@advantech.com.tw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: dac: ad5593r: Dynamically set AD5593R channel modes | expand |
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); > +}
On Thu, 27 Aug 2020 05:33:08 +0000 William.Sung <William.Sung@advantech.com.tw> wrote: > Hi Andy, > > Thanks for you to take your time reviewing this patch. Would you please let me explain your questions? > > ========================================================================= > > +/* Parameters for dynamic channel mode setting */ static u8 > > +update_channel_mode; static u8 new_channel_modes[AD559XR_CHANNEL_NR]; > > Huh?! Global variables?! > > --- > > > +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? > > --- > => William: > > The module ad5593r is dependent on the module ad5592r-base, there is also another module ad5592r dependent on it. > In the original progress of AD5593R probe up: > 1. ad5593r_probe(), and then it will call the expose function ad5592r_probe() in ad5592r-base. > 2. During ad5592r_probe() function, it will: > * Create ad5593r/ad5592r state structure (including the channel mode settings buffer) > * Read all 8 channel mode settings from acpi/dt and write to the state structure > * According to the channel mode settings, write the appropriate value to the registers > > Would you please think about a scenario: > The channel modes descript in the acpi/dt are 4 GPIOs and 4 ADCs > If a user wants to change the usage of the channel to 2GPIOs, 4 ADCs, and 2 DACs, no interface can do this. > The only way that he can do it is by modifying the settings in either acpi or dt. Sorry, but no to doing this. If a user actually wants to do this then they need to use something like a device tree overlay. The only reason to make such a change is because the external hardware connected to those pins has changed. The person making that hardware change should be describing the hardware that is sitting on those DAC and GPIO lines. It may seem overly restrictive and I appreciate that quick routes are handy for makers etc, but there are standard ways of supporting such hardware configurability. If those do not meet your requirements it is those standard ways that should be improved, not adding a custom hack for a specific driver. Jonathan > > To increase the flexibility, we use module parameters for the user specifying the desired setting without modifying the acpi/dt. > However, during ad5593r_probe() function, the state structure in ad5592r-base has not been created. > Since the other module ad5592r is also dependent on ad5592r-base, we try to keep the compatibility for both ad5592r/ad5593r as > possible. So we export a function ad5592r_update_default_channel_modes to catch the settings and store in the global variable > as the buffer. And the it can keep the original process flow until the module intent to allocate the channel modes. > > If the user manually probe the module without specifying the parameters the module will keep the original flow: Read from acpi/dt as > the channel mode setting. > > ============================================================================== > > + if (strlen(ch_mode) != AD559XR_CHANNEL_NR) > > This is interesting... > > --- > > => William: > My thought is to prevent the typing error. If the length of the input parameter is not matching the number of channels, we can ignore it. > Is that a weird way to do it? > > ============================================================================== > > + pr_err("%s: invalid(%c) in index(%d)\n", > > + __func__, new_mode[idx], idx); > > Oh... > > --- > > => William > > Maybe it is more appropriate not to show this message, doesn't it? > > ============================================================================== > > + if (kstrtou8(tmp, 10, &new_ch_modes[AD559XR_CHANNEL_NR > > + - idx - 1])) { > > Shadowing errors? > > --- > > => William > > In the prototype of kstrtou8, it will return int to indicate this function is successful or not. > I just want to make sure all the translations from string to integer are correct. > > ============================================================================== > > For the others, I'll fix these. > > Thanks again and please kindly give me your advice if any. > > Best regards, > > William Sung > Phone: +886-2-2792-7818 ext: 7644 > Advantech Co., Ltd. > o
diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c index cc4875660a69..cd69a34fa21e 100644 --- a/drivers/iio/dac/ad5592r-base.c +++ b/drivers/iio/dac/ad5592r-base.c @@ -21,6 +21,10 @@ #include "ad5592r-base.h" +/* Parameters for dynamic channel mode setting */ +static u8 update_channel_mode; +static u8 new_channel_modes[AD559XR_CHANNEL_NR]; + static int ad5592r_gpio_get(struct gpio_chip *chip, unsigned offset) { struct ad5592r_state *st = gpiochip_get_data(chip); @@ -132,7 +136,7 @@ static int ad5592r_gpio_init(struct ad5592r_state *st) st->gpiochip.label = dev_name(st->dev); st->gpiochip.base = -1; - st->gpiochip.ngpio = 8; + st->gpiochip.ngpio = AD559XR_CHANNEL_NR; st->gpiochip.parent = st->dev; st->gpiochip.can_sleep = true; st->gpiochip.direction_input = ad5592r_gpio_direction_input; @@ -287,6 +291,14 @@ static int ad5592r_set_channel_modes(struct ad5592r_state *st) return ret; } +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]; +} + static int ad5592r_reset_channel_modes(struct ad5592r_state *st) { int i; @@ -532,6 +544,10 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev) st->channel_offstate[reg] = tmp; } + /* Update default channel modes set by external module */ + if (update_channel_mode == 1) + ad5592r_set_def_channel_modes(st); + channels = devm_kcalloc(st->dev, 1 + 2 * num_channels, sizeof(*channels), GFP_KERNEL); @@ -567,7 +583,7 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev) } channels[curr_channel].type = IIO_TEMP; - channels[curr_channel].channel = 8; + channels[curr_channel].channel = AD559XR_CHANNEL_NR; channels[curr_channel].info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET); @@ -589,6 +605,17 @@ static void ad5592r_init_scales(struct ad5592r_state *st, int vref_mV) div_s64_rem(tmp * 2, 1000000000LL, &st->scale_avail[1][1]); } +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]; + +} +EXPORT_SYMBOL_GPL(ad5592r_update_default_channel_modes); + int ad5592r_probe(struct device *dev, const char *name, const struct ad5592r_rw_ops *ops) { @@ -603,7 +630,7 @@ int ad5592r_probe(struct device *dev, const char *name, st = iio_priv(iio_dev); st->dev = dev; st->ops = ops; - st->num_channels = 8; + st->num_channels = AD559XR_CHANNEL_NR; dev_set_drvdata(dev, iio_dev); st->reg = devm_regulator_get_optional(dev, "vref"); diff --git a/drivers/iio/dac/ad5592r-base.h b/drivers/iio/dac/ad5592r-base.h index 23dac2f1ff8a..40ad6369e660 100644 --- a/drivers/iio/dac/ad5592r-base.h +++ b/drivers/iio/dac/ad5592r-base.h @@ -39,6 +39,9 @@ enum ad5592r_registers { #define AD5592R_REG_CTRL_ADC_RANGE BIT(5) #define AD5592R_REG_CTRL_DAC_RANGE BIT(4) +/* Define quantity of channels of AD5592R/AD5593R */ +#define AD559XR_CHANNEL_NR 8 + struct ad5592r_rw_ops { int (*write_dac)(struct ad5592r_state *st, unsigned chan, u16 value); int (*read_adc)(struct ad5592r_state *st, unsigned chan, u16 *value); @@ -69,6 +72,7 @@ struct ad5592r_state { __be16 spi_msg_nop; }; +void ad5592r_update_default_channel_modes(u8 *new_modes); int ad5592r_probe(struct device *dev, const char *name, const struct ad5592r_rw_ops *ops); int ad5592r_remove(struct device *dev); diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c index 1fbe9c019c7f..a19331d91406 100644 --- a/drivers/iio/dac/ad5593r.c +++ b/drivers/iio/dac/ad5593r.c @@ -21,6 +21,10 @@ #define AD5593R_MODE_GPIO_READBACK (6 << 4) #define AD5593R_MODE_REG_READBACK (7 << 4) +/* Parameters for dynamic channel mode setting */ +static char *ch_mode = ""; +module_param(ch_mode, charp, 0400); + static int ad5593r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value) { struct i2c_client *i2c = to_i2c_client(st->dev); @@ -92,9 +96,53 @@ static const struct ad5592r_rw_ops ad5593r_rw_ops = { .gpio_read = ad5593r_gpio_read, }; +static void ad5593r_check_new_channel_mode(void) +{ + char *new_mode = NULL, tmp[2]; + u8 new_ch_modes[AD559XR_CHANNEL_NR]; + int idx = 0; + + if (strlen(ch_mode) != AD559XR_CHANNEL_NR) + 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); + 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])) { + /* Something error when converting the string to integer */ + /* Ignore this settings */ + pr_err("%s: kstr error idx(%d)\n", __func__, idx); + return; + } + } + + ad5592r_update_default_channel_modes(new_ch_modes); +} + static int ad5593r_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { + ad5593r_check_new_channel_mode(); return ad5592r_probe(&i2c->dev, id->name, &ad5593r_rw_ops); }
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 Signed-off-by: William Sung <william.sung@advantech.com.tw> --- drivers/iio/dac/ad5592r-base.c | 33 ++++++++++++++++++++--- drivers/iio/dac/ad5592r-base.h | 4 +++ drivers/iio/dac/ad5593r.c | 48 ++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 3 deletions(-)