| Message ID | 20180403153635.8228-1-peda@axentia.se |
|---|---|
| Headers | show |
| Series | iio: add unit converter | expand |
On 04/03/2018 10:36 AM, Peter Rosin wrote: > Hi! > > This driver implements support for voltage dividers and current > sense circuits. It's pretty generic and should be easily adaptable > to other linear scaling purposes... > I really like this idea, defining channel scaling / channel type conversion in DT will be very useful. So much so that I would recommend this not be a use specific driver but instead moved into the IIO core. This would allow defining these channel conversions in the device node itself, so as to not need a separate node just for the converter (the conversion is not a device and probably should not have it's own node anyway). It would also help with enabling this support for buffered readers/writers and not just devices only supporting _raw reads/writes. Andrew > The driver is still named "unit converter", because it was not > clear to me that there was a real problem with the driver being > named that. I got the impression that the naming discussion in > v1 was mainly about the category, and that it kind of looked odd > and non-specific with unit-converter in the DT bindings, but > what do I know? > > Cheers, > Peter > > Changes since v1: https://lkml.org/lkml/2018/3/19/801 > - Put the driver in the new afe category (Analog Front Ends) and do not > move the iio-mux driver. > - Do not refer to the source channel as "parent", use "source" instead. > - Have the DT compatible drive the target unit, instead of relying on a > "type" DT-property for that. > - In the DT bindings, use an unnamed source channel. > - Do not set up writes to _RAW (sorry Phil) as I don't need it and have > not tested it. It's easy to add back if needed. > - Fail if the source channel does not support _RAW or _SCALE. > - Fix various spelling issues. > - Fix various code style issues. > > Peter Rosin (2): > dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider > iio: afe: unit-converter: new driver > > .../bindings/iio/afe/current-sense-circuit.txt | 45 ++++ > .../bindings/iio/afe/voltage-divider.txt | 45 ++++ > MAINTAINERS | 8 + > drivers/iio/Kconfig | 1 + > drivers/iio/Makefile | 1 + > drivers/iio/afe/Kconfig | 18 ++ > drivers/iio/afe/Makefile | 6 + > drivers/iio/afe/iio-unit-converter.c | 257 +++++++++++++++++++++ > 8 files changed, 381 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt > create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt > create mode 100644 drivers/iio/afe/Kconfig > create mode 100644 drivers/iio/afe/Makefile > create mode 100644 drivers/iio/afe/iio-unit-converter.c > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-04-03 19:41, Andrew F. Davis wrote: > On 04/03/2018 10:36 AM, Peter Rosin wrote: >> Hi! >> >> This driver implements support for voltage dividers and current >> sense circuits. It's pretty generic and should be easily adaptable >> to other linear scaling purposes... >> > > I really like this idea, defining channel scaling / channel type > conversion in DT will be very useful. So much so that I would recommend > this not be a use specific driver but instead moved into the IIO core. > > This would allow defining these channel conversions in the device node > itself, so as to not need a separate node just for the converter (the > conversion is not a device and probably should not have it's own node > anyway). It would also help with enabling this support for buffered > readers/writers and not just devices only supporting _raw reads/writes. In my case, the voltage dividers and the current sense circuit are very much real, and I can point my finger at them on the board. Sure, they are not ICs, but to not call them devices is wrong IMHO, and the ADC which is involved have very little to do with the voltage dividers and the fact that it is involved in sensing current. This is not an argument for not moving the functionally to the core though, but that has some problems AFAICT. Because you need some kind of clever and generic binding to make the core do its thing, and it might not be easy to come up with something that fits all devices? And if we do put this in the core, that opens the door for more complex unit converters later on (non-linear etc), further complicating the generic bindings. That said, I'm obviously biased since I want this to get in sooner rather than later... Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/03/2018 01:09 PM, Peter Rosin wrote: > On 2018-04-03 19:41, Andrew F. Davis wrote: >> On 04/03/2018 10:36 AM, Peter Rosin wrote: >>> Hi! >>> >>> This driver implements support for voltage dividers and current >>> sense circuits. It's pretty generic and should be easily adaptable >>> to other linear scaling purposes... >>> >> >> I really like this idea, defining channel scaling / channel type >> conversion in DT will be very useful. So much so that I would recommend >> this not be a use specific driver but instead moved into the IIO core. >> >> This would allow defining these channel conversions in the device node >> itself, so as to not need a separate node just for the converter (the >> conversion is not a device and probably should not have it's own node >> anyway). It would also help with enabling this support for buffered >> readers/writers and not just devices only supporting _raw reads/writes. > > In my case, the voltage dividers and the current sense circuit are very > much real, and I can point my finger at them on the board. Sure, they > are not ICs, but to not call them devices is wrong IMHO, and the ADC > which is involved have very little to do with the voltage dividers and > the fact that it is involved in sensing current. I would argue support resistors are still part of the "current sensing device", even if they are physically external to the ADC IC. As you say though, it doesn't really matter that much to this discussion. > This is not an argument > for not moving the functionally to the core though, but that has some > problems AFAICT. Because you need some kind of clever and generic > binding to make the core do its thing, and it might not be easy to come > up with something that fits all devices? And if we do put this in the > core, that opens the door for more complex unit converters later on > (non-linear etc), further complicating the generic bindings. > For the simple case of linear scaling of the channel input/output, and type conversion I don't see much problem. They will not need to be extended, new bindings can be added for more complex cases. When more complex cases show up additional bindings will be needed either way. > That said, I'm obviously biased since I want this to get in sooner > rather than later... > Adding this functionality to the IIO core doesn't really block this patch-set, it will just make this patch-set unneeded in the future if/when support goes into the core.. Andrew > Cheers, > Peter > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 3 Apr 2018 17:36:35 +0200 Peter Rosin <peda@axentia.se> wrote: > If an ADC channel measures the midpoint of a voltage divider, the > interesting voltage is often the voltage over the full resistance. > E.g. if the full voltage is too big for the ADC to handle. > Likewise, if an ADC channel measures the voltage across a resistor, > the interesting value is often the current through the resistor. > > This driver solves both problems by allowing to linearly scale a channel > and by allowing changes to the type of the channel. Or both. > > Signed-off-by: Peter Rosin <peda@axentia.se> One passing comment inline but nothing that really matters.. Looks good to me and I'll be happy to take it once we are sure everyone is happy with the devicetree bindings. Thanks, Jonathan > --- > MAINTAINERS | 1 + > drivers/iio/Kconfig | 1 + > drivers/iio/Makefile | 1 + > drivers/iio/afe/Kconfig | 18 +++ > drivers/iio/afe/Makefile | 6 + > drivers/iio/afe/iio-unit-converter.c | 257 +++++++++++++++++++++++++++++++++++ > 6 files changed, 284 insertions(+) > create mode 100644 drivers/iio/afe/Kconfig > create mode 100644 drivers/iio/afe/Makefile > create mode 100644 drivers/iio/afe/iio-unit-converter.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9dbe5019c6bd..f9835521eec6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6895,6 +6895,7 @@ L: linux-iio@vger.kernel.org > S: Maintained > F: Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt > F: Documentation/devicetree/bindings/iio/afe/voltage-divider.txt > +F: drivers/iio/afe/iio-unit-converter.c > > IKANOS/ADI EAGLE ADSL USB DRIVER > M: Matthieu Castet <castet.matthieu@free.fr> > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig > index b3c8c6ef0dff..d69e85a8bdc3 100644 > --- a/drivers/iio/Kconfig > +++ b/drivers/iio/Kconfig > @@ -70,6 +70,7 @@ config IIO_TRIGGERED_EVENT > > source "drivers/iio/accel/Kconfig" > source "drivers/iio/adc/Kconfig" > +source "drivers/iio/afe/Kconfig" > source "drivers/iio/amplifiers/Kconfig" > source "drivers/iio/chemical/Kconfig" > source "drivers/iio/common/Kconfig" > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile > index b16b2e9ddc40..d8cba9c229c0 100644 > --- a/drivers/iio/Makefile > +++ b/drivers/iio/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_IIO_TRIGGERED_EVENT) += industrialio-triggered-event.o > > obj-y += accel/ > obj-y += adc/ > +obj-y += afe/ > obj-y += amplifiers/ > obj-y += buffer/ > obj-y += chemical/ > diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig > new file mode 100644 > index 000000000000..75acbe7eed15 > --- /dev/null > +++ b/drivers/iio/afe/Kconfig > @@ -0,0 +1,18 @@ > +# > +# Analog Front End drivers > +# > +# When adding new entries keep the list in alphabetical order > + > +menu "Analog Front Ends" > + > +config IIO_UNIT_CONVERTER > + tristate "IIO unit converter" > + depends on OF || COMPILE_TEST > + help > + Say yes here to build support for the IIO unit converter > + that handles voltage dividers and current sense circuits. > + > + To compile this driver as a module, choose M here: the > + module will be called iio-unit-converter. > + > +endmenu > diff --git a/drivers/iio/afe/Makefile b/drivers/iio/afe/Makefile > new file mode 100644 > index 000000000000..7691cc5b809a > --- /dev/null > +++ b/drivers/iio/afe/Makefile > @@ -0,0 +1,6 @@ > +# > +# Makefile for industrial I/O Analog Front Ends (AFE) > +# > + > +# When adding new entries keep the list in alphabetical order > +obj-$(CONFIG_IIO_UNIT_CONVERTER) += iio-unit-converter.o > diff --git a/drivers/iio/afe/iio-unit-converter.c b/drivers/iio/afe/iio-unit-converter.c > new file mode 100644 > index 000000000000..43429543cc29 > --- /dev/null > +++ b/drivers/iio/afe/iio-unit-converter.c > @@ -0,0 +1,257 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * IIO unit converter > + * > + * Copyright (C) 2018 Axentia Technologies AB > + * > + * Author: Peter Rosin <peda@axentia.se> > + */ > + > +#include <linux/err.h> > +#include <linux/iio/consumer.h> > +#include <linux/iio/iio.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > + > +struct unit_converter_cfg { > + enum iio_chan_type type; > +}; > + > +enum unit_converter_variant { > + CURRENT_SENSE_CIRCUIT, > + VOLTAGE_DIVIDER, > +}; > + > +static const struct unit_converter_cfg unit_converter_cfg[] = { > + [CURRENT_SENSE_CIRCUIT] = { > + .type = IIO_CURRENT, > + }, > + [VOLTAGE_DIVIDER] = { > + .type = IIO_VOLTAGE, > + }, > +}; > + > +struct unit_converter { > + const struct unit_converter_cfg *cfg; > + struct iio_channel *source; > + struct iio_dev *indio_dev; > + struct iio_chan_spec chan; > + struct iio_chan_spec_ext_info *ext_info; > + s32 numerator; > + s32 denominator; > +}; > + > +static int unit_converter_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct unit_converter *uc = iio_priv(indio_dev); > + unsigned long long tmp; > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + return iio_read_channel_raw(uc->source, val); > + > + case IIO_CHAN_INFO_SCALE: > + ret = iio_read_channel_scale(uc->source, val, val2); > + switch (ret) { > + case IIO_VAL_FRACTIONAL: > + *val *= uc->numerator; > + *val2 *= uc->denominator; > + return ret; > + case IIO_VAL_INT: > + *val *= uc->numerator; > + if (uc->denominator == 1) > + return ret; > + *val2 = uc->denominator; > + return IIO_VAL_FRACTIONAL; > + case IIO_VAL_FRACTIONAL_LOG2: > + tmp = *val * 1000000000LL; > + do_div(tmp, uc->denominator); > + tmp *= uc->numerator; > + do_div(tmp, 1000000000LL); > + *val = tmp; > + return ret; > + default: > + return -EOPNOTSUPP; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int unit_converter_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, int *length, > + long mask) > +{ > + struct unit_converter *uc = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + *type = IIO_VAL_INT; > + return iio_read_avail_channel_raw(uc->source, vals, length); > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info unit_converter_info = { > + .read_raw = unit_converter_read_raw, > + .read_avail = unit_converter_read_avail, > +}; > + > +static ssize_t unit_converter_read_ext_info(struct iio_dev *indio_dev, > + uintptr_t private, > + struct iio_chan_spec const *chan, > + char *buf) > +{ > + struct unit_converter *uc = iio_priv(indio_dev); > + > + return iio_read_channel_ext_info(uc->source, > + uc->ext_info[private].name, > + buf); > +} > + > +static ssize_t unit_converter_write_ext_info(struct iio_dev *indio_dev, > + uintptr_t private, > + struct iio_chan_spec const *chan, > + const char *buf, size_t len) > +{ > + struct unit_converter *uc = iio_priv(indio_dev); > + > + return iio_write_channel_ext_info(uc->source, > + uc->ext_info[private].name, > + buf, len); > +} > + > +static int unit_converter_configure_channel(struct device *dev, > + struct unit_converter *uc) > +{ > + struct iio_chan_spec *chan = &uc->chan; > + struct iio_chan_spec const *schan = uc->source->channel; > + > + chan->indexed = 1; > + chan->output = schan->output; > + chan->ext_info = uc->ext_info; > + chan->type = uc->cfg->type; > + > + if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) || > + !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) { > + dev_err(dev, "source channel does not support raw/scale\n"); > + return -EINVAL; > + } > + > + chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE); > + > + if (iio_channel_has_available(schan, IIO_CHAN_INFO_RAW)) > + chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW); > + > + return 0; > +} > + > +static const struct of_device_id unit_converter_match[] = { > + { .compatible = "current-sense-circuit", > + .data = &unit_converter_cfg[CURRENT_SENSE_CIRCUIT], }, > + { .compatible = "voltage-divider", > + .data = &unit_converter_cfg[VOLTAGE_DIVIDER], }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, unit_converter_match); > + > +static int unit_converter_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct iio_dev *indio_dev; > + struct iio_channel *source; > + struct unit_converter *uc; > + int sizeof_ext_info; > + int sizeof_priv; > + int i; > + int ret; > + > + source = devm_iio_channel_get(dev, NULL); > + if (IS_ERR(source)) { > + if (PTR_ERR(source) != -EPROBE_DEFER) > + dev_err(dev, "failed to get source channel\n"); > + return PTR_ERR(source); > + } > + > + sizeof_ext_info = iio_get_channel_ext_info_count(source); > + if (sizeof_ext_info) { > + sizeof_ext_info += 1; /* one extra entry for the sentinel */ > + sizeof_ext_info *= sizeof(*uc->ext_info); > + } > + > + sizeof_priv = sizeof(*uc) + sizeof_ext_info; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof_priv); > + if (!indio_dev) > + return -ENOMEM; > + > + uc = iio_priv(indio_dev); > + > + uc->cfg = of_device_get_match_data(dev); > + uc->numerator = 1; > + uc->denominator = 1; > + device_property_read_u32(dev, "numerator", &uc->numerator); > + device_property_read_u32(dev, "denominator", &uc->denominator); > + if (!uc->numerator || !uc->denominator) { > + dev_err(dev, "invalid scaling factor.\n"); > + return -EINVAL; > + } > + > + platform_set_drvdata(pdev, indio_dev); > + > + uc->source = source; > + > + indio_dev->name = dev_name(dev); > + indio_dev->dev.parent = dev; > + indio_dev->info = &unit_converter_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = &uc->chan; > + indio_dev->num_channels = 1; > + if (sizeof_ext_info) { > + uc->ext_info = devm_kmemdup(dev, > + source->channel->ext_info, > + sizeof_ext_info, GFP_KERNEL); > + if (!uc->ext_info) > + return -ENOMEM; > + > + for (i = 0; uc->ext_info[i].name; ++i) { > + if (source->channel->ext_info[i].read) > + uc->ext_info[i].read = unit_converter_read_ext_info; > + if (source->channel->ext_info[i].write) > + uc->ext_info[i].write = unit_converter_write_ext_info; > + uc->ext_info[i].private = i; > + } > + } > + > + ret = unit_converter_configure_channel(dev, uc); > + if (ret) > + return ret; > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + dev_err(dev, "failed to register iio device\n"); This made me check if we already report errors if registration fails. We do in a lot of cases but not quite all. It might make sense to improve that error reporting in the core and drop it in any drivers. I don't care that strongly about it though... > + > + return ret; > +} > + > +static struct platform_driver unit_converter_driver = { > + .probe = unit_converter_probe, > + .driver = { > + .name = "iio-unit-converter", > + .of_match_table = unit_converter_match, > + }, > +}; > +module_platform_driver(unit_converter_driver); > + > +MODULE_DESCRIPTION("IIO unit converter driver"); > +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>"); > +MODULE_LICENSE("GPL v2"); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 03, 2018 at 08:09:08PM +0200, Peter Rosin wrote: > On 2018-04-03 19:41, Andrew F. Davis wrote: > > On 04/03/2018 10:36 AM, Peter Rosin wrote: > >> Hi! > >> > >> This driver implements support for voltage dividers and current > >> sense circuits. It's pretty generic and should be easily adaptable > >> to other linear scaling purposes... > >> > > > > I really like this idea, defining channel scaling / channel type > > conversion in DT will be very useful. So much so that I would recommend > > this not be a use specific driver but instead moved into the IIO core. > > > > This would allow defining these channel conversions in the device node > > itself, so as to not need a separate node just for the converter (the > > conversion is not a device and probably should not have it's own node > > anyway). It would also help with enabling this support for buffered > > readers/writers and not just devices only supporting _raw reads/writes. > > In my case, the voltage dividers and the current sense circuit are very > much real, and I can point my finger at them on the board. Sure, they > are not ICs, but to not call them devices is wrong IMHO, and the ADC > which is involved have very little to do with the voltage dividers and > the fact that it is involved in sensing current. This is not an argument > for not moving the functionally to the core though, but that has some > problems AFAICT. Because you need some kind of clever and generic > binding to make the core do its thing, and it might not be easy to come > up with something that fits all devices? And if we do put this in the > core, that opens the door for more complex unit converters later on > (non-linear etc), further complicating the generic bindings. I agree. We've learned the hard way that even things like the LED diode itself need to be described and not just the controller. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html