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@xxxxxxxxx> 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@xxxxxxxxx> > 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@xxxxxxxxx> > > 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@xxxxxxxxx>"); > > +MODULE_DESCRIPTION("Linear Technology LTC2309 ADC"); > > +MODULE_LICENSE("GPL v2"); > > >
