| Message ID | 20240220153553.2432-6-mitrutzceclan@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add support for LTC6373 | expand |
On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote: > This adds support for LTC6373 36 V Fully-Differential Programmable-Gain > Instrumentation Amplifier with 25 pA Input Bias Current. > The user can program the gain to one of seven available settings through > a 3-bit parallel interface (A2 to A0). > > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> > --- Just one minor comment. With that: Reviewed-by: Nuno Sa <nuno.sa@analog.com> > drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++- > 1 file changed, 120 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c > index 77872e2dfdfe..50c86c2d28d7 100644 > --- a/drivers/iio/amplifiers/hmc425a.c > +++ b/drivers/iio/amplifiers/hmc425a.c > @@ -2,9 +2,10 @@ > /* > * HMC425A and similar Gain Amplifiers > * > - * Copyright 2020 Analog Devices Inc. > + * Copyright 2020, 2024 Analog Devices Inc. > */ ... > > > +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev, > + uintptr_t private, > + const struct iio_chan_spec *chan, > + char *buf) > +{ > + struct hmc425a_state *st = iio_priv(indio_dev); > + > + return sysfs_emit(buf, "%d\n", st->powerdown); Well, in theory the read should also be protected with the lock... - Nuno Sá
On Wed, 21 Feb 2024 14:23:51 +0100 Nuno Sá <noname.nuno@gmail.com> wrote: > On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote: > > This adds support for LTC6373 36 V Fully-Differential Programmable-Gain > > Instrumentation Amplifier with 25 pA Input Bias Current. > > The user can program the gain to one of seven available settings through > > a 3-bit parallel interface (A2 to A0). > > > > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> > > --- > > Just one minor comment. With that: > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > > > drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++- > > 1 file changed, 120 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c > > index 77872e2dfdfe..50c86c2d28d7 100644 > > --- a/drivers/iio/amplifiers/hmc425a.c > > +++ b/drivers/iio/amplifiers/hmc425a.c > > @@ -2,9 +2,10 @@ > > /* > > * HMC425A and similar Gain Amplifiers > > * > > - * Copyright 2020 Analog Devices Inc. > > + * Copyright 2020, 2024 Analog Devices Inc. > > */ > > ... > > > > > > > +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev, > > + uintptr_t private, > > + const struct iio_chan_spec *chan, > > + char *buf) > > +{ > > + struct hmc425a_state *st = iio_priv(indio_dev); > > + > > + return sysfs_emit(buf, "%d\n", st->powerdown); > > Well, in theory the read should also be protected with the lock... Only reason I can think of for that is potential read tearing. If that happens on a bool we are going to be in a mess so I think this is in practice fine without, though paranoia might suggest locking. It can race against it being powered down but that effectively happens even if we do have a lock as we either see the value before or after a racing power down event and we have no way of knowing which. Applied rest series to iio.git togreg branch and pushed out as testing. Duitru, if you can figure out what happened to your message thread before sending more patches that would be great. The IDs for patches 0-5 go 20240220153553.2432-1-mitrutzceclan@gmail.com 20240220153553.2432-3-mitrutzceclan@gmail.com 20240220153553.2432-5-mitrutzceclan@gmail.com 20240220153553.2432-2-mitrutzceclan@gmail.com 20240220153553.2432-4-mitrutzceclan@gmail.com 20240220153553.2432-6-mitrutzceclan@gmail.com Which really confuses my email client and patchwork. https://patchwork.kernel.org/project/linux-iio/list/?series=827901 > > - Nuno Sá >
On Sat, 2024-02-24 at 17:54 +0000, Jonathan Cameron wrote: > On Wed, 21 Feb 2024 14:23:51 +0100 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote: > > > This adds support for LTC6373 36 V Fully-Differential Programmable-Gain > > > Instrumentation Amplifier with 25 pA Input Bias Current. > > > The user can program the gain to one of seven available settings through > > > a 3-bit parallel interface (A2 to A0). > > > > > > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> > > > --- > > > > Just one minor comment. With that: > > > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > > > > > drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++- > > > 1 file changed, 120 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/iio/amplifiers/hmc425a.c > > > b/drivers/iio/amplifiers/hmc425a.c > > > index 77872e2dfdfe..50c86c2d28d7 100644 > > > --- a/drivers/iio/amplifiers/hmc425a.c > > > +++ b/drivers/iio/amplifiers/hmc425a.c > > > @@ -2,9 +2,10 @@ > > > /* > > > * HMC425A and similar Gain Amplifiers > > > * > > > - * Copyright 2020 Analog Devices Inc. > > > + * Copyright 2020, 2024 Analog Devices Inc. > > > */ > > > > ... > > > > > > > > > > > +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev, > > > + uintptr_t private, > > > + const struct iio_chan_spec *chan, > > > + char *buf) > > > +{ > > > + struct hmc425a_state *st = iio_priv(indio_dev); > > > + > > > + return sysfs_emit(buf, "%d\n", st->powerdown); > > > > Well, in theory the read should also be protected with the lock... > > Only reason I can think of for that is potential read tearing. > If that happens on a bool we are going to be in a mess so I think this > is in practice fine without, though paranoia might suggest locking. Yeah, also mentioned it for correctness. I mean, in theory, read_once() should be more that enough in here but I often find that too much for using in "simple" drivers where a lock is surely easier to understand for someone reading the code. Now, about your bool comment, I used to think like that until I saw the LF rcu mentorship video from Paul. I'm fairly sure he comes up with some "crazy" possibility about the CPU/compiler screwing you even on a char (IIRC, he was also arguing about not using read_once() on a bool). Now, practically speaking, I tend to agree that for the archs we care this will likely never be an issue but yeah, not 100% correct kernel code IMO. - Nuno Sá
diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c index 77872e2dfdfe..50c86c2d28d7 100644 --- a/drivers/iio/amplifiers/hmc425a.c +++ b/drivers/iio/amplifiers/hmc425a.c @@ -2,9 +2,10 @@ /* * HMC425A and similar Gain Amplifiers * - * Copyright 2020 Analog Devices Inc. + * Copyright 2020, 2024 Analog Devices Inc. */ +#include <linux/bits.h> #include <linux/bitops.h> #include <linux/device.h> #include <linux/err.h> @@ -12,6 +13,7 @@ #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> #include <linux/kernel.h> +#include <linux/math.h> #include <linux/mod_devicetable.h> #include <linux/module.h> #include <linux/platform_device.h> @@ -20,10 +22,24 @@ #include <linux/regulator/consumer.h> #include <linux/sysfs.h> +/* + * The LTC6373 amplifier supports configuring gain using GPIO's with the following + * values (OUTPUT_V / INPUT_V): 0(shutdown), 0.25, 0.5, 1, 2, 4, 8, 16 + * + * Except for the shutdown value, all can be converted to dB using 20 * log10(x) + * From here, it is observed that all values are multiples of the '2' gain setting, + * with the correspondent of 6.020dB. + */ +#define LTC6373_CONVERSION_CONSTANT 6020 +#define LTC6373_MIN_GAIN_CODE 0x6 +#define LTC6373_CONVERSION_MASK GENMASK(2, 0) +#define LTC6373_SHUTDOWN GENMASK(2, 0) + enum hmc425a_type { ID_HMC425A, ID_HMC540S, - ID_ADRF5740 + ID_ADRF5740, + ID_LTC6373, }; struct hmc425a_chip_info { @@ -34,6 +50,8 @@ struct hmc425a_chip_info { int gain_min; int gain_max; int default_gain; + int powerdown_val; + bool has_powerdown; int (*gain_dB_to_code)(int gain, int *code); int (*code_to_gain_dB)(int code, int *val, int *val2); @@ -44,6 +62,7 @@ struct hmc425a_state { const struct hmc425a_chip_info *chip_info; struct gpio_descs *gpios; u32 gain; + bool powerdown; }; static int gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *code) @@ -58,6 +77,8 @@ static int gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *cod if (gain > inf->gain_max || gain < inf->gain_min) return -EINVAL; + if (st->powerdown) + return -EPERM; return st->chip_info->gain_dB_to_code(gain, code); } @@ -83,8 +104,17 @@ static int adrf5740_gain_dB_to_code(int gain, int *code) return 0; } +static int ltc6373_gain_dB_to_code(int gain, int *code) +{ + *code = ~(DIV_ROUND_CLOSEST(gain, LTC6373_CONVERSION_CONSTANT) + 3) + & LTC6373_CONVERSION_MASK; + return 0; +} + static int code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2) { + if (st->powerdown) + return -EPERM; return st->chip_info->code_to_gain_dB(st->gain, val, val2); } @@ -114,6 +144,16 @@ static int adrf5740_code_to_gain_dB(int code, int *val, int *val2) return 0; } +static int ltc6373_code_to_gain_dB(int code, int *val, int *val2) +{ + int gain = ((~code & LTC6373_CONVERSION_MASK) - 3) * + LTC6373_CONVERSION_CONSTANT; + + *val = gain / 1000; + *val2 = (gain % 1000) * 1000; + return 0; +} + static int hmc425a_write(struct iio_dev *indio_dev, u32 value) { struct hmc425a_state *st = iio_priv(indio_dev); @@ -191,6 +231,48 @@ static const struct iio_info hmc425a_info = { .write_raw_get_fmt = &hmc425a_write_raw_get_fmt, }; +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct hmc425a_state *st = iio_priv(indio_dev); + + return sysfs_emit(buf, "%d\n", st->powerdown); +} + +static ssize_t ltc6373_write_powerdown(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, + size_t len) +{ + struct hmc425a_state *st = iio_priv(indio_dev); + bool powerdown; + int code, ret; + + ret = kstrtobool(buf, &powerdown); + if (ret) + return ret; + + mutex_lock(&st->lock); + st->powerdown = powerdown; + code = (powerdown) ? LTC6373_SHUTDOWN : st->gain; + hmc425a_write(indio_dev, code); + mutex_unlock(&st->lock); + return len; +} + +static const struct iio_chan_spec_ext_info ltc6373_ext_info[] = { + { + .name = "powerdown", + .read = ltc6373_read_powerdown, + .write = ltc6373_write_powerdown, + .shared = IIO_SEPARATE, + }, + {} +}; + #define HMC425A_CHAN(_channel) \ { \ .type = IIO_VOLTAGE, \ @@ -200,10 +282,24 @@ static const struct iio_info hmc425a_info = { .info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ } +#define LTC6373_CHAN(_channel) \ +{ \ + .type = IIO_VOLTAGE, \ + .output = 1, \ + .indexed = 1, \ + .channel = _channel, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ + .ext_info = ltc6373_ext_info, \ +} + static const struct iio_chan_spec hmc425a_channels[] = { HMC425A_CHAN(0), }; +static const struct iio_chan_spec ltc6373_channels[] = { + LTC6373_CHAN(0), +}; + static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = { [ID_HMC425A] = { .name = "hmc425a", @@ -238,6 +334,19 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = { .gain_dB_to_code = adrf5740_gain_dB_to_code, .code_to_gain_dB = adrf5740_code_to_gain_dB, }, + [ID_LTC6373] = { + .name = "ltc6373", + .channels = ltc6373_channels, + .num_channels = ARRAY_SIZE(ltc6373_channels), + .num_gpios = 3, + .gain_min = -12041, /* gain setting x0.25*/ + .gain_max = 24082, /* gain setting x16 */ + .default_gain = LTC6373_MIN_GAIN_CODE, + .powerdown_val = LTC6373_SHUTDOWN, + .has_powerdown = true, + .gain_dB_to_code = ltc6373_gain_dB_to_code, + .code_to_gain_dB = ltc6373_code_to_gain_dB, + }, }; static int hmc425a_probe(struct platform_device *pdev) @@ -278,8 +387,13 @@ static int hmc425a_probe(struct platform_device *pdev) indio_dev->info = &hmc425a_info; indio_dev->modes = INDIO_DIRECT_MODE; - /* Set default gain */ - hmc425a_write(indio_dev, st->gain); + if (st->chip_info->has_powerdown) { + st->powerdown = true; + hmc425a_write(indio_dev, st->chip_info->powerdown_val); + } else { + /* Set default gain */ + hmc425a_write(indio_dev, st->gain); + } return devm_iio_device_register(&pdev->dev, indio_dev); } @@ -292,6 +406,8 @@ static const struct of_device_id hmc425a_of_match[] = { .data = &hmc425a_chip_info_tbl[ID_HMC540S]}, { .compatible = "adi,adrf5740", .data = &hmc425a_chip_info_tbl[ID_ADRF5740]}, + { .compatible = "adi,ltc6373", + .data = &hmc425a_chip_info_tbl[ID_LTC6373]}, {} }; MODULE_DEVICE_TABLE(of, hmc425a_of_match);
This adds support for LTC6373 36 V Fully-Differential Programmable-Gain Instrumentation Amplifier with 25 pA Input Bias Current. The user can program the gain to one of seven available settings through a 3-bit parallel interface (A2 to A0). Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> --- drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 4 deletions(-)