run-time change of configuration of ad74412

run-time change of configuration of ad74412

[Rasmus Villemoes]

Hi,

My customer wants to be able to change the configuration of the four
channels of the ad74412 at run-time; their board can be used in various
scenarios, and having to specify the functions in device tree is too
inflexible.

Is there any precedent in other iio drivers for such a configuration
change, and/or is it feasible to implement this in the ad74413r.c driver?

I do not need to be able to change it continuously, just once after
userspace has come up and before anything actually starts making use of
the device, but it is not possible for me to know the correct
configuration in the bootloader, so having U-Boot patch the dtb is not
an option. A somewhat hacky way would be to build the driver as a
module, and allow module parameter(s) to overrule whatever is in
devicetree, but that doesn't really work if there was more than one
ad74412/ad74413 present, unless one invents and parses some weird syntax
to have certain settings apply to $this-chipselect on $that-bus.

Rasmus

Re: run-time change of configuration of ad74412

[Nuno Sá]

On Fri, 2022-12-02 at 12:16 +0100, Rasmus Villemoes wrote:
> Hi,
> 
> My customer wants to be able to change the configuration of the four
> channels of the ad74412 at run-time; their board can be used in
> various
> scenarios, and having to specify the functions in device tree is too
> inflexible.
> 
> Is there any precedent in other iio drivers for such a configuration
> change, and/or is it feasible to implement this in the ad74413r.c
> driver?
> 
> I do not need to be able to change it continuously, just once after
> userspace has come up and before anything actually starts making use
> of
> the device, but it is not possible for me to know the correct
> configuration in the bootloader, so having U-Boot patch the dtb is
> not
> an option. A somewhat hacky way would be to build the driver as a
> module, and allow module parameter(s) to overrule whatever is in
> devicetree, but that doesn't really work if there was more than one
> ad74412/ad74413 present, unless one invents and parses some weird
> syntax
> to have certain settings apply to $this-chipselect on $that-bus.
> 
> Rasmus

Hi Rasmus,

I did not looked too deep into this but from what you described it
sounds like a nasty hack that I'm not sure if it's feasable... Would
devicetree overlays be an option for you? Some userspace daemon could
decide which one to load depending on the usecase?

- Nuno Sá

[POC] iio: ad74413: allow channel configuration to be given via module parameters

[Rasmus Villemoes]

Just to see how it would look, I tried doing the below. Since a board
may have more than one ad74412/ad74413, one needs to be able to
differentiate between them. So for now each module parameter channelX
(X=0,1,2,3) accepts a space-separated list of <label>:<function>,
where label is matched against the label property in device tree, but
also allowing * to match any, which is more convenient when one knows
there is only one device.

Aside from the missing documentation (MODULE_PARM_DESC), there are of
course various details to hash out. E.g., should the function be
specified with a raw integer as here, or should we take a text string
"voltage-output", "current-input-ext-power" etc. and translate those?
Should we use space or comma or semicolon as separator? And so on.

I also considered whether instead of the label one should instead
match on the OF_FULLNAME,
e.g. /soc@0/bus@30800000/spi@30840000/ad74412r@0, but that's a lot
more complicated, and I assume that anybody that has more than one of
these chips would anyway assign a label so that they can distinguish
their /sys/bus/iio/... directories.

I should also note that it is not unprecedented for modules to take
parameters that do some sort of (ad hoc) parsing to apply settings
per-device. For example:

- ignore_wake in drivers/gpio/gpiolib-acpi.c
- mtdparts in drivers/mtd/parsers/cmdlinepart.c
- pci_devs_to_hide in drivers/xen/xen-pciback/pci_stub.c
- quirks in drivers/hid/usbhid/hid-core.c

So the question is, is there any chance that anything like this could
be accepted? If so, I'll of course spin this into a real patch with
proper MODULE_PARM_DESC and commit log etc.

This has been tested doing

  insmod ad74413r.ko 'channel0=*:1' 'channel1=*:3' 'channel2=*:2' 'channel3=*:4'

and seeing that the channels did indeed come up as expected, where the
device tree specified CH_FUNC_HIGH_IMPEDANCE for all of them.

---
 drivers/iio/addac/ad74413r.c | 37 ++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index ef873737c33a..284bdb358dec 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -1146,6 +1146,35 @@ static const struct ad74413r_channels ad74413r_channels_map[] = {
 	[CH_FUNC_CURRENT_INPUT_LOOP_POWER_HART] = AD74413R_CHANNELS(current_input),
 };
 
+static char *channel_function[AD74413R_CHANNEL_MAX];
+
+static int ad74413r_get_channel_function(struct ad74413r_state *st, u32 index,
+					 struct fwnode_handle *channel_node)
+{
+	const char *s, *c, *label;
+	u32 func, label_len;
+
+	if (device_property_read_string(st->dev, "label", &label))
+		label = "";
+
+	label_len = strlen(label);
+	for (s = channel_function[index]; s; s = strchr(s, ' ')) {
+		s = skip_spaces(s);
+		c = strchr(s, ':');
+		if (!c)
+			break;
+
+		if ((c - s == label_len && !memcmp(s, label, label_len)) ||
+		    (c - s == 1 && *s == '*'))
+			return simple_strtol(c + 1, NULL, 10);
+	}
+
+	func = CH_FUNC_HIGH_IMPEDANCE;
+	fwnode_property_read_u32(channel_node, "adi,ch-func", &func);
+
+	return func;
+}
+
 static int ad74413r_parse_channel_config(struct iio_dev *indio_dev,
 					 struct fwnode_handle *channel_node)
 {
@@ -1171,8 +1200,7 @@ static int ad74413r_parse_channel_config(struct iio_dev *indio_dev,
 		return -EINVAL;
 	}
 
-	config->func = CH_FUNC_HIGH_IMPEDANCE;
-	fwnode_property_read_u32(channel_node, "adi,ch-func", &config->func);
+	config->func = ad74413r_get_channel_function(st, index, channel_node);
 
 	if (config->func < CH_FUNC_MIN || config->func > CH_FUNC_MAX) {
 		dev_err(st->dev, "Invalid channel function %u\n", config->func);
@@ -1494,6 +1522,11 @@ static struct spi_driver ad74413r_driver = {
 	.id_table = ad74413r_spi_id,
 };
 
+module_param_named(channel0, channel_function[0], charp, 0444);
+module_param_named(channel1, channel_function[1], charp, 0444);
+module_param_named(channel2, channel_function[2], charp, 0444);
+module_param_named(channel3, channel_function[3], charp, 0444);
+
 module_driver(ad74413r_driver,
 	      ad74413r_register_driver,
 	      ad74413r_unregister_driver);
-- 
2.37.2

RE: [POC] iio: ad74413: allow channel configuration to be given via module parameters

[Tanislav, Cosmin]

> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Sent: Thursday, December 8, 2022 3:34 PM
> To: Nuno Sá <noname.nuno@gmail.com>; linux-iio@vger.kernel.org
> Cc: Jonathan Cameron <jic23@kernel.org>; Tanislav, Cosmin
> <Cosmin.Tanislav@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Subject: [POC] iio: ad74413: allow channel configuration to be given via
> module parameters
> 
> [External]
> 
> Just to see how it would look, I tried doing the below. Since a board
> may have more than one ad74412/ad74413, one needs to be able to
> differentiate between them. So for now each module parameter channelX
> (X=0,1,2,3) accepts a space-separated list of <label>:<function>,
> where label is matched against the label property in device tree, but
> also allowing * to match any, which is more convenient when one knows
> there is only one device.
> 
> Aside from the missing documentation (MODULE_PARM_DESC), there are of
> course various details to hash out. E.g., should the function be
> specified with a raw integer as here, or should we take a text string
> "voltage-output", "current-input-ext-power" etc. and translate those?
> Should we use space or comma or semicolon as separator? And so on.
> 
> I also considered whether instead of the label one should instead
> match on the OF_FULLNAME,
> e.g. /soc@0/bus@30800000/spi@30840000/ad74412r@0, but that's a lot
> more complicated, and I assume that anybody that has more than one of
> these chips would anyway assign a label so that they can distinguish
> their /sys/bus/iio/... directories.
> 
> I should also note that it is not unprecedented for modules to take
> parameters that do some sort of (ad hoc) parsing to apply settings
> per-device. For example:
> 
> - ignore_wake in drivers/gpio/gpiolib-acpi.c
> - mtdparts in drivers/mtd/parsers/cmdlinepart.c
> - pci_devs_to_hide in drivers/xen/xen-pciback/pci_stub.c
> - quirks in drivers/hid/usbhid/hid-core.c
> 
> So the question is, is there any chance that anything like this could
> be accepted? If so, I'll of course spin this into a real patch with
> proper MODULE_PARM_DESC and commit log etc.
> 
> This has been tested doing
> 
>   insmod ad74413r.ko 'channel0=*:1' 'channel1=*:3' 'channel2=*:2'
> 'channel3=*:4'
> 
> and seeing that the channels did indeed come up as expected, where the
> device tree specified CH_FUNC_HIGH_IMPEDANCE for all of them.
> 

Nuno previously mentioned dynamically loading device tree overlays,
which seems like a much cleaner solution to me. Have you looked into
that?

Re: [POC] iio: ad74413: allow channel configuration to be given via module parameters

[Jonathan Cameron]

On Fri, 9 Dec 2022 08:46:40 +0000
"Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:

> > -----Original Message-----
> > From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > Sent: Thursday, December 8, 2022 3:34 PM
> > To: Nuno Sá <noname.nuno@gmail.com>; linux-iio@vger.kernel.org
> > Cc: Jonathan Cameron <jic23@kernel.org>; Tanislav, Cosmin
> > <Cosmin.Tanislav@analog.com>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > Subject: [POC] iio: ad74413: allow channel configuration to be given via
> > module parameters
> > 
> > [External]
> > 
> > Just to see how it would look, I tried doing the below. Since a board
> > may have more than one ad74412/ad74413, one needs to be able to
> > differentiate between them. So for now each module parameter channelX
> > (X=0,1,2,3) accepts a space-separated list of <label>:<function>,
> > where label is matched against the label property in device tree, but
> > also allowing * to match any, which is more convenient when one knows
> > there is only one device.
> > 
> > Aside from the missing documentation (MODULE_PARM_DESC), there are of
> > course various details to hash out. E.g., should the function be
> > specified with a raw integer as here, or should we take a text string
> > "voltage-output", "current-input-ext-power" etc. and translate those?
> > Should we use space or comma or semicolon as separator? And so on.
> > 
> > I also considered whether instead of the label one should instead
> > match on the OF_FULLNAME,
> > e.g. /soc@0/bus@30800000/spi@30840000/ad74412r@0, but that's a lot
> > more complicated, and I assume that anybody that has more than one of
> > these chips would anyway assign a label so that they can distinguish
> > their /sys/bus/iio/... directories.
> > 
> > I should also note that it is not unprecedented for modules to take
> > parameters that do some sort of (ad hoc) parsing to apply settings
> > per-device. For example:
> > 
> > - ignore_wake in drivers/gpio/gpiolib-acpi.c
> > - mtdparts in drivers/mtd/parsers/cmdlinepart.c
> > - pci_devs_to_hide in drivers/xen/xen-pciback/pci_stub.c
> > - quirks in drivers/hid/usbhid/hid-core.c
> > 
> > So the question is, is there any chance that anything like this could
> > be accepted? If so, I'll of course spin this into a real patch with
> > proper MODULE_PARM_DESC and commit log etc.
> > 
> > This has been tested doing
> > 
> >   insmod ad74413r.ko 'channel0=*:1' 'channel1=*:3' 'channel2=*:2'
> > 'channel3=*:4'
> > 
> > and seeing that the channels did indeed come up as expected, where the
> > device tree specified CH_FUNC_HIGH_IMPEDANCE for all of them.
> >   
> 
> Nuno previously mentioned dynamically loading device tree overlays,
> which seems like a much cleaner solution to me. Have you looked into
> that?
> 

We are unlikely to take anything else I'm afraid. So hopefully
device tree overlays will work for you.

Dynamic configuration of pretty much anything about input OR output would
be potentially fine (we'd probably want to add limits on output settings
which I think we've done for some devices).  A userspace interface to
switch between those is much more of a corner case and could lead to
hardware damage depending on exactly what is connected to the pins.

Jonathan