CMT: [RESEND][PATCH 1/1][SRU][F] UBUNTU: SAUCE: iio: dac: ad5593r: Dynamically set AD5593R channel modes
AceLan Kao
acelan.kao at canonical.com
Tue Mar 16 11:57:31 UTC 2021
Ah, yes, I'll submit v2 later.
This is a ODM patch, so it doesn't need to be included in other series,
I'll mark this patch as UBUNTU: ODM: and do the required modifications
at next revision.
Kelsey Skunberg <kelsey.skunberg at canonical.com> 於 2021年3月11日 週四 上午5:55寫道：
>
> Hi AceLan,
>
> Noticed this patch is still sitting on our "needs more information" list.
> Any updates or follow ups you can provide to Marcelo's response?
>
> Thank you!
>
> -Kelsey
>
> On 2021-01-22 14:13:05 , Marcelo Henrique Cerri wrote:
> > Not a comprehensive review, but:
> >
> > 1. If we are adding that to Focal, it makes sense to keep newer series
> > (G and H) consistent too.
> >
> > 2. I would run the patch through some static analysis tool (such as
> > smatch[1]), besides running a regular test, just to be safe.
> >
> > 3. I have a few inline questions bellow.
> >
> > [1] http://smatch.sourceforge.net/
> >
> > On Wed, Jan 13, 2021 at 11:33:18AM +0800, AceLan Kao wrote:
> > > From: William Sung <william.sung at advantech.com.tw>
> > >
> > > BugLink: https://bugs.launchpad.net/bugs/1895612
> > >
> > > 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 at advantech.com.tw>
> > > Signed-off-by: AceLan Kao <acelan.kao at canonical.com>
> > > Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> > > ---
> > > drivers/iio/dac/ad5592r-base.c | 21 +++++++++++++---
> > > drivers/iio/dac/ad5592r-base.h | 4 ++++
> > > drivers/iio/dac/ad5593r.c | 44 ++++++++++++++++++++++++++++++++++
> > > 3 files changed, 66 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> > > index 424922cad1e3..25ff9f5df5dd 100644
> > > --- a/drivers/iio/dac/ad5592r-base.c
> > > +++ b/drivers/iio/dac/ad5592r-base.c
> > > @@ -22,6 +22,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);
> > > @@ -133,7 +137,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;
> > > @@ -535,6 +539,10 @@ static int ad5592r_alloc_channels(struct ad5592r_state *st)
> > > st->channel_offstate[reg] = tmp;
> > > }
> > >
> > > + /* Update default channel modes set by external module */
> > > + if (update_channel_mode == 1)
> > > + memcpy(st->channel_modes, new_channel_modes, ARRAY_SIZE(st->channel_modes));
> > > +
> > > channels = devm_kcalloc(st->dev,
> > > 1 + 2 * num_channels, sizeof(*channels),
> > > GFP_KERNEL);
> > > @@ -570,7 +578,7 @@ static int ad5592r_alloc_channels(struct ad5592r_state *st)
> > > }
> > >
> > > 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);
> > > @@ -592,6 +600,13 @@ 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)
> >
> > const u8 * new_modes
> >
> > > +{
> > > + update_channel_mode = 1;
> > > + memcpy(new_channel_modes, new_modes, AD559XR_CHANNEL_NR);
> > > +}
> > > +EXPORT_SYMBOL_GPL(ad5592r_update_default_channel_modes);
> >
> > Is it really necessary to export this function?
> >
> > > +
> > > int ad5592r_probe(struct device *dev, const char *name,
> > > const struct ad5592r_rw_ops *ops)
> > > {
> > > @@ -606,7 +621,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 4774e4cd9c11..6fc120dc341e 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);
> > > @@ -68,6 +71,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 44ea3b8117d0..7511949278ac 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,49 @@ 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, tmp[2];
> >
> > Just nit-picking but I don't see any reason for these two variables
> > (new_mode or tmp). Why is tmp an array?
> >
> > > + u8 new_ch_modes[AD559XR_CHANNEL_NR];
> > > + int idx;
> > > +
> > > + if (strlen(ch_mode) != AD559XR_CHANNEL_NR)
> > > + return;
> >
> > Are you sure you want to silently ignore the parameter if its invalid?
> > The same question is valid for the following returns below.
> >
> > > +
> > > + 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:
> > > + /* Invalid setting exist, ignore the settings */
> > > + 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])) {
> > > + /* Converting error, ignore the settings */
> > > + 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);
> > > }
> > >
> > > --
> > > 2.25.1
> > >
> > >
> > > --
> > > kernel-team mailing list
> > > kernel-team at lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
> >
> > --
> > Regards,
> > Marcelo
> >
>
>
>
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
More information about the kernel-team
mailing list