[PATCH 0/1][SRU][F] Dynamically set AD5593R channel modes

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/1][SRU][F] Dynamically set AD5593R channel modes

AceLan Kao
BugLink: https://bugs.launchpad.net/bugs/1895612

[Impact]
On Advantech UNO-420 development platform, we need a generic way to set
the pin mode.
Upstream would like us to leverage ACPI override method to change the
modes, but in Ubuntu core, we can't generate a new initramfs and boot up
with the generated initramfs.

[Fix]
Add a module parameter to assign pin modes while loading ad5593r driver.

[Test]
Verified on Advantech UNO-420 platform.

[Regression Potential]
Low, it checks the length of the passed parameter, and checks every bytes
in it to make sure it's a valid argument.

William Sung (1):
  iio: dac: ad5593r: Dynamically set AD5593R channel modes

 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(-)

--
2.25.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

AceLan Kao
From: William Sung <[hidden email]>

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 <[hidden email]>
Signed-off-by: AceLan Kao <[hidden email]>
---
 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)
+{
+ 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);
 }
 
--
2.25.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Kleber Souza
On 15.09.20 07:08, AceLan Kao wrote:

> From: William Sung <[hidden email]>
>
> 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 <[hidden email]>
> Signed-off-by: AceLan Kao <[hidden email]>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

> ---
>  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)
> +{
> + 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);
>  }
>  
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Stefan Bader-2
In reply to this post by AceLan Kao
On 15.09.20 07:08, AceLan Kao wrote:

> From: William Sung <[hidden email]>
>
> 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 <[hidden email]>
> Signed-off-by: AceLan Kao <[hidden email]>
> ---
This will introduce a new interface which will not exist anywhere else than just
the one kernel/series this was submitted for. IMO this does not belong into a
generic distro kernel.

I tend to say no to this unless there is some really good reason for it.
Otherwise this needs some approach that works for us and upstream...

-Stefan


>  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)
> +{
> + 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);
>  }
>  
>




--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

AceLan Kao
Hi Stefan,

1. For our current project and contract, we only need this in focal kernel.
The introduced feature should be used by the project only. And for the
following requirements after focal,
the pin definitions should be defined in DSDT table.
We need to config the pin modes during development cycle, and the pin
modes are not confirmed yet currently.

2. We have a lengthy discussion with upstream[1], and the maintainer think
we should use ACPI override method to modify the values.
But this method requires to create a new initramfs, and this is no
allowed in our ubuntu core system.

1. https://patchwork.kernel.org/patch/11732183/

Stefan Bader <[hidden email]> 於 2020年9月17日 週四 下午4:22寫道:

>
> On 15.09.20 07:08, AceLan Kao wrote:
> > From: William Sung <[hidden email]>
> >
> > 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 <[hidden email]>
> > Signed-off-by: AceLan Kao <[hidden email]>
> > ---
>
> This will introduce a new interface which will not exist anywhere else than just
> the one kernel/series this was submitted for. IMO this does not belong into a
> generic distro kernel.
>
> I tend to say no to this unless there is some really good reason for it.
> Otherwise this needs some approach that works for us and upstream...
>
> -Stefan
>
>
> >  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)
> > +{
> > +     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);
> >  }
> >
> >
>
>
>
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Brad Figg-2
In reply to this post by Stefan Bader-2
On Thu, Sep 17, 2020 at 10:22:31AM +0200, Stefan Bader wrote:

> On 15.09.20 07:08, AceLan Kao wrote:
> > From: William Sung <[hidden email]>
> >
> > 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 <[hidden email]>
> > Signed-off-by: AceLan Kao <[hidden email]>
> > ---
>
> This will introduce a new interface which will not exist anywhere else than just
> the one kernel/series this was submitted for. IMO this does not belong into a
> generic distro kernel.
>
> I tend to say no to this unless there is some really good reason for it.
> Otherwise this needs some approach that works for us and upstream...
>
> -Stefan

Stefan,

1. Is there an issue with the quality of the patch?

2. Are you concerned that this has an unacceptable risk for regression?

3. Are you suggesting that a new, optimized, customer specific kernel be created
   for this driver?

Can you elaborate on why, in your opinion, this does not belong in a generic
distro kernel?

