* [PATCH v2 0/2] Add support for the TI ADS8344 A/DC chips @ 2019-03-22 11:11 Gregory CLEMENT 2019-03-22 11:11 ` [PATCH v2 1/2] dt-bindings: iio: adc: Add bindings for " Gregory CLEMENT 2019-03-22 11:11 ` [PATCH v2 2/2] iio: adc: Add driver for the " Gregory CLEMENT 0 siblings, 2 replies; 6+ messages in thread From: Gregory CLEMENT @ 2019-03-22 11:11 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio Cc: Rob Herring, devicetree, Thomas Petazzoni, linux-arm-kernel, Alexandre Belloni, Gregory CLEMENT Hello, this is the second version of a small series adding the support for the Texas Instruments ADS8344 ADC chip. This chip has a 16-bit 8-Channel ADC and is access directly through SPI. The first patch add the binding documentation and the second one, the driver itself. Changelog: v1->v2: - Remove the mention to the #io-channel-cells in the first patch, as there is no consumer. - Remove scan_index and scan_type, they are not used. - Use defines for building the command and remove the comment. - Remove the goto and the label in the probe() function as the label was only use once. - Remove the "#ifdef CONFIG_OF", the driver already depends on OF. - Don't use of_match_ptr and rename ads8344_dt_id to ads8344_of_match. Gregory Gregory CLEMENT (2): dt-bindings: iio: adc: Add bindings for TI ADS8344 A/DC chips iio: adc: Add driver for the TI ADS8344 A/DC chips .../bindings/iio/adc/ti-ads8344.txt | 19 ++ drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ti-ads8344.c | 200 ++++++++++++++++++ 4 files changed, 230 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-ads8344.txt create mode 100644 drivers/iio/adc/ti-ads8344.c -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] dt-bindings: iio: adc: Add bindings for TI ADS8344 A/DC chips 2019-03-22 11:11 [PATCH v2 0/2] Add support for the TI ADS8344 A/DC chips Gregory CLEMENT @ 2019-03-22 11:11 ` Gregory CLEMENT 2019-03-31 6:41 ` Rob Herring 2019-03-22 11:11 ` [PATCH v2 2/2] iio: adc: Add driver for the " Gregory CLEMENT 1 sibling, 1 reply; 6+ messages in thread From: Gregory CLEMENT @ 2019-03-22 11:11 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio Cc: Rob Herring, devicetree, Thomas Petazzoni, linux-arm-kernel, Alexandre Belloni, Gregory CLEMENT This adds device tree bindings for the TI ADS8344 A/DC chips. Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> --- .../bindings/iio/adc/ti-ads8344.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-ads8344.txt diff --git a/Documentation/devicetree/bindings/iio/adc/ti-ads8344.txt b/Documentation/devicetree/bindings/iio/adc/ti-ads8344.txt new file mode 100644 index 000000000000..e47c3759a82b --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/ti-ads8344.txt @@ -0,0 +1,19 @@ +* Texas Instruments ADS8344 A/DC chip + +Required properties: + - compatible: Must be "ti,ads8344" + - reg: SPI chip select number for the device + - vref-supply: phandle to a regulator node that supplies the + reference voltage + +Recommended properties: + - spi-max-frequency: Definition as per + Documentation/devicetree/bindings/spi/spi-bus.txt + +Example: +adc@0 { + compatible = "ti,ads8344"; + reg = <0>; + vref-supply = <&refin_supply>; + spi-max-frequency = <10000000>; +}; -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add bindings for TI ADS8344 A/DC chips 2019-03-22 11:11 ` [PATCH v2 1/2] dt-bindings: iio: adc: Add bindings for " Gregory CLEMENT @ 2019-03-31 6:41 ` Rob Herring 0 siblings, 0 replies; 6+ messages in thread From: Rob Herring @ 2019-03-31 6:41 UTC (permalink / raw) To: Gregory CLEMENT Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, devicetree, Thomas Petazzoni, linux-arm-kernel, Alexandre Belloni, Gregory CLEMENT On Fri, 22 Mar 2019 12:11:07 +0100, Gregory CLEMENT wrote: > This adds device tree bindings for the TI ADS8344 A/DC chips. > > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> > --- > .../bindings/iio/adc/ti-ads8344.txt | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-ads8344.txt > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] iio: adc: Add driver for the TI ADS8344 A/DC chips 2019-03-22 11:11 [PATCH v2 0/2] Add support for the TI ADS8344 A/DC chips Gregory CLEMENT 2019-03-22 11:11 ` [PATCH v2 1/2] dt-bindings: iio: adc: Add bindings for " Gregory CLEMENT @ 2019-03-22 11:11 ` Gregory CLEMENT 2019-03-24 15:16 ` Himanshu Jha 1 sibling, 1 reply; 6+ messages in thread From: Gregory CLEMENT @ 2019-03-22 11:11 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio Cc: Rob Herring, devicetree, Thomas Petazzoni, linux-arm-kernel, Alexandre Belloni, Gregory CLEMENT This adds support for the Texas Instruments ADS8344 ADC chip. This chip has a 16-bit 8-Channel ADC and is access directly through SPI. Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> --- drivers/iio/adc/Kconfig | 10 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ti-ads8344.c | 200 +++++++++++++++++++++++++++++++++++ 3 files changed, 211 insertions(+) create mode 100644 drivers/iio/adc/ti-ads8344.c diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 76db6e5cc296..447d3a871746 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -967,6 +967,16 @@ config TI_ADS7950 To compile this driver as a module, choose M here: the module will be called ti-ads7950. +config TI_ADS8344 + tristate "Texas Instruments ADS8344" + depends on SPI && OF + help + If you say yes here you get support for Texas Instruments ADS8344 + ADC chips + + This driver can also be built as a module. If so, the module will be + called ti-ads8344. + config TI_ADS8688 tristate "Texas Instruments ADS8688" depends on SPI && OF diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 6fcebd167524..1f3ae934111d 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -87,6 +87,7 @@ obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o +obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c new file mode 100644 index 000000000000..649ed05bf1c9 --- /dev/null +++ b/drivers/iio/adc/ti-ads8344.c @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * ADS8344 16-bit 8-Channel ADC driver + * + * Author: Gregory CLEMENT <gregory.clement@bootlin.com> + * + * Datasheet: http://www.ti.com/lit/ds/symlink/ads8344.pdf + */ + +#include <linux/delay.h> +#include <linux/iio/buffer.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> + +#define ADS8344_START BIT(7) +#define ADS8344_SINGLE_END BIT(2) +#define ADS8344_CHANNEL(channel) ((channel) << 4) +#define ADS8344_CLOCK_INTERNAL 0x2 /* PD1 = 1 and PD0 = 0 */ + +struct ads8344 { + struct spi_device *spi; + struct regulator *reg; + struct mutex lock; + + u8 tx_buf ____cacheline_aligned; + u16 rx_buf; +}; + +#define ADS8344_VOLTAGE_CHANNEL(chan, si) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + } + +#define ADS8344_VOLTAGE_CHANNEL_DIFF(chan1, chan2, si) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (chan1), \ + .channel2 = (chan2), \ + .differential = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + } + +static const struct iio_chan_spec ads8344_channels[] = { + ADS8344_VOLTAGE_CHANNEL(0, 0), + ADS8344_VOLTAGE_CHANNEL(1, 4), + ADS8344_VOLTAGE_CHANNEL(2, 1), + ADS8344_VOLTAGE_CHANNEL(3, 5), + ADS8344_VOLTAGE_CHANNEL(4, 2), + ADS8344_VOLTAGE_CHANNEL(5, 6), + ADS8344_VOLTAGE_CHANNEL(6, 3), + ADS8344_VOLTAGE_CHANNEL(7, 7), + ADS8344_VOLTAGE_CHANNEL_DIFF(0, 1, 8), + ADS8344_VOLTAGE_CHANNEL_DIFF(2, 3, 9), + ADS8344_VOLTAGE_CHANNEL_DIFF(4, 5, 10), + ADS8344_VOLTAGE_CHANNEL_DIFF(6, 7, 11), + ADS8344_VOLTAGE_CHANNEL_DIFF(1, 0, 12), + ADS8344_VOLTAGE_CHANNEL_DIFF(3, 2, 13), + ADS8344_VOLTAGE_CHANNEL_DIFF(5, 4, 14), + ADS8344_VOLTAGE_CHANNEL_DIFF(7, 6, 15), +}; + +static int ads8344_adc_conversion(struct ads8344 *adc, int channel, + bool differential) +{ + struct spi_device *spi = adc->spi; + int ret; + + adc->tx_buf = ADS8344_START; + if (!differential) + adc->tx_buf |= ADS8344_SINGLE_END; + adc->tx_buf |= ADS8344_CHANNEL(channel); + adc->tx_buf |= ADS8344_CLOCK_INTERNAL; + + ret = spi_write(spi, &adc->tx_buf, 1); + if (ret) + return ret; + + udelay(9); + + ret = spi_read(spi, &adc->rx_buf, 2); + if (ret) + return ret; + + return adc->rx_buf; +} + +static int ads8344_read_raw(struct iio_dev *iio, + struct iio_chan_spec const *channel, int *value, + int *shift, long mask) +{ + struct ads8344 *adc = iio_priv(iio); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + mutex_lock(&adc->lock); + *value = ads8344_adc_conversion(adc, channel->scan_index, + channel->differential); + mutex_unlock(&adc->lock); + if (*value < 0) + return *value; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *value = regulator_get_voltage(adc->reg); + if (*value < 0) + return *value; + + /* convert regulator output voltage to mV */ + *value /= 1000; + *shift = 16; + + return IIO_VAL_FRACTIONAL_LOG2; + default: + return -EINVAL; + } +} + +static const struct iio_info ads8344_info = { + .read_raw = ads8344_read_raw, +}; + +static int ads8344_probe(struct spi_device *spi) +{ + struct iio_dev *indio_dev; + struct ads8344 *adc; + int ret; + + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); + if (!indio_dev) + return -ENOMEM; + + adc = iio_priv(indio_dev); + adc->spi = spi; + mutex_init(&adc->lock); + + indio_dev->name = dev_name(&spi->dev); + indio_dev->dev.parent = &spi->dev; + indio_dev->dev.of_node = spi->dev.of_node; + indio_dev->info = &ads8344_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = ads8344_channels; + indio_dev->num_channels = ARRAY_SIZE(ads8344_channels); + + adc->reg = devm_regulator_get(&spi->dev, "vref"); + if (IS_ERR(adc->reg)) + return PTR_ERR(adc->reg); + + ret = regulator_enable(adc->reg); + if (ret) + return ret; + + spi_set_drvdata(spi, indio_dev); + + ret = iio_device_register(indio_dev); + if (ret) { + regulator_disable(adc->reg); + return ret; + } + + return 0; +} + +static int ads8344_remove(struct spi_device *spi) +{ + struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct ads8344 *adc = iio_priv(indio_dev); + + iio_device_unregister(indio_dev); + regulator_disable(adc->reg); + + return 0; +} + +static const struct of_device_id ads8344_of_match[] = { + { .compatible = "ti,ads8344", }, + {} +}; +MODULE_DEVICE_TABLE(of, ads8344_dt_ids); + +static struct spi_driver ads8344_driver = { + .driver = { + .name = "ads8344", + .of_match_table = ads8344_of_match, + }, + .probe = ads8344_probe, + .remove = ads8344_remove, +}; +module_spi_driver(ads8344_driver); + +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@bootlin.com>"); +MODULE_DESCRIPTION("ADS8344 driver"); +MODULE_LICENSE("GPL v2"); -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] iio: adc: Add driver for the TI ADS8344 A/DC chips 2019-03-22 11:11 ` [PATCH v2 2/2] iio: adc: Add driver for the " Gregory CLEMENT @ 2019-03-24 15:16 ` Himanshu Jha 2019-03-24 16:01 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Himanshu Jha @ 2019-03-24 15:16 UTC (permalink / raw) To: Gregory CLEMENT, Jonathan Cameron Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, Rob Herring, devicetree, Thomas Petazzoni, linux-arm-kernel, Alexandre Belloni On Fri, Mar 22, 2019 at 12:11:08PM +0100, Gregory CLEMENT wrote: > This adds support for the Texas Instruments ADS8344 ADC chip. This chip > has a 16-bit 8-Channel ADC and is access directly through SPI. > > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> > --- > drivers/iio/adc/Kconfig | 10 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ti-ads8344.c | 200 +++++++++++++++++++++++++++++++++++ > 3 files changed, 211 insertions(+) > create mode 100644 drivers/iio/adc/ti-ads8344.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 76db6e5cc296..447d3a871746 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -967,6 +967,16 @@ config TI_ADS7950 > To compile this driver as a module, choose M here: the > module will be called ti-ads7950. > > +config TI_ADS8344 > + tristate "Texas Instruments ADS8344" > + depends on SPI && OF > + help > + If you say yes here you get support for Texas Instruments ADS8344 > + ADC chips > + > + This driver can also be built as a module. If so, the module will be > + called ti-ads8344. > + > config TI_ADS8688 > tristate "Texas Instruments ADS8688" > depends on SPI && OF > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 6fcebd167524..1f3ae934111d 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -87,6 +87,7 @@ obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o > obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o > obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o > obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o > +obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o > obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o > obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o > obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o > diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c > new file mode 100644 > index 000000000000..649ed05bf1c9 > --- /dev/null > +++ b/drivers/iio/adc/ti-ads8344.c > @@ -0,0 +1,200 @@ > +// SPDX-License-Identifier: GPL-2.0+ Inconsistency in License. > > + * ADS8344 16-bit 8-Channel ADC driver > + * > + * Author: Gregory CLEMENT <gregory.clement@bootlin.com> > + * > + * Datasheet: http://www.ti.com/lit/ds/symlink/ads8344.pdf > + */ > + > +#include <linux/delay.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> > +#include <linux/module.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > + > +#define ADS8344_START BIT(7) > +#define ADS8344_SINGLE_END BIT(2) > +#define ADS8344_CHANNEL(channel) ((channel) << 4) > +#define ADS8344_CLOCK_INTERNAL 0x2 /* PD1 = 1 and PD0 = 0 */ > + > +struct ads8344 { > + struct spi_device *spi; > + struct regulator *reg; > + struct mutex lock; This requires a comment explaining its purpose. checkpatch issues a warning IIRC. > + > + u8 tx_buf ____cacheline_aligned; > + u16 rx_buf; > +}; > + > +#define ADS8344_VOLTAGE_CHANNEL(chan, si) \ > + { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = chan, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + } > + > +#define ADS8344_VOLTAGE_CHANNEL_DIFF(chan1, chan2, si) \ > + { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = (chan1), \ > + .channel2 = (chan2), \ > + .differential = 1, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + } > + > +static const struct iio_chan_spec ads8344_channels[] = { > + ADS8344_VOLTAGE_CHANNEL(0, 0), > + ADS8344_VOLTAGE_CHANNEL(1, 4), > + ADS8344_VOLTAGE_CHANNEL(2, 1), > + ADS8344_VOLTAGE_CHANNEL(3, 5), > + ADS8344_VOLTAGE_CHANNEL(4, 2), > + ADS8344_VOLTAGE_CHANNEL(5, 6), > + ADS8344_VOLTAGE_CHANNEL(6, 3), > + ADS8344_VOLTAGE_CHANNEL(7, 7), > + ADS8344_VOLTAGE_CHANNEL_DIFF(0, 1, 8), > + ADS8344_VOLTAGE_CHANNEL_DIFF(2, 3, 9), > + ADS8344_VOLTAGE_CHANNEL_DIFF(4, 5, 10), > + ADS8344_VOLTAGE_CHANNEL_DIFF(6, 7, 11), > + ADS8344_VOLTAGE_CHANNEL_DIFF(1, 0, 12), > + ADS8344_VOLTAGE_CHANNEL_DIFF(3, 2, 13), > + ADS8344_VOLTAGE_CHANNEL_DIFF(5, 4, 14), > + ADS8344_VOLTAGE_CHANNEL_DIFF(7, 6, 15), > +}; > + > +static int ads8344_adc_conversion(struct ads8344 *adc, int channel, > + bool differential) > +{ > + struct spi_device *spi = adc->spi; > + int ret; > + > + adc->tx_buf = ADS8344_START; > + if (!differential) > + adc->tx_buf |= ADS8344_SINGLE_END; > + adc->tx_buf |= ADS8344_CHANNEL(channel); > + adc->tx_buf |= ADS8344_CLOCK_INTERNAL; > + > + ret = spi_write(spi, &adc->tx_buf, 1); > + if (ret) > + return ret; > + > + udelay(9); > + > + ret = spi_read(spi, &adc->rx_buf, 2); > + if (ret) > + return ret; > + > + return adc->rx_buf; > +} > + > +static int ads8344_read_raw(struct iio_dev *iio, > + struct iio_chan_spec const *channel, int *value, > + int *shift, long mask) > +{ > + struct ads8344 *adc = iio_priv(iio); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&adc->lock); Just curious: What would happen if you don't lock ? I'm interested in looking for whatever happens in such a case and how to exploit it ? Also, bus transactions are often atomic using their locking/unlocking procedures. So, why do we need locking procedures in iio drivers itself ? > + *value = ads8344_adc_conversion(adc, channel->scan_index, > + channel->differential); > + mutex_unlock(&adc->lock); > + if (*value < 0) > + return *value; > + > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + *value = regulator_get_voltage(adc->reg); > + if (*value < 0) > + return *value; > + > + /* convert regulator output voltage to mV */ > + *value /= 1000; > + *shift = 16; > + > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info ads8344_info = { > + .read_raw = ads8344_read_raw, > +}; > + > +static int ads8344_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct ads8344 *adc; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > + if (!indio_dev) > + return -ENOMEM; > + > + adc = iio_priv(indio_dev); > + adc->spi = spi; > + mutex_init(&adc->lock); > + > + indio_dev->name = dev_name(&spi->dev); > + indio_dev->dev.parent = &spi->dev; > + indio_dev->dev.of_node = spi->dev.of_node; > + indio_dev->info = &ads8344_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = ads8344_channels; > + indio_dev->num_channels = ARRAY_SIZE(ads8344_channels); > + > + adc->reg = devm_regulator_get(&spi->dev, "vref"); > + if (IS_ERR(adc->reg)) > + return PTR_ERR(adc->reg); > + > + ret = regulator_enable(adc->reg); > + if (ret) > + return ret; > + > + spi_set_drvdata(spi, indio_dev); > + > + ret = iio_device_register(indio_dev); > + if (ret) { > + regulator_disable(adc->reg); > + return ret; > + } IDK but it is advised not to mix devm_* with regular functions. If there is any possibilty to use `devm_add_action_or_reset` here ? This would help get rid of `ads8344_remove` and help smooth unwinding in failure w/o any race. > + return 0; > +} > + > +static int ads8344_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct ads8344 *adc = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + regulator_disable(adc->reg); > + > + return 0; > +} > + > +static const struct of_device_id ads8344_of_match[] = { > + { .compatible = "ti,ads8344", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, ads8344_dt_ids); > + > +static struct spi_driver ads8344_driver = { > + .driver = { > + .name = "ads8344", > + .of_match_table = ads8344_of_match, > + }, > + .probe = ads8344_probe, > + .remove = ads8344_remove, > +}; > +module_spi_driver(ads8344_driver); > + > +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@bootlin.com>"); > +MODULE_DESCRIPTION("ADS8344 driver"); > +MODULE_LICENSE("GPL v2"); this is a mismatch. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] iio: adc: Add driver for the TI ADS8344 A/DC chips 2019-03-24 15:16 ` Himanshu Jha @ 2019-03-24 16:01 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2019-03-24 16:01 UTC (permalink / raw) To: Himanshu Jha Cc: Gregory CLEMENT, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, Rob Herring, devicetree, Thomas Petazzoni, linux-arm-kernel, Alexandre Belloni On Sun, 24 Mar 2019 20:46:25 +0530 Himanshu Jha <himanshujha199640@gmail.com> wrote: > On Fri, Mar 22, 2019 at 12:11:08PM +0100, Gregory CLEMENT wrote: > > This adds support for the Texas Instruments ADS8344 ADC chip. This chip > > has a 16-bit 8-Channel ADC and is access directly through SPI. > > > > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> Hi Gregory / Himanshu, Comments inline, though mostly addressing Himanshu's questions as I was here ;) I'm happy with the code, though up to you if you want to look at taking everything to device managed allocators. Just the license issue that is stopping me applying this. Jonathan > > --- > > drivers/iio/adc/Kconfig | 10 ++ > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/ti-ads8344.c | 200 +++++++++++++++++++++++++++++++++++ > > 3 files changed, 211 insertions(+) > > create mode 100644 drivers/iio/adc/ti-ads8344.c > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index 76db6e5cc296..447d3a871746 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -967,6 +967,16 @@ config TI_ADS7950 > > To compile this driver as a module, choose M here: the > > module will be called ti-ads7950. > > > > +config TI_ADS8344 > > + tristate "Texas Instruments ADS8344" > > + depends on SPI && OF > > + help > > + If you say yes here you get support for Texas Instruments ADS8344 > > + ADC chips > > + > > + This driver can also be built as a module. If so, the module will be > > + called ti-ads8344. > > + > > config TI_ADS8688 > > tristate "Texas Instruments ADS8688" > > depends on SPI && OF > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > index 6fcebd167524..1f3ae934111d 100644 > > --- a/drivers/iio/adc/Makefile > > +++ b/drivers/iio/adc/Makefile > > @@ -87,6 +87,7 @@ obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o > > obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o > > obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o > > obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o > > +obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o > > obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o > > obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o > > obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o > > diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c > > new file mode 100644 > > index 000000000000..649ed05bf1c9 > > --- /dev/null > > +++ b/drivers/iio/adc/ti-ads8344.c > > @@ -0,0 +1,200 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > Inconsistency in License. > Good spot. That alone means we are going to need a v3. > > > > + * ADS8344 16-bit 8-Channel ADC driver > > + * > > + * Author: Gregory CLEMENT <gregory.clement@bootlin.com> > > + * > > + * Datasheet: http://www.ti.com/lit/ds/symlink/ads8344.pdf > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/iio.h> > > +#include <linux/module.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/spi/spi.h> > > + > > +#define ADS8344_START BIT(7) > > +#define ADS8344_SINGLE_END BIT(2) > > +#define ADS8344_CHANNEL(channel) ((channel) << 4) > > +#define ADS8344_CLOCK_INTERNAL 0x2 /* PD1 = 1 and PD0 = 0 */ > > + > > +struct ads8344 { > > + struct spi_device *spi; > > + struct regulator *reg; > > + struct mutex lock; > > This requires a comment explaining its purpose. > checkpatch issues a warning IIRC. In this particular case that might have answered the question you have below! > > > + > > + u8 tx_buf ____cacheline_aligned; > > + u16 rx_buf; > > +}; > > + > > +#define ADS8344_VOLTAGE_CHANNEL(chan, si) \ > > + { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .channel = chan, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + } > > + > > +#define ADS8344_VOLTAGE_CHANNEL_DIFF(chan1, chan2, si) \ > > + { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .channel = (chan1), \ > > + .channel2 = (chan2), \ > > + .differential = 1, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + } > > + > > +static const struct iio_chan_spec ads8344_channels[] = { > > + ADS8344_VOLTAGE_CHANNEL(0, 0), > > + ADS8344_VOLTAGE_CHANNEL(1, 4), > > + ADS8344_VOLTAGE_CHANNEL(2, 1), > > + ADS8344_VOLTAGE_CHANNEL(3, 5), > > + ADS8344_VOLTAGE_CHANNEL(4, 2), > > + ADS8344_VOLTAGE_CHANNEL(5, 6), > > + ADS8344_VOLTAGE_CHANNEL(6, 3), > > + ADS8344_VOLTAGE_CHANNEL(7, 7), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(0, 1, 8), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(2, 3, 9), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(4, 5, 10), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(6, 7, 11), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(1, 0, 12), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(3, 2, 13), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(5, 4, 14), > > + ADS8344_VOLTAGE_CHANNEL_DIFF(7, 6, 15), > > +}; > > + > > +static int ads8344_adc_conversion(struct ads8344 *adc, int channel, > > + bool differential) > > +{ > > + struct spi_device *spi = adc->spi; > > + int ret; > > + > > + adc->tx_buf = ADS8344_START; > > + if (!differential) > > + adc->tx_buf |= ADS8344_SINGLE_END; > > + adc->tx_buf |= ADS8344_CHANNEL(channel); > > + adc->tx_buf |= ADS8344_CLOCK_INTERNAL; > > + > > + ret = spi_write(spi, &adc->tx_buf, 1); > > + if (ret) > > + return ret; > > + > > + udelay(9); > > + > > + ret = spi_read(spi, &adc->rx_buf, 2); > > + if (ret) > > + return ret; > > + > > + return adc->rx_buf; > > +} > > + > > +static int ads8344_read_raw(struct iio_dev *iio, > > + struct iio_chan_spec const *channel, int *value, > > + int *shift, long mask) > > +{ > > + struct ads8344 *adc = iio_priv(iio); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + mutex_lock(&adc->lock); > > Just curious: > What would happen if you don't lock ? Well as I'm here I'll answer. Key here is that there is nothing stopping two concurrent reads of a sysfs file. In this particular case the lock is protecting adc->tx_buff and rx_buff. As a simple example, imagine two calls are reading different channels. As they can race, it's possible both reads occur such that we either do two bus reads with the same channel set, or we end up doing one read after the other, but both functions end up using the same buffer contents. Upshot is that we end up reading the wrong channel. > > I'm interested in looking for whatever happens in such a case > and how to exploit it ? > > Also, bus transactions are often atomic using their locking/unlocking > procedures. So, why do we need locking procedures in iio drivers itself ? Yes, this could be avoided, but only at the cost of separate buffers, made tricky by the dma alignment requirements that mean we can't use them off the stack. One option would be to use spi_write_then_read, which uses the SPI bus locking + some local buffers thus avoiding the problem. However the need to have a sleep between them stops that option. You can think of it as the bus locks prevent multiple users of the bus (which may be different drivers) and the IIO locks prevent multiple users of IIO device instance specific data. Hence there is no nice way to do it with a single lock as they have different / overlapping scopes. > > > + *value = ads8344_adc_conversion(adc, channel->scan_index, > > + channel->differential); > > + mutex_unlock(&adc->lock); > > + if (*value < 0) > > + return *value; > > + > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + *value = regulator_get_voltage(adc->reg); > > + if (*value < 0) > > + return *value; > > + > > + /* convert regulator output voltage to mV */ > > + *value /= 1000; > > + *shift = 16; > > + > > + return IIO_VAL_FRACTIONAL_LOG2; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static const struct iio_info ads8344_info = { > > + .read_raw = ads8344_read_raw, > > +}; > > + > > +static int ads8344_probe(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev; > > + struct ads8344 *adc; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + adc = iio_priv(indio_dev); > > + adc->spi = spi; > > + mutex_init(&adc->lock); > > + > > + indio_dev->name = dev_name(&spi->dev); > > + indio_dev->dev.parent = &spi->dev; > > + indio_dev->dev.of_node = spi->dev.of_node; > > + indio_dev->info = &ads8344_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = ads8344_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ads8344_channels); > > + > > + adc->reg = devm_regulator_get(&spi->dev, "vref"); > > + if (IS_ERR(adc->reg)) > > + return PTR_ERR(adc->reg); > > + > > + ret = regulator_enable(adc->reg); > > + if (ret) > > + return ret; > > + > > + spi_set_drvdata(spi, indio_dev); > > + > > + ret = iio_device_register(indio_dev); > > + if (ret) { > > + regulator_disable(adc->reg); > > + return ret; > > + } > > IDK but it is advised not to mix devm_* with regular functions. > > If there is any possibilty to use `devm_add_action_or_reset` here ? > > This would help get rid of `ads8344_remove` and help smooth > unwinding in failure w/o any race. There is no problem as long as you keep in mind that devm_ unwinding occurs after remove occurs. That means that, to maintain ordering of remove being the opposite of probe, once you hit something that needs unwinding manually, you should use the none devm functions for everything after it. Hence I think what we have here is correct. The devm_add_action_or_reset might be worthwhile to avoid the need to manually unwind anything at all though. > > > + return 0; > > +} > > + > > +static int ads8344_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct ads8344 *adc = iio_priv(indio_dev); > > + > > + iio_device_unregister(indio_dev); > > + regulator_disable(adc->reg); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id ads8344_of_match[] = { > > + { .compatible = "ti,ads8344", }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, ads8344_dt_ids); > > + > > +static struct spi_driver ads8344_driver = { > > + .driver = { > > + .name = "ads8344", > > + .of_match_table = ads8344_of_match, > > + }, > > + .probe = ads8344_probe, > > + .remove = ads8344_remove, > > +}; > > +module_spi_driver(ads8344_driver); > > + > > +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@bootlin.com>"); > > +MODULE_DESCRIPTION("ADS8344 driver"); > > > > +MODULE_LICENSE("GPL v2"); > > this is a mismatch. > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-31 6:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-22 11:11 [PATCH v2 0/2] Add support for the TI ADS8344 A/DC chips Gregory CLEMENT 2019-03-22 11:11 ` [PATCH v2 1/2] dt-bindings: iio: adc: Add bindings for " Gregory CLEMENT 2019-03-31 6:41 ` Rob Herring 2019-03-22 11:11 ` [PATCH v2 2/2] iio: adc: Add driver for the " Gregory CLEMENT 2019-03-24 15:16 ` Himanshu Jha 2019-03-24 16:01 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).