| Message ID | 20230825-ltc2309-v2-2-6d75f2b3fb50@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: add LTC2309 support | expand |
Hi Liam, kernel test robot noticed the following build warnings: [auto build test WARNING on a5e505a99ca748583dbe558b691be1b26f05d678] url: https://github.com/intel-lab-lkp/linux/commits/Liam-Beguin/dt-bindings-iio-adc-add-lltc-ltc2309-bindings/20230826-022412 base: a5e505a99ca748583dbe558b691be1b26f05d678 patch link: https://lore.kernel.org/r/20230825-ltc2309-v2-2-6d75f2b3fb50%40gmail.com patch subject: [PATCH v2 2/2] iio: adc: add ltc2309 support config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230826/202308260324.RYZ1IVWw-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260324.RYZ1IVWw-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308260324.RYZ1IVWw-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/iio/adc/ltc2309.c:163:6: warning: no previous prototype for 'ltc2309_regulator_disable' [-Wmissing-prototypes] 163 | void ltc2309_regulator_disable(void *regulator) | ^~~~~~~~~~~~~~~~~~~~~~~~~ vim +/ltc2309_regulator_disable +163 drivers/iio/adc/ltc2309.c 162 > 163 void ltc2309_regulator_disable(void *regulator) 164 { 165 struct regulator *r = (struct regulator *)regulator; 166 167 regulator_disable(r); 168 } 169
On Fri, 25 Aug 2023 14:20:59 -0400 Liam Beguin <liambeguin@gmail.com> wrote: > The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface. > > This implements support for all single-ended and differential channels, > in unipolar mode only. > > Signed-off-by: Liam Beguin <liambeguin@gmail.com> Hi Liam, Some comments inline. Thanks, Jonathan > --- > drivers/iio/adc/Kconfig | 10 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ltc2309.c | 248 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 259 insertions(+) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index dc14bde31ac1..6ec18e02faf9 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -607,6 +607,16 @@ config LPC32XX_ADC > activate only one via device tree selection. Provides direct access > via sysfs. > > +config LTC2309 > + tristate "Linear Technology LTC2309 ADC driver" > + depends on I2C > + help > + Say yes here to build support for Linear Technology LTC2309, a low > + noise, low power, 8-channel, 12-bit SAR ADC > + > + This driver can also be built as a module. If so, the module will > + be called ltc2309. > + > config LTC2471 > tristate "Linear Technology LTC2471 and LTC2473 ADC driver" > depends on I2C > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index eb6e891790fb..fbd86184ec94 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o > obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o > obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o > +obj-$(CONFIG_LTC2309) += ltc2309.o > obj-$(CONFIG_LTC2471) += ltc2471.o > obj-$(CONFIG_LTC2485) += ltc2485.o > obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o > diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c > new file mode 100644 > index 000000000000..145f3c63d157 > --- /dev/null > +++ b/drivers/iio/adc/ltc2309.c > @@ -0,0 +1,248 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface. > + * > + * Datasheet: > + * https://www.analog.com/media/en/technical-documentation/data-sheets/2309fd.pdf > + * > + * Copyright (c) 2023, Liam Beguin <liambeguin@gmail.com> Blank line here doesn't add anything useful. I'll drop it if not much else comes up. > + * > + */ > +#include <linux/bitfield.h> > +#include <linux/i2c.h> > +#include <linux/iio/iio.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/regulator/consumer.h> > + > +#define DRIVER_NAME "ltc2309" I'd rather see this string directly in line within the code. In general the places you used it could have different strings, so it's much nicer just to see the string itself where it is used. > +#define LTC2309_ADC_RESOLUTION 12 > + > +#define LTC2309_DIN_CH_MASK GENMASK(7, 4) > +#define LTC2309_DIN_SDN BIT(7) > +#define LTC2309_DIN_OSN BIT(6) > +#define LTC2309_DIN_S1 BIT(5) > +#define LTC2309_DIN_S0 BIT(4) > +#define LTC2309_DIN_UNI BIT(3) > +#define LTC2309_DIN_SLEEP BIT(2) > + > +/* struct ltc2309 - internal device data structure /* * struct ltc2039 for comments in IIO. Also, this is kernel doc so should be /** * struct ... > + * > + * @dev: Device reference > + * @client: I2C reference > + * @vref: External reference source > + * @lock: Lock to serialize data access > + * @vref_mv Internal voltage reference > + */ > +struct ltc2309 { > + struct device *dev; > + struct i2c_client *client; > + struct regulator *vref; > + struct mutex lock; /* serialize data access */ > + int vref_mv; > +}; > + > +/* Order matches expected channel address, See datasheet Table 1. */ > +enum ltc2309_channels { > + LTC2309_CH0_CH1 = 0, > + LTC2309_CH2_CH3, > + LTC2309_CH4_CH5, > + LTC2309_CH6_CH7, > + LTC2309_CH1_CH0, > + LTC2309_CH3_CH2, > + LTC2309_CH5_CH4, > + LTC2309_CH7_CH6, > + LTC2309_CH0, > + LTC2309_CH2, > + LTC2309_CH4, > + LTC2309_CH6, > + LTC2309_CH1, > + LTC2309_CH3, > + LTC2309_CH5, > + LTC2309_CH7, > +}; > + > +#define LTC2309_CHAN(_chan, _addr) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .address = _addr, \ > + .channel = _chan, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .datasheet_name = "CH" #_chan, \ I'd not bother providing a datasheet name it it's so directly related to the channel index which is readily available. It just adds ABI that is fairly pointless as it isn't conveying any information. > +} > + > +#define LTC2309_DIFF_CHAN(_chan, _chan2, _addr) { \ > + .type = IIO_VOLTAGE, \ > + .differential = 1, \ > + .indexed = 1, \ > + .address = _addr, \ > + .channel = _chan, \ > + .channel2 = _chan2, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .datasheet_name = "CH" #_chan "-CH" #_chan2, \ As above - this is obvious anyway, I'd drop the datasheet_name. It's optional so just don't provide it ;) > +} > + > +static const struct iio_chan_spec ltc2309_channels[] = { > + LTC2309_CHAN(0, LTC2309_CH0), > + LTC2309_CHAN(1, LTC2309_CH1), > + LTC2309_CHAN(2, LTC2309_CH2), > + LTC2309_CHAN(3, LTC2309_CH3), > + LTC2309_CHAN(4, LTC2309_CH4), > + LTC2309_CHAN(5, LTC2309_CH5), > + LTC2309_CHAN(6, LTC2309_CH6), > + LTC2309_CHAN(7, LTC2309_CH7), > + LTC2309_DIFF_CHAN(0, 1, LTC2309_CH0_CH1), > + LTC2309_DIFF_CHAN(2, 3, LTC2309_CH2_CH3), > + LTC2309_DIFF_CHAN(4, 5, LTC2309_CH4_CH5), > + LTC2309_DIFF_CHAN(6, 7, LTC2309_CH6_CH7), > + LTC2309_DIFF_CHAN(1, 0, LTC2309_CH1_CH0), > + LTC2309_DIFF_CHAN(3, 2, LTC2309_CH3_CH2), > + LTC2309_DIFF_CHAN(5, 4, LTC2309_CH5_CH4), > + LTC2309_DIFF_CHAN(7, 6, LTC2309_CH7_CH6), > +}; > + > +static int ltc2309_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct ltc2309 *ltc2309 = iio_priv(indio_dev); > + u16 buf; > + int ret; > + u8 din; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + din = FIELD_PREP(LTC2309_DIN_CH_MASK, chan->address & 0x0f) | > + FIELD_PREP(LTC2309_DIN_UNI, 1) | > + FIELD_PREP(LTC2309_DIN_SLEEP, 0); > + > + mutex_lock(<c2309->lock); > + ret = i2c_smbus_write_byte(ltc2309->client, din); > + if (ret < 0) { > + dev_err(ltc2309->dev, "i2c command failed: %pe\n", > + ERR_PTR(ret)); > + goto out; This sort of messy mutex handling for error paths is a strong signal that the code would be better with a little utility function around which you take the locks. mutex_lock(<c2309->lock); ret = ltc2309_read_raw(...); // move the field prep in there as well even though it expands the scope a tiny bit mutex_unlock() if (ret) ... carry on. > + } > + > + ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2); > + if (ret < 0) { > + dev_err(ltc2309->dev, "i2c read failed: %pe\n", > + ERR_PTR(ret)); > + goto out; > + } > + > + *val = be16_to_cpu(buf) >> 4; This doesn't really need to be under the lock, but if it makes sense to do it in the utility function I suggest above then that's fine. > + mutex_unlock(<c2309->lock); > + > + ret = IIO_VAL_INT; return IIO_VAL_INT; > + break; > + case IIO_CHAN_INFO_SCALE: > + *val = ltc2309->vref_mv; > + *val2 = LTC2309_ADC_RESOLUTION; > + ret = IIO_VAL_FRACTIONAL_LOG2; return IIO_VAL_FRACTIONAL_LOG2 and get rid of the rbeadk. > + break; > + default: > + ret = -EINVAL; return -EINVAL; > + break; > + } > + > + return ret; > + > +out: > + mutex_unlock(<c2309->lock); > + return ret; > +} > + > +static const struct iio_info ltc2309_info = { > + .read_raw = ltc2309_read_raw, > +}; > + > +void ltc2309_regulator_disable(void *regulator) > +{ > + struct regulator *r = (struct regulator *)regulator; > + > + regulator_disable(r); > +} > + > +static int ltc2309_probe(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev; > + struct ltc2309 *ltc2309; > + int ret = 0; Always set in paths where it is used so don't set it here. > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ltc2309)); > + if (!indio_dev) > + return -ENOMEM; > + > + i2c_set_clientdata(client, indio_dev); Is this used? > + > + ltc2309 = iio_priv(indio_dev); > + ltc2309->dev = &indio_dev->dev; > + ltc2309->client = client; > + ltc2309->vref_mv = 4096; /* Default to the internal ref */ > + > + indio_dev->name = DRIVER_NAME; > + indio_dev->dev.parent = &client->dev; That's not been needed for quite a long time as devm_iio_device_alloc() sets it. Ultimately it's here: https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-core.c#L1645 > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = ltc2309_channels; > + indio_dev->num_channels = ARRAY_SIZE(ltc2309_channels); > + indio_dev->info = <c2309_info; > + > + ltc2309->vref = devm_regulator_get_optional(&client->dev, "vref"); > + if (!IS_ERR_OR_NULL(ltc2309->vref)) { What if you get a request to defer? To use devm_regulator_get_optional() and detect the cases where the regulator is not provided separately from errors during the probe. IIRC check for PTR_ERR(-ENODEV) as meaning one wasn't supplied. However do check I have that right. > + ret = regulator_enable(ltc2309->vref); > + if (ret) { > + dev_err(ltc2309->dev, "failed to enable vref\n"); > + return ret; dev_err_probe() for all error messages in a probe function. > + } > + > + ret = devm_add_action_or_reset(ltc2309->dev, > + ltc2309_regulator_disable, > + ltc2309->vref); > + if (ret) { > + dev_err(ltc2309->dev, > + "failed to add regulator_disable action: %d\n", > + ret); return dev_err_probe() > + return ret; > + } > + > + ret = regulator_get_voltage(ltc2309->vref); > + if (ret < 0) > + return ret; > + > + ltc2309->vref_mv = ret / 1000; > + } > + > + mutex_init(<c2309->lock); > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > +static const struct of_device_id ltc2309_of_match[] = { > + { .compatible = "lltc,ltc2309" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ltc2309_of_match); > + > +static const struct i2c_device_id ltc2309_id[] = { > + {"ltc2309", 0}, Don't provide data unless it's doing anything useful. Also, consistency on spacing after { and before } > + {}, No comma on a list terminator like this one as nothing can come after it that isn't a bug. > +}; > +MODULE_DEVICE_TABLE(i2c, ltc2309_id); > + > +static struct i2c_driver ltc2309_driver = { > + .driver = { > + .name = DRIVER_NAME, > + .of_match_table = ltc2309_of_match, > + }, > + .probe = ltc2309_probe, > + .id_table = ltc2309_id, > +}; > +module_i2c_driver(ltc2309_driver); > + > +MODULE_AUTHOR("Liam Beguin <liambeguin@gmail.com>"); > +MODULE_DESCRIPTION("Linear Technology LTC2309 ADC"); > +MODULE_LICENSE("GPL v2"); >
On Sun, Aug 27, 2023 at 06:45:42PM +0100, Jonathan Cameron wrote: > On Fri, 25 Aug 2023 14:20:59 -0400 > Liam Beguin <liambeguin@gmail.com> wrote: > > > The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface. > > > > This implements support for all single-ended and differential channels, > > in unipolar mode only. > > > > Signed-off-by: Liam Beguin <liambeguin@gmail.com> > Hi Liam, > > Some comments inline. > > Thanks, > > Jonathan Hi Jonathan, Thanks for reviewing, I'll address your comments and resend shortly. Liam > > --- > > drivers/iio/adc/Kconfig | 10 ++ > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/ltc2309.c | 248 ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 259 insertions(+) > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index dc14bde31ac1..6ec18e02faf9 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -607,6 +607,16 @@ config LPC32XX_ADC > > activate only one via device tree selection. Provides direct access > > via sysfs. > > > > +config LTC2309 > > + tristate "Linear Technology LTC2309 ADC driver" > > + depends on I2C > > + help > > + Say yes here to build support for Linear Technology LTC2309, a low > > + noise, low power, 8-channel, 12-bit SAR ADC > > + > > + This driver can also be built as a module. If so, the module will > > + be called ltc2309. > > + > > config LTC2471 > > tristate "Linear Technology LTC2471 and LTC2473 ADC driver" > > depends on I2C > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > index eb6e891790fb..fbd86184ec94 100644 > > --- a/drivers/iio/adc/Makefile > > +++ b/drivers/iio/adc/Makefile > > @@ -56,6 +56,7 @@ obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o > > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o > > obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o > > obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o > > +obj-$(CONFIG_LTC2309) += ltc2309.o > > obj-$(CONFIG_LTC2471) += ltc2471.o > > obj-$(CONFIG_LTC2485) += ltc2485.o > > obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o > > diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c > > new file mode 100644 > > index 000000000000..145f3c63d157 > > --- /dev/null > > +++ b/drivers/iio/adc/ltc2309.c > > @@ -0,0 +1,248 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface. > > + * > > + * Datasheet: > > + * https://www.analog.com/media/en/technical-documentation/data-sheets/2309fd.pdf > > + * > > + * Copyright (c) 2023, Liam Beguin <liambeguin@gmail.com> > > Blank line here doesn't add anything useful. I'll drop it if not much else > comes up. > > > + * > > + */ > > +#include <linux/bitfield.h> > > +#include <linux/i2c.h> > > +#include <linux/iio/iio.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/regulator/consumer.h> > > + > > +#define DRIVER_NAME "ltc2309" > I'd rather see this string directly in line within the code. > In general the places you used it could have different strings, so it's > much nicer just to see the string itself where it is used. > > > +#define LTC2309_ADC_RESOLUTION 12 > > + > > +#define LTC2309_DIN_CH_MASK GENMASK(7, 4) > > +#define LTC2309_DIN_SDN BIT(7) > > +#define LTC2309_DIN_OSN BIT(6) > > +#define LTC2309_DIN_S1 BIT(5) > > +#define LTC2309_DIN_S0 BIT(4) > > +#define LTC2309_DIN_UNI BIT(3) > > +#define LTC2309_DIN_SLEEP BIT(2) > > + > > +/* struct ltc2309 - internal device data structure > /* > * struct ltc2039 > > > for comments in IIO. > > Also, this is kernel doc so should be > /** > * struct ... > > > + * > > + * @dev: Device reference > > + * @client: I2C reference > > + * @vref: External reference source > > + * @lock: Lock to serialize data access > > + * @vref_mv Internal voltage reference > > + */ > > +struct ltc2309 { > > + struct device *dev; > > + struct i2c_client *client; > > + struct regulator *vref; > > + struct mutex lock; /* serialize data access */ > > + int vref_mv; > > +}; > > + > > +/* Order matches expected channel address, See datasheet Table 1. */ > > +enum ltc2309_channels { > > + LTC2309_CH0_CH1 = 0, > > + LTC2309_CH2_CH3, > > + LTC2309_CH4_CH5, > > + LTC2309_CH6_CH7, > > + LTC2309_CH1_CH0, > > + LTC2309_CH3_CH2, > > + LTC2309_CH5_CH4, > > + LTC2309_CH7_CH6, > > + LTC2309_CH0, > > + LTC2309_CH2, > > + LTC2309_CH4, > > + LTC2309_CH6, > > + LTC2309_CH1, > > + LTC2309_CH3, > > + LTC2309_CH5, > > + LTC2309_CH7, > > +}; > > + > > +#define LTC2309_CHAN(_chan, _addr) { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .address = _addr, \ > > + .channel = _chan, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + .datasheet_name = "CH" #_chan, \ > > I'd not bother providing a datasheet name it it's so directly related > to the channel index which is readily available. It just adds > ABI that is fairly pointless as it isn't conveying any information. > > > +} > > + > > +#define LTC2309_DIFF_CHAN(_chan, _chan2, _addr) { \ > > + .type = IIO_VOLTAGE, \ > > + .differential = 1, \ > > + .indexed = 1, \ > > + .address = _addr, \ > > + .channel = _chan, \ > > + .channel2 = _chan2, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + .datasheet_name = "CH" #_chan "-CH" #_chan2, \ > > As above - this is obvious anyway, I'd drop the datasheet_name. > It's optional so just don't provide it ;) > > > +} > > + > > +static const struct iio_chan_spec ltc2309_channels[] = { > > + LTC2309_CHAN(0, LTC2309_CH0), > > + LTC2309_CHAN(1, LTC2309_CH1), > > + LTC2309_CHAN(2, LTC2309_CH2), > > + LTC2309_CHAN(3, LTC2309_CH3), > > + LTC2309_CHAN(4, LTC2309_CH4), > > + LTC2309_CHAN(5, LTC2309_CH5), > > + LTC2309_CHAN(6, LTC2309_CH6), > > + LTC2309_CHAN(7, LTC2309_CH7), > > + LTC2309_DIFF_CHAN(0, 1, LTC2309_CH0_CH1), > > + LTC2309_DIFF_CHAN(2, 3, LTC2309_CH2_CH3), > > + LTC2309_DIFF_CHAN(4, 5, LTC2309_CH4_CH5), > > + LTC2309_DIFF_CHAN(6, 7, LTC2309_CH6_CH7), > > + LTC2309_DIFF_CHAN(1, 0, LTC2309_CH1_CH0), > > + LTC2309_DIFF_CHAN(3, 2, LTC2309_CH3_CH2), > > + LTC2309_DIFF_CHAN(5, 4, LTC2309_CH5_CH4), > > + LTC2309_DIFF_CHAN(7, 6, LTC2309_CH7_CH6), > > +}; > > + > > +static int ltc2309_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, int *val, > > + int *val2, long mask) > > +{ > > + struct ltc2309 *ltc2309 = iio_priv(indio_dev); > > + u16 buf; > > + int ret; > > + u8 din; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + din = FIELD_PREP(LTC2309_DIN_CH_MASK, chan->address & 0x0f) | > > + FIELD_PREP(LTC2309_DIN_UNI, 1) | > > + FIELD_PREP(LTC2309_DIN_SLEEP, 0); > > + > > + mutex_lock(<c2309->lock); > > + ret = i2c_smbus_write_byte(ltc2309->client, din); > > + if (ret < 0) { > > + dev_err(ltc2309->dev, "i2c command failed: %pe\n", > > + ERR_PTR(ret)); > > + goto out; > > This sort of messy mutex handling for error paths is a strong signal > that the code would be better with a little utility function around which > you take the locks. > > mutex_lock(<c2309->lock); > ret = ltc2309_read_raw(...); // move the field prep in there as well even though it expands the scope a tiny bit > mutex_unlock() > if (ret) > ... > > carry on. > > > + } > > + > > + ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2); > > + if (ret < 0) { > > + dev_err(ltc2309->dev, "i2c read failed: %pe\n", > > + ERR_PTR(ret)); > > + goto out; > > + } > > + > > + *val = be16_to_cpu(buf) >> 4; > This doesn't really need to be under the lock, but if it makes sense to > do it in the utility function I suggest above then that's fine. > > > + mutex_unlock(<c2309->lock); > > + > > + ret = IIO_VAL_INT; > > return IIO_VAL_INT; > > > + break; > > + case IIO_CHAN_INFO_SCALE: > > + *val = ltc2309->vref_mv; > > + *val2 = LTC2309_ADC_RESOLUTION; > > + ret = IIO_VAL_FRACTIONAL_LOG2; > return IIO_VAL_FRACTIONAL_LOG2 and get rid of the rbeadk. > > + break; > > + default: > > + ret = -EINVAL; > return -EINVAL; > > + break; > > + } > > + > > + return ret; > > + > > +out: > > + mutex_unlock(<c2309->lock); > > + return ret; > > +} > > + > > +static const struct iio_info ltc2309_info = { > > + .read_raw = ltc2309_read_raw, > > +}; > > + > > +void ltc2309_regulator_disable(void *regulator) > > +{ > > + struct regulator *r = (struct regulator *)regulator; > > + > > + regulator_disable(r); > > +} > > + > > +static int ltc2309_probe(struct i2c_client *client) > > +{ > > + struct iio_dev *indio_dev; > > + struct ltc2309 *ltc2309; > > + int ret = 0; > > Always set in paths where it is used so don't set it here. > > > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ltc2309)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + i2c_set_clientdata(client, indio_dev); > > Is this used? > > > + > > + ltc2309 = iio_priv(indio_dev); > > + ltc2309->dev = &indio_dev->dev; > > + ltc2309->client = client; > > + ltc2309->vref_mv = 4096; /* Default to the internal ref */ > > + > > + indio_dev->name = DRIVER_NAME; > > + indio_dev->dev.parent = &client->dev; > > That's not been needed for quite a long time as devm_iio_device_alloc() > sets it. Ultimately it's here: > https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-core.c#L1645 > > > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = ltc2309_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ltc2309_channels); > > + indio_dev->info = <c2309_info; > > + > > + ltc2309->vref = devm_regulator_get_optional(&client->dev, "vref"); > > + if (!IS_ERR_OR_NULL(ltc2309->vref)) { > > What if you get a request to defer? To use devm_regulator_get_optional() > and detect the cases where the regulator is not provided separately from > errors during the probe. IIRC check for PTR_ERR(-ENODEV) as meaning > one wasn't supplied. However do check I have that right. > > > + ret = regulator_enable(ltc2309->vref); > > + if (ret) { > > + dev_err(ltc2309->dev, "failed to enable vref\n"); > > + return ret; > > dev_err_probe() for all error messages in a probe function. > > > + } > > + > > + ret = devm_add_action_or_reset(ltc2309->dev, > > + ltc2309_regulator_disable, > > + ltc2309->vref); > > + if (ret) { > > + dev_err(ltc2309->dev, > > + "failed to add regulator_disable action: %d\n", > > + ret); > return dev_err_probe() > > > + return ret; > > + } > > + > > + ret = regulator_get_voltage(ltc2309->vref); > > + if (ret < 0) > > + return ret; > > + > > + ltc2309->vref_mv = ret / 1000; > > + } > > + > > + mutex_init(<c2309->lock); > > + > > + return devm_iio_device_register(&client->dev, indio_dev); > > +} > > + > > +static const struct of_device_id ltc2309_of_match[] = { > > + { .compatible = "lltc,ltc2309" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, ltc2309_of_match); > > + > > +static const struct i2c_device_id ltc2309_id[] = { > > + {"ltc2309", 0}, > > Don't provide data unless it's doing anything useful. > Also, consistency on spacing after { and before } > > > + {}, > > No comma on a list terminator like this one as nothing can come > after it that isn't a bug. > > > +}; > > +MODULE_DEVICE_TABLE(i2c, ltc2309_id); > > + > > +static struct i2c_driver ltc2309_driver = { > > + .driver = { > > + .name = DRIVER_NAME, > > + .of_match_table = ltc2309_of_match, > > + }, > > + .probe = ltc2309_probe, > > + .id_table = ltc2309_id, > > +}; > > +module_i2c_driver(ltc2309_driver); > > + > > +MODULE_AUTHOR("Liam Beguin <liambeguin@gmail.com>"); > > +MODULE_DESCRIPTION("Linear Technology LTC2309 ADC"); > > +MODULE_LICENSE("GPL v2"); > > >
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index dc14bde31ac1..6ec18e02faf9 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -607,6 +607,16 @@ config LPC32XX_ADC activate only one via device tree selection. Provides direct access via sysfs. +config LTC2309 + tristate "Linear Technology LTC2309 ADC driver" + depends on I2C + help + Say yes here to build support for Linear Technology LTC2309, a low + noise, low power, 8-channel, 12-bit SAR ADC + + This driver can also be built as a module. If so, the module will + be called ltc2309. + config LTC2471 tristate "Linear Technology LTC2471 and LTC2473 ADC driver" depends on I2C diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index eb6e891790fb..fbd86184ec94 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -56,6 +56,7 @@ obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o +obj-$(CONFIG_LTC2309) += ltc2309.o obj-$(CONFIG_LTC2471) += ltc2471.o obj-$(CONFIG_LTC2485) += ltc2485.o obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c new file mode 100644 index 000000000000..145f3c63d157 --- /dev/null +++ b/drivers/iio/adc/ltc2309.c @@ -0,0 +1,248 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface. + * + * Datasheet: + * https://www.analog.com/media/en/technical-documentation/data-sheets/2309fd.pdf + * + * Copyright (c) 2023, Liam Beguin <liambeguin@gmail.com> + * + */ +#include <linux/bitfield.h> +#include <linux/i2c.h> +#include <linux/iio/iio.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/regulator/consumer.h> + +#define DRIVER_NAME "ltc2309" +#define LTC2309_ADC_RESOLUTION 12 + +#define LTC2309_DIN_CH_MASK GENMASK(7, 4) +#define LTC2309_DIN_SDN BIT(7) +#define LTC2309_DIN_OSN BIT(6) +#define LTC2309_DIN_S1 BIT(5) +#define LTC2309_DIN_S0 BIT(4) +#define LTC2309_DIN_UNI BIT(3) +#define LTC2309_DIN_SLEEP BIT(2) + +/* struct ltc2309 - internal device data structure + * + * @dev: Device reference + * @client: I2C reference + * @vref: External reference source + * @lock: Lock to serialize data access + * @vref_mv Internal voltage reference + */ +struct ltc2309 { + struct device *dev; + struct i2c_client *client; + struct regulator *vref; + struct mutex lock; /* serialize data access */ + int vref_mv; +}; + +/* Order matches expected channel address, See datasheet Table 1. */ +enum ltc2309_channels { + LTC2309_CH0_CH1 = 0, + LTC2309_CH2_CH3, + LTC2309_CH4_CH5, + LTC2309_CH6_CH7, + LTC2309_CH1_CH0, + LTC2309_CH3_CH2, + LTC2309_CH5_CH4, + LTC2309_CH7_CH6, + LTC2309_CH0, + LTC2309_CH2, + LTC2309_CH4, + LTC2309_CH6, + LTC2309_CH1, + LTC2309_CH3, + LTC2309_CH5, + LTC2309_CH7, +}; + +#define LTC2309_CHAN(_chan, _addr) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .address = _addr, \ + .channel = _chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .datasheet_name = "CH" #_chan, \ +} + +#define LTC2309_DIFF_CHAN(_chan, _chan2, _addr) { \ + .type = IIO_VOLTAGE, \ + .differential = 1, \ + .indexed = 1, \ + .address = _addr, \ + .channel = _chan, \ + .channel2 = _chan2, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .datasheet_name = "CH" #_chan "-CH" #_chan2, \ +} + +static const struct iio_chan_spec ltc2309_channels[] = { + LTC2309_CHAN(0, LTC2309_CH0), + LTC2309_CHAN(1, LTC2309_CH1), + LTC2309_CHAN(2, LTC2309_CH2), + LTC2309_CHAN(3, LTC2309_CH3), + LTC2309_CHAN(4, LTC2309_CH4), + LTC2309_CHAN(5, LTC2309_CH5), + LTC2309_CHAN(6, LTC2309_CH6), + LTC2309_CHAN(7, LTC2309_CH7), + LTC2309_DIFF_CHAN(0, 1, LTC2309_CH0_CH1), + LTC2309_DIFF_CHAN(2, 3, LTC2309_CH2_CH3), + LTC2309_DIFF_CHAN(4, 5, LTC2309_CH4_CH5), + LTC2309_DIFF_CHAN(6, 7, LTC2309_CH6_CH7), + LTC2309_DIFF_CHAN(1, 0, LTC2309_CH1_CH0), + LTC2309_DIFF_CHAN(3, 2, LTC2309_CH3_CH2), + LTC2309_DIFF_CHAN(5, 4, LTC2309_CH5_CH4), + LTC2309_DIFF_CHAN(7, 6, LTC2309_CH7_CH6), +}; + +static int ltc2309_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + struct ltc2309 *ltc2309 = iio_priv(indio_dev); + u16 buf; + int ret; + u8 din; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + din = FIELD_PREP(LTC2309_DIN_CH_MASK, chan->address & 0x0f) | + FIELD_PREP(LTC2309_DIN_UNI, 1) | + FIELD_PREP(LTC2309_DIN_SLEEP, 0); + + mutex_lock(<c2309->lock); + ret = i2c_smbus_write_byte(ltc2309->client, din); + if (ret < 0) { + dev_err(ltc2309->dev, "i2c command failed: %pe\n", + ERR_PTR(ret)); + goto out; + } + + ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2); + if (ret < 0) { + dev_err(ltc2309->dev, "i2c read failed: %pe\n", + ERR_PTR(ret)); + goto out; + } + + *val = be16_to_cpu(buf) >> 4; + mutex_unlock(<c2309->lock); + + ret = IIO_VAL_INT; + break; + case IIO_CHAN_INFO_SCALE: + *val = ltc2309->vref_mv; + *val2 = LTC2309_ADC_RESOLUTION; + ret = IIO_VAL_FRACTIONAL_LOG2; + break; + default: + ret = -EINVAL; + break; + } + + return ret; + +out: + mutex_unlock(<c2309->lock); + return ret; +} + +static const struct iio_info ltc2309_info = { + .read_raw = ltc2309_read_raw, +}; + +void ltc2309_regulator_disable(void *regulator) +{ + struct regulator *r = (struct regulator *)regulator; + + regulator_disable(r); +} + +static int ltc2309_probe(struct i2c_client *client) +{ + struct iio_dev *indio_dev; + struct ltc2309 *ltc2309; + int ret = 0; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ltc2309)); + if (!indio_dev) + return -ENOMEM; + + i2c_set_clientdata(client, indio_dev); + + ltc2309 = iio_priv(indio_dev); + ltc2309->dev = &indio_dev->dev; + ltc2309->client = client; + ltc2309->vref_mv = 4096; /* Default to the internal ref */ + + indio_dev->name = DRIVER_NAME; + indio_dev->dev.parent = &client->dev; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = ltc2309_channels; + indio_dev->num_channels = ARRAY_SIZE(ltc2309_channels); + indio_dev->info = <c2309_info; + + ltc2309->vref = devm_regulator_get_optional(&client->dev, "vref"); + if (!IS_ERR_OR_NULL(ltc2309->vref)) { + ret = regulator_enable(ltc2309->vref); + if (ret) { + dev_err(ltc2309->dev, "failed to enable vref\n"); + return ret; + } + + ret = devm_add_action_or_reset(ltc2309->dev, + ltc2309_regulator_disable, + ltc2309->vref); + if (ret) { + dev_err(ltc2309->dev, + "failed to add regulator_disable action: %d\n", + ret); + return ret; + } + + ret = regulator_get_voltage(ltc2309->vref); + if (ret < 0) + return ret; + + ltc2309->vref_mv = ret / 1000; + } + + mutex_init(<c2309->lock); + + return devm_iio_device_register(&client->dev, indio_dev); +} + +static const struct of_device_id ltc2309_of_match[] = { + { .compatible = "lltc,ltc2309" }, + { } +}; +MODULE_DEVICE_TABLE(of, ltc2309_of_match); + +static const struct i2c_device_id ltc2309_id[] = { + {"ltc2309", 0}, + {}, +}; +MODULE_DEVICE_TABLE(i2c, ltc2309_id); + +static struct i2c_driver ltc2309_driver = { + .driver = { + .name = DRIVER_NAME, + .of_match_table = ltc2309_of_match, + }, + .probe = ltc2309_probe, + .id_table = ltc2309_id, +}; +module_i2c_driver(ltc2309_driver); + +MODULE_AUTHOR("Liam Beguin <liambeguin@gmail.com>"); +MODULE_DESCRIPTION("Linear Technology LTC2309 ADC"); +MODULE_LICENSE("GPL v2");
The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface. This implements support for all single-ended and differential channels, in unipolar mode only. Signed-off-by: Liam Beguin <liambeguin@gmail.com> --- drivers/iio/adc/Kconfig | 10 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ltc2309.c | 248 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 259 insertions(+)