Brad

>
>
> >  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)
> > +{
> > + 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);
> >  }
> >  
> >
>
>
>
>




> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team


--
Brad Figg [hidden email] http://www.canonical.com

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/1][SRU][F] Dynamically set AD5593R channel modes

Stefan Bader-2
In reply to this post by AceLan Kao
On 15.09.20 07:08, AceLan Kao wrote:

> BugLink: https://bugs.launchpad.net/bugs/1895612
>
> [Impact]
> On Advantech UNO-420 development platform, we need a generic way to set
> the pin mode.
> Upstream would like us to leverage ACPI override method to change the
> modes, but in Ubuntu core, we can't generate a new initramfs and boot up
> with the generated initramfs.
>
> [Fix]
> Add a module parameter to assign pin modes while loading ad5593r driver.
>
> [Test]
> Verified on Advantech UNO-420 platform.
>
> [Regression Potential]
> Low, it checks the length of the passed parameter, and checks every bytes
> in it to make sure it's a valid argument.
>
> William Sung (1):
>   iio: dac: ad5593r: Dynamically set AD5593R channel modes
>
>  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(-)
>
I did discuss this with Andy today. Technically I guess this is ok if we are
sure that you cannot damage the HW or leverage the interface for attacks by
passing either random or certain sequences of numbers. The allowed values seem
to be checked, so the question only is what one could do with random
alternations of those.

The other thing we want to avoid is to have a module argument name either clash
with a possible upstream name or be taken as something that will be carried
onward into later series.

So question: could we prefix the paramenter with ubuntu, so it would become
ubuntu_ch_mode=... Then maybe we could add Documentation/admin-guide/
ubuntu-kernel-parameters.txt which explains the usage and also clarifies that
this is an experimental option which may or (rather) may not be present in newer
kernels.

-Stefan


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/1][SRU][F] Dynamically set AD5593R channel modes

AceLan Kao
Sorry, I missed this email.
Let me answer your questions below

Stefan Bader <[hidden email]> 於 2020年10月13日 週二 下午5:08寫道:

>
> On 15.09.20 07:08, AceLan Kao wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1895612
> >
> > [Impact]
> > On Advantech UNO-420 development platform, we need a generic way to set
> > the pin mode.
> > Upstream would like us to leverage ACPI override method to change the
> > modes, but in Ubuntu core, we can't generate a new initramfs and boot up
> > with the generated initramfs.
> >
> > [Fix]
> > Add a module parameter to assign pin modes while loading ad5593r driver.
> >
> > [Test]
> > Verified on Advantech UNO-420 platform.
> >
> > [Regression Potential]
> > Low, it checks the length of the passed parameter, and checks every bytes
> > in it to make sure it's a valid argument.
> >
> > William Sung (1):
> >   iio: dac: ad5593r: Dynamically set AD5593R channel modes
> >
> >  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(-)
> >
>
> I did discuss this with Andy today. Technically I guess this is ok if we are
> sure that you cannot damage the HW or leverage the interface for attacks by
> passing either random or certain sequences of numbers. The allowed values seem
> to be checked, so the question only is what one could do with random
> alternations of those.
The pin modes are for the devices connected to those pins, so it
should not break the system,
but maybe lead to the devices unable to work.

>
> The other thing we want to avoid is to have a module argument name either clash
> with a possible upstream name or be taken as something that will be carried
> onward into later series.
>
> So question: could we prefix the paramenter with ubuntu, so it would become
> ubuntu_ch_mode=... Then maybe we could add Documentation/admin-guide/
> ubuntu-kernel-parameters.txt which explains the usage and also clarifies that
> this is an experimental option which may or (rather) may not be present in newer
> kernels.
I can't tell if this is a good idea.
I agree we should have some notes somewhere to describe the patches' life cycle,
maybe written it on patch description directly, like ODM: 5.4, to
specify this patch is from ODM and stay in 5.4 only.
And for the parameter rename, I don't think it's necessary. Upstream
had rejected this patch,
so there should be no parameter name conflict in the future.
And the parameter is for a limited group of people who already know
how to use it,
and should know whom to complain if this parameter is gone. So, create
an a document for them is not so important.
>
> -Stefan
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team