The TI LMP92064 is a dual 12 Bit ADC connected via SPI.
The two channels are intended for simultaneous measurements of the voltage
across- and current through a load to allow accurate instantaneous power
measurements.
The driver does not yet take advantage of this feature, as buffering is not
yet implemented.
Changes from v1 -> v2:
- Rebase from 6.0 to 6.1-rc2 to get access to devm_regulator_get_enable.
- Use regmap instead of raw SPI commands.
This fixes multiple issues in the v1:
- Remove need to assemble register address using bit shifts.
- Remove non DMA-safe stack-allocated buffers.
- Regmap has internal lock handling, removing the need for locking in the
driver read code using mlock.
- Use be16_to_cpu() instead of manually assembling values using bit shifts.
- Use generic device_property_read_u32() instead of devicetree specific
of_property_read_u32().
- Rename the "shunt-resistor" device property to "shunt-resistor-micro-ohms".
- Add supply regulator support for the two voltage domains of the chip
(vdd and vdig).
- Only perform soft reset if no GPIO line for hard resets is available.
- Change the error returned if the device does not respond after a reset
from "EBUSY" to "ENXIO" to indicate that this is likely a persistent
error (like a broken connection).
- Don't set the SPI mode manually.
- Provide a spi_device_id table.
- Declare local variables in reverse christmas tree order.
- Fix formatting of multi-line comments and some whitespace issues.
Signed-off-by: Leonard Göhrs <[email protected]>
---
MAINTAINERS | 8 +
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-lmp92064.c | 316 ++++++++++++++++++++++++++++++++++
4 files changed, 335 insertions(+)
create mode 100644 drivers/iio/adc/ti-lmp92064.c
diff --git a/MAINTAINERS b/MAINTAINERS
index e04d944005ba..e716f4596d56 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20608,6 +20608,14 @@ S: Maintained
F: sound/soc/codecs/isabelle*
F: sound/soc/codecs/lm49453*
+TI LMP92064 ADC DRIVER
+M: Leonard Göhrs <[email protected]>
+R: [email protected]
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/iio/adc/ti,lmp92064.yaml
+F: drivers/iio/adc/ti-lmp92064.c
+
TI PCM3060 ASoC CODEC DRIVER
M: Kirill Marinushkin <[email protected]>
L: [email protected] (moderated for non-subscribers)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 791612ca6012..6875fecc6360 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1234,6 +1234,16 @@ config TI_AM335X_ADC
To compile this driver as a module, choose M here: the module will be
called ti_am335x_adc.
+config TI_LMP92064
+ tristate "Texas Instruments LMP92064 ADC driver"
+ depends on SPI
+ help
+ Say yes here to build support for the LMP92064 Precision Current and Voltage
+ sensor.
+
+ This driver can also be built as a module. If so, the module will be called
+ ti-lmp92064.
+
config TI_TLC4541
tristate "Texas Instruments TLC4541 ADC driver"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 46caba7a010c..e8175c97ebeb 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
+obj-$(CONFIG_TI_LMP92064) += ti-lmp92064.o
obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o
obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
diff --git a/drivers/iio/adc/ti-lmp92064.c b/drivers/iio/adc/ti-lmp92064.c
new file mode 100644
index 000000000000..0e49e0c2c9db
--- /dev/null
+++ b/drivers/iio/adc/ti-lmp92064.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Texas Instruments LMP92064 SPI ADC driver
+ *
+ * Copyright (c) 2022 Leonard Göhrs <[email protected]>, Pengutronix
+ *
+ * Based on linux/drivers/iio/adc/ti-tsc2046.c
+ * Copyright (c) 2021 Oleksij Rempel <[email protected]>, Pengutronix
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/driver.h>
+
+#define TI_LMP92064_REG_CONFIG_A 0x0000
+#define TI_LMP92064_REG_CONFIG_B 0x0001
+#define TI_LMP92064_REG_CHIP_REV 0x0006
+
+#define TI_LMP92064_REG_MFR_ID1 0x000C
+#define TI_LMP92064_REG_MFR_ID2 0x000D
+
+#define TI_LMP92064_REG_REG_UPDATE 0x000F
+#define TI_LMP92064_REG_CONFIG_REG 0x0100
+#define TI_LMP92064_REG_STATUS 0x0103
+
+#define TI_LMP92064_REG_DATA_VOUT_LSB 0x0200
+#define TI_LMP92064_REG_DATA_VOUT_MSB 0x0201
+#define TI_LMP92064_REG_DATA_COUT_LSB 0x0202
+#define TI_LMP92064_REG_DATA_COUT_MSB 0x0203
+
+#define TI_LMP92064_VAL_CONFIG_A 0x99
+#define TI_LMP92064_VAL_CONFIG_B 0x00
+#define TI_LMP92064_VAL_STATUS_OK 0x01
+
+#define TI_LMP92064_CHAN_INC 0
+#define TI_LMP92064_CHAN_INV 1
+
+static const struct regmap_range lmp92064_readable_reg_ranges[] = {
+ regmap_reg_range(TI_LMP92064_REG_CONFIG_A, TI_LMP92064_REG_CHIP_REV),
+ regmap_reg_range(TI_LMP92064_REG_MFR_ID1, TI_LMP92064_REG_MFR_ID2),
+ regmap_reg_range(TI_LMP92064_REG_REG_UPDATE, TI_LMP92064_REG_REG_UPDATE),
+ regmap_reg_range(TI_LMP92064_REG_CONFIG_REG, TI_LMP92064_REG_CONFIG_REG),
+ regmap_reg_range(TI_LMP92064_REG_STATUS, TI_LMP92064_REG_STATUS),
+ regmap_reg_range(TI_LMP92064_REG_DATA_VOUT_LSB, TI_LMP92064_REG_DATA_COUT_MSB),
+};
+
+static const struct regmap_access_table lmp92064_readable_regs = {
+ .yes_ranges = lmp92064_readable_reg_ranges,
+ .n_yes_ranges = ARRAY_SIZE(lmp92064_readable_reg_ranges),
+};
+
+static const struct regmap_range lmp92064_writable_reg_ranges[] = {
+ regmap_reg_range(TI_LMP92064_REG_CONFIG_A, TI_LMP92064_REG_CONFIG_B),
+ regmap_reg_range(TI_LMP92064_REG_REG_UPDATE, TI_LMP92064_REG_REG_UPDATE),
+ regmap_reg_range(TI_LMP92064_REG_CONFIG_REG, TI_LMP92064_REG_CONFIG_REG),
+};
+
+static const struct regmap_access_table lmp92064_writable_regs = {
+ .yes_ranges = lmp92064_writable_reg_ranges,
+ .n_yes_ranges = ARRAY_SIZE(lmp92064_writable_reg_ranges),
+};
+
+static const struct regmap_config lmp92064_spi_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
+ .max_register = TI_LMP92064_REG_DATA_COUT_MSB,
+ .rd_table = &lmp92064_readable_regs,
+ .wr_table = &lmp92064_writable_regs,
+};
+
+struct lmp92064_adc_priv {
+ int shunt_resistor_uohm;
+ struct spi_device *spi;
+ struct regmap *regmap;
+};
+
+static const struct iio_chan_spec lmp92064_adc_channels[] = {
+ {
+ .type = IIO_CURRENT,
+ .address = TI_LMP92064_CHAN_INC,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .datasheet_name = "INC",
+ },
+ {
+ .type = IIO_VOLTAGE,
+ .address = TI_LMP92064_CHAN_INV,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .datasheet_name = "INV",
+ },
+};
+
+static int lmp92064_read_meas(struct lmp92064_adc_priv *priv, u16 *res)
+{
+ __be16 raw[2];
+ int ret;
+
+ /*
+ * The ADC only latches in new samples if all DATA registers are read
+ * in descending sequential order.
+ * The ADC auto-decrements the register index with each clocked byte.
+ * Read both channels in single SPI transfer by selecting the highest
+ * register using the command below and clocking out all four data
+ * bytes.
+ */
+
+ ret = regmap_bulk_read(priv->regmap, TI_LMP92064_REG_DATA_COUT_MSB,
+ &raw, sizeof(raw));
+
+ if (ret) {
+ dev_err(&priv->spi->dev, "regmap_bulk_read failed: %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+
+ res[0] = be16_to_cpu(raw[0]);
+ res[1] = be16_to_cpu(raw[1]);
+
+ return 0;
+}
+
+static int lmp92064_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct lmp92064_adc_priv *priv = iio_priv(indio_dev);
+ u16 raw[2];
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = lmp92064_read_meas(priv, raw);
+ if (ret < 0)
+ return ret;
+
+ *val = (chan->address == TI_LMP92064_CHAN_INC) ? raw[0] : raw[1];
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->address == TI_LMP92064_CHAN_INC) {
+ /*
+ * processed (mA) = raw * current_lsb (mA)
+ * current_lsb (mA) = shunt_voltage_lsb (nV) / shunt_resistor (uOhm)
+ * shunt_voltage_lsb (nV) = 81920000 / 4096 = 20000
+ */
+ *val = 20000;
+ *val2 = priv->shunt_resistor_uohm;
+ } else {
+ /*
+ * processed (mV) = raw * voltage_lsb (mV)
+ * voltage_lsb (mV) = 2048 / 4096
+ */
+ *val = 2048;
+ *val2 = 4096;
+ }
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int lmp92064_reset(struct lmp92064_adc_priv *priv,
+ struct gpio_desc *gpio_reset)
+{
+ unsigned int status;
+ int ret, i;
+
+ if (gpio_reset) {
+ /* Perform a hard reset if gpio_reset is available */
+ gpiod_set_value_cansleep(gpio_reset, 1);
+ usleep_range(1, 10);
+ gpiod_set_value_cansleep(gpio_reset, 0);
+ usleep_range(500, 750);
+ } else {
+ /*
+ * Perform a soft-reset if not.
+ * Also write default values to config registers that are not
+ * affected by soft reset
+ */
+ ret = regmap_write(priv->regmap, TI_LMP92064_REG_CONFIG_A,
+ TI_LMP92064_VAL_CONFIG_A);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(priv->regmap, TI_LMP92064_REG_CONFIG_B,
+ TI_LMP92064_VAL_CONFIG_B);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Wait for the device to signal readiness */
+ for (i = 0; i < 10; i++) {
+ ret = regmap_read(priv->regmap, TI_LMP92064_REG_STATUS, &status);
+
+ if (ret < 0)
+ return ret;
+
+ if (status == TI_LMP92064_VAL_STATUS_OK)
+ return 0;
+
+ usleep_range(1000, 2000);
+ }
+
+ /*
+ * No (correct) response received.
+ * Device is mostly likely not connected to the bus.
+ */
+ return -ENXIO;
+}
+
+static const struct iio_info lmp92064_adc_info = {
+ .read_raw = lmp92064_read_raw,
+};
+
+static int lmp92064_adc_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct lmp92064_adc_priv *priv;
+ struct gpio_desc *gpio_reset;
+ struct iio_dev *indio_dev;
+ u32 shunt_resistor_uohm;
+ struct regmap *regmap;
+ int ret;
+
+ ret = spi_setup(spi);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Error in SPI setup\n");
+
+ regmap = devm_regmap_init_spi(spi, &lmp92064_spi_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to set up SPI regmap\n");
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ priv = iio_priv(indio_dev);
+
+ priv->spi = spi;
+ priv->regmap = regmap;
+
+ ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
+ &shunt_resistor_uohm);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Failed to get shunt-resistor value\n");
+
+ /*
+ * The shunt resistance is passed to userspace as the denominator of an iio
+ * fraction. Make sure it is in range for that.
+ */
+ if (shunt_resistor_uohm == 0 || shunt_resistor_uohm > INT_MAX) {
+ dev_err(dev, "Shunt resistance is out of range\n");
+ return -EINVAL;
+ }
+
+ priv->shunt_resistor_uohm = shunt_resistor_uohm;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return ret;
+
+ ret = devm_regulator_get_enable(dev, "vdig");
+ if (ret)
+ return ret;
+
+ gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(gpio_reset))
+ return dev_err_probe(dev, PTR_ERR(gpio_reset),
+ "Failed to get GPIO reset pin\n");
+
+ ret = lmp92064_reset(priv, gpio_reset);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to reset device\n");
+
+ indio_dev->name = "lmp92064";
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = lmp92064_adc_channels;
+ indio_dev->num_channels = ARRAY_SIZE(lmp92064_adc_channels);
+ indio_dev->info = &lmp92064_adc_info;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct spi_device_id lmp92064_id_table[] = {
+ { "lmp92064", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, lmp92064_id_table);
+
+static const struct of_device_id lmp92064_of_table[] = {
+ { .compatible = "ti,lmp92064" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, lmp92064_of_table);
+
+static struct spi_driver lmp92064_adc_driver = {
+ .driver = {
+ .name = "lmp92064",
+ .of_match_table = lmp92064_of_table,
+ },
+ .probe = lmp92064_adc_probe,
+};
+module_spi_driver(lmp92064_adc_driver);
+
+MODULE_AUTHOR("Leonard Göhrs <[email protected]>");
+MODULE_DESCRIPTION("TI LMP92064 ADC");
+MODULE_LICENSE("GPL");
--
2.30.2
Hi Leonard,
I love your patch! Perhaps something to improve:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.1-rc3 next-20221101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Leonard-G-hrs/dt-bindings-iio-adc-add-TI-LMP92064-controller/20221101-145036
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20221101064804.720050-2-l.goehrs%40pengutronix.de
patch subject: [PATCH v2 2/2] iio: adc: add ADC driver for the TI LMP92064 controller
config: x86_64-allyesconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/4b264b5bc9425f051a3ce5c5a9a30a66d3a0a477
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Leonard-G-hrs/dt-bindings-iio-adc-add-TI-LMP92064-controller/20221101-145036
git checkout 4b264b5bc9425f051a3ce5c5a9a30a66d3a0a477
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/iio/adc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/iio/adc/ti-lmp92064.c: In function 'lmp92064_adc_probe':
drivers/iio/adc/ti-lmp92064.c:267:15: error: implicit declaration of function 'devm_regulator_get_enable'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
267 | ret = devm_regulator_get_enable(dev, "vdd");
| ^~~~~~~~~~~~~~~~~~~~~~~~~
| devm_regulator_get_optional
At top level:
>> drivers/iio/adc/ti-lmp92064.c:293:35: warning: 'lmp92064_id_table' defined but not used [-Wunused-const-variable=]
293 | static const struct spi_device_id lmp92064_id_table[] = {
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/lmp92064_id_table +293 drivers/iio/adc/ti-lmp92064.c
292
> 293 static const struct spi_device_id lmp92064_id_table[] = {
294 { "lmp92064", 0 },
295 {}
296 };
297 MODULE_DEVICE_TABLE(spi, lmp92064_id_table);
298
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, 1 Nov 2022 18:36:14 +0800
kernel test robot <[email protected]> wrote:
> Hi Leonard,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on jic23-iio/togreg]
> [also build test WARNING on linus/master v6.1-rc3 next-20221101]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Leonard-G-hrs/dt-bindings-iio-adc-add-TI-LMP92064-controller/20221101-145036
> base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> patch link: https://lore.kernel.org/r/20221101064804.720050-2-l.goehrs%40pengutronix.de
> patch subject: [PATCH v2 2/2] iio: adc: add ADC driver for the TI LMP92064 controller
> config: x86_64-allyesconfig
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> reproduce (this is a W=1 build):
> # https://github.com/intel-lab-lkp/linux/commit/4b264b5bc9425f051a3ce5c5a9a30a66d3a0a477
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Leonard-G-hrs/dt-bindings-iio-adc-add-TI-LMP92064-controller/20221101-145036
> git checkout 4b264b5bc9425f051a3ce5c5a9a30a66d3a0a477
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/iio/adc/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> drivers/iio/adc/ti-lmp92064.c: In function 'lmp92064_adc_probe':
> drivers/iio/adc/ti-lmp92064.c:267:15: error: implicit declaration of function 'devm_regulator_get_enable'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
> 267 | ret = devm_regulator_get_enable(dev, "vdd");
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
> | devm_regulator_get_optional
That's odd. it's definitely in the trees referenced above and there are stubs
for when regulator support isn't built. No idea...
> At top level:
> >> drivers/iio/adc/ti-lmp92064.c:293:35: warning: 'lmp92064_id_table' defined but not used [-Wunused-const-variable=]
> 293 | static const struct spi_device_id lmp92064_id_table[] = {
> | ^~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
This one is obvious though as the relevant entry in the struct spi_driver
is not set.
>
>
> vim +/lmp92064_id_table +293 drivers/iio/adc/ti-lmp92064.c
>
> 292
> > 293 static const struct spi_device_id lmp92064_id_table[] = {
> 294 { "lmp92064", 0 },
> 295 {}
> 296 };
> 297 MODULE_DEVICE_TABLE(spi, lmp92064_id_table);
> 298
>
On Tue, 1 Nov 2022 07:48:04 +0100
Leonard Göhrs <[email protected]> wrote:
> The TI LMP92064 is a dual 12 Bit ADC connected via SPI.
> The two channels are intended for simultaneous measurements of the voltage
> across- and current through a load to allow accurate instantaneous power
> measurements.
> The driver does not yet take advantage of this feature, as buffering is not
> yet implemented.
>
> Changes from v1 -> v2:
Hi Leonard,
As with the previous patch, please move the change logs below the ---
>
> - Rebase from 6.0 to 6.1-rc2 to get access to devm_regulator_get_enable.
> - Use regmap instead of raw SPI commands.
> This fixes multiple issues in the v1:
> - Remove need to assemble register address using bit shifts.
> - Remove non DMA-safe stack-allocated buffers.
> - Regmap has internal lock handling, removing the need for locking in the
> driver read code using mlock.
> - Use be16_to_cpu() instead of manually assembling values using bit shifts.
> - Use generic device_property_read_u32() instead of devicetree specific
> of_property_read_u32().
> - Rename the "shunt-resistor" device property to "shunt-resistor-micro-ohms".
> - Add supply regulator support for the two voltage domains of the chip
> (vdd and vdig).
> - Only perform soft reset if no GPIO line for hard resets is available.
> - Change the error returned if the device does not respond after a reset
> from "EBUSY" to "ENXIO" to indicate that this is likely a persistent
> error (like a broken connection).
> - Don't set the SPI mode manually.
> - Provide a spi_device_id table.
> - Declare local variables in reverse christmas tree order.
> - Fix formatting of multi-line comments and some whitespace issues.
>
> Signed-off-by: Leonard Göhrs <[email protected]>
> ---
If you weren't going to need to do a v3 anyway for the DT binding, I'd
probably just have fixed up the comments inline and applied it.
Ah well, will be easy to pick up as v3! We have a few more weeks this
cycle, so hopefully you can do a quick update in next week or so.
Some of the comments are more for reference (probably by me)
a later date than suggestions for changes.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ti-lmp92064.c b/drivers/iio/adc/ti-lmp92064.c
> new file mode 100644
> index 000000000000..0e49e0c2c9db
> --- /dev/null
> +++ b/drivers/iio/adc/ti-lmp92064.c
> @@ -0,0 +1,316 @@
> +
> +#define TI_LMP92064_CHAN_INC 0
> +#define TI_LMP92064_CHAN_INV 1
These abbreviations are a bit unfortunate as INC commonly means increment
and INV commonly means invert. Still they are used on the datasheet so
perhaps better to leave them as you have it.
> +
> +static int lmp92064_read_meas(struct lmp92064_adc_priv *priv, u16 *res)
> +{
> + __be16 raw[2];
> + int ret;
> +
> + /*
> + * The ADC only latches in new samples if all DATA registers are read
> + * in descending sequential order.
> + * The ADC auto-decrements the register index with each clocked byte.
> + * Read both channels in single SPI transfer by selecting the highest
> + * register using the command below and clocking out all four data
> + * bytes.
> + */
> +
> + ret = regmap_bulk_read(priv->regmap, TI_LMP92064_REG_DATA_COUT_MSB,
> + &raw, sizeof(raw));
Hmm. I've been a bit overly strict on this in the past.
The underlying SPI bus bulk accessors require DMA safe buffers - but the
reads are actually done use spi_write_then_read() which bounce buffers.
There is the added complication that regmap actually bounced everything
for other reasons at the moment but might not do so in future (fairly sure
it didn't a while back). Anyhow upshot is this is (I think) fine but you'll
see me fairly recently commenting on need for DMA safe buffers for
regmap_bulk_reads(). Going forwards I'll probably only raise this for
bulk writes.
> +
> + if (ret) {
> + dev_err(&priv->spi->dev, "regmap_bulk_read failed: %pe\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> + res[0] = be16_to_cpu(raw[0]);
> + res[1] = be16_to_cpu(raw[1]);
> +
> + return 0;
> +}
> +
> +static int lmp92064_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct lmp92064_adc_priv *priv = iio_priv(indio_dev);
> + u16 raw[2];
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = lmp92064_read_meas(priv, raw);
> + if (ret < 0)
> + return ret;
> +
> + *val = (chan->address == TI_LMP92064_CHAN_INC) ? raw[0] : raw[1];
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->address == TI_LMP92064_CHAN_INC) {
> + /*
> + * processed (mA) = raw * current_lsb (mA)
> + * current_lsb (mA) = shunt_voltage_lsb (nV) / shunt_resistor (uOhm)
> + * shunt_voltage_lsb (nV) = 81920000 / 4096 = 20000
> + */
> + *val = 20000;
> + *val2 = priv->shunt_resistor_uohm;
> + } else {
> + /*
> + * processed (mV) = raw * voltage_lsb (mV)
Extra space before processed?
> + * voltage_lsb (mV) = 2048 / 4096
> + */
> + *val = 2048;
> + *val2 = 4096;
> + }
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int lmp92064_reset(struct lmp92064_adc_priv *priv,
> + struct gpio_desc *gpio_reset)
> +{
> + unsigned int status;
> + int ret, i;
> +
> + if (gpio_reset) {
> + /* Perform a hard reset if gpio_reset is available */
> + gpiod_set_value_cansleep(gpio_reset, 1);
> + usleep_range(1, 10);
> + gpiod_set_value_cansleep(gpio_reset, 0);
> + usleep_range(500, 750);
> + } else {
> + /*
> + * Perform a soft-reset if not.
> + * Also write default values to config registers that are not
> + * affected by soft reset
> + */
> + ret = regmap_write(priv->regmap, TI_LMP92064_REG_CONFIG_A,
> + TI_LMP92064_VAL_CONFIG_A);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, TI_LMP92064_REG_CONFIG_B,
> + TI_LMP92064_VAL_CONFIG_B);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* Wait for the device to signal readiness */
Good to add a comment here on why these particular time parameters.
At minimum say how long the reset is documented to take.
> + for (i = 0; i < 10; i++) {
> + ret = regmap_read(priv->regmap, TI_LMP92064_REG_STATUS, &status);
> +
For style consistency no blank line here (to keep the error handler and function
call visually in one block.
> + if (ret < 0)
> + return ret;
> +
> + if (status == TI_LMP92064_VAL_STATUS_OK)
> + return 0;
> +
> + usleep_range(1000, 2000);
> + }
> +
> + /*
> + * No (correct) response received.
> + * Device is mostly likely not connected to the bus.
> + */
> + return -ENXIO;
> +}
> +
> +static const struct iio_info lmp92064_adc_info = {
> + .read_raw = lmp92064_read_raw,
> +};
> +
> +static int lmp92064_adc_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct lmp92064_adc_priv *priv;
> + struct gpio_desc *gpio_reset;
> + struct iio_dev *indio_dev;
> + u32 shunt_resistor_uohm;
> + struct regmap *regmap;
> + int ret;
> +
> + ret = spi_setup(spi);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Error in SPI setup\n");
> +
> + regmap = devm_regmap_init_spi(spi, &lmp92064_spi_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to set up SPI regmap\n");
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + priv = iio_priv(indio_dev);
> +
> + priv->spi = spi;
> + priv->regmap = regmap;
> +
> + ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> + &shunt_resistor_uohm);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Failed to get shunt-resistor value\n");
> +
> + /*
> + * The shunt resistance is passed to userspace as the denominator of an iio
> + * fraction. Make sure it is in range for that.
> + */
> + if (shunt_resistor_uohm == 0 || shunt_resistor_uohm > INT_MAX) {
> + dev_err(dev, "Shunt resistance is out of range\n");
> + return -EINVAL;
> + }
> +
> + priv->shunt_resistor_uohm = shunt_resistor_uohm;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable(dev, "vdig");
> + if (ret)
> + return ret;
> +
> + gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(gpio_reset))
> + return dev_err_probe(dev, PTR_ERR(gpio_reset),
> + "Failed to get GPIO reset pin\n");
> +
> + ret = lmp92064_reset(priv, gpio_reset);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to reset device\n");
> +
> + indio_dev->name = "lmp92064";
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = lmp92064_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(lmp92064_adc_channels);
> + indio_dev->info = &lmp92064_adc_info;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct spi_device_id lmp92064_id_table[] = {
> + { "lmp92064", 0 },
I would not set the data until you need it (i.e. when adding support for other
parts. Setting it explicitly gives the impression we are using it.
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, lmp92064_id_table);
> +
> +static const struct of_device_id lmp92064_of_table[] = {
> + { .compatible = "ti,lmp92064" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lmp92064_of_table);
> +
> +static struct spi_driver lmp92064_adc_driver = {
> + .driver = {
> + .name = "lmp92064",
> + .of_match_table = lmp92064_of_table,
> + },
> + .probe = lmp92064_adc_probe,
Missing the id_table being set as per the 0-day bot mail.
> +};
> +module_spi_driver(lmp92064_adc_driver);
> +
> +MODULE_AUTHOR("Leonard Göhrs <[email protected]>");
> +MODULE_DESCRIPTION("TI LMP92064 ADC");
> +MODULE_LICENSE("GPL");