diff mbox series

[RESEND,1/1,SRU,F] UBUNTU: SAUCE: iio: dac: ad5593r: Dynamically set AD5593R channel modes

Message ID 20210113033318.101357-2-acelan.kao@canonical.com
State New
Headers show
Series Dynamically set AD5593R channel modes | expand

Commit Message

AceLan Kao Jan. 13, 2021, 3:33 a.m. UTC 
From: William Sung <william.sung@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@advantech.com.tw>
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
Acked-by: Kleber Sacilotto de Souza <kleber.souza@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(-)

Comments

Marcelo Henrique Cerri Jan. 22, 2021, 5:13 p.m. UTC | #1 
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@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@advantech.com.tw>
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> Acked-by: Kleber Sacilotto de Souza <kleber.souza@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@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kelsey Skunberg March 10, 2021, 9:55 p.m. UTC | #2 
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@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@advantech.com.tw>
> > Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> > Acked-by: Kleber Sacilotto de Souza <kleber.souza@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@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
> 
> -- 
> Regards,
> Marcelo
> 



> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
AceLan Kao March 16, 2021, 11:57 a.m. UTC | #3 
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@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@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@advantech.com.tw>
> > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> > > Acked-by: Kleber Sacilotto de Souza <kleber.souza@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@lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
> >
> > --
> > Regards,
> > Marcelo
> >
>
>
>
> > --
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
diff mbox series

Patch

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)
+{
+	update_channel_mode = 1;
+	memcpy(new_channel_modes, new_modes, AD559XR_CHANNEL_NR);
+}
+EXPORT_SYMBOL_GPL(ad5592r_update_default_channel_modes);
+
 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];
+	u8 new_ch_modes[AD559XR_CHANNEL_NR];
+	int idx;
+
+	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:
+			/* 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);
 }