[v2,2/2] iio: adc: add ADC driver for the TI LMP92064 controller

  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 <l.goehrs@pengutronix.de>
---
 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
  

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 <lkp@intel.com>

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
  

On Tue, 1 Nov 2022 18:36:14 +0800
kernel test robot <lkp@intel.com> 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 <lkp@intel.com>
> 
> 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 <l.goehrs@pengutronix.de> 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 <l.goehrs@pengutronix.de>
> ---
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 <kernel@pengutronix.de>");
> +MODULE_DESCRIPTION("TI LMP92064 ADC");
> +MODULE_LICENSE("GPL");