Message ID | 20210703084224.31623-1-andreas@kemnade.info |
---|---|
Headers | show |
Series | mfd: rn5t618: Extend ADC support | expand |
On Sat, 3 Jul 2021 10:42:20 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > Add devicetree support so that consumers can reference the channels > via devicetree, especially the power subdevice can make use of that > to provide voltage_now properties. Does the mapping vary from board to board? Often these mappings are internal to the chip so might as well be provided hard coded in the relevant drivers rather than via DT. See drivers that have iio_map structure arrays. > > Andreas Kemnade (4): > dt-bindings: mfd: ricoh,rn5t618: ADC related nodes and properties > mfd: rn5t618: Add of compatibles for ADC and power > iio: rn5t618: Add devicetree support > power: supply: rn5t618: Add voltage_now property > > .../bindings/mfd/ricoh,rn5t618.yaml | 53 ++++++++++++++++++ > drivers/iio/adc/rn5t618-adc.c | 14 ++++- > drivers/mfd/rn5t618.c | 6 +- > drivers/power/supply/rn5t618_power.c | 56 +++++++++++++++++++ > 4 files changed, 126 insertions(+), 3 deletions(-) >
On Sat, 3 Jul 2021 10:42:22 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > This allows having devicetree nodes for the subdevices. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/mfd/rn5t618.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > index 384acb459427..b916c7471ca3 100644 > --- a/drivers/mfd/rn5t618.c > +++ b/drivers/mfd/rn5t618.c > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > }; > > static const struct mfd_cell rc5t619_cells[] = { > - { .name = "rn5t618-adc" }, > - { .name = "rn5t618-power" }, > + { .name = "rn5t618-adc", > + .of_compatible = "ricoh,rc5t619-adc" }, Odd to have a name of 618 and a compatible of 619. Why? Definitely deserves a comment if this is necessary for some reason! > + { .name = "rn5t618-power", > + .of_compatible = "ricoh,rc5t619-power" }, > { .name = "rn5t618-regulator" }, > { .name = "rc5t619-rtc" }, > { .name = "rn5t618-wdt" },
On Sat, 3 Jul 2021 10:42:23 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > Add devicetree support to be able to easily get the channels > from another device. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/iio/adc/rn5t618-adc.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/rn5t618-adc.c b/drivers/iio/adc/rn5t618-adc.c > index 7010c4276947..feba19f91574 100644 > --- a/drivers/iio/adc/rn5t618-adc.c > +++ b/drivers/iio/adc/rn5t618-adc.c > @@ -12,6 +12,7 @@ > #include <linux/init.h> > #include <linux/module.h> > #include <linux/mfd/rn5t618.h> > +#include <linux/of_device.h> Why? mod_devicetable.h probably the one you want. > #include <linux/platform_device.h> > #include <linux/completion.h> > #include <linux/regmap.h> > @@ -218,6 +219,7 @@ static int rn5t618_adc_probe(struct platform_device *pdev) > init_completion(&adc->conv_completion); > > iio_dev->name = dev_name(&pdev->dev); > + iio_dev->dev.of_node = pdev->dev.of_node; That should (now) be set by the IIO core for you via the devm_iio_device_register() call below if it's not already set. > iio_dev->info = &rn5t618_adc_iio_info; > iio_dev->modes = INDIO_DIRECT_MODE; > iio_dev->channels = rn5t618_adc_iio_channels; > @@ -242,9 +244,19 @@ static int rn5t618_adc_probe(struct platform_device *pdev) > return devm_iio_device_register(adc->dev, iio_dev); > } > > +#ifdef CONFIG_OF > +static const struct of_device_id rn5t618_adc_of_match[] = { > + { .compatible = "ricoh,rc5t619-adc", }, > + { .compatible = "ricoh,rn5t618-adc", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, rn5t618_adc_of_match); > +#endif > + > static struct platform_driver rn5t618_adc_driver = { > .driver = { > - .name = "rn5t618-adc", > + .name = "rn5t618-adc", > + .of_match_table = of_match_ptr(rn5t618_adc_of_match), Given cost of that table is totally trivial (and I'm trying to get rid of this pattern in IIO as it's noisy and adds little :) drop the of_match_ptr protection and the ifdefs above. > }, > .probe = rn5t618_adc_probe, > };
On Sat, 3 Jul 2021 10:42:24 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > Read voltage_now via IIO and provide the property. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> Hi Andreas, One comment inline, but looks fine to me in general. > --- > drivers/power/supply/rn5t618_power.c | 56 ++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/drivers/power/supply/rn5t618_power.c b/drivers/power/supply/rn5t618_power.c > index 819061918b2a..b062208c8a91 100644 > --- a/drivers/power/supply/rn5t618_power.c > +++ b/drivers/power/supply/rn5t618_power.c > @@ -9,10 +9,12 @@ > #include <linux/device.h> > #include <linux/bitops.h> > #include <linux/errno.h> > +#include <linux/iio/consumer.h> > #include <linux/init.h> > #include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/mfd/rn5t618.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/power_supply.h> > #include <linux/regmap.h> > @@ -64,6 +66,8 @@ struct rn5t618_power_info { > struct power_supply *battery; > struct power_supply *usb; > struct power_supply *adp; > + struct iio_channel *channel_vusb; > + struct iio_channel *channel_vadp; > int irq; > }; > > @@ -77,6 +81,7 @@ static enum power_supply_usb_type rn5t618_usb_types[] = { > static enum power_supply_property rn5t618_usb_props[] = { > /* input current limit is not very accurate */ > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_USB_TYPE, > POWER_SUPPLY_PROP_ONLINE, > @@ -85,6 +90,7 @@ static enum power_supply_property rn5t618_usb_props[] = { > static enum power_supply_property rn5t618_adp_props[] = { > /* input current limit is not very accurate */ > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_ONLINE, > }; > @@ -464,6 +470,16 @@ static int rn5t618_adp_get_property(struct power_supply *psy, > > val->intval = FROM_CUR_REG(regval); > break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + if (!info->channel_vadp) > + return -ENODATA; > + > + ret = iio_read_channel_processed(info->channel_vadp, &val->intval); > + if (ret < 0) > + return ret; > + > + val->intval *= 1000; > + break; > default: > return -EINVAL; > } > @@ -589,6 +605,16 @@ static int rn5t618_usb_get_property(struct power_supply *psy, > val->intval = FROM_CUR_REG(regval); > } > break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + if (!info->channel_vusb) > + return -ENODATA; > + > + ret = iio_read_channel_processed(info->channel_vusb, &val->intval); > + if (ret < 0) > + return ret; > + > + val->intval *= 1000; > + break; > default: > return -EINVAL; > } > @@ -711,6 +737,28 @@ static int rn5t618_power_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, info); > > + info->channel_vusb = devm_iio_channel_get(&pdev->dev, "vusb"); > + if (IS_ERR(info->channel_vusb)) { > + ret = PTR_ERR(info->channel_vusb); > + if (ret == -EPROBE_DEFER) > + return ret; > + > + dev_warn(&pdev->dev, "could not request vusb iio channel (%d)", > + ret); I wonder if you should distinguish between -ENODEV to indicate there wasn't one specified vs something else when wrong. Might not be worth bothering as there are not many things that could go wrong (things like small memory allocations which will never fail..) > + info->channel_vusb = NULL; > + } > + > + info->channel_vadp = devm_iio_channel_get(&pdev->dev, "vadp"); > + if (IS_ERR(info->channel_vadp)) { > + ret = PTR_ERR(info->channel_vadp); > + if (ret == -EPROBE_DEFER) > + return ret; > + > + dev_warn(&pdev->dev, "could not request vadp iio channel (%d)", > + ret); > + info->channel_vadp = NULL; > + } > + > ret = regmap_read(info->rn5t618->regmap, RN5T618_CONTROL, &v); > if (ret) > return ret; > @@ -778,9 +826,17 @@ static int rn5t618_power_probe(struct platform_device *pdev) > return 0; > } > > +static const struct of_device_id rn5t618_power_of_match[] = { > + {.compatible = "ricoh,rc5t619-power", }, > + {.compatible = "ricoh,rn5t618-power", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, rn5t618_power_of_match); > + > static struct platform_driver rn5t618_power_driver = { > .driver = { > .name = "rn5t618-power", > + .of_match_table = of_match_ptr(rn5t618_power_of_match), > }, > .probe = rn5t618_power_probe, > };
Hi, On Sat, 3 Jul 2021 17:04:05 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 3 Jul 2021 10:42:22 +0200 > Andreas Kemnade <andreas@kemnade.info> wrote: > > > This allows having devicetree nodes for the subdevices. > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > drivers/mfd/rn5t618.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > index 384acb459427..b916c7471ca3 100644 > > --- a/drivers/mfd/rn5t618.c > > +++ b/drivers/mfd/rn5t618.c > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > }; > > > > static const struct mfd_cell rc5t619_cells[] = { > > - { .name = "rn5t618-adc" }, > > - { .name = "rn5t618-power" }, > > + { .name = "rn5t618-adc", > > + .of_compatible = "ricoh,rc5t619-adc" }, > > Odd to have a name of 618 and a compatible of 619. Why? > Definitely deserves a comment if this is necessary for some reason! > Background of this whole thing: Both RN5T618 and RC5T619 have an ADC. I would expect the driver to work for both but I cannot prove that. No RN5T618 here to test. Maybe it requires some quirks, probably not. The only hint I have is that a) I use register definitions added to the kernel for RN5T618 support b) public datasheets looks same regarding the ADC. c) out-of-tree code for the RN5T618 does not give a clear picture, it shows no differences but is not complete enough to judge. To avoid introducing untested things, I am only adding it to the rc5t619_cell list. I would really hope that someone does some rn5t618 tests... Because everything else which is both for the RN5T618 and RC5T619 uses rn5t618 as a name, I continued that practise. The subdevice driver also gets the information whether it is a rn5t618 or rc5t619 via rn5t618->variant, so it can handle things. > > + { .name = "rn5t618-power", > > + .of_compatible = "ricoh,rc5t619-power" }, Similar situation here. > > { .name = "rn5t618-regulator" }, > > { .name = "rc5t619-rtc" }, and this one definitively only exists in the rc5t619. > > { .name = "rn5t618-wdt" }, > > Regards, Andreas
Hi, On Sat, 3 Jul 2021 16:59:50 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 3 Jul 2021 10:42:20 +0200 > Andreas Kemnade <andreas@kemnade.info> wrote: > > > Add devicetree support so that consumers can reference the channels > > via devicetree, especially the power subdevice can make use of that > > to provide voltage_now properties. > > Does the mapping vary from board to board? Often these mappings are > internal to the chip so might as well be provided hard coded in the > relevant drivers rather than via DT. See drivers that have iio_map > structure arrays. > Most things are internal to the chip, but AIN1/AIN0 are external and could be connected to anything. Regards, Andreas
On Sat, 3 Jul 2021 18:39:40 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > Hi, > > On Sat, 3 Jul 2021 16:59:50 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sat, 3 Jul 2021 10:42:20 +0200 > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > Add devicetree support so that consumers can reference the channels > > > via devicetree, especially the power subdevice can make use of that > > > to provide voltage_now properties. > > > > Does the mapping vary from board to board? Often these mappings are > > internal to the chip so might as well be provided hard coded in the > > relevant drivers rather than via DT. See drivers that have iio_map > > structure arrays. > > > Most things are internal to the chip, but > AIN1/AIN0 are external and could be connected to anything. > hmm, iio_map stuff looks nice, so before messing with devicetree, I could solve 90% of the problem by just using iio_map? For my use cases it is enough to have the internal stuff at the moment. That would simplify stuff a lot. So I could go forward with the iio_map stuff now, and if there is a use case for AIN1/0, the devicetree stuff can be added later? Regards, Andreas
On Sat, 3 Jul 2021 18:55:40 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > On Sat, 3 Jul 2021 18:39:40 +0200 > Andreas Kemnade <andreas@kemnade.info> wrote: > > > Hi, > > > > On Sat, 3 Jul 2021 16:59:50 +0100 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > On Sat, 3 Jul 2021 10:42:20 +0200 > > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > > > Add devicetree support so that consumers can reference the channels > > > > via devicetree, especially the power subdevice can make use of that > > > > to provide voltage_now properties. > > > > > > Does the mapping vary from board to board? Often these mappings are > > > internal to the chip so might as well be provided hard coded in the > > > relevant drivers rather than via DT. See drivers that have iio_map > > > structure arrays. > > > > > Most things are internal to the chip, but > > AIN1/AIN0 are external and could be connected to anything. > > > hmm, iio_map stuff looks nice, so before messing with devicetree, > I could solve 90% of the problem by just using iio_map? For my use > cases it is enough to have the internal stuff at the moment. That would > simplify stuff a lot. > > So I could go forward with the iio_map stuff now, and if there is a use > case for AIN1/0, the devicetree stuff can be added later? I was just thinking the same. I 'think' that it will first try to find a mapping via device tree and then use the iio_map stuff. So you can probably get away with a mixture of the two. Worth testing that works though (hook up iio-hwmon to AIN0 perhaps whilst also using the iio_map approach). I might be completely wrong though and am not aware of anyone currently doing this... Jonathan > > Regards, > Andreas
On Sat, 3 Jul 2021 18:31:11 +0200 Andreas Kemnade <andreas@kemnade.info> wrote: > Hi, > > On Sat, 3 Jul 2021 17:04:05 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sat, 3 Jul 2021 10:42:22 +0200 > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > This allows having devicetree nodes for the subdevices. > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > --- > > > drivers/mfd/rn5t618.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > index 384acb459427..b916c7471ca3 100644 > > > --- a/drivers/mfd/rn5t618.c > > > +++ b/drivers/mfd/rn5t618.c > > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > > }; > > > > > > static const struct mfd_cell rc5t619_cells[] = { > > > - { .name = "rn5t618-adc" }, > > > - { .name = "rn5t618-power" }, > > > + { .name = "rn5t618-adc", > > > + .of_compatible = "ricoh,rc5t619-adc" }, > > > > Odd to have a name of 618 and a compatible of 619. Why? > > Definitely deserves a comment if this is necessary for some reason! > > > Background of this whole thing: > Both RN5T618 and RC5T619 have an ADC. I would expect the driver to work > for both but I cannot prove that. No RN5T618 here to test. Maybe it > requires some quirks, probably not. The only hint I have is that > a) I use register definitions added to the kernel for RN5T618 support > b) public datasheets looks same regarding the ADC. > c) out-of-tree code for the RN5T618 does not give a clear picture, it > shows no differences but is not complete enough to judge. > > To avoid introducing untested things, I am only adding it to the > rc5t619_cell list. I would really hope that someone does some rn5t618 > tests... Because everything else which is both for the RN5T618 and > RC5T619 uses rn5t618 as a name, I continued that practise. > > The subdevice driver also gets the information whether it is a rn5t618 > or rc5t619 via rn5t618->variant, so it can handle things. > > > > + { .name = "rn5t618-power", > > > + .of_compatible = "ricoh,rc5t619-power" }, > > Similar situation here. > > > > { .name = "rn5t618-regulator" }, > > > { .name = "rc5t619-rtc" }, > and this one definitively only exists in the rc5t619. All sounds reasonable to me. Dt binding wise we'd normally handle this with a double compatible, with the more part specific one first. That way we can diverge later if we need to, but don't need to care about it until an identified need has occurred. compatible = "ricoh,rc5t619-adc", "ricoh,rc5t618-adc"; (or something like that) Anyhow, for now perhaps a comment to express briefly what you state above. > > > > { .name = "rn5t618-wdt" }, > > > > > > Regards, > Andreas
On Sat, 03 Jul 2021, Jonathan Cameron wrote: > On Sat, 3 Jul 2021 10:42:22 +0200 > Andreas Kemnade <andreas@kemnade.info> wrote: > > > This allows having devicetree nodes for the subdevices. > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > drivers/mfd/rn5t618.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > index 384acb459427..b916c7471ca3 100644 > > --- a/drivers/mfd/rn5t618.c > > +++ b/drivers/mfd/rn5t618.c > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > }; > > > > static const struct mfd_cell rc5t619_cells[] = { > > - { .name = "rn5t618-adc" }, > > - { .name = "rn5t618-power" }, > > + { .name = "rn5t618-adc", > > + .of_compatible = "ricoh,rc5t619-adc" }, > > Odd to have a name of 618 and a compatible of 619. Why? > Definitely deserves a comment if this is necessary for some reason! Actually this is the norm. We have lots of drivers named after the *first* device they supported before expansion. > > + { .name = "rn5t618-power", > > + .of_compatible = "ricoh,rc5t619-power" }, > > { .name = "rn5t618-regulator" }, > > { .name = "rc5t619-rtc" }, > > { .name = "rn5t618-wdt" }, >
On Sat, 03 Jul 2021, Andreas Kemnade wrote: > This allows having devicetree nodes for the subdevices. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/mfd/rn5t618.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > index 384acb459427..b916c7471ca3 100644 > --- a/drivers/mfd/rn5t618.c > +++ b/drivers/mfd/rn5t618.c > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > }; > > static const struct mfd_cell rc5t619_cells[] = { > - { .name = "rn5t618-adc" }, > - { .name = "rn5t618-power" }, > + { .name = "rn5t618-adc", > + .of_compatible = "ricoh,rc5t619-adc" }, > + { .name = "rn5t618-power", > + .of_compatible = "ricoh,rc5t619-power" }, If you're converting entries from single to multi-line, you should place the coding lines one different ones to the brackets. Also, if ordering is unimportant, could you move the multi-line entries to the bottom please? > { .name = "rn5t618-regulator" }, > { .name = "rc5t619-rtc" }, > { .name = "rn5t618-wdt" },
On Mon, 5 Jul 2021 08:36:08 +0100 Lee Jones <lee.jones@linaro.org> wrote: > On Sat, 03 Jul 2021, Jonathan Cameron wrote: > > > On Sat, 3 Jul 2021 10:42:22 +0200 > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > This allows having devicetree nodes for the subdevices. > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > --- > > > drivers/mfd/rn5t618.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > index 384acb459427..b916c7471ca3 100644 > > > --- a/drivers/mfd/rn5t618.c > > > +++ b/drivers/mfd/rn5t618.c > > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > > }; > > > > > > static const struct mfd_cell rc5t619_cells[] = { > > > - { .name = "rn5t618-adc" }, > > > - { .name = "rn5t618-power" }, > > > + { .name = "rn5t618-adc", > > > + .of_compatible = "ricoh,rc5t619-adc" }, > > > > Odd to have a name of 618 and a compatible of 619. Why? > > Definitely deserves a comment if this is necessary for some reason! > > Actually this is the norm. We have lots of drivers named after the > *first* device they supported before expansion. Ah. I'd missed that this cells array is specific to the 5t619, though if the driver is the same I'd also expect it to be needed for the 5t618 entry. > > > > + { .name = "rn5t618-power", > > > + .of_compatible = "ricoh,rc5t619-power" }, > > > { .name = "rn5t618-regulator" }, > > > { .name = "rc5t619-rtc" }, > > > { .name = "rn5t618-wdt" }, > > >
On Mon, 5 Jul 2021 09:31:29 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 5 Jul 2021 08:36:08 +0100 > Lee Jones <lee.jones@linaro.org> wrote: > > > On Sat, 03 Jul 2021, Jonathan Cameron wrote: > > > > > On Sat, 3 Jul 2021 10:42:22 +0200 > > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > > > This allows having devicetree nodes for the subdevices. > > > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > > --- > > > > drivers/mfd/rn5t618.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c > > > > index 384acb459427..b916c7471ca3 100644 > > > > --- a/drivers/mfd/rn5t618.c > > > > +++ b/drivers/mfd/rn5t618.c > > > > @@ -24,8 +24,10 @@ static const struct mfd_cell rn5t618_cells[] = { > > > > }; > > > > > > > > static const struct mfd_cell rc5t619_cells[] = { > > > > - { .name = "rn5t618-adc" }, > > > > - { .name = "rn5t618-power" }, > > > > + { .name = "rn5t618-adc", > > > > + .of_compatible = "ricoh,rc5t619-adc" }, > > > > > > Odd to have a name of 618 and a compatible of 619. Why? > > > Definitely deserves a comment if this is necessary for some reason! > > > > Actually this is the norm. We have lots of drivers named after the > > *first* device they supported before expansion. > > Ah. I'd missed that this cells array is specific to the 5t619, though if > the driver is the same I'd also expect it to be needed for the 5t618 entry. > Well, yes, it is needed for the 5t618 also. But if I would add it, it would be untested. And that second shorter array is also used for the rn5t567 which does not have an ADC, So I we need three arrays there. Regards, Andreas
On Sun, 4 Jul 2021 17:10:23 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 3 Jul 2021 18:55:40 +0200 > Andreas Kemnade <andreas@kemnade.info> wrote: > > > On Sat, 3 Jul 2021 18:39:40 +0200 > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > Hi, > > > > > > On Sat, 3 Jul 2021 16:59:50 +0100 > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > On Sat, 3 Jul 2021 10:42:20 +0200 > > > > Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > > > > > Add devicetree support so that consumers can reference the channels > > > > > via devicetree, especially the power subdevice can make use of that > > > > > to provide voltage_now properties. > > > > > > > > Does the mapping vary from board to board? Often these mappings are > > > > internal to the chip so might as well be provided hard coded in the > > > > relevant drivers rather than via DT. See drivers that have iio_map > > > > structure arrays. > > > > > > > Most things are internal to the chip, but > > > AIN1/AIN0 are external and could be connected to anything. > > > > > hmm, iio_map stuff looks nice, so before messing with devicetree, > > I could solve 90% of the problem by just using iio_map? For my use > > cases it is enough to have the internal stuff at the moment. That would > > simplify stuff a lot. > > > > So I could go forward with the iio_map stuff now, and if there is a use > > case for AIN1/0, the devicetree stuff can be added later? > > I was just thinking the same. I 'think' that it will first try to find > a mapping via device tree and then use the iio_map stuff. > > So you can probably get away with a mixture of the two. > Worth testing that works though (hook up iio-hwmon to AIN0 perhaps whilst > also using the iio_map approach). > > I might be completely wrong though and am not aware of anyone currently > doing this... > I tested that approach, It works, so I will first post a series with just the iio_map stuff and later the devicetree stuff. Regards, Andreas