* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
@ 2017-03-25 15:45 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2017-03-25 15:45 UTC (permalink / raw)
To: Jacopo Mondi, geert-Td1EMuHUCqxL1ZNQvxDV9g,
wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 24/03/17 15:28, Jacopo Mondi wrote:
> Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
> with 12-bits ADC interface.
>
> Datasheet publicly available at:
> https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
A few more little things inline. Coming together nicely.
The channel set here is just odd enough that it might aid review to have
a quick listing of the resulting sysfs entries. One or two authors do
this an it is always useful for a quick sanity check.
Perhaps even a set of typical values. Put this below the --- as we don't
need it in the git history, only to assist lazy reviewers like me ;)
Thanks,
Jonathan
> ---
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 601 insertions(+)
> create mode 100644 drivers/iio/adc/max9611.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7a..82f2e7b8 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -354,6 +354,16 @@ config MAX1363
> To compile this driver as a module, choose M here: the module will be
> called max1363.
>
> +config MAX9611
> + tristate "Maxim max9611/max9612 ADC driver"
> + depends on I2C
> + help
> + Say yes here to build support for Maxim max9611/max9612 current sense
> + amplifier with 12-bits ADC interface.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called max9611.
> +
> config MCP320X
> tristate "Microchip Technology MCP3x01/02/04/08"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d001262..149f979 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
> obj-$(CONFIG_MAX1027) += max1027.o
> obj-$(CONFIG_MAX11100) += max11100.o
> obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_MAX9611) += max9611.o
> obj-$(CONFIG_MCP320X) += mcp320x.o
> obj-$(CONFIG_MCP3422) += mcp3422.o
> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> new file mode 100644
> index 0000000..61566ec
> --- /dev/null
> +++ b/drivers/iio/adc/max9611.c
> @@ -0,0 +1,590 @@
> +/*
> + * iio/adc/max9611.c
> + *
> + * Maxim max9611/max9612 high side current sense amplifier with
> + * 12-bit ADC interface.
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * This driver supports input common-mode voltage, current-sense
> + * amplifier with programmable gains and die temperature reading from
> + * Maxim max9611/max9612.
> + *
> + * Op-amp, analog comparator, and watchdog functionalities are not
> + * supported by this driver.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +
> +#define DRIVER_NAME "max9611"
> +
> +/* max9611 register addresses */
> +#define MAX9611_REG_CSA_DATA 0x00
> +#define MAX9611_REG_RS_DATA 0x02
> +#define MAX9611_REG_TEMP_DATA 0x08
> +#define MAX9611_REG_CTRL1 0x0a
> +#define MAX9611_REG_CTRL2 0x0b
> +
> +/* max9611 REG1 mux configuration options */
> +#define MAX9611_MUX_MASK 0x07
> +#define MAX9611_MUX_SENSE_1x 0x00
> +#define MAX9611_MUX_SENSE_4x 0x01
> +#define MAX9611_MUX_SENSE_8x 0x02
> +#define MAX9611_INPUT_VOLT 0x03
> +#define MAX9611_MUX_TEMP 0x06
> +
> +/* max9611 voltage (both csa and input) helper macros */
> +#define MAX9611_VOLTAGE_SHIFT 0x04
> +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
> +
> +/*
> + * max9611 current sense amplifier voltage output:
> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> + *
> + * GAIN LSB (nV) OFFSET (LSB steps)
> + * 1x 107500 1
> + * 4x 26880 1
> + * 8x 13440 3
> + *
> + * The complete formula to calculate current sense voltage is:
> + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> + */
> +#define CSA_VOLT_1x_LSB_nV 107500
> +#define CSA_VOLT_4x_LSB_nV 26880
> +#define CSA_VOLT_8x_LSB_nV 13440
> +
> +#define CSA_VOLT_1x_OFFS_RAW 1
> +#define CSA_VOLT_4x_OFFS_RAW 1
> +#define CSA_VOLT_8x_OFFS_RAW 3
> +
> +/*
> + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> + *
> + * The complete formula to calculate input common voltage is:
> + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> + */
> +#define CIM_VOLTAGE_LSB_mV 14
> +#define CIM_VOLTAGE_OFFSET_RAW 1
> +
> +/*
> + * max9611 temperature reading: LSB is 0.48 degrees Celsius
> + *
> + * The complete formula to calculate temperature is:
> + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
> + */
I'd prefer these defines to be prefixed with MAX9611_
Easiest to just do the lot. Some of these are 'standard' enough
the might well clash with something that turns up in an included header
at somepoint in the future.
> +#define TEMP_SHIFT 0x07
> +#define TEMP_MAX_RAW_POS 0x7f80
> +#define TEMP_MAX_RAW_NEG 0xff80
> +#define TEMP_MIN_RAW_NEG 0xd980
> +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
> +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
> +#define TEMP_LSB_mC 480
> +#define TEMP_SCALE_NUM 1000
> +#define TEMP_SCALE_DIV 2083
> +
> +struct max9611_dev {
> + struct device *dev;
> + struct i2c_client *i2c_client;
> + struct mutex lock;
> + unsigned int shunt_resistor_uohm;
> +};
> +
> +enum max9611_conf_ids {
> + CONF_SENSE_1x,
> + CONF_SENSE_4x,
> + CONF_SENSE_8x,
> + CONF_IN_VOLT,
> + CONF_TEMP,
> +};
> +
> +/**
> + * max9611_mux_conf - associate ADC mux configuration with register address
> + * where data shall be read from
> + */
> +static unsigned int max9611_mux_conf[][2] = {
> + /* CONF_SENSE_1x */
> + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
> + /* CONF_SENSE_4x */
> + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
> + /* CONF_SENSE_8x */
> + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
> + /* CONF_IN_VOLT */
> + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
> + /* CONF_TEMP */
> + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
> +};
> +
> +enum max9611_csa_gain {
> + CSA_GAIN_1x,
> + CSA_GAIN_4x,
> + CSA_GAIN_8x,
> +};
> +
> +enum max9611_csa_gain_params {
> + CSA_GAIN_LSB_nV,
> + CSA_GAIN_OFFS_RAW,
> +};
> +
> +/**
> + * max9611_csa_gain_conf - associate gain multiplier with LSB and
> + * offset values.
> + *
> + * Group together parameters associated with configurable gain
> + * on current sense amplifier path to ADC interface.
> + * Current sense read routine adjusts gain until it gets a meaningful
> + * value; use this structure to retrieve the correct LSB and offset values.
> + */
> +static unsigned int max9611_gain_conf[][2] = {
> + { /* [0] CSA_GAIN_1x */
> + CSA_VOLT_1x_LSB_nV,
> + CSA_VOLT_1x_OFFS_RAW,
> + },
> + { /* [1] CSA_GAIN_4x */
> + CSA_VOLT_4x_LSB_nV,
> + CSA_VOLT_4x_OFFS_RAW,
> + },
> + { /* [2] CSA_GAIN_8x */
> + CSA_VOLT_8x_LSB_nV,
> + CSA_VOLT_8x_OFFS_RAW,
> + },
> +};
> +
> +enum max9611_chan_addrs {
> + MAX9611_CHAN_VOLTAGE_INPUT,
> + MAX9611_CHAN_VOLTAGE_SENSE,
> + MAX9611_CHAN_TEMPERATURE,
> + MAX9611_CHAN_CURRENT_LOAD,
> + MAX9611_CHAN_POWER_LOAD,
> +};
> +
> +static struct iio_chan_spec max9611_channels[] = {
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .address = MAX9611_CHAN_TEMPERATURE,
> + },
> + {
> + .type = IIO_VOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .address = MAX9611_CHAN_VOLTAGE_INPUT,
> + .indexed = 1,
> + .channel = 1,
> + },
> + {
> + .type = IIO_VOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .address = MAX9611_CHAN_VOLTAGE_SENSE,
> + .indexed = 1,
> + .channel = 0,
Unusual to have the channels in here other than in channel order...
> + },
> + {
> + .type = IIO_CURRENT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .address = MAX9611_CHAN_CURRENT_LOAD,
> + },
> + {
> + .type = IIO_POWER,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .address = MAX9611_CHAN_POWER_LOAD
> + },
> +};
> +
> +/**
> + * max9611_read_single() - read a single vale from ADC interface
value
> + *
> + * Data registers are 16 bit long, spread between two 8 bit registers
> + * with consecutive addresses.
> + * Configure ADC mux first, then read register at address "reg_addr".
> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
> + *
> + * @max9611: max9611 device
> + * @selector: index for mux and register configuration
> + * @raw_val: the value returned from ADC
> + */
> +static int max9611_read_single(struct max9611_dev *max9611,
> + enum max9611_conf_ids selector,
> + u16 *raw_val)
> +{
> + int ret;
> +
> + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
> + u8 reg_addr = max9611_mux_conf[selector][1];
> +
> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> + MAX9611_REG_CTRL1, mux_conf);
> + if (ret) {
> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> + MAX9611_REG_CTRL1, mux_conf);
> + return ret;
> + }
> +
> + /*
> + * need a delay here to make register configuration
> + * stabilize. 1 msec at least, from empirical testing.
> + */
> + usleep_range(1000, 2000);
> +
> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
> + if (ret < 0) {
> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> + reg_addr);
> + return ret;
> + }
> + *raw_val = ret;
> +
> + return 0;
> +}
> +
> +/**
> + * max9611_read_csa_voltage() - read current sense amplifier output voltage
> + *
> + * Current sense amplifier output voltage is read through a configurable
> + * 1x, 4x or 8x gain.
> + * Start with plain 1x gain, and adjust gain control properly until a
> + * meaningful value is read from ADC output.
> + *
> + * @max9611: max9611 device
> + * @adc_raw: raw value read from ADC output
> + * @csa_gain: gain configuration option selector
> + */
> +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
> + u16 *adc_raw,
> + enum max9611_csa_gain *csa_gain)
> +{
> + enum max9611_conf_ids gain_selectors[] = {
> + CONF_SENSE_1x,
> + CONF_SENSE_4x,
> + CONF_SENSE_8x
> + };
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
> + if (ret)
> + return ret;
> +
> + if (*adc_raw > 0) {
> + *csa_gain = gain_selectors[i];
> + return 0;
> + }
> + }
> +
> + return -EIO;
> +}
> +
> +static int max9611_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct max9611_dev *dev = iio_priv(indio_dev);
> + enum max9611_csa_gain gain_selector;
> + unsigned int *csa_gain;
> + u16 adc_data;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&dev->lock);
> +
> + switch (chan->address) {
> + case MAX9611_CHAN_TEMPERATURE:
> + ret = max9611_read_single(dev, CONF_TEMP,
> + &adc_data);
> + if (ret)
I'm not personally keen on jumping out of deep indentations like this
just save on repeating one line. I'd pull the unlock back here and return
directly as I feel it'll improve readability.
Actually come to think of it, why does the lock need to be held for
the next line anyway? adc_data is on the stack so doesn't matter if we
have concurrent readers, once the i2c transaction is finished.
Just unlock before checking ret.
> + goto unlock_fail;
> +
> + *val = TEMP_RAW(adc_data);
> +
> + mutex_unlock(&dev->lock);
> + return IIO_VAL_INT;
> +
> + case MAX9611_CHAN_VOLTAGE_INPUT:
> + ret = max9611_read_single(dev, CONF_IN_VOLT,
> + &adc_data);
> + if (ret)
> + goto unlock_fail;
> +
> + *val = MAX9611_VOLTAGE_RAW(adc_data);
> +
> + mutex_unlock(&dev->lock);
> + return IIO_VAL_INT;
> + }
> +
> + case IIO_CHAN_INFO_OFFSET:
> + switch (chan->address) {
> + case MAX9611_CHAN_VOLTAGE_INPUT:
> + *val = CIM_VOLTAGE_OFFSET_RAW;
> +
> + return IIO_VAL_INT;
> + }
> +
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->address) {
> + case MAX9611_CHAN_TEMPERATURE:
> + *val = TEMP_SCALE_NUM;
> + *val2 = TEMP_SCALE_DIV;
> +
> + return IIO_VAL_FRACTIONAL;
> +
> + case MAX9611_CHAN_VOLTAGE_INPUT:
> + *val = CIM_VOLTAGE_LSB_mV;
> + return IIO_VAL_INT;
> + }
> +
> + case IIO_CHAN_INFO_PROCESSED:
> + mutex_lock(&dev->lock);
> +
> + switch (chan->address) {
> + case MAX9611_CHAN_VOLTAGE_SENSE:
> + /*
> + * processed (mV): (raw - offset) * LSB (nV) / 10^6
> + *
> + * Even if max9611 can output raw csa voltage readings,
> + * use a produced value as scale depends on gain.
> + */
> + ret = max9611_read_csa_voltage(dev, &adc_data,
> + &gain_selector);
> + if (ret)
> + goto unlock_fail;
> +
> + csa_gain = max9611_gain_conf[gain_selector];
> +
> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> + csa_gain[CSA_GAIN_LSB_nV];
> + *val2 = 1000000;
> +
> + mutex_unlock(&dev->lock);
> + return IIO_VAL_FRACTIONAL;
> +
> + case MAX9611_CHAN_CURRENT_LOAD:
> + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
> + ret = max9611_read_csa_voltage(dev, &adc_data,
> + &gain_selector);
> + if (ret)
> + goto unlock_fail;
> +
> + csa_gain = max9611_gain_conf[gain_selector];
> +
> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> + csa_gain[CSA_GAIN_LSB_nV];
> + *val2 = dev->shunt_resistor_uohm;
> +
> + mutex_unlock(&dev->lock);
> + return IIO_VAL_FRACTIONAL;
> +
> + case MAX9611_CHAN_POWER_LOAD:
> + /*
> + * processed (mW): Vin (mV) * Vcsa (uV) /
> + * Rshunt (uOhm)
> + */
> + ret = max9611_read_single(dev, CONF_IN_VOLT,
> + &adc_data);
> + if (ret)
> + goto unlock_fail;
> +
> + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> + CIM_VOLTAGE_LSB_mV;
> +
> + ret = max9611_read_csa_voltage(dev, &adc_data,
> + &gain_selector);
> + if (ret)
> + goto unlock_fail;
> +
> + csa_gain = max9611_gain_conf[gain_selector];
> +
> + /* divide by 10^3 here to avoid 32bit overflow */
> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
> + csa_gain[CSA_GAIN_LSB_nV] / 1000;
> + *val2 = dev->shunt_resistor_uohm;
> +
> + mutex_unlock(&dev->lock);
> + return IIO_VAL_FRACTIONAL;
> + }
> + }
> +
> + ret = -EINVAL;
> +
> +unlock_fail:
> + mutex_unlock(&dev->lock);
> + return ret;
> +
> +}
> +
> +static ssize_t max9611_shunt_resistor_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
> +}
> +
> +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
> + max9611_shunt_resistor_show, NULL, 0);
> +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
> + max9611_shunt_resistor_show, NULL, 0);
> +
> +static struct attribute *max9611_attributes[] = {
> + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
> + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group max9611_attribute_group = {
> + .attrs = max9611_attributes,
> +};
> +
> +static const struct iio_info indio_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = max9611_read_raw,
> + .attrs = &max9611_attribute_group,
> +};
> +
> +static int max9611_init(struct max9611_dev *max9611)
> +{
> + struct i2c_client *client = max9611->i2c_client;
> + u16 regval;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WRITE_BYTE |
> + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> + dev_err(max9611->dev,
> + "No smbus support in I2c adapter: aborting probe.\n");
This isn't necessarily an accurate message. I2c adapter might support
smbus_read_byte only rather than word reads for example.
Maybe make it more explict as to what we need?
> + return -EINVAL;
> + }
> +
> + /* Configure MUX to read temperature */
> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> + if (ret) {
> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> + return ret;
> + }
> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> + MAX9611_REG_CTRL2, 0);
> + if (ret) {
> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> + MAX9611_REG_CTRL2, 0);
> + return ret;
> + }
> + usleep_range(1000, 2000);
> +
> + /* Make sure die temperature is in range to test communications. */
> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
> + MAX9611_REG_TEMP_DATA);
> + if (ret < 0) {
> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> + MAX9611_REG_TEMP_DATA);
> + return ret;
> + }
> + regval = ret & ~TEMP_MASK;
> +
> + if ((regval > TEMP_MAX_RAW_POS &&
> + regval < TEMP_MIN_RAW_NEG) ||
> + regval > TEMP_MAX_RAW_NEG) {
> + dev_err(max9611->dev,
> + "Invalid value received from ADC 0x%4x: aborting\n",
> + regval);
> + return -EIO;
> + }
> +
> + /* Mux shall be zeroed back before applying other configurations */
> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> + MAX9611_REG_CTRL1, 0);
> + if (ret) {
> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> + MAX9611_REG_CTRL1, 0);
> + return ret;
> + }
> + usleep_range(1000, 2000);
> +
> + return 0;
> +}
> +
> +static int max9611_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + const char * const shunt_res_prop = "shunt-resistor-uohm";
> + struct device_node *of_node = client->dev.of_node;
> + struct max9611_dev *max9611;
> + struct iio_dev *indio_dev;
> + unsigned int of_shunt;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
> + if (IS_ERR(indio_dev))
> + return PTR_ERR(indio_dev);
> +
> + i2c_set_clientdata(client, indio_dev);
> +
> + max9611 = iio_priv(indio_dev);
> + max9611->dev = &client->dev;
> + max9611->i2c_client = client;
> + mutex_init(&max9611->lock);
> +
> + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> + if (ret) {
> + dev_err(&client->dev,
> + "Missing %s property for %s node\n",
> + shunt_res_prop, of_node->full_name);
> + return ret;
> + }
> + max9611->shunt_resistor_uohm = of_shunt;
> +
> + ret = max9611_init(max9611);
> + if (ret)
> + return ret;
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->dev.of_node = client->dev.of_node;
> + indio_dev->name = client->dev.of_node->name;
What's this going to give for the name? Name in IIO is supposed to
be an indication of the part rather than anything more explicit.
That's not easily obtained from device tree directly...
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &indio_info;
> + indio_dev->channels = max9611_channels;
> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct of_device_id max9611_of_table[] = {
> + {.compatible = "maxim,max9611"},
> + {.compatible = "maxim,max9612"},
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, max9611_of_table);
> +
> +static struct i2c_driver max9611_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = max9611_of_table,
> + },
> + .probe = max9611_probe,
> +};
> +module_i2c_driver(max9611_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> +MODULE_LICENSE("GPL v2");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-25 15:45 ` Jonathan Cameron
(?)
@ 2017-03-25 16:13 ` Peter Meerwald-Stadler
2017-03-25 16:34 ` Jonathan Cameron
` (2 more replies)
-1 siblings, 3 replies; 39+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-25 16:13 UTC (permalink / raw)
To: Jacopo Mondi, Jonathan Cameron; +Cc: geert, wsa+renesas, lars, linux-iio
On Sat, 25 Mar 2017, Jonathan Cameron wrote:
some more comments
> On 24/03/17 15:28, Jacopo Mondi wrote:
> > Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
> > with 12-bits ADC interface.
> >
> > Datasheet publicly available at:
> > https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> A few more little things inline. Coming together nicely.
>
> The channel set here is just odd enough that it might aid review to have
> a quick listing of the resulting sysfs entries. One or two authors do
> this an it is always useful for a quick sanity check.
>
> Perhaps even a set of typical values. Put this below the --- as we don't
> need it in the git history, only to assist lazy reviewers like me ;)
>
> Thanks,
>
> Jonathan
> > ---
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 601 insertions(+)
> > create mode 100644 drivers/iio/adc/max9611.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index dedae7a..82f2e7b8 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -354,6 +354,16 @@ config MAX1363
> > To compile this driver as a module, choose M here: the module will be
> > called max1363.
> >
> > +config MAX9611
> > + tristate "Maxim max9611/max9612 ADC driver"
> > + depends on I2C
> > + help
> > + Say yes here to build support for Maxim max9611/max9612 current sense
> > + amplifier with 12-bits ADC interface.
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called max9611.
> > +
> > config MCP320X
> > tristate "Microchip Technology MCP3x01/02/04/08"
> > depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index d001262..149f979 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
> > obj-$(CONFIG_MAX1027) += max1027.o
> > obj-$(CONFIG_MAX11100) += max11100.o
> > obj-$(CONFIG_MAX1363) += max1363.o
> > +obj-$(CONFIG_MAX9611) += max9611.o
> > obj-$(CONFIG_MCP320X) += mcp320x.o
> > obj-$(CONFIG_MCP3422) += mcp3422.o
> > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > new file mode 100644
> > index 0000000..61566ec
> > --- /dev/null
> > +++ b/drivers/iio/adc/max9611.c
> > @@ -0,0 +1,590 @@
> > +/*
> > + * iio/adc/max9611.c
> > + *
> > + * Maxim max9611/max9612 high side current sense amplifier with
> > + * 12-bit ADC interface.
> > + *
> > + * Copyright (C) 2017 Jacopo Mondi
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +/*
> > + * This driver supports input common-mode voltage, current-sense
> > + * amplifier with programmable gains and die temperature reading from
> > + * Maxim max9611/max9612.
> > + *
> > + * Op-amp, analog comparator, and watchdog functionalities are not
> > + * supported by this driver.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/module.h>
> > +
> > +#define DRIVER_NAME "max9611"
> > +
> > +/* max9611 register addresses */
> > +#define MAX9611_REG_CSA_DATA 0x00
> > +#define MAX9611_REG_RS_DATA 0x02
> > +#define MAX9611_REG_TEMP_DATA 0x08
> > +#define MAX9611_REG_CTRL1 0x0a
> > +#define MAX9611_REG_CTRL2 0x0b
> > +
> > +/* max9611 REG1 mux configuration options */
> > +#define MAX9611_MUX_MASK 0x07
could use GENMASK()
> > +#define MAX9611_MUX_SENSE_1x 0x00
> > +#define MAX9611_MUX_SENSE_4x 0x01
> > +#define MAX9611_MUX_SENSE_8x 0x02
> > +#define MAX9611_INPUT_VOLT 0x03
> > +#define MAX9611_MUX_TEMP 0x06
maybe avoid mixed-case #defines
> > +/* max9611 voltage (both csa and input) helper macros */
> > +#define MAX9611_VOLTAGE_SHIFT 0x04
> > +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
> > +
> > +/*
> > + * max9611 current sense amplifier voltage output:
> > + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> > + *
> > + * GAIN LSB (nV) OFFSET (LSB steps)
> > + * 1x 107500 1
> > + * 4x 26880 1
> > + * 8x 13440 3
> > + *
> > + * The complete formula to calculate current sense voltage is:
> > + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> > + */
> > +#define CSA_VOLT_1x_LSB_nV 107500
> > +#define CSA_VOLT_4x_LSB_nV 26880
> > +#define CSA_VOLT_8x_LSB_nV 13440
> > +
> > +#define CSA_VOLT_1x_OFFS_RAW 1
> > +#define CSA_VOLT_4x_OFFS_RAW 1
> > +#define CSA_VOLT_8x_OFFS_RAW 3
> > +
> > +/*
> > + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> > + *
> > + * The complete formula to calculate input common voltage is:
> > + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> > + */
> > +#define CIM_VOLTAGE_LSB_mV 14
> > +#define CIM_VOLTAGE_OFFSET_RAW 1
> > +
> > +/*
> > + * max9611 temperature reading: LSB is 0.48 degrees Celsius
> > + *
> > + * The complete formula to calculate temperature is:
> > + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
> > + */
> I'd prefer these defines to be prefixed with MAX9611_
> Easiest to just do the lot. Some of these are 'standard' enough
> the might well clash with something that turns up in an included header
> at somepoint in the future.
>
> > +#define TEMP_SHIFT 0x07
> > +#define TEMP_MAX_RAW_POS 0x7f80
> > +#define TEMP_MAX_RAW_NEG 0xff80
> > +#define TEMP_MIN_RAW_NEG 0xd980
> > +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
could use GENMASK()
> > +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
> > +#define TEMP_LSB_mC 480
> > +#define TEMP_SCALE_NUM 1000
> > +#define TEMP_SCALE_DIV 2083
> > +
> > +struct max9611_dev {
> > + struct device *dev;
> > + struct i2c_client *i2c_client;
> > + struct mutex lock;
> > + unsigned int shunt_resistor_uohm;
> > +};
> > +
> > +enum max9611_conf_ids {
> > + CONF_SENSE_1x,
> > + CONF_SENSE_4x,
> > + CONF_SENSE_8x,
> > + CONF_IN_VOLT,
> > + CONF_TEMP,
> > +};
> > +
> > +/**
> > + * max9611_mux_conf - associate ADC mux configuration with register address
> > + * where data shall be read from
> > + */
> > +static unsigned int max9611_mux_conf[][2] = {
const
> > + /* CONF_SENSE_1x */
> > + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
> > + /* CONF_SENSE_4x */
> > + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
> > + /* CONF_SENSE_8x */
> > + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
> > + /* CONF_IN_VOLT */
> > + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
> > + /* CONF_TEMP */
> > + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
> > +};
> > +
> > +enum max9611_csa_gain {
> > + CSA_GAIN_1x,
> > + CSA_GAIN_4x,
> > + CSA_GAIN_8x,
> > +};
> > +
> > +enum max9611_csa_gain_params {
> > + CSA_GAIN_LSB_nV,
> > + CSA_GAIN_OFFS_RAW,
> > +};
> > +
> > +/**
> > + * max9611_csa_gain_conf - associate gain multiplier with LSB and
> > + * offset values.
> > + *
> > + * Group together parameters associated with configurable gain
> > + * on current sense amplifier path to ADC interface.
> > + * Current sense read routine adjusts gain until it gets a meaningful
> > + * value; use this structure to retrieve the correct LSB and offset values.
> > + */
> > +static unsigned int max9611_gain_conf[][2] = {
const
> > + { /* [0] CSA_GAIN_1x */
> > + CSA_VOLT_1x_LSB_nV,
> > + CSA_VOLT_1x_OFFS_RAW,
> > + },
> > + { /* [1] CSA_GAIN_4x */
> > + CSA_VOLT_4x_LSB_nV,
> > + CSA_VOLT_4x_OFFS_RAW,
> > + },
> > + { /* [2] CSA_GAIN_8x */
> > + CSA_VOLT_8x_LSB_nV,
> > + CSA_VOLT_8x_OFFS_RAW,
> > + },
> > +};
> > +
> > +enum max9611_chan_addrs {
> > + MAX9611_CHAN_VOLTAGE_INPUT,
> > + MAX9611_CHAN_VOLTAGE_SENSE,
> > + MAX9611_CHAN_TEMPERATURE,
> > + MAX9611_CHAN_CURRENT_LOAD,
> > + MAX9611_CHAN_POWER_LOAD,
> > +};
> > +
> > +static struct iio_chan_spec max9611_channels[] = {
const
> > + {
> > + .type = IIO_TEMP,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + .address = MAX9611_CHAN_TEMPERATURE,
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_OFFSET),
> > + .address = MAX9611_CHAN_VOLTAGE_INPUT,
> > + .indexed = 1,
> > + .channel = 1,
why is this indexed?
should be the only raw voltage channel
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_VOLTAGE_SENSE,
> > + .indexed = 1,
> > + .channel = 0,
> Unusual to have the channels in here other than in channel order...
why is this indexed?
should be the only processed voltage channel
> > + },
> > + {
> > + .type = IIO_CURRENT,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_CURRENT_LOAD,
> > + },
> > + {
> > + .type = IIO_POWER,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_POWER_LOAD
> > + },
> > +};
> > +
> > +/**
> > + * max9611_read_single() - read a single vale from ADC interface
> value
> > + *
> > + * Data registers are 16 bit long, spread between two 8 bit registers
> > + * with consecutive addresses.
> > + * Configure ADC mux first, then read register at address "reg_addr".
> > + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> > + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
maybe worth noting that data is MSB first or big-endian
> > + *
> > + * @max9611: max9611 device
> > + * @selector: index for mux and register configuration
> > + * @raw_val: the value returned from ADC
> > + */
> > +static int max9611_read_single(struct max9611_dev *max9611,
> > + enum max9611_conf_ids selector,
> > + u16 *raw_val)
> > +{
> > + int ret;
> > +
> > + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
> > + u8 reg_addr = max9611_mux_conf[selector][1];
> > +
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, mux_conf);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, mux_conf);
> > + return ret;
> > + }
> > +
> > + /*
> > + * need a delay here to make register configuration
> > + * stabilize. 1 msec at least, from empirical testing.
> > + */
> > + usleep_range(1000, 2000);
> > +
> > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
> > + if (ret < 0) {
> > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > + reg_addr);
> > + return ret;
> > + }
> > + *raw_val = ret;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * max9611_read_csa_voltage() - read current sense amplifier output voltage
> > + *
> > + * Current sense amplifier output voltage is read through a configurable
> > + * 1x, 4x or 8x gain.
> > + * Start with plain 1x gain, and adjust gain control properly until a
> > + * meaningful value is read from ADC output.
> > + *
> > + * @max9611: max9611 device
> > + * @adc_raw: raw value read from ADC output
> > + * @csa_gain: gain configuration option selector
> > + */
> > +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
> > + u16 *adc_raw,
> > + enum max9611_csa_gain *csa_gain)
> > +{
> > + enum max9611_conf_ids gain_selectors[] = {
> > + CONF_SENSE_1x,
> > + CONF_SENSE_4x,
> > + CONF_SENSE_8x
> > + };
> > + unsigned int i;
> > + int ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> > + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
> > + if (ret)
> > + return ret;
> > +
> > + if (*adc_raw > 0) {
> > + *csa_gain = gain_selectors[i];
> > + return 0;
> > + }
> > + }
> > +
> > + return -EIO;
> > +}
> > +
> > +static int max9611_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct max9611_dev *dev = iio_priv(indio_dev);
> > + enum max9611_csa_gain gain_selector;
> > + unsigned int *csa_gain;
> > + u16 adc_data;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&dev->lock);
> > +
> > + switch (chan->address) {
> > + case MAX9611_CHAN_TEMPERATURE:
> > + ret = max9611_read_single(dev, CONF_TEMP,
> > + &adc_data);
> > + if (ret)
> I'm not personally keen on jumping out of deep indentations like this
> just save on repeating one line. I'd pull the unlock back here and return
> directly as I feel it'll improve readability.
> Actually come to think of it, why does the lock need to be held for
> the next line anyway? adc_data is on the stack so doesn't matter if we
> have concurrent readers, once the i2c transaction is finished.
> Just unlock before checking ret.
> > + goto unlock_fail;
> > +
> > + *val = TEMP_RAW(adc_data);
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_INT;
> > +
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > + &adc_data);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + *val = MAX9611_VOLTAGE_RAW(adc_data);
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_INT;
> > + }
> > +
this falls through, no break?
I'd move
mutex_unlock(&dev->lock);
return IIO_VAL_INT;
here
> > + case IIO_CHAN_INFO_OFFSET:
> > + switch (chan->address) {
rather pointless for only one case?
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + *val = CIM_VOLTAGE_OFFSET_RAW;
> > +
> > + return IIO_VAL_INT;
> > + }
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->address) {
> > + case MAX9611_CHAN_TEMPERATURE:
> > + *val = TEMP_SCALE_NUM;
> > + *val2 = TEMP_SCALE_DIV;
> > +
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + *val = CIM_VOLTAGE_LSB_mV;
> > + return IIO_VAL_INT;
default: return -EINVAL;
> > + }
break; or something
> > +
> > + case IIO_CHAN_INFO_PROCESSED:
> > + mutex_lock(&dev->lock);
> > +
> > + switch (chan->address) {
> > + case MAX9611_CHAN_VOLTAGE_SENSE:
> > + /*
> > + * processed (mV): (raw - offset) * LSB (nV) / 10^6
> > + *
> > + * Even if max9611 can output raw csa voltage readings,
> > + * use a produced value as scale depends on gain.
> > + */
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV];
> > + *val2 = 1000000;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_CURRENT_LOAD:
> > + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV];
> > + *val2 = dev->shunt_resistor_uohm;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_POWER_LOAD:
> > + /*
> > + * processed (mW): Vin (mV) * Vcsa (uV) /
> > + * Rshunt (uOhm)
> > + */
> > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > + &adc_data);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + CIM_VOLTAGE_LSB_mV;
> > +
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + /* divide by 10^3 here to avoid 32bit overflow */
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV] / 1000;
> > + *val2 = dev->shunt_resistor_uohm;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > + }
> > + }
> > +
> > + ret = -EINVAL;
> > +
> > +unlock_fail:
> > + mutex_unlock(&dev->lock);
> > + return ret;
> > +
> > +}
> > +
> > +static ssize_t max9611_shunt_resistor_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
> > +
> > + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
> > +}
> > +
> > +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
> > + max9611_shunt_resistor_show, NULL, 0);
> > +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
> > + max9611_shunt_resistor_show, NULL, 0);
> > +
> > +static struct attribute *max9611_attributes[] = {
> > + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
> > + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group max9611_attribute_group = {
> > + .attrs = max9611_attributes,
> > +};
> > +
> > +static const struct iio_info indio_info = {
> > + .driver_module = THIS_MODULE,
> > + .read_raw = max9611_read_raw,
> > + .attrs = &max9611_attribute_group,
> > +};
> > +
> > +static int max9611_init(struct max9611_dev *max9611)
> > +{
> > + struct i2c_client *client = max9611->i2c_client;
> > + u16 regval;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> > + dev_err(max9611->dev,
> > + "No smbus support in I2c adapter: aborting probe.\n");
> This isn't necessarily an accurate message. I2c adapter might support
> smbus_read_byte only rather than word reads for example.
>
> Maybe make it more explict as to what we need?
> > + return -EINVAL;
> > + }
> > +
> > + /* Configure MUX to read temperature */
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > + return ret;
> > + }
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL2, 0);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL2, 0);
> > + return ret;
> > + }
> > + usleep_range(1000, 2000);
> > +
> > + /* Make sure die temperature is in range to test communications. */
> > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
> > + MAX9611_REG_TEMP_DATA);
> > + if (ret < 0) {
> > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > + MAX9611_REG_TEMP_DATA);
> > + return ret;
> > + }
> > + regval = ret & ~TEMP_MASK;
> > +
> > + if ((regval > TEMP_MAX_RAW_POS &&
> > + regval < TEMP_MIN_RAW_NEG) ||
> > + regval > TEMP_MAX_RAW_NEG) {
> > + dev_err(max9611->dev,
> > + "Invalid value received from ADC 0x%4x: aborting\n",
> > + regval);
> > + return -EIO;
> > + }
> > +
> > + /* Mux shall be zeroed back before applying other configurations */
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, 0);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, 0);
> > + return ret;
> > + }
> > + usleep_range(1000, 2000);
> > +
> > + return 0;
> > +}
> > +
> > +static int max9611_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + const char * const shunt_res_prop = "shunt-resistor-uohm";
> > + struct device_node *of_node = client->dev.of_node;
> > + struct max9611_dev *max9611;
> > + struct iio_dev *indio_dev;
> > + unsigned int of_shunt;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
> > + if (IS_ERR(indio_dev))
> > + return PTR_ERR(indio_dev);
> > +
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + max9611 = iio_priv(indio_dev);
> > + max9611->dev = &client->dev;
> > + max9611->i2c_client = client;
> > + mutex_init(&max9611->lock);
> > +
> > + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> > + if (ret) {
> > + dev_err(&client->dev,
> > + "Missing %s property for %s node\n",
> > + shunt_res_prop, of_node->full_name);
> > + return ret;
> > + }
> > + max9611->shunt_resistor_uohm = of_shunt;
> > +
> > + ret = max9611_init(max9611);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->dev.of_node = client->dev.of_node;
> > + indio_dev->name = client->dev.of_node->name;
> What's this going to give for the name? Name in IIO is supposed to
> be an indication of the part rather than anything more explicit.
> That's not easily obtained from device tree directly...
>
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &indio_info;
> > + indio_dev->channels = max9611_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
> > +
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id max9611_of_table[] = {
> > + {.compatible = "maxim,max9611"},
> > + {.compatible = "maxim,max9612"},
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, max9611_of_table);
> > +
> > +static struct i2c_driver max9611_driver = {
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = max9611_of_table,
> > + },
> > + .probe = max9611_probe,
> > +};
> > +module_i2c_driver(max9611_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
> > +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> > +MODULE_LICENSE("GPL v2");
> >
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-25 16:13 ` Peter Meerwald-Stadler
@ 2017-03-25 16:34 ` Jonathan Cameron
2017-03-26 10:08 ` jacopo
2017-03-27 8:30 ` jacopo
2 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2017-03-25 16:34 UTC (permalink / raw)
To: Peter Meerwald-Stadler, Jacopo Mondi; +Cc: geert, wsa+renesas, lars, linux-iio
On 25/03/17 16:13, Peter Meerwald-Stadler wrote:
>
> On Sat, 25 Mar 2017, Jonathan Cameron wrote:
>
> some more comments
>
>> On 24/03/17 15:28, Jacopo Mondi wrote:
>>> Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
>>> with 12-bits ADC interface.
>>>
>>> Datasheet publicly available at:
>>> https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> A few more little things inline. Coming together nicely.
>>
>> The channel set here is just odd enough that it might aid review to have
>> a quick listing of the resulting sysfs entries. One or two authors do
>> this an it is always useful for a quick sanity check.
>>
>> Perhaps even a set of typical values. Put this below the --- as we don't
>> need it in the git history, only to assist lazy reviewers like me ;)
>>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> drivers/iio/adc/Kconfig | 10 +
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 601 insertions(+)
>>> create mode 100644 drivers/iio/adc/max9611.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index dedae7a..82f2e7b8 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -354,6 +354,16 @@ config MAX1363
>>> To compile this driver as a module, choose M here: the module will be
>>> called max1363.
>>>
>>> +config MAX9611
>>> + tristate "Maxim max9611/max9612 ADC driver"
>>> + depends on I2C
>>> + help
>>> + Say yes here to build support for Maxim max9611/max9612 current sense
>>> + amplifier with 12-bits ADC interface.
>>> +
>>> + To compile this driver as a module, choose M here: the module will be
>>> + called max9611.
>>> +
>>> config MCP320X
>>> tristate "Microchip Technology MCP3x01/02/04/08"
>>> depends on SPI
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index d001262..149f979 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>>> obj-$(CONFIG_MAX1027) += max1027.o
>>> obj-$(CONFIG_MAX11100) += max11100.o
>>> obj-$(CONFIG_MAX1363) += max1363.o
>>> +obj-$(CONFIG_MAX9611) += max9611.o
>>> obj-$(CONFIG_MCP320X) += mcp320x.o
>>> obj-$(CONFIG_MCP3422) += mcp3422.o
>>> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>>> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
>>> new file mode 100644
>>> index 0000000..61566ec
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/max9611.c
>>> @@ -0,0 +1,590 @@
>>> +/*
>>> + * iio/adc/max9611.c
>>> + *
>>> + * Maxim max9611/max9612 high side current sense amplifier with
>>> + * 12-bit ADC interface.
>>> + *
>>> + * Copyright (C) 2017 Jacopo Mondi
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +/*
>>> + * This driver supports input common-mode voltage, current-sense
>>> + * amplifier with programmable gains and die temperature reading from
>>> + * Maxim max9611/max9612.
>>> + *
>>> + * Op-amp, analog comparator, and watchdog functionalities are not
>>> + * supported by this driver.
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/module.h>
>>> +
>>> +#define DRIVER_NAME "max9611"
>>> +
>>> +/* max9611 register addresses */
>>> +#define MAX9611_REG_CSA_DATA 0x00
>>> +#define MAX9611_REG_RS_DATA 0x02
>>> +#define MAX9611_REG_TEMP_DATA 0x08
>>> +#define MAX9611_REG_CTRL1 0x0a
>>> +#define MAX9611_REG_CTRL2 0x0b
>>> +
>>> +/* max9611 REG1 mux configuration options */
>>> +#define MAX9611_MUX_MASK 0x07
>
> could use GENMASK()
>
>>> +#define MAX9611_MUX_SENSE_1x 0x00
>>> +#define MAX9611_MUX_SENSE_4x 0x01
>>> +#define MAX9611_MUX_SENSE_8x 0x02
>>> +#define MAX9611_INPUT_VOLT 0x03
>>> +#define MAX9611_MUX_TEMP 0x06
>
> maybe avoid mixed-case #defines
>
>>> +/* max9611 voltage (both csa and input) helper macros */
>>> +#define MAX9611_VOLTAGE_SHIFT 0x04
>>> +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
>>> +
>>> +/*
>>> + * max9611 current sense amplifier voltage output:
>>> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
>>> + *
>>> + * GAIN LSB (nV) OFFSET (LSB steps)
>>> + * 1x 107500 1
>>> + * 4x 26880 1
>>> + * 8x 13440 3
>>> + *
>>> + * The complete formula to calculate current sense voltage is:
>>> + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
>>> + */
>>> +#define CSA_VOLT_1x_LSB_nV 107500
>>> +#define CSA_VOLT_4x_LSB_nV 26880
>>> +#define CSA_VOLT_8x_LSB_nV 13440
>>> +
>>> +#define CSA_VOLT_1x_OFFS_RAW 1
>>> +#define CSA_VOLT_4x_OFFS_RAW 1
>>> +#define CSA_VOLT_8x_OFFS_RAW 3
>>> +
>>> +/*
>>> + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
>>> + *
>>> + * The complete formula to calculate input common voltage is:
>>> + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
>>> + */
>>> +#define CIM_VOLTAGE_LSB_mV 14
>>> +#define CIM_VOLTAGE_OFFSET_RAW 1
>>> +
>>> +/*
>>> + * max9611 temperature reading: LSB is 0.48 degrees Celsius
>>> + *
>>> + * The complete formula to calculate temperature is:
>>> + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
>>> + */
>> I'd prefer these defines to be prefixed with MAX9611_
>> Easiest to just do the lot. Some of these are 'standard' enough
>> the might well clash with something that turns up in an included header
>> at somepoint in the future.
>>
>>> +#define TEMP_SHIFT 0x07
>>> +#define TEMP_MAX_RAW_POS 0x7f80
>>> +#define TEMP_MAX_RAW_NEG 0xff80
>>> +#define TEMP_MIN_RAW_NEG 0xd980
>>> +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
>
> could use GENMASK()
>
>>> +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
>>> +#define TEMP_LSB_mC 480
>>> +#define TEMP_SCALE_NUM 1000
>>> +#define TEMP_SCALE_DIV 2083
>>> +
>>> +struct max9611_dev {
>>> + struct device *dev;
>>> + struct i2c_client *i2c_client;
>>> + struct mutex lock;
>>> + unsigned int shunt_resistor_uohm;
>>> +};
>>> +
>>> +enum max9611_conf_ids {
>>> + CONF_SENSE_1x,
>>> + CONF_SENSE_4x,
>>> + CONF_SENSE_8x,
>>> + CONF_IN_VOLT,
>>> + CONF_TEMP,
>>> +};
>>> +
>>> +/**
>>> + * max9611_mux_conf - associate ADC mux configuration with register address
>>> + * where data shall be read from
>>> + */
>>> +static unsigned int max9611_mux_conf[][2] = {
>
> const
>
>>> + /* CONF_SENSE_1x */
>>> + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_SENSE_4x */
>>> + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_SENSE_8x */
>>> + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_IN_VOLT */
>>> + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
>>> + /* CONF_TEMP */
>>> + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
>>> +};
>>> +
>>> +enum max9611_csa_gain {
>>> + CSA_GAIN_1x,
>>> + CSA_GAIN_4x,
>>> + CSA_GAIN_8x,
>>> +};
>>> +
>>> +enum max9611_csa_gain_params {
>>> + CSA_GAIN_LSB_nV,
>>> + CSA_GAIN_OFFS_RAW,
>>> +};
>>> +
>>> +/**
>>> + * max9611_csa_gain_conf - associate gain multiplier with LSB and
>>> + * offset values.
>>> + *
>>> + * Group together parameters associated with configurable gain
>>> + * on current sense amplifier path to ADC interface.
>>> + * Current sense read routine adjusts gain until it gets a meaningful
>>> + * value; use this structure to retrieve the correct LSB and offset values.
>>> + */
>>> +static unsigned int max9611_gain_conf[][2] = {
>
> const
>
>>> + { /* [0] CSA_GAIN_1x */
>>> + CSA_VOLT_1x_LSB_nV,
>>> + CSA_VOLT_1x_OFFS_RAW,
>>> + },
>>> + { /* [1] CSA_GAIN_4x */
>>> + CSA_VOLT_4x_LSB_nV,
>>> + CSA_VOLT_4x_OFFS_RAW,
>>> + },
>>> + { /* [2] CSA_GAIN_8x */
>>> + CSA_VOLT_8x_LSB_nV,
>>> + CSA_VOLT_8x_OFFS_RAW,
>>> + },
>>> +};
>>> +
>>> +enum max9611_chan_addrs {
>>> + MAX9611_CHAN_VOLTAGE_INPUT,
>>> + MAX9611_CHAN_VOLTAGE_SENSE,
>>> + MAX9611_CHAN_TEMPERATURE,
>>> + MAX9611_CHAN_CURRENT_LOAD,
>>> + MAX9611_CHAN_POWER_LOAD,
>>> +};
>>> +
>>> +static struct iio_chan_spec max9611_channels[] = {
>
> const
>
>>> + {
>>> + .type = IIO_TEMP,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE),
>>> + .address = MAX9611_CHAN_TEMPERATURE,
>>> + },
>>> + {
>>> + .type = IIO_VOLTAGE,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>> + BIT(IIO_CHAN_INFO_OFFSET),
>>> + .address = MAX9611_CHAN_VOLTAGE_INPUT,
>>> + .indexed = 1,
>>> + .channel = 1,
>
> why is this indexed?
> should be the only raw voltage channel
>
>
>>> + },
>>> + {
>>> + .type = IIO_VOLTAGE,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_VOLTAGE_SENSE,
>>> + .indexed = 1,
>>> + .channel = 0,
>> Unusual to have the channels in here other than in channel order...
>
> why is this indexed?
> should be the only processed voltage channel
I'd rather we didn't make this distinction. Potentially might
cause trouble down the line if we have events or something similar.
>
>
>>> + },
>>> + {
>>> + .type = IIO_CURRENT,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_CURRENT_LOAD,
>>> + },
>>> + {
>>> + .type = IIO_POWER,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_POWER_LOAD
>>> + },
>>> +};
>>> +
>>> +/**
>>> + * max9611_read_single() - read a single vale from ADC interface
>> value
>>> + *
>>> + * Data registers are 16 bit long, spread between two 8 bit registers
>>> + * with consecutive addresses.
>>> + * Configure ADC mux first, then read register at address "reg_addr".
>>> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
>>> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>
> maybe worth noting that data is MSB first or big-endian
>
>>> + *
>>> + * @max9611: max9611 device
>>> + * @selector: index for mux and register configuration
>>> + * @raw_val: the value returned from ADC
>>> + */
>>> +static int max9611_read_single(struct max9611_dev *max9611,
>>> + enum max9611_conf_ids selector,
>>> + u16 *raw_val)
>>> +{
>>> + int ret;
>>> +
>>> + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
>>> + u8 reg_addr = max9611_mux_conf[selector][1];
>>> +
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, mux_conf);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, mux_conf);
>>> + return ret;
>>> + }
>>> +
>>> + /*
>>> + * need a delay here to make register configuration
>>> + * stabilize. 1 msec at least, from empirical testing.
>>> + */
>>> + usleep_range(1000, 2000);
>>> +
>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
>>> + if (ret < 0) {
>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>> + reg_addr);
>>> + return ret;
>>> + }
>>> + *raw_val = ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * max9611_read_csa_voltage() - read current sense amplifier output voltage
>>> + *
>>> + * Current sense amplifier output voltage is read through a configurable
>>> + * 1x, 4x or 8x gain.
>>> + * Start with plain 1x gain, and adjust gain control properly until a
>>> + * meaningful value is read from ADC output.
>>> + *
>>> + * @max9611: max9611 device
>>> + * @adc_raw: raw value read from ADC output
>>> + * @csa_gain: gain configuration option selector
>>> + */
>>> +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
>>> + u16 *adc_raw,
>>> + enum max9611_csa_gain *csa_gain)
>>> +{
>>> + enum max9611_conf_ids gain_selectors[] = {
>>> + CONF_SENSE_1x,
>>> + CONF_SENSE_4x,
>>> + CONF_SENSE_8x
>>> + };
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
>>> + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (*adc_raw > 0) {
>>> + *csa_gain = gain_selectors[i];
>>> + return 0;
>>> + }
>>> + }
>>> +
>>> + return -EIO;
>>> +}
>>> +
>>> +static int max9611_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct max9611_dev *dev = iio_priv(indio_dev);
>>> + enum max9611_csa_gain gain_selector;
>>> + unsigned int *csa_gain;
>>> + u16 adc_data;
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + mutex_lock(&dev->lock);
>>> +
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_TEMPERATURE:
>>> + ret = max9611_read_single(dev, CONF_TEMP,
>>> + &adc_data);
>>> + if (ret)
>> I'm not personally keen on jumping out of deep indentations like this
>> just save on repeating one line. I'd pull the unlock back here and return
>> directly as I feel it'll improve readability.
>> Actually come to think of it, why does the lock need to be held for
>> the next line anyway? adc_data is on the stack so doesn't matter if we
>> have concurrent readers, once the i2c transaction is finished.
>> Just unlock before checking ret.
>>> + goto unlock_fail;
>>> +
>>> + *val = TEMP_RAW(adc_data);
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_INT;
>>> +
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>> + &adc_data);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data);
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>
> this falls through, no break?
>
> I'd move
> mutex_unlock(&dev->lock);
> return IIO_VAL_INT;
> here
>
>
>>> + case IIO_CHAN_INFO_OFFSET:
>>> + switch (chan->address) {
>
> rather pointless for only one case?
>
>
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + *val = CIM_VOLTAGE_OFFSET_RAW;
>>> +
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_TEMPERATURE:
>>> + *val = TEMP_SCALE_NUM;
>>> + *val2 = TEMP_SCALE_DIV;
>>> +
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + *val = CIM_VOLTAGE_LSB_mV;
>>> + return IIO_VAL_INT;
>
> default: return -EINVAL;
>
>>> + }
>
> break; or something
>
>>> +
>>> + case IIO_CHAN_INFO_PROCESSED:
>>> + mutex_lock(&dev->lock);
>>> +
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_VOLTAGE_SENSE:
>>> + /*
>>> + * processed (mV): (raw - offset) * LSB (nV) / 10^6
>>> + *
>>> + * Even if max9611 can output raw csa voltage readings,
>>> + * use a produced value as scale depends on gain.
>>> + */
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV];
>>> + *val2 = 1000000;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_CURRENT_LOAD:
>>> + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV];
>>> + *val2 = dev->shunt_resistor_uohm;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_POWER_LOAD:
>>> + /*
>>> + * processed (mW): Vin (mV) * Vcsa (uV) /
>>> + * Rshunt (uOhm)
>>> + */
>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>> + &adc_data);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + CIM_VOLTAGE_LSB_mV;
>>> +
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + /* divide by 10^3 here to avoid 32bit overflow */
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV] / 1000;
>>> + *val2 = dev->shunt_resistor_uohm;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> + }
>>> + }
>>> +
>>> + ret = -EINVAL;
>>> +
>>> +unlock_fail:
>>> + mutex_unlock(&dev->lock);
>>> + return ret;
>>> +
>>> +}
>>> +
>>> +static ssize_t max9611_shunt_resistor_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
>>> +
>>> + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
>>> + max9611_shunt_resistor_show, NULL, 0);
>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
>>> + max9611_shunt_resistor_show, NULL, 0);
>>> +
>>> +static struct attribute *max9611_attributes[] = {
>>> + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
>>> + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group max9611_attribute_group = {
>>> + .attrs = max9611_attributes,
>>> +};
>>> +
>>> +static const struct iio_info indio_info = {
>>> + .driver_module = THIS_MODULE,
>>> + .read_raw = max9611_read_raw,
>>> + .attrs = &max9611_attribute_group,
>>> +};
>>> +
>>> +static int max9611_init(struct max9611_dev *max9611)
>>> +{
>>> + struct i2c_client *client = max9611->i2c_client;
>>> + u16 regval;
>>> + int ret;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_WRITE_BYTE |
>>> + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
>>> + dev_err(max9611->dev,
>>> + "No smbus support in I2c adapter: aborting probe.\n");
>> This isn't necessarily an accurate message. I2c adapter might support
>> smbus_read_byte only rather than word reads for example.
>>
>> Maybe make it more explict as to what we need?
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Configure MUX to read temperature */
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>> + return ret;
>>> + }
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL2, 0);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL2, 0);
>>> + return ret;
>>> + }
>>> + usleep_range(1000, 2000);
>>> +
>>> + /* Make sure die temperature is in range to test communications. */
>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
>>> + MAX9611_REG_TEMP_DATA);
>>> + if (ret < 0) {
>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>> + MAX9611_REG_TEMP_DATA);
>>> + return ret;
>>> + }
>>> + regval = ret & ~TEMP_MASK;
>>> +
>>> + if ((regval > TEMP_MAX_RAW_POS &&
>>> + regval < TEMP_MIN_RAW_NEG) ||
>>> + regval > TEMP_MAX_RAW_NEG) {
>>> + dev_err(max9611->dev,
>>> + "Invalid value received from ADC 0x%4x: aborting\n",
>>> + regval);
>>> + return -EIO;
>>> + }
>>> +
>>> + /* Mux shall be zeroed back before applying other configurations */
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, 0);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, 0);
>>> + return ret;
>>> + }
>>> + usleep_range(1000, 2000);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int max9611_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + const char * const shunt_res_prop = "shunt-resistor-uohm";
>>> + struct device_node *of_node = client->dev.of_node;
>>> + struct max9611_dev *max9611;
>>> + struct iio_dev *indio_dev;
>>> + unsigned int of_shunt;
>>> + int ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
>>> + if (IS_ERR(indio_dev))
>>> + return PTR_ERR(indio_dev);
>>> +
>>> + i2c_set_clientdata(client, indio_dev);
>>> +
>>> + max9611 = iio_priv(indio_dev);
>>> + max9611->dev = &client->dev;
>>> + max9611->i2c_client = client;
>>> + mutex_init(&max9611->lock);
>>> +
>>> + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
>>> + if (ret) {
>>> + dev_err(&client->dev,
>>> + "Missing %s property for %s node\n",
>>> + shunt_res_prop, of_node->full_name);
>>> + return ret;
>>> + }
>>> + max9611->shunt_resistor_uohm = of_shunt;
>>> +
>>> + ret = max9611_init(max9611);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + indio_dev->dev.parent = &client->dev;
>>> + indio_dev->dev.of_node = client->dev.of_node;
>>> + indio_dev->name = client->dev.of_node->name;
>> What's this going to give for the name? Name in IIO is supposed to
>> be an indication of the part rather than anything more explicit.
>> That's not easily obtained from device tree directly...
>>
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->info = &indio_info;
>>> + indio_dev->channels = max9611_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
>>> +
>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +static const struct of_device_id max9611_of_table[] = {
>>> + {.compatible = "maxim,max9611"},
>>> + {.compatible = "maxim,max9612"},
>>> + { },
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
>>> +
>>> +static struct i2c_driver max9611_driver = {
>>> + .driver = {
>>> + .name = DRIVER_NAME,
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = max9611_of_table,
>>> + },
>>> + .probe = max9611_probe,
>>> +};
>>> +module_i2c_driver(max9611_driver);
>>> +
>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-25 16:13 ` Peter Meerwald-Stadler
2017-03-25 16:34 ` Jonathan Cameron
@ 2017-03-26 10:08 ` jacopo
2017-03-26 10:24 ` Jonathan Cameron
2017-03-27 8:30 ` jacopo
2 siblings, 1 reply; 39+ messages in thread
From: jacopo @ 2017-03-26 10:08 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Jacopo Mondi, Jonathan Cameron, geert, wsa+renesas, lars,
linux-iio
Hi Peter,
thanks for review
On Sat, Mar 25, 2017 at 05:13:38PM +0100, Peter Meerwald-Stadler wrote:
>
> On Sat, 25 Mar 2017, Jonathan Cameron wrote:
>
> some more comments
>
> > On 24/03/17 15:28, Jacopo Mondi wrote:
> > > Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
> > > with 12-bits ADC interface.
> > >
> > > Datasheet publicly available at:
> > > https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > A few more little things inline. Coming together nicely.
> >
> > The channel set here is just odd enough that it might aid review to have
> > a quick listing of the resulting sysfs entries. One or two authors do
> > this an it is always useful for a quick sanity check.
> >
> > Perhaps even a set of typical values. Put this below the --- as we don't
> > need it in the git history, only to assist lazy reviewers like me ;)
> >
> > Thanks,
> >
> > Jonathan
> > > ---
> > > drivers/iio/adc/Kconfig | 10 +
> > > drivers/iio/adc/Makefile | 1 +
> > > drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 601 insertions(+)
> > > create mode 100644 drivers/iio/adc/max9611.c
> > >
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index dedae7a..82f2e7b8 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -354,6 +354,16 @@ config MAX1363
> > > To compile this driver as a module, choose M here: the module will be
> > > called max1363.
> > >
> > > +config MAX9611
> > > + tristate "Maxim max9611/max9612 ADC driver"
> > > + depends on I2C
> > > + help
> > > + Say yes here to build support for Maxim max9611/max9612 current sense
> > > + amplifier with 12-bits ADC interface.
> > > +
> > > + To compile this driver as a module, choose M here: the module will be
> > > + called max9611.
> > > +
> > > config MCP320X
> > > tristate "Microchip Technology MCP3x01/02/04/08"
> > > depends on SPI
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index d001262..149f979 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
> > > obj-$(CONFIG_MAX1027) += max1027.o
> > > obj-$(CONFIG_MAX11100) += max11100.o
> > > obj-$(CONFIG_MAX1363) += max1363.o
> > > +obj-$(CONFIG_MAX9611) += max9611.o
> > > obj-$(CONFIG_MCP320X) += mcp320x.o
> > > obj-$(CONFIG_MCP3422) += mcp3422.o
> > > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > > new file mode 100644
> > > index 0000000..61566ec
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -0,0 +1,590 @@
> > > +/*
> > > + * iio/adc/max9611.c
> > > + *
> > > + * Maxim max9611/max9612 high side current sense amplifier with
> > > + * 12-bit ADC interface.
> > > + *
> > > + * Copyright (C) 2017 Jacopo Mondi
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +
> > > +/*
> > > + * This driver supports input common-mode voltage, current-sense
> > > + * amplifier with programmable gains and die temperature reading from
> > > + * Maxim max9611/max9612.
> > > + *
> > > + * Op-amp, analog comparator, and watchdog functionalities are not
> > > + * supported by this driver.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/module.h>
> > > +
> > > +#define DRIVER_NAME "max9611"
> > > +
> > > +/* max9611 register addresses */
> > > +#define MAX9611_REG_CSA_DATA 0x00
> > > +#define MAX9611_REG_RS_DATA 0x02
> > > +#define MAX9611_REG_TEMP_DATA 0x08
> > > +#define MAX9611_REG_CTRL1 0x0a
> > > +#define MAX9611_REG_CTRL2 0x0b
> > > +
> > > +/* max9611 REG1 mux configuration options */
> > > +#define MAX9611_MUX_MASK 0x07
>
> could use GENMASK()
>
> > > +#define MAX9611_MUX_SENSE_1x 0x00
> > > +#define MAX9611_MUX_SENSE_4x 0x01
> > > +#define MAX9611_MUX_SENSE_8x 0x02
> > > +#define MAX9611_INPUT_VOLT 0x03
> > > +#define MAX9611_MUX_TEMP 0x06
>
> maybe avoid mixed-case #defines
>
> > > +/* max9611 voltage (both csa and input) helper macros */
> > > +#define MAX9611_VOLTAGE_SHIFT 0x04
> > > +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
> > > +
> > > +/*
> > > + * max9611 current sense amplifier voltage output:
> > > + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> > > + *
> > > + * GAIN LSB (nV) OFFSET (LSB steps)
> > > + * 1x 107500 1
> > > + * 4x 26880 1
> > > + * 8x 13440 3
> > > + *
> > > + * The complete formula to calculate current sense voltage is:
> > > + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> > > + */
> > > +#define CSA_VOLT_1x_LSB_nV 107500
> > > +#define CSA_VOLT_4x_LSB_nV 26880
> > > +#define CSA_VOLT_8x_LSB_nV 13440
> > > +
> > > +#define CSA_VOLT_1x_OFFS_RAW 1
> > > +#define CSA_VOLT_4x_OFFS_RAW 1
> > > +#define CSA_VOLT_8x_OFFS_RAW 3
> > > +
> > > +/*
> > > + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> > > + *
> > > + * The complete formula to calculate input common voltage is:
> > > + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> > > + */
> > > +#define CIM_VOLTAGE_LSB_mV 14
> > > +#define CIM_VOLTAGE_OFFSET_RAW 1
> > > +
> > > +/*
> > > + * max9611 temperature reading: LSB is 0.48 degrees Celsius
> > > + *
> > > + * The complete formula to calculate temperature is:
> > > + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
> > > + */
> > I'd prefer these defines to be prefixed with MAX9611_
> > Easiest to just do the lot. Some of these are 'standard' enough
> > the might well clash with something that turns up in an included header
> > at somepoint in the future.
> >
> > > +#define TEMP_SHIFT 0x07
> > > +#define TEMP_MAX_RAW_POS 0x7f80
> > > +#define TEMP_MAX_RAW_NEG 0xff80
> > > +#define TEMP_MIN_RAW_NEG 0xd980
> > > +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
>
> could use GENMASK()
>
> > > +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
> > > +#define TEMP_LSB_mC 480
> > > +#define TEMP_SCALE_NUM 1000
> > > +#define TEMP_SCALE_DIV 2083
> > > +
> > > +struct max9611_dev {
> > > + struct device *dev;
> > > + struct i2c_client *i2c_client;
> > > + struct mutex lock;
> > > + unsigned int shunt_resistor_uohm;
> > > +};
> > > +
> > > +enum max9611_conf_ids {
> > > + CONF_SENSE_1x,
> > > + CONF_SENSE_4x,
> > > + CONF_SENSE_8x,
> > > + CONF_IN_VOLT,
> > > + CONF_TEMP,
> > > +};
> > > +
> > > +/**
> > > + * max9611_mux_conf - associate ADC mux configuration with register address
> > > + * where data shall be read from
> > > + */
> > > +static unsigned int max9611_mux_conf[][2] = {
>
> const
>
> > > + /* CONF_SENSE_1x */
> > > + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
> > > + /* CONF_SENSE_4x */
> > > + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
> > > + /* CONF_SENSE_8x */
> > > + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
> > > + /* CONF_IN_VOLT */
> > > + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
> > > + /* CONF_TEMP */
> > > + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
> > > +};
> > > +
> > > +enum max9611_csa_gain {
> > > + CSA_GAIN_1x,
> > > + CSA_GAIN_4x,
> > > + CSA_GAIN_8x,
> > > +};
> > > +
> > > +enum max9611_csa_gain_params {
> > > + CSA_GAIN_LSB_nV,
> > > + CSA_GAIN_OFFS_RAW,
> > > +};
> > > +
> > > +/**
> > > + * max9611_csa_gain_conf - associate gain multiplier with LSB and
> > > + * offset values.
> > > + *
> > > + * Group together parameters associated with configurable gain
> > > + * on current sense amplifier path to ADC interface.
> > > + * Current sense read routine adjusts gain until it gets a meaningful
> > > + * value; use this structure to retrieve the correct LSB and offset values.
> > > + */
> > > +static unsigned int max9611_gain_conf[][2] = {
>
> const
>
> > > + { /* [0] CSA_GAIN_1x */
> > > + CSA_VOLT_1x_LSB_nV,
> > > + CSA_VOLT_1x_OFFS_RAW,
> > > + },
> > > + { /* [1] CSA_GAIN_4x */
> > > + CSA_VOLT_4x_LSB_nV,
> > > + CSA_VOLT_4x_OFFS_RAW,
> > > + },
> > > + { /* [2] CSA_GAIN_8x */
> > > + CSA_VOLT_8x_LSB_nV,
> > > + CSA_VOLT_8x_OFFS_RAW,
> > > + },
> > > +};
> > > +
> > > +enum max9611_chan_addrs {
> > > + MAX9611_CHAN_VOLTAGE_INPUT,
> > > + MAX9611_CHAN_VOLTAGE_SENSE,
> > > + MAX9611_CHAN_TEMPERATURE,
> > > + MAX9611_CHAN_CURRENT_LOAD,
> > > + MAX9611_CHAN_POWER_LOAD,
> > > +};
> > > +
> > > +static struct iio_chan_spec max9611_channels[] = {
>
> const
>
> > > + {
> > > + .type = IIO_TEMP,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> > > + .address = MAX9611_CHAN_TEMPERATURE,
> > > + },
> > > + {
> > > + .type = IIO_VOLTAGE,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > + BIT(IIO_CHAN_INFO_SCALE) |
> > > + BIT(IIO_CHAN_INFO_OFFSET),
> > > + .address = MAX9611_CHAN_VOLTAGE_INPUT,
> > > + .indexed = 1,
> > > + .channel = 1,
>
> why is this indexed?
> should be the only raw voltage channel
>
>
> > > + },
> > > + {
> > > + .type = IIO_VOLTAGE,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > > + .address = MAX9611_CHAN_VOLTAGE_SENSE,
> > > + .indexed = 1,
> > > + .channel = 0,
> > Unusual to have the channels in here other than in channel order...
>
> why is this indexed?
> should be the only processed voltage channel
>
Jonathan already replied on this, but I would add that with no
indexing this would appear in userspace as:
in_voltage_raw
in_voltage_scale
in_voltage_offset
in_offset_processed
which is confusing at best.
Hope is ok if I keep this indexed (in correct order as Jonathan said)
>
> > > + },
> > > + {
> > > + .type = IIO_CURRENT,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > > + .address = MAX9611_CHAN_CURRENT_LOAD,
> > > + },
> > > + {
> > > + .type = IIO_POWER,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > > + .address = MAX9611_CHAN_POWER_LOAD
> > > + },
> > > +};
> > > +
> > > +/**
> > > + * max9611_read_single() - read a single vale from ADC interface
> > value
> > > + *
> > > + * Data registers are 16 bit long, spread between two 8 bit registers
> > > + * with consecutive addresses.
> > > + * Configure ADC mux first, then read register at address "reg_addr".
> > > + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> > > + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>
> maybe worth noting that data is MSB first or big-endian
>
> > > + *
> > > + * @max9611: max9611 device
> > > + * @selector: index for mux and register configuration
> > > + * @raw_val: the value returned from ADC
> > > + */
> > > +static int max9611_read_single(struct max9611_dev *max9611,
> > > + enum max9611_conf_ids selector,
> > > + u16 *raw_val)
> > > +{
> > > + int ret;
> > > +
> > > + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
> > > + u8 reg_addr = max9611_mux_conf[selector][1];
> > > +
> > > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > > + MAX9611_REG_CTRL1, mux_conf);
> > > + if (ret) {
> > > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > > + MAX9611_REG_CTRL1, mux_conf);
> > > + return ret;
> > > + }
> > > +
> > > + /*
> > > + * need a delay here to make register configuration
> > > + * stabilize. 1 msec at least, from empirical testing.
> > > + */
> > > + usleep_range(1000, 2000);
> > > +
> > > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
> > > + if (ret < 0) {
> > > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > > + reg_addr);
> > > + return ret;
> > > + }
> > > + *raw_val = ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * max9611_read_csa_voltage() - read current sense amplifier output voltage
> > > + *
> > > + * Current sense amplifier output voltage is read through a configurable
> > > + * 1x, 4x or 8x gain.
> > > + * Start with plain 1x gain, and adjust gain control properly until a
> > > + * meaningful value is read from ADC output.
> > > + *
> > > + * @max9611: max9611 device
> > > + * @adc_raw: raw value read from ADC output
> > > + * @csa_gain: gain configuration option selector
> > > + */
> > > +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
> > > + u16 *adc_raw,
> > > + enum max9611_csa_gain *csa_gain)
> > > +{
> > > + enum max9611_conf_ids gain_selectors[] = {
> > > + CONF_SENSE_1x,
> > > + CONF_SENSE_4x,
> > > + CONF_SENSE_8x
> > > + };
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> > > + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (*adc_raw > 0) {
> > > + *csa_gain = gain_selectors[i];
> > > + return 0;
> > > + }
> > > + }
> > > +
> > > + return -EIO;
> > > +}
> > > +
> > > +static int max9611_read_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int *val, int *val2, long mask)
> > > +{
> > > + struct max9611_dev *dev = iio_priv(indio_dev);
> > > + enum max9611_csa_gain gain_selector;
> > > + unsigned int *csa_gain;
> > > + u16 adc_data;
> > > + int ret;
> > > +
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + mutex_lock(&dev->lock);
> > > +
> > > + switch (chan->address) {
> > > + case MAX9611_CHAN_TEMPERATURE:
> > > + ret = max9611_read_single(dev, CONF_TEMP,
> > > + &adc_data);
> > > + if (ret)
> > I'm not personally keen on jumping out of deep indentations like this
> > just save on repeating one line. I'd pull the unlock back here and return
> > directly as I feel it'll improve readability.
> > Actually come to think of it, why does the lock need to be held for
> > the next line anyway? adc_data is on the stack so doesn't matter if we
> > have concurrent readers, once the i2c transaction is finished.
> > Just unlock before checking ret.
> > > + goto unlock_fail;
> > > +
> > > + *val = TEMP_RAW(adc_data);
> > > +
> > > + mutex_unlock(&dev->lock);
> > > + return IIO_VAL_INT;
> > > +
> > > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > > + &adc_data);
> > > + if (ret)
> > > + goto unlock_fail;
> > > +
> > > + *val = MAX9611_VOLTAGE_RAW(adc_data);
> > > +
> > > + mutex_unlock(&dev->lock);
> > > + return IIO_VAL_INT;
> > > + }
> > > +
>
> this falls through, no break?
>
> I'd move
> mutex_unlock(&dev->lock);
> return IIO_VAL_INT;
> here
>
>
> > > + case IIO_CHAN_INFO_OFFSET:
> > > + switch (chan->address) {
>
> rather pointless for only one case?
>
>
> > > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > > + *val = CIM_VOLTAGE_OFFSET_RAW;
> > > +
> > > + return IIO_VAL_INT;
> > > + }
> > > +
> > > + case IIO_CHAN_INFO_SCALE:
> > > + switch (chan->address) {
> > > + case MAX9611_CHAN_TEMPERATURE:
> > > + *val = TEMP_SCALE_NUM;
> > > + *val2 = TEMP_SCALE_DIV;
> > > +
> > > + return IIO_VAL_FRACTIONAL;
> > > +
> > > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > > + *val = CIM_VOLTAGE_LSB_mV;
> > > + return IIO_VAL_INT;
>
> default: return -EINVAL;
>
> > > + }
>
> break; or something
>
I'll rewrite this with this and other comments in mind
Thanks
j
> > > +
> > > + case IIO_CHAN_INFO_PROCESSED:
> > > + mutex_lock(&dev->lock);
> > > +
> > > + switch (chan->address) {
> > > + case MAX9611_CHAN_VOLTAGE_SENSE:
> > > + /*
> > > + * processed (mV): (raw - offset) * LSB (nV) / 10^6
> > > + *
> > > + * Even if max9611 can output raw csa voltage readings,
> > > + * use a produced value as scale depends on gain.
> > > + */
> > > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > > + &gain_selector);
> > > + if (ret)
> > > + goto unlock_fail;
> > > +
> > > + csa_gain = max9611_gain_conf[gain_selector];
> > > +
> > > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > > + csa_gain[CSA_GAIN_LSB_nV];
> > > + *val2 = 1000000;
> > > +
> > > + mutex_unlock(&dev->lock);
> > > + return IIO_VAL_FRACTIONAL;
> > > +
> > > + case MAX9611_CHAN_CURRENT_LOAD:
> > > + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
> > > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > > + &gain_selector);
> > > + if (ret)
> > > + goto unlock_fail;
> > > +
> > > + csa_gain = max9611_gain_conf[gain_selector];
> > > +
> > > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > > + csa_gain[CSA_GAIN_LSB_nV];
> > > + *val2 = dev->shunt_resistor_uohm;
> > > +
> > > + mutex_unlock(&dev->lock);
> > > + return IIO_VAL_FRACTIONAL;
> > > +
> > > + case MAX9611_CHAN_POWER_LOAD:
> > > + /*
> > > + * processed (mW): Vin (mV) * Vcsa (uV) /
> > > + * Rshunt (uOhm)
> > > + */
> > > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > > + &adc_data);
> > > + if (ret)
> > > + goto unlock_fail;
> > > +
> > > + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
> > > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > > + CIM_VOLTAGE_LSB_mV;
> > > +
> > > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > > + &gain_selector);
> > > + if (ret)
> > > + goto unlock_fail;
> > > +
> > > + csa_gain = max9611_gain_conf[gain_selector];
> > > +
> > > + /* divide by 10^3 here to avoid 32bit overflow */
> > > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > > + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
> > > + csa_gain[CSA_GAIN_LSB_nV] / 1000;
> > > + *val2 = dev->shunt_resistor_uohm;
> > > +
> > > + mutex_unlock(&dev->lock);
> > > + return IIO_VAL_FRACTIONAL;
> > > + }
> > > + }
> > > +
> > > + ret = -EINVAL;
> > > +
> > > +unlock_fail:
> > > + mutex_unlock(&dev->lock);
> > > + return ret;
> > > +
> > > +}
> > > +
> > > +static ssize_t max9611_shunt_resistor_show(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
> > > +
> > > + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
> > > +}
> > > +
> > > +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
> > > + max9611_shunt_resistor_show, NULL, 0);
> > > +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
> > > + max9611_shunt_resistor_show, NULL, 0);
> > > +
> > > +static struct attribute *max9611_attributes[] = {
> > > + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
> > > + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
> > > + NULL,
> > > +};
> > > +
> > > +static const struct attribute_group max9611_attribute_group = {
> > > + .attrs = max9611_attributes,
> > > +};
> > > +
> > > +static const struct iio_info indio_info = {
> > > + .driver_module = THIS_MODULE,
> > > + .read_raw = max9611_read_raw,
> > > + .attrs = &max9611_attribute_group,
> > > +};
> > > +
> > > +static int max9611_init(struct max9611_dev *max9611)
> > > +{
> > > + struct i2c_client *client = max9611->i2c_client;
> > > + u16 regval;
> > > + int ret;
> > > +
> > > + if (!i2c_check_functionality(client->adapter,
> > > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > > + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> > > + dev_err(max9611->dev,
> > > + "No smbus support in I2c adapter: aborting probe.\n");
> > This isn't necessarily an accurate message. I2c adapter might support
> > smbus_read_byte only rather than word reads for example.
> >
> > Maybe make it more explict as to what we need?
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Configure MUX to read temperature */
> > > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > > + if (ret) {
> > > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > > + return ret;
> > > + }
> > > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > > + MAX9611_REG_CTRL2, 0);
> > > + if (ret) {
> > > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > > + MAX9611_REG_CTRL2, 0);
> > > + return ret;
> > > + }
> > > + usleep_range(1000, 2000);
> > > +
> > > + /* Make sure die temperature is in range to test communications. */
> > > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
> > > + MAX9611_REG_TEMP_DATA);
> > > + if (ret < 0) {
> > > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > > + MAX9611_REG_TEMP_DATA);
> > > + return ret;
> > > + }
> > > + regval = ret & ~TEMP_MASK;
> > > +
> > > + if ((regval > TEMP_MAX_RAW_POS &&
> > > + regval < TEMP_MIN_RAW_NEG) ||
> > > + regval > TEMP_MAX_RAW_NEG) {
> > > + dev_err(max9611->dev,
> > > + "Invalid value received from ADC 0x%4x: aborting\n",
> > > + regval);
> > > + return -EIO;
> > > + }
> > > +
> > > + /* Mux shall be zeroed back before applying other configurations */
> > > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > > + MAX9611_REG_CTRL1, 0);
> > > + if (ret) {
> > > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > > + MAX9611_REG_CTRL1, 0);
> > > + return ret;
> > > + }
> > > + usleep_range(1000, 2000);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int max9611_probe(struct i2c_client *client,
> > > + const struct i2c_device_id *id)
> > > +{
> > > + const char * const shunt_res_prop = "shunt-resistor-uohm";
> > > + struct device_node *of_node = client->dev.of_node;
> > > + struct max9611_dev *max9611;
> > > + struct iio_dev *indio_dev;
> > > + unsigned int of_shunt;
> > > + int ret;
> > > +
> > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
> > > + if (IS_ERR(indio_dev))
> > > + return PTR_ERR(indio_dev);
> > > +
> > > + i2c_set_clientdata(client, indio_dev);
> > > +
> > > + max9611 = iio_priv(indio_dev);
> > > + max9611->dev = &client->dev;
> > > + max9611->i2c_client = client;
> > > + mutex_init(&max9611->lock);
> > > +
> > > + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> > > + if (ret) {
> > > + dev_err(&client->dev,
> > > + "Missing %s property for %s node\n",
> > > + shunt_res_prop, of_node->full_name);
> > > + return ret;
> > > + }
> > > + max9611->shunt_resistor_uohm = of_shunt;
> > > +
> > > + ret = max9611_init(max9611);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + indio_dev->dev.parent = &client->dev;
> > > + indio_dev->dev.of_node = client->dev.of_node;
> > > + indio_dev->name = client->dev.of_node->name;
> > What's this going to give for the name? Name in IIO is supposed to
> > be an indication of the part rather than anything more explicit.
> > That's not easily obtained from device tree directly...
> >
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > + indio_dev->info = &indio_info;
> > > + indio_dev->channels = max9611_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
> > > +
> > > + return devm_iio_device_register(&client->dev, indio_dev);
> > > +}
> > > +
> > > +static const struct of_device_id max9611_of_table[] = {
> > > + {.compatible = "maxim,max9611"},
> > > + {.compatible = "maxim,max9612"},
> > > + { },
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, max9611_of_table);
> > > +
> > > +static struct i2c_driver max9611_driver = {
> > > + .driver = {
> > > + .name = DRIVER_NAME,
> > > + .owner = THIS_MODULE,
> > > + .of_match_table = max9611_of_table,
> > > + },
> > > + .probe = max9611_probe,
> > > +};
> > > +module_i2c_driver(max9611_driver);
> > > +
> > > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
> > > +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> > > +MODULE_LICENSE("GPL v2");
> > >
> >
>
> --
>
> Peter Meerwald-Stadler
> Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-26 10:08 ` jacopo
@ 2017-03-26 10:24 ` Jonathan Cameron
2017-03-26 10:50 ` jacopo
0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Cameron @ 2017-03-26 10:24 UTC (permalink / raw)
To: jacopo, Peter Meerwald-Stadler
Cc: Jacopo Mondi, geert, wsa+renesas, lars, linux-iio
On 26/03/17 11:08, jacopo wrote:
> Hi Peter,
> thanks for review
>
> On Sat, Mar 25, 2017 at 05:13:38PM +0100, Peter Meerwald-Stadler wrote:
>>
>> On Sat, 25 Mar 2017, Jonathan Cameron wrote:
>>
>> some more comments
>>
>>> On 24/03/17 15:28, Jacopo Mondi wrote:
>>>> Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
>>>> with 12-bits ADC interface.
>>>>
>>>> Datasheet publicly available at:
>>>> https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
>>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> A few more little things inline. Coming together nicely.
>>>
>>> The channel set here is just odd enough that it might aid review to have
>>> a quick listing of the resulting sysfs entries. One or two authors do
>>> this an it is always useful for a quick sanity check.
>>>
>>> Perhaps even a set of typical values. Put this below the --- as we don't
>>> need it in the git history, only to assist lazy reviewers like me ;)
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>> ---
>>>> drivers/iio/adc/Kconfig | 10 +
>>>> drivers/iio/adc/Makefile | 1 +
>>>> drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 601 insertions(+)
>>>> create mode 100644 drivers/iio/adc/max9611.c
>>>>
>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>> index dedae7a..82f2e7b8 100644
>>>> --- a/drivers/iio/adc/Kconfig
>>>> +++ b/drivers/iio/adc/Kconfig
>>>> @@ -354,6 +354,16 @@ config MAX1363
>>>> To compile this driver as a module, choose M here: the module will be
>>>> called max1363.
>>>>
>>>> +config MAX9611
>>>> + tristate "Maxim max9611/max9612 ADC driver"
>>>> + depends on I2C
>>>> + help
>>>> + Say yes here to build support for Maxim max9611/max9612 current sense
>>>> + amplifier with 12-bits ADC interface.
>>>> +
>>>> + To compile this driver as a module, choose M here: the module will be
>>>> + called max9611.
>>>> +
>>>> config MCP320X
>>>> tristate "Microchip Technology MCP3x01/02/04/08"
>>>> depends on SPI
>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>> index d001262..149f979 100644
>>>> --- a/drivers/iio/adc/Makefile
>>>> +++ b/drivers/iio/adc/Makefile
>>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>>>> obj-$(CONFIG_MAX1027) += max1027.o
>>>> obj-$(CONFIG_MAX11100) += max11100.o
>>>> obj-$(CONFIG_MAX1363) += max1363.o
>>>> +obj-$(CONFIG_MAX9611) += max9611.o
>>>> obj-$(CONFIG_MCP320X) += mcp320x.o
>>>> obj-$(CONFIG_MCP3422) += mcp3422.o
>>>> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>>>> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
>>>> new file mode 100644
>>>> index 0000000..61566ec
>>>> --- /dev/null
>>>> +++ b/drivers/iio/adc/max9611.c
>>>> @@ -0,0 +1,590 @@
>>>> +/*
>>>> + * iio/adc/max9611.c
>>>> + *
>>>> + * Maxim max9611/max9612 high side current sense amplifier with
>>>> + * 12-bit ADC interface.
>>>> + *
>>>> + * Copyright (C) 2017 Jacopo Mondi
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +
>>>> +/*
>>>> + * This driver supports input common-mode voltage, current-sense
>>>> + * amplifier with programmable gains and die temperature reading from
>>>> + * Maxim max9611/max9612.
>>>> + *
>>>> + * Op-amp, analog comparator, and watchdog functionalities are not
>>>> + * supported by this driver.
>>>> + */
>>>> +
>>>> +#include <linux/delay.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/sysfs.h>
>>>> +#include <linux/module.h>
>>>> +
>>>> +#define DRIVER_NAME "max9611"
>>>> +
>>>> +/* max9611 register addresses */
>>>> +#define MAX9611_REG_CSA_DATA 0x00
>>>> +#define MAX9611_REG_RS_DATA 0x02
>>>> +#define MAX9611_REG_TEMP_DATA 0x08
>>>> +#define MAX9611_REG_CTRL1 0x0a
>>>> +#define MAX9611_REG_CTRL2 0x0b
>>>> +
>>>> +/* max9611 REG1 mux configuration options */
>>>> +#define MAX9611_MUX_MASK 0x07
>>
>> could use GENMASK()
>>
>>>> +#define MAX9611_MUX_SENSE_1x 0x00
>>>> +#define MAX9611_MUX_SENSE_4x 0x01
>>>> +#define MAX9611_MUX_SENSE_8x 0x02
>>>> +#define MAX9611_INPUT_VOLT 0x03
>>>> +#define MAX9611_MUX_TEMP 0x06
>>
>> maybe avoid mixed-case #defines
>>
>>>> +/* max9611 voltage (both csa and input) helper macros */
>>>> +#define MAX9611_VOLTAGE_SHIFT 0x04
>>>> +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
>>>> +
>>>> +/*
>>>> + * max9611 current sense amplifier voltage output:
>>>> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
>>>> + *
>>>> + * GAIN LSB (nV) OFFSET (LSB steps)
>>>> + * 1x 107500 1
>>>> + * 4x 26880 1
>>>> + * 8x 13440 3
>>>> + *
>>>> + * The complete formula to calculate current sense voltage is:
>>>> + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
>>>> + */
>>>> +#define CSA_VOLT_1x_LSB_nV 107500
>>>> +#define CSA_VOLT_4x_LSB_nV 26880
>>>> +#define CSA_VOLT_8x_LSB_nV 13440
>>>> +
>>>> +#define CSA_VOLT_1x_OFFS_RAW 1
>>>> +#define CSA_VOLT_4x_OFFS_RAW 1
>>>> +#define CSA_VOLT_8x_OFFS_RAW 3
>>>> +
>>>> +/*
>>>> + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
>>>> + *
>>>> + * The complete formula to calculate input common voltage is:
>>>> + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
>>>> + */
>>>> +#define CIM_VOLTAGE_LSB_mV 14
>>>> +#define CIM_VOLTAGE_OFFSET_RAW 1
>>>> +
>>>> +/*
>>>> + * max9611 temperature reading: LSB is 0.48 degrees Celsius
>>>> + *
>>>> + * The complete formula to calculate temperature is:
>>>> + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
>>>> + */
>>> I'd prefer these defines to be prefixed with MAX9611_
>>> Easiest to just do the lot. Some of these are 'standard' enough
>>> the might well clash with something that turns up in an included header
>>> at somepoint in the future.
>>>
>>>> +#define TEMP_SHIFT 0x07
>>>> +#define TEMP_MAX_RAW_POS 0x7f80
>>>> +#define TEMP_MAX_RAW_NEG 0xff80
>>>> +#define TEMP_MIN_RAW_NEG 0xd980
>>>> +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
>>
>> could use GENMASK()
>>
>>>> +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
>>>> +#define TEMP_LSB_mC 480
>>>> +#define TEMP_SCALE_NUM 1000
>>>> +#define TEMP_SCALE_DIV 2083
>>>> +
>>>> +struct max9611_dev {
>>>> + struct device *dev;
>>>> + struct i2c_client *i2c_client;
>>>> + struct mutex lock;
>>>> + unsigned int shunt_resistor_uohm;
>>>> +};
>>>> +
>>>> +enum max9611_conf_ids {
>>>> + CONF_SENSE_1x,
>>>> + CONF_SENSE_4x,
>>>> + CONF_SENSE_8x,
>>>> + CONF_IN_VOLT,
>>>> + CONF_TEMP,
>>>> +};
>>>> +
>>>> +/**
>>>> + * max9611_mux_conf - associate ADC mux configuration with register address
>>>> + * where data shall be read from
>>>> + */
>>>> +static unsigned int max9611_mux_conf[][2] = {
>>
>> const
>>
>>>> + /* CONF_SENSE_1x */
>>>> + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
>>>> + /* CONF_SENSE_4x */
>>>> + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
>>>> + /* CONF_SENSE_8x */
>>>> + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
>>>> + /* CONF_IN_VOLT */
>>>> + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
>>>> + /* CONF_TEMP */
>>>> + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
>>>> +};
>>>> +
>>>> +enum max9611_csa_gain {
>>>> + CSA_GAIN_1x,
>>>> + CSA_GAIN_4x,
>>>> + CSA_GAIN_8x,
>>>> +};
>>>> +
>>>> +enum max9611_csa_gain_params {
>>>> + CSA_GAIN_LSB_nV,
>>>> + CSA_GAIN_OFFS_RAW,
>>>> +};
>>>> +
>>>> +/**
>>>> + * max9611_csa_gain_conf - associate gain multiplier with LSB and
>>>> + * offset values.
>>>> + *
>>>> + * Group together parameters associated with configurable gain
>>>> + * on current sense amplifier path to ADC interface.
>>>> + * Current sense read routine adjusts gain until it gets a meaningful
>>>> + * value; use this structure to retrieve the correct LSB and offset values.
>>>> + */
>>>> +static unsigned int max9611_gain_conf[][2] = {
>>
>> const
>>
>>>> + { /* [0] CSA_GAIN_1x */
>>>> + CSA_VOLT_1x_LSB_nV,
>>>> + CSA_VOLT_1x_OFFS_RAW,
>>>> + },
>>>> + { /* [1] CSA_GAIN_4x */
>>>> + CSA_VOLT_4x_LSB_nV,
>>>> + CSA_VOLT_4x_OFFS_RAW,
>>>> + },
>>>> + { /* [2] CSA_GAIN_8x */
>>>> + CSA_VOLT_8x_LSB_nV,
>>>> + CSA_VOLT_8x_OFFS_RAW,
>>>> + },
>>>> +};
>>>> +
>>>> +enum max9611_chan_addrs {
>>>> + MAX9611_CHAN_VOLTAGE_INPUT,
>>>> + MAX9611_CHAN_VOLTAGE_SENSE,
>>>> + MAX9611_CHAN_TEMPERATURE,
>>>> + MAX9611_CHAN_CURRENT_LOAD,
>>>> + MAX9611_CHAN_POWER_LOAD,
>>>> +};
>>>> +
>>>> +static struct iio_chan_spec max9611_channels[] = {
>>
>> const
>>
>>>> + {
>>>> + .type = IIO_TEMP,
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>> + BIT(IIO_CHAN_INFO_SCALE),
>>>> + .address = MAX9611_CHAN_TEMPERATURE,
>>>> + },
>>>> + {
>>>> + .type = IIO_VOLTAGE,
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>>> + BIT(IIO_CHAN_INFO_OFFSET),
>>>> + .address = MAX9611_CHAN_VOLTAGE_INPUT,
>>>> + .indexed = 1,
>>>> + .channel = 1,
>>
>> why is this indexed?
>> should be the only raw voltage channel
>>
>>
>>>> + },
>>>> + {
>>>> + .type = IIO_VOLTAGE,
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>>> + .address = MAX9611_CHAN_VOLTAGE_SENSE,
>>>> + .indexed = 1,
>>>> + .channel = 0,
>>> Unusual to have the channels in here other than in channel order...
>>
>> why is this indexed?
>> should be the only processed voltage channel
>>
>
> Jonathan already replied on this, but I would add that with no
> indexing this would appear in userspace as:
>
> in_voltage_raw
> in_voltage_scale
> in_voltage_offset
> in_offset_processed
>
> which is confusing at best.
I hope that last one is in_voltage_processed!
>
> Hope is ok if I keep this indexed (in correct order as Jonathan said)
>
>>
>>>> + },
>>>> + {
>>>> + .type = IIO_CURRENT,
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>>> + .address = MAX9611_CHAN_CURRENT_LOAD,
>>>> + },
>>>> + {
>>>> + .type = IIO_POWER,
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>>> + .address = MAX9611_CHAN_POWER_LOAD
>>>> + },
>>>> +};
>>>> +
>>>> +/**
>>>> + * max9611_read_single() - read a single vale from ADC interface
>>> value
>>>> + *
>>>> + * Data registers are 16 bit long, spread between two 8 bit registers
>>>> + * with consecutive addresses.
>>>> + * Configure ADC mux first, then read register at address "reg_addr".
>>>> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
>>>> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>>
>> maybe worth noting that data is MSB first or big-endian
>>
>>>> + *
>>>> + * @max9611: max9611 device
>>>> + * @selector: index for mux and register configuration
>>>> + * @raw_val: the value returned from ADC
>>>> + */
>>>> +static int max9611_read_single(struct max9611_dev *max9611,
>>>> + enum max9611_conf_ids selector,
>>>> + u16 *raw_val)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
>>>> + u8 reg_addr = max9611_mux_conf[selector][1];
>>>> +
>>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>>> + MAX9611_REG_CTRL1, mux_conf);
>>>> + if (ret) {
>>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>>> + MAX9611_REG_CTRL1, mux_conf);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /*
>>>> + * need a delay here to make register configuration
>>>> + * stabilize. 1 msec at least, from empirical testing.
>>>> + */
>>>> + usleep_range(1000, 2000);
>>>> +
>>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
>>>> + if (ret < 0) {
>>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>>> + reg_addr);
>>>> + return ret;
>>>> + }
>>>> + *raw_val = ret;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * max9611_read_csa_voltage() - read current sense amplifier output voltage
>>>> + *
>>>> + * Current sense amplifier output voltage is read through a configurable
>>>> + * 1x, 4x or 8x gain.
>>>> + * Start with plain 1x gain, and adjust gain control properly until a
>>>> + * meaningful value is read from ADC output.
>>>> + *
>>>> + * @max9611: max9611 device
>>>> + * @adc_raw: raw value read from ADC output
>>>> + * @csa_gain: gain configuration option selector
>>>> + */
>>>> +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
>>>> + u16 *adc_raw,
>>>> + enum max9611_csa_gain *csa_gain)
>>>> +{
>>>> + enum max9611_conf_ids gain_selectors[] = {
>>>> + CONF_SENSE_1x,
>>>> + CONF_SENSE_4x,
>>>> + CONF_SENSE_8x
>>>> + };
>>>> + unsigned int i;
>>>> + int ret;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
>>>> + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (*adc_raw > 0) {
>>>> + *csa_gain = gain_selectors[i];
>>>> + return 0;
>>>> + }
>>>> + }
>>>> +
>>>> + return -EIO;
>>>> +}
>>>> +
>>>> +static int max9611_read_raw(struct iio_dev *indio_dev,
>>>> + struct iio_chan_spec const *chan,
>>>> + int *val, int *val2, long mask)
>>>> +{
>>>> + struct max9611_dev *dev = iio_priv(indio_dev);
>>>> + enum max9611_csa_gain gain_selector;
>>>> + unsigned int *csa_gain;
>>>> + u16 adc_data;
>>>> + int ret;
>>>> +
>>>> + switch (mask) {
>>>> + case IIO_CHAN_INFO_RAW:
>>>> + mutex_lock(&dev->lock);
>>>> +
>>>> + switch (chan->address) {
>>>> + case MAX9611_CHAN_TEMPERATURE:
>>>> + ret = max9611_read_single(dev, CONF_TEMP,
>>>> + &adc_data);
>>>> + if (ret)
>>> I'm not personally keen on jumping out of deep indentations like this
>>> just save on repeating one line. I'd pull the unlock back here and return
>>> directly as I feel it'll improve readability.
>>> Actually come to think of it, why does the lock need to be held for
>>> the next line anyway? adc_data is on the stack so doesn't matter if we
>>> have concurrent readers, once the i2c transaction is finished.
>>> Just unlock before checking ret.
>>>> + goto unlock_fail;
>>>> +
>>>> + *val = TEMP_RAW(adc_data);
>>>> +
>>>> + mutex_unlock(&dev->lock);
>>>> + return IIO_VAL_INT;
>>>> +
>>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>>> + &adc_data);
>>>> + if (ret)
>>>> + goto unlock_fail;
>>>> +
>>>> + *val = MAX9611_VOLTAGE_RAW(adc_data);
>>>> +
>>>> + mutex_unlock(&dev->lock);
>>>> + return IIO_VAL_INT;
>>>> + }
>>>> +
>>
>> this falls through, no break?
>>
>> I'd move
>> mutex_unlock(&dev->lock);
>> return IIO_VAL_INT;
>> here
>>
>>
>>>> + case IIO_CHAN_INFO_OFFSET:
>>>> + switch (chan->address) {
>>
>> rather pointless for only one case?
>>
>>
>>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>>> + *val = CIM_VOLTAGE_OFFSET_RAW;
>>>> +
>>>> + return IIO_VAL_INT;
>>>> + }
>>>> +
>>>> + case IIO_CHAN_INFO_SCALE:
>>>> + switch (chan->address) {
>>>> + case MAX9611_CHAN_TEMPERATURE:
>>>> + *val = TEMP_SCALE_NUM;
>>>> + *val2 = TEMP_SCALE_DIV;
>>>> +
>>>> + return IIO_VAL_FRACTIONAL;
>>>> +
>>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>>> + *val = CIM_VOLTAGE_LSB_mV;
>>>> + return IIO_VAL_INT;
>>
>> default: return -EINVAL;
>>
>>>> + }
>>
>> break; or something
>>
>
> I'll rewrite this with this and other comments in mind
>
> Thanks
> j
>
>>>> +
>>>> + case IIO_CHAN_INFO_PROCESSED:
>>>> + mutex_lock(&dev->lock);
>>>> +
>>>> + switch (chan->address) {
>>>> + case MAX9611_CHAN_VOLTAGE_SENSE:
>>>> + /*
>>>> + * processed (mV): (raw - offset) * LSB (nV) / 10^6
>>>> + *
>>>> + * Even if max9611 can output raw csa voltage readings,
>>>> + * use a produced value as scale depends on gain.
>>>> + */
>>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>>> + &gain_selector);
>>>> + if (ret)
>>>> + goto unlock_fail;
>>>> +
>>>> + csa_gain = max9611_gain_conf[gain_selector];
>>>> +
>>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>>> + csa_gain[CSA_GAIN_LSB_nV];
>>>> + *val2 = 1000000;
>>>> +
>>>> + mutex_unlock(&dev->lock);
>>>> + return IIO_VAL_FRACTIONAL;
>>>> +
>>>> + case MAX9611_CHAN_CURRENT_LOAD:
>>>> + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
>>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>>> + &gain_selector);
>>>> + if (ret)
>>>> + goto unlock_fail;
>>>> +
>>>> + csa_gain = max9611_gain_conf[gain_selector];
>>>> +
>>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>>> + csa_gain[CSA_GAIN_LSB_nV];
>>>> + *val2 = dev->shunt_resistor_uohm;
>>>> +
>>>> + mutex_unlock(&dev->lock);
>>>> + return IIO_VAL_FRACTIONAL;
>>>> +
>>>> + case MAX9611_CHAN_POWER_LOAD:
>>>> + /*
>>>> + * processed (mW): Vin (mV) * Vcsa (uV) /
>>>> + * Rshunt (uOhm)
>>>> + */
>>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>>> + &adc_data);
>>>> + if (ret)
>>>> + goto unlock_fail;
>>>> +
>>>> + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
>>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>>> + CIM_VOLTAGE_LSB_mV;
>>>> +
>>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>>> + &gain_selector);
>>>> + if (ret)
>>>> + goto unlock_fail;
>>>> +
>>>> + csa_gain = max9611_gain_conf[gain_selector];
>>>> +
>>>> + /* divide by 10^3 here to avoid 32bit overflow */
>>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>>> + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
>>>> + csa_gain[CSA_GAIN_LSB_nV] / 1000;
>>>> + *val2 = dev->shunt_resistor_uohm;
>>>> +
>>>> + mutex_unlock(&dev->lock);
>>>> + return IIO_VAL_FRACTIONAL;
>>>> + }
>>>> + }
>>>> +
>>>> + ret = -EINVAL;
>>>> +
>>>> +unlock_fail:
>>>> + mutex_unlock(&dev->lock);
>>>> + return ret;
>>>> +
>>>> +}
>>>> +
>>>> +static ssize_t max9611_shunt_resistor_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
>>>> +
>>>> + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
>>>> +}
>>>> +
>>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
>>>> + max9611_shunt_resistor_show, NULL, 0);
>>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
>>>> + max9611_shunt_resistor_show, NULL, 0);
>>>> +
>>>> +static struct attribute *max9611_attributes[] = {
>>>> + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
>>>> + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
>>>> + NULL,
>>>> +};
>>>> +
>>>> +static const struct attribute_group max9611_attribute_group = {
>>>> + .attrs = max9611_attributes,
>>>> +};
>>>> +
>>>> +static const struct iio_info indio_info = {
>>>> + .driver_module = THIS_MODULE,
>>>> + .read_raw = max9611_read_raw,
>>>> + .attrs = &max9611_attribute_group,
>>>> +};
>>>> +
>>>> +static int max9611_init(struct max9611_dev *max9611)
>>>> +{
>>>> + struct i2c_client *client = max9611->i2c_client;
>>>> + u16 regval;
>>>> + int ret;
>>>> +
>>>> + if (!i2c_check_functionality(client->adapter,
>>>> + I2C_FUNC_SMBUS_WRITE_BYTE |
>>>> + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
>>>> + dev_err(max9611->dev,
>>>> + "No smbus support in I2c adapter: aborting probe.\n");
>>> This isn't necessarily an accurate message. I2c adapter might support
>>> smbus_read_byte only rather than word reads for example.
>>>
>>> Maybe make it more explict as to what we need?
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + /* Configure MUX to read temperature */
>>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>>> + if (ret) {
>>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>>> + return ret;
>>>> + }
>>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>>> + MAX9611_REG_CTRL2, 0);
>>>> + if (ret) {
>>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>>> + MAX9611_REG_CTRL2, 0);
>>>> + return ret;
>>>> + }
>>>> + usleep_range(1000, 2000);
>>>> +
>>>> + /* Make sure die temperature is in range to test communications. */
>>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
>>>> + MAX9611_REG_TEMP_DATA);
>>>> + if (ret < 0) {
>>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>>> + MAX9611_REG_TEMP_DATA);
>>>> + return ret;
>>>> + }
>>>> + regval = ret & ~TEMP_MASK;
>>>> +
>>>> + if ((regval > TEMP_MAX_RAW_POS &&
>>>> + regval < TEMP_MIN_RAW_NEG) ||
>>>> + regval > TEMP_MAX_RAW_NEG) {
>>>> + dev_err(max9611->dev,
>>>> + "Invalid value received from ADC 0x%4x: aborting\n",
>>>> + regval);
>>>> + return -EIO;
>>>> + }
>>>> +
>>>> + /* Mux shall be zeroed back before applying other configurations */
>>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>>> + MAX9611_REG_CTRL1, 0);
>>>> + if (ret) {
>>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>>> + MAX9611_REG_CTRL1, 0);
>>>> + return ret;
>>>> + }
>>>> + usleep_range(1000, 2000);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int max9611_probe(struct i2c_client *client,
>>>> + const struct i2c_device_id *id)
>>>> +{
>>>> + const char * const shunt_res_prop = "shunt-resistor-uohm";
>>>> + struct device_node *of_node = client->dev.of_node;
>>>> + struct max9611_dev *max9611;
>>>> + struct iio_dev *indio_dev;
>>>> + unsigned int of_shunt;
>>>> + int ret;
>>>> +
>>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
>>>> + if (IS_ERR(indio_dev))
>>>> + return PTR_ERR(indio_dev);
>>>> +
>>>> + i2c_set_clientdata(client, indio_dev);
>>>> +
>>>> + max9611 = iio_priv(indio_dev);
>>>> + max9611->dev = &client->dev;
>>>> + max9611->i2c_client = client;
>>>> + mutex_init(&max9611->lock);
>>>> +
>>>> + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
>>>> + if (ret) {
>>>> + dev_err(&client->dev,
>>>> + "Missing %s property for %s node\n",
>>>> + shunt_res_prop, of_node->full_name);
>>>> + return ret;
>>>> + }
>>>> + max9611->shunt_resistor_uohm = of_shunt;
>>>> +
>>>> + ret = max9611_init(max9611);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + indio_dev->dev.parent = &client->dev;
>>>> + indio_dev->dev.of_node = client->dev.of_node;
>>>> + indio_dev->name = client->dev.of_node->name;
>>> What's this going to give for the name? Name in IIO is supposed to
>>> be an indication of the part rather than anything more explicit.
>>> That's not easily obtained from device tree directly...
>>>
>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>> + indio_dev->info = &indio_info;
>>>> + indio_dev->channels = max9611_channels;
>>>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
>>>> +
>>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>>> +}
>>>> +
>>>> +static const struct of_device_id max9611_of_table[] = {
>>>> + {.compatible = "maxim,max9611"},
>>>> + {.compatible = "maxim,max9612"},
>>>> + { },
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
>>>> +
>>>> +static struct i2c_driver max9611_driver = {
>>>> + .driver = {
>>>> + .name = DRIVER_NAME,
>>>> + .owner = THIS_MODULE,
>>>> + .of_match_table = max9611_of_table,
>>>> + },
>>>> + .probe = max9611_probe,
>>>> +};
>>>> +module_i2c_driver(max9611_driver);
>>>> +
>>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
>>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>
>>
>> --
>>
>> Peter Meerwald-Stadler
>> Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-26 10:24 ` Jonathan Cameron
@ 2017-03-26 10:50 ` jacopo
0 siblings, 0 replies; 39+ messages in thread
From: jacopo @ 2017-03-26 10:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Peter Meerwald-Stadler, Jacopo Mondi, geert, wsa+renesas, lars,
linux-iio
Hi Jonathan,
[snip]
On Sun, Mar 26, 2017 at 11:24:54AM +0100, Jonathan Cameron wrote:
> >>>> +static struct iio_chan_spec max9611_channels[] = {
> >>
> >> const
> >>
> >>>> + {
> >>>> + .type = IIO_TEMP,
> >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >>>> + BIT(IIO_CHAN_INFO_SCALE),
> >>>> + .address = MAX9611_CHAN_TEMPERATURE,
> >>>> + },
> >>>> + {
> >>>> + .type = IIO_VOLTAGE,
> >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >>>> + BIT(IIO_CHAN_INFO_SCALE) |
> >>>> + BIT(IIO_CHAN_INFO_OFFSET),
> >>>> + .address = MAX9611_CHAN_VOLTAGE_INPUT,
> >>>> + .indexed = 1,
> >>>> + .channel = 1,
> >>
> >> why is this indexed?
> >> should be the only raw voltage channel
> >>
> >>
> >>>> + },
> >>>> + {
> >>>> + .type = IIO_VOLTAGE,
> >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> >>>> + .address = MAX9611_CHAN_VOLTAGE_SENSE,
> >>>> + .indexed = 1,
> >>>> + .channel = 0,
> >>> Unusual to have the channels in here other than in channel order...
> >>
> >> why is this indexed?
> >> should be the only processed voltage channel
> >>
> >
> > Jonathan already replied on this, but I would add that with no
> > indexing this would appear in userspace as:
> >
> > in_voltage_raw
> > in_voltage_scale
> > in_voltage_offset
> > in_offset_processed
> >
> > which is confusing at best.
> I hope that last one is in_voltage_processed!
Sure it is, I just confused channel name and mask name when writing it
down here :)
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-25 16:13 ` Peter Meerwald-Stadler
2017-03-25 16:34 ` Jonathan Cameron
2017-03-26 10:08 ` jacopo
@ 2017-03-27 8:30 ` jacopo
2 siblings, 0 replies; 39+ messages in thread
From: jacopo @ 2017-03-27 8:30 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Jacopo Mondi, Jonathan Cameron, geert, wsa+renesas, lars,
linux-iio
Hi Jonathan, Peter,
On Sat, Mar 25, 2017 at 05:13:38PM +0100, Peter Meerwald-Stadler wrote:
>
> On Sat, 25 Mar 2017, Jonathan Cameron wrote:
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + mutex_lock(&dev->lock);
> > > +
> > > + switch (chan->address) {
> > > + case MAX9611_CHAN_TEMPERATURE:
> > > + ret = max9611_read_single(dev, CONF_TEMP,
> > > + &adc_data);
> > > + if (ret)
> > I'm not personally keen on jumping out of deep indentations like this
> > just save on repeating one line. I'd pull the unlock back here and return
> > directly as I feel it'll improve readability.
> > Actually come to think of it, why does the lock need to be held for
> > the next line anyway? adc_data is on the stack so doesn't matter if we
> > have concurrent readers, once the i2c transaction is finished.
> > Just unlock before checking ret.
[snip]
> > > +
> > > + mutex_unlock(&dev->lock);
> > > + return IIO_VAL_INT;
> > > + }
> > > +
>
> this falls through, no break?
>
> I'd move
> mutex_unlock(&dev->lock);
> return IIO_VAL_INT;
> here
>
On the two comments above: re-thinking on this a bit, the only part
that actually needs protection is gain selection routine, which
depends on consecutive reads of what is returned from the ADC bus.
All other variables (including the ones coming from outside, as val
and val2 are) are not shared and do no need protection at this level.
I should move the mutex in max9611_read_* functions (maybe even
read_single() does not need that, as I2c bus access is protected
already), and simplify the read_raw() one.
Just saying it out loud so there will be no surprises in v4.
Thanks
j
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
@ 2017-03-25 17:21 ` jacopo
0 siblings, 0 replies; 39+ messages in thread
From: jacopo @ 2017-03-25 17:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jacopo Mondi, geert, wsa+renesas, magnus.damm, laurent.pinchart,
knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio,
linux-renesas-soc, devicetree
Hi Jonathan,
thanks for review
On Sat, Mar 25, 2017 at 03:45:05PM +0000, Jonathan Cameron wrote:
> On 24/03/17 15:28, Jacopo Mondi wrote:
> > Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
> > with 12-bits ADC interface.
> >
> > Datasheet publicly available at:
> > https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> A few more little things inline. Coming together nicely.
>
> The channel set here is just odd enough that it might aid review to have
> a quick listing of the resulting sysfs entries. One or two authors do
> this an it is always useful for a quick sanity check.
>
> Perhaps even a set of typical values. Put this below the --- as we don't
> need it in the git history, only to assist lazy reviewers like me ;)
>
I included the output of iio_info in the cover letter, I can add some
more info here for sure!
> Thanks,
>
> Jonathan
> > ---
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 601 insertions(+)
> > create mode 100644 drivers/iio/adc/max9611.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index dedae7a..82f2e7b8 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -354,6 +354,16 @@ config MAX1363
> > To compile this driver as a module, choose M here: the module will be
> > called max1363.
> >
> > +config MAX9611
> > + tristate "Maxim max9611/max9612 ADC driver"
> > + depends on I2C
> > + help
> > + Say yes here to build support for Maxim max9611/max9612 current sense
> > + amplifier with 12-bits ADC interface.
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called max9611.
> > +
> > config MCP320X
> > tristate "Microchip Technology MCP3x01/02/04/08"
> > depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index d001262..149f979 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
> > obj-$(CONFIG_MAX1027) += max1027.o
> > obj-$(CONFIG_MAX11100) += max11100.o
> > obj-$(CONFIG_MAX1363) += max1363.o
> > +obj-$(CONFIG_MAX9611) += max9611.o
> > obj-$(CONFIG_MCP320X) += mcp320x.o
> > obj-$(CONFIG_MCP3422) += mcp3422.o
> > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > new file mode 100644
> > index 0000000..61566ec
> > --- /dev/null
> > +++ b/drivers/iio/adc/max9611.c
> > @@ -0,0 +1,590 @@
> > +/*
> > + * iio/adc/max9611.c
> > + *
> > + * Maxim max9611/max9612 high side current sense amplifier with
> > + * 12-bit ADC interface.
> > + *
> > + * Copyright (C) 2017 Jacopo Mondi
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +/*
> > + * This driver supports input common-mode voltage, current-sense
> > + * amplifier with programmable gains and die temperature reading from
> > + * Maxim max9611/max9612.
> > + *
> > + * Op-amp, analog comparator, and watchdog functionalities are not
> > + * supported by this driver.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/module.h>
> > +
> > +#define DRIVER_NAME "max9611"
> > +
> > +/* max9611 register addresses */
> > +#define MAX9611_REG_CSA_DATA 0x00
> > +#define MAX9611_REG_RS_DATA 0x02
> > +#define MAX9611_REG_TEMP_DATA 0x08
> > +#define MAX9611_REG_CTRL1 0x0a
> > +#define MAX9611_REG_CTRL2 0x0b
> > +
> > +/* max9611 REG1 mux configuration options */
> > +#define MAX9611_MUX_MASK 0x07
> > +#define MAX9611_MUX_SENSE_1x 0x00
> > +#define MAX9611_MUX_SENSE_4x 0x01
> > +#define MAX9611_MUX_SENSE_8x 0x02
> > +#define MAX9611_INPUT_VOLT 0x03
> > +#define MAX9611_MUX_TEMP 0x06
> > +
> > +/* max9611 voltage (both csa and input) helper macros */
> > +#define MAX9611_VOLTAGE_SHIFT 0x04
> > +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
> > +
> > +/*
> > + * max9611 current sense amplifier voltage output:
> > + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> > + *
> > + * GAIN LSB (nV) OFFSET (LSB steps)
> > + * 1x 107500 1
> > + * 4x 26880 1
> > + * 8x 13440 3
> > + *
> > + * The complete formula to calculate current sense voltage is:
> > + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> > + */
> > +#define CSA_VOLT_1x_LSB_nV 107500
> > +#define CSA_VOLT_4x_LSB_nV 26880
> > +#define CSA_VOLT_8x_LSB_nV 13440
> > +
> > +#define CSA_VOLT_1x_OFFS_RAW 1
> > +#define CSA_VOLT_4x_OFFS_RAW 1
> > +#define CSA_VOLT_8x_OFFS_RAW 3
> > +
> > +/*
> > + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> > + *
> > + * The complete formula to calculate input common voltage is:
> > + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> > + */
> > +#define CIM_VOLTAGE_LSB_mV 14
> > +#define CIM_VOLTAGE_OFFSET_RAW 1
> > +
> > +/*
> > + * max9611 temperature reading: LSB is 0.48 degrees Celsius
> > + *
> > + * The complete formula to calculate temperature is:
> > + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
> > + */
> I'd prefer these defines to be prefixed with MAX9611_
> Easiest to just do the lot. Some of these are 'standard' enough
> the might well clash with something that turns up in an included header
> at somepoint in the future.
>
> > +#define TEMP_SHIFT 0x07
> > +#define TEMP_MAX_RAW_POS 0x7f80
> > +#define TEMP_MAX_RAW_NEG 0xff80
> > +#define TEMP_MIN_RAW_NEG 0xd980
> > +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
> > +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
> > +#define TEMP_LSB_mC 480
> > +#define TEMP_SCALE_NUM 1000
> > +#define TEMP_SCALE_DIV 2083
> > +
> > +struct max9611_dev {
> > + struct device *dev;
> > + struct i2c_client *i2c_client;
> > + struct mutex lock;
> > + unsigned int shunt_resistor_uohm;
> > +};
> > +
> > +enum max9611_conf_ids {
> > + CONF_SENSE_1x,
> > + CONF_SENSE_4x,
> > + CONF_SENSE_8x,
> > + CONF_IN_VOLT,
> > + CONF_TEMP,
> > +};
> > +
> > +/**
> > + * max9611_mux_conf - associate ADC mux configuration with register address
> > + * where data shall be read from
> > + */
> > +static unsigned int max9611_mux_conf[][2] = {
> > + /* CONF_SENSE_1x */
> > + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
> > + /* CONF_SENSE_4x */
> > + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
> > + /* CONF_SENSE_8x */
> > + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
> > + /* CONF_IN_VOLT */
> > + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
> > + /* CONF_TEMP */
> > + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
> > +};
> > +
> > +enum max9611_csa_gain {
> > + CSA_GAIN_1x,
> > + CSA_GAIN_4x,
> > + CSA_GAIN_8x,
> > +};
> > +
> > +enum max9611_csa_gain_params {
> > + CSA_GAIN_LSB_nV,
> > + CSA_GAIN_OFFS_RAW,
> > +};
> > +
> > +/**
> > + * max9611_csa_gain_conf - associate gain multiplier with LSB and
> > + * offset values.
> > + *
> > + * Group together parameters associated with configurable gain
> > + * on current sense amplifier path to ADC interface.
> > + * Current sense read routine adjusts gain until it gets a meaningful
> > + * value; use this structure to retrieve the correct LSB and offset values.
> > + */
> > +static unsigned int max9611_gain_conf[][2] = {
> > + { /* [0] CSA_GAIN_1x */
> > + CSA_VOLT_1x_LSB_nV,
> > + CSA_VOLT_1x_OFFS_RAW,
> > + },
> > + { /* [1] CSA_GAIN_4x */
> > + CSA_VOLT_4x_LSB_nV,
> > + CSA_VOLT_4x_OFFS_RAW,
> > + },
> > + { /* [2] CSA_GAIN_8x */
> > + CSA_VOLT_8x_LSB_nV,
> > + CSA_VOLT_8x_OFFS_RAW,
> > + },
> > +};
> > +
> > +enum max9611_chan_addrs {
> > + MAX9611_CHAN_VOLTAGE_INPUT,
> > + MAX9611_CHAN_VOLTAGE_SENSE,
> > + MAX9611_CHAN_TEMPERATURE,
> > + MAX9611_CHAN_CURRENT_LOAD,
> > + MAX9611_CHAN_POWER_LOAD,
> > +};
> > +
> > +static struct iio_chan_spec max9611_channels[] = {
> > + {
> > + .type = IIO_TEMP,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + .address = MAX9611_CHAN_TEMPERATURE,
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_OFFSET),
> > + .address = MAX9611_CHAN_VOLTAGE_INPUT,
> > + .indexed = 1,
> > + .channel = 1,
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_VOLTAGE_SENSE,
> > + .indexed = 1,
> > + .channel = 0,
> Unusual to have the channels in here other than in channel order...
> > + },
> > + {
> > + .type = IIO_CURRENT,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_CURRENT_LOAD,
> > + },
> > + {
> > + .type = IIO_POWER,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_POWER_LOAD
> > + },
> > +};
> > +
> > +/**
> > + * max9611_read_single() - read a single vale from ADC interface
> value
> > + *
> > + * Data registers are 16 bit long, spread between two 8 bit registers
> > + * with consecutive addresses.
> > + * Configure ADC mux first, then read register at address "reg_addr".
> > + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> > + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
> > + *
> > + * @max9611: max9611 device
> > + * @selector: index for mux and register configuration
> > + * @raw_val: the value returned from ADC
> > + */
> > +static int max9611_read_single(struct max9611_dev *max9611,
> > + enum max9611_conf_ids selector,
> > + u16 *raw_val)
> > +{
> > + int ret;
> > +
> > + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
> > + u8 reg_addr = max9611_mux_conf[selector][1];
> > +
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, mux_conf);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, mux_conf);
> > + return ret;
> > + }
> > +
> > + /*
> > + * need a delay here to make register configuration
> > + * stabilize. 1 msec at least, from empirical testing.
> > + */
> > + usleep_range(1000, 2000);
> > +
> > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
> > + if (ret < 0) {
> > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > + reg_addr);
> > + return ret;
> > + }
> > + *raw_val = ret;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * max9611_read_csa_voltage() - read current sense amplifier output voltage
> > + *
> > + * Current sense amplifier output voltage is read through a configurable
> > + * 1x, 4x or 8x gain.
> > + * Start with plain 1x gain, and adjust gain control properly until a
> > + * meaningful value is read from ADC output.
> > + *
> > + * @max9611: max9611 device
> > + * @adc_raw: raw value read from ADC output
> > + * @csa_gain: gain configuration option selector
> > + */
> > +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
> > + u16 *adc_raw,
> > + enum max9611_csa_gain *csa_gain)
> > +{
> > + enum max9611_conf_ids gain_selectors[] = {
> > + CONF_SENSE_1x,
> > + CONF_SENSE_4x,
> > + CONF_SENSE_8x
> > + };
> > + unsigned int i;
> > + int ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> > + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
> > + if (ret)
> > + return ret;
> > +
> > + if (*adc_raw > 0) {
> > + *csa_gain = gain_selectors[i];
> > + return 0;
> > + }
> > + }
> > +
> > + return -EIO;
> > +}
> > +
> > +static int max9611_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct max9611_dev *dev = iio_priv(indio_dev);
> > + enum max9611_csa_gain gain_selector;
> > + unsigned int *csa_gain;
> > + u16 adc_data;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&dev->lock);
> > +
> > + switch (chan->address) {
> > + case MAX9611_CHAN_TEMPERATURE:
> > + ret = max9611_read_single(dev, CONF_TEMP,
> > + &adc_data);
> > + if (ret)
> I'm not personally keen on jumping out of deep indentations like this
> just save on repeating one line. I'd pull the unlock back here and return
> directly as I feel it'll improve readability.
I'll change it, with this and Peter's suggestion on this function in
his review.
> Actually come to think of it, why does the lock need to be held for
> the next line anyway? adc_data is on the stack so doesn't matter if we
> have concurrent readers, once the i2c transaction is finished.
> Just unlock before checking ret.
I naively overvalued 'style' (ret assignment and check in two
consecutive lines) over practicality... I'll change this
> > + goto unlock_fail;
> > +
> > + *val = TEMP_RAW(adc_data);
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_INT;
> > +
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > + &adc_data);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + *val = MAX9611_VOLTAGE_RAW(adc_data);
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_INT;
> > + }
> > +
> > + case IIO_CHAN_INFO_OFFSET:
> > + switch (chan->address) {
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + *val = CIM_VOLTAGE_OFFSET_RAW;
> > +
> > + return IIO_VAL_INT;
> > + }
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->address) {
> > + case MAX9611_CHAN_TEMPERATURE:
> > + *val = TEMP_SCALE_NUM;
> > + *val2 = TEMP_SCALE_DIV;
> > +
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + *val = CIM_VOLTAGE_LSB_mV;
> > + return IIO_VAL_INT;
> > + }
> > +
> > + case IIO_CHAN_INFO_PROCESSED:
> > + mutex_lock(&dev->lock);
> > +
> > + switch (chan->address) {
> > + case MAX9611_CHAN_VOLTAGE_SENSE:
> > + /*
> > + * processed (mV): (raw - offset) * LSB (nV) / 10^6
> > + *
> > + * Even if max9611 can output raw csa voltage readings,
> > + * use a produced value as scale depends on gain.
> > + */
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV];
> > + *val2 = 1000000;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_CURRENT_LOAD:
> > + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV];
> > + *val2 = dev->shunt_resistor_uohm;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_POWER_LOAD:
> > + /*
> > + * processed (mW): Vin (mV) * Vcsa (uV) /
> > + * Rshunt (uOhm)
> > + */
> > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > + &adc_data);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + CIM_VOLTAGE_LSB_mV;
> > +
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + /* divide by 10^3 here to avoid 32bit overflow */
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV] / 1000;
> > + *val2 = dev->shunt_resistor_uohm;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > + }
> > + }
> > +
> > + ret = -EINVAL;
> > +
> > +unlock_fail:
> > + mutex_unlock(&dev->lock);
> > + return ret;
> > +
> > +}
> > +
> > +static ssize_t max9611_shunt_resistor_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
> > +
> > + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
> > +}
> > +
> > +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
> > + max9611_shunt_resistor_show, NULL, 0);
> > +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
> > + max9611_shunt_resistor_show, NULL, 0);
> > +
> > +static struct attribute *max9611_attributes[] = {
> > + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
> > + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group max9611_attribute_group = {
> > + .attrs = max9611_attributes,
> > +};
> > +
> > +static const struct iio_info indio_info = {
> > + .driver_module = THIS_MODULE,
> > + .read_raw = max9611_read_raw,
> > + .attrs = &max9611_attribute_group,
> > +};
> > +
> > +static int max9611_init(struct max9611_dev *max9611)
> > +{
> > + struct i2c_client *client = max9611->i2c_client;
> > + u16 regval;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> > + dev_err(max9611->dev,
> > + "No smbus support in I2c adapter: aborting probe.\n");
> This isn't necessarily an accurate message. I2c adapter might support
> smbus_read_byte only rather than word reads for example.
>
> Maybe make it more explict as to what we need?
Chopped away some details to make it fit in 80 columns.. I'll make it
more verbose...
> > + return -EINVAL;
> > + }
> > +
> > + /* Configure MUX to read temperature */
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > + return ret;
> > + }
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL2, 0);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL2, 0);
> > + return ret;
> > + }
> > + usleep_range(1000, 2000);
> > +
> > + /* Make sure die temperature is in range to test communications. */
> > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
> > + MAX9611_REG_TEMP_DATA);
> > + if (ret < 0) {
> > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > + MAX9611_REG_TEMP_DATA);
> > + return ret;
> > + }
> > + regval = ret & ~TEMP_MASK;
> > +
> > + if ((regval > TEMP_MAX_RAW_POS &&
> > + regval < TEMP_MIN_RAW_NEG) ||
> > + regval > TEMP_MAX_RAW_NEG) {
> > + dev_err(max9611->dev,
> > + "Invalid value received from ADC 0x%4x: aborting\n",
> > + regval);
> > + return -EIO;
> > + }
> > +
> > + /* Mux shall be zeroed back before applying other configurations */
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, 0);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, 0);
> > + return ret;
> > + }
> > + usleep_range(1000, 2000);
> > +
> > + return 0;
> > +}
> > +
> > +static int max9611_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + const char * const shunt_res_prop = "shunt-resistor-uohm";
> > + struct device_node *of_node = client->dev.of_node;
> > + struct max9611_dev *max9611;
> > + struct iio_dev *indio_dev;
> > + unsigned int of_shunt;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
> > + if (IS_ERR(indio_dev))
> > + return PTR_ERR(indio_dev);
> > +
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + max9611 = iio_priv(indio_dev);
> > + max9611->dev = &client->dev;
> > + max9611->i2c_client = client;
> > + mutex_init(&max9611->lock);
> > +
> > + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> > + if (ret) {
> > + dev_err(&client->dev,
> > + "Missing %s property for %s node\n",
> > + shunt_res_prop, of_node->full_name);
> > + return ret;
> > + }
> > + max9611->shunt_resistor_uohm = of_shunt;
> > +
> > + ret = max9611_init(max9611);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->dev.of_node = client->dev.of_node;
> > + indio_dev->name = client->dev.of_node->name;
> What's this going to give for the name? Name in IIO is supposed to
> be an indication of the part rather than anything more explicit.
> That's not easily obtained from device tree directly...
>
I used the one coming from device tree as otherwise device entries
have the same name, and I wanted to have it to inclued the unit
address (eg. adc@7c and not just adc)
But from you comment I guess it's fine just adc, so I'll change this
back to v1).
Thanks
j
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &indio_info;
> > + indio_dev->channels = max9611_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
> > +
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id max9611_of_table[] = {
> > + {.compatible = "maxim,max9611"},
> > + {.compatible = "maxim,max9612"},
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, max9611_of_table);
> > +
> > +static struct i2c_driver max9611_driver = {
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = max9611_of_table,
> > + },
> > + .probe = max9611_probe,
> > +};
> > +module_i2c_driver(max9611_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
> > +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> > +MODULE_LICENSE("GPL v2");
> >
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
@ 2017-03-25 17:21 ` jacopo
0 siblings, 0 replies; 39+ messages in thread
From: jacopo @ 2017-03-25 17:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jacopo Mondi, geert-Td1EMuHUCqxL1ZNQvxDV9g,
wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Hi Jonathan,
thanks for review
On Sat, Mar 25, 2017 at 03:45:05PM +0000, Jonathan Cameron wrote:
> On 24/03/17 15:28, Jacopo Mondi wrote:
> > Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
> > with 12-bits ADC interface.
> >
> > Datasheet publicly available at:
> > https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> A few more little things inline. Coming together nicely.
>
> The channel set here is just odd enough that it might aid review to have
> a quick listing of the resulting sysfs entries. One or two authors do
> this an it is always useful for a quick sanity check.
>
> Perhaps even a set of typical values. Put this below the --- as we don't
> need it in the git history, only to assist lazy reviewers like me ;)
>
I included the output of iio_info in the cover letter, I can add some
more info here for sure!
> Thanks,
>
> Jonathan
> > ---
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 601 insertions(+)
> > create mode 100644 drivers/iio/adc/max9611.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index dedae7a..82f2e7b8 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -354,6 +354,16 @@ config MAX1363
> > To compile this driver as a module, choose M here: the module will be
> > called max1363.
> >
> > +config MAX9611
> > + tristate "Maxim max9611/max9612 ADC driver"
> > + depends on I2C
> > + help
> > + Say yes here to build support for Maxim max9611/max9612 current sense
> > + amplifier with 12-bits ADC interface.
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called max9611.
> > +
> > config MCP320X
> > tristate "Microchip Technology MCP3x01/02/04/08"
> > depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index d001262..149f979 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
> > obj-$(CONFIG_MAX1027) += max1027.o
> > obj-$(CONFIG_MAX11100) += max11100.o
> > obj-$(CONFIG_MAX1363) += max1363.o
> > +obj-$(CONFIG_MAX9611) += max9611.o
> > obj-$(CONFIG_MCP320X) += mcp320x.o
> > obj-$(CONFIG_MCP3422) += mcp3422.o
> > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > new file mode 100644
> > index 0000000..61566ec
> > --- /dev/null
> > +++ b/drivers/iio/adc/max9611.c
> > @@ -0,0 +1,590 @@
> > +/*
> > + * iio/adc/max9611.c
> > + *
> > + * Maxim max9611/max9612 high side current sense amplifier with
> > + * 12-bit ADC interface.
> > + *
> > + * Copyright (C) 2017 Jacopo Mondi
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +/*
> > + * This driver supports input common-mode voltage, current-sense
> > + * amplifier with programmable gains and die temperature reading from
> > + * Maxim max9611/max9612.
> > + *
> > + * Op-amp, analog comparator, and watchdog functionalities are not
> > + * supported by this driver.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/module.h>
> > +
> > +#define DRIVER_NAME "max9611"
> > +
> > +/* max9611 register addresses */
> > +#define MAX9611_REG_CSA_DATA 0x00
> > +#define MAX9611_REG_RS_DATA 0x02
> > +#define MAX9611_REG_TEMP_DATA 0x08
> > +#define MAX9611_REG_CTRL1 0x0a
> > +#define MAX9611_REG_CTRL2 0x0b
> > +
> > +/* max9611 REG1 mux configuration options */
> > +#define MAX9611_MUX_MASK 0x07
> > +#define MAX9611_MUX_SENSE_1x 0x00
> > +#define MAX9611_MUX_SENSE_4x 0x01
> > +#define MAX9611_MUX_SENSE_8x 0x02
> > +#define MAX9611_INPUT_VOLT 0x03
> > +#define MAX9611_MUX_TEMP 0x06
> > +
> > +/* max9611 voltage (both csa and input) helper macros */
> > +#define MAX9611_VOLTAGE_SHIFT 0x04
> > +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
> > +
> > +/*
> > + * max9611 current sense amplifier voltage output:
> > + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> > + *
> > + * GAIN LSB (nV) OFFSET (LSB steps)
> > + * 1x 107500 1
> > + * 4x 26880 1
> > + * 8x 13440 3
> > + *
> > + * The complete formula to calculate current sense voltage is:
> > + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> > + */
> > +#define CSA_VOLT_1x_LSB_nV 107500
> > +#define CSA_VOLT_4x_LSB_nV 26880
> > +#define CSA_VOLT_8x_LSB_nV 13440
> > +
> > +#define CSA_VOLT_1x_OFFS_RAW 1
> > +#define CSA_VOLT_4x_OFFS_RAW 1
> > +#define CSA_VOLT_8x_OFFS_RAW 3
> > +
> > +/*
> > + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> > + *
> > + * The complete formula to calculate input common voltage is:
> > + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> > + */
> > +#define CIM_VOLTAGE_LSB_mV 14
> > +#define CIM_VOLTAGE_OFFSET_RAW 1
> > +
> > +/*
> > + * max9611 temperature reading: LSB is 0.48 degrees Celsius
> > + *
> > + * The complete formula to calculate temperature is:
> > + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
> > + */
> I'd prefer these defines to be prefixed with MAX9611_
> Easiest to just do the lot. Some of these are 'standard' enough
> the might well clash with something that turns up in an included header
> at somepoint in the future.
>
> > +#define TEMP_SHIFT 0x07
> > +#define TEMP_MAX_RAW_POS 0x7f80
> > +#define TEMP_MAX_RAW_NEG 0xff80
> > +#define TEMP_MIN_RAW_NEG 0xd980
> > +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
> > +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
> > +#define TEMP_LSB_mC 480
> > +#define TEMP_SCALE_NUM 1000
> > +#define TEMP_SCALE_DIV 2083
> > +
> > +struct max9611_dev {
> > + struct device *dev;
> > + struct i2c_client *i2c_client;
> > + struct mutex lock;
> > + unsigned int shunt_resistor_uohm;
> > +};
> > +
> > +enum max9611_conf_ids {
> > + CONF_SENSE_1x,
> > + CONF_SENSE_4x,
> > + CONF_SENSE_8x,
> > + CONF_IN_VOLT,
> > + CONF_TEMP,
> > +};
> > +
> > +/**
> > + * max9611_mux_conf - associate ADC mux configuration with register address
> > + * where data shall be read from
> > + */
> > +static unsigned int max9611_mux_conf[][2] = {
> > + /* CONF_SENSE_1x */
> > + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
> > + /* CONF_SENSE_4x */
> > + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
> > + /* CONF_SENSE_8x */
> > + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
> > + /* CONF_IN_VOLT */
> > + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
> > + /* CONF_TEMP */
> > + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
> > +};
> > +
> > +enum max9611_csa_gain {
> > + CSA_GAIN_1x,
> > + CSA_GAIN_4x,
> > + CSA_GAIN_8x,
> > +};
> > +
> > +enum max9611_csa_gain_params {
> > + CSA_GAIN_LSB_nV,
> > + CSA_GAIN_OFFS_RAW,
> > +};
> > +
> > +/**
> > + * max9611_csa_gain_conf - associate gain multiplier with LSB and
> > + * offset values.
> > + *
> > + * Group together parameters associated with configurable gain
> > + * on current sense amplifier path to ADC interface.
> > + * Current sense read routine adjusts gain until it gets a meaningful
> > + * value; use this structure to retrieve the correct LSB and offset values.
> > + */
> > +static unsigned int max9611_gain_conf[][2] = {
> > + { /* [0] CSA_GAIN_1x */
> > + CSA_VOLT_1x_LSB_nV,
> > + CSA_VOLT_1x_OFFS_RAW,
> > + },
> > + { /* [1] CSA_GAIN_4x */
> > + CSA_VOLT_4x_LSB_nV,
> > + CSA_VOLT_4x_OFFS_RAW,
> > + },
> > + { /* [2] CSA_GAIN_8x */
> > + CSA_VOLT_8x_LSB_nV,
> > + CSA_VOLT_8x_OFFS_RAW,
> > + },
> > +};
> > +
> > +enum max9611_chan_addrs {
> > + MAX9611_CHAN_VOLTAGE_INPUT,
> > + MAX9611_CHAN_VOLTAGE_SENSE,
> > + MAX9611_CHAN_TEMPERATURE,
> > + MAX9611_CHAN_CURRENT_LOAD,
> > + MAX9611_CHAN_POWER_LOAD,
> > +};
> > +
> > +static struct iio_chan_spec max9611_channels[] = {
> > + {
> > + .type = IIO_TEMP,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + .address = MAX9611_CHAN_TEMPERATURE,
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_OFFSET),
> > + .address = MAX9611_CHAN_VOLTAGE_INPUT,
> > + .indexed = 1,
> > + .channel = 1,
> > + },
> > + {
> > + .type = IIO_VOLTAGE,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_VOLTAGE_SENSE,
> > + .indexed = 1,
> > + .channel = 0,
> Unusual to have the channels in here other than in channel order...
> > + },
> > + {
> > + .type = IIO_CURRENT,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_CURRENT_LOAD,
> > + },
> > + {
> > + .type = IIO_POWER,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .address = MAX9611_CHAN_POWER_LOAD
> > + },
> > +};
> > +
> > +/**
> > + * max9611_read_single() - read a single vale from ADC interface
> value
> > + *
> > + * Data registers are 16 bit long, spread between two 8 bit registers
> > + * with consecutive addresses.
> > + * Configure ADC mux first, then read register at address "reg_addr".
> > + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> > + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
> > + *
> > + * @max9611: max9611 device
> > + * @selector: index for mux and register configuration
> > + * @raw_val: the value returned from ADC
> > + */
> > +static int max9611_read_single(struct max9611_dev *max9611,
> > + enum max9611_conf_ids selector,
> > + u16 *raw_val)
> > +{
> > + int ret;
> > +
> > + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
> > + u8 reg_addr = max9611_mux_conf[selector][1];
> > +
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, mux_conf);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, mux_conf);
> > + return ret;
> > + }
> > +
> > + /*
> > + * need a delay here to make register configuration
> > + * stabilize. 1 msec at least, from empirical testing.
> > + */
> > + usleep_range(1000, 2000);
> > +
> > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
> > + if (ret < 0) {
> > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > + reg_addr);
> > + return ret;
> > + }
> > + *raw_val = ret;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * max9611_read_csa_voltage() - read current sense amplifier output voltage
> > + *
> > + * Current sense amplifier output voltage is read through a configurable
> > + * 1x, 4x or 8x gain.
> > + * Start with plain 1x gain, and adjust gain control properly until a
> > + * meaningful value is read from ADC output.
> > + *
> > + * @max9611: max9611 device
> > + * @adc_raw: raw value read from ADC output
> > + * @csa_gain: gain configuration option selector
> > + */
> > +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
> > + u16 *adc_raw,
> > + enum max9611_csa_gain *csa_gain)
> > +{
> > + enum max9611_conf_ids gain_selectors[] = {
> > + CONF_SENSE_1x,
> > + CONF_SENSE_4x,
> > + CONF_SENSE_8x
> > + };
> > + unsigned int i;
> > + int ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> > + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
> > + if (ret)
> > + return ret;
> > +
> > + if (*adc_raw > 0) {
> > + *csa_gain = gain_selectors[i];
> > + return 0;
> > + }
> > + }
> > +
> > + return -EIO;
> > +}
> > +
> > +static int max9611_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct max9611_dev *dev = iio_priv(indio_dev);
> > + enum max9611_csa_gain gain_selector;
> > + unsigned int *csa_gain;
> > + u16 adc_data;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&dev->lock);
> > +
> > + switch (chan->address) {
> > + case MAX9611_CHAN_TEMPERATURE:
> > + ret = max9611_read_single(dev, CONF_TEMP,
> > + &adc_data);
> > + if (ret)
> I'm not personally keen on jumping out of deep indentations like this
> just save on repeating one line. I'd pull the unlock back here and return
> directly as I feel it'll improve readability.
I'll change it, with this and Peter's suggestion on this function in
his review.
> Actually come to think of it, why does the lock need to be held for
> the next line anyway? adc_data is on the stack so doesn't matter if we
> have concurrent readers, once the i2c transaction is finished.
> Just unlock before checking ret.
I naively overvalued 'style' (ret assignment and check in two
consecutive lines) over practicality... I'll change this
> > + goto unlock_fail;
> > +
> > + *val = TEMP_RAW(adc_data);
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_INT;
> > +
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > + &adc_data);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + *val = MAX9611_VOLTAGE_RAW(adc_data);
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_INT;
> > + }
> > +
> > + case IIO_CHAN_INFO_OFFSET:
> > + switch (chan->address) {
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + *val = CIM_VOLTAGE_OFFSET_RAW;
> > +
> > + return IIO_VAL_INT;
> > + }
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->address) {
> > + case MAX9611_CHAN_TEMPERATURE:
> > + *val = TEMP_SCALE_NUM;
> > + *val2 = TEMP_SCALE_DIV;
> > +
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_VOLTAGE_INPUT:
> > + *val = CIM_VOLTAGE_LSB_mV;
> > + return IIO_VAL_INT;
> > + }
> > +
> > + case IIO_CHAN_INFO_PROCESSED:
> > + mutex_lock(&dev->lock);
> > +
> > + switch (chan->address) {
> > + case MAX9611_CHAN_VOLTAGE_SENSE:
> > + /*
> > + * processed (mV): (raw - offset) * LSB (nV) / 10^6
> > + *
> > + * Even if max9611 can output raw csa voltage readings,
> > + * use a produced value as scale depends on gain.
> > + */
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV];
> > + *val2 = 1000000;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_CURRENT_LOAD:
> > + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV];
> > + *val2 = dev->shunt_resistor_uohm;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > +
> > + case MAX9611_CHAN_POWER_LOAD:
> > + /*
> > + * processed (mW): Vin (mV) * Vcsa (uV) /
> > + * Rshunt (uOhm)
> > + */
> > + ret = max9611_read_single(dev, CONF_IN_VOLT,
> > + &adc_data);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
> > + *val = MAX9611_VOLTAGE_RAW(adc_data) *
> > + CIM_VOLTAGE_LSB_mV;
> > +
> > + ret = max9611_read_csa_voltage(dev, &adc_data,
> > + &gain_selector);
> > + if (ret)
> > + goto unlock_fail;
> > +
> > + csa_gain = max9611_gain_conf[gain_selector];
> > +
> > + /* divide by 10^3 here to avoid 32bit overflow */
> > + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> > + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
> > + csa_gain[CSA_GAIN_LSB_nV] / 1000;
> > + *val2 = dev->shunt_resistor_uohm;
> > +
> > + mutex_unlock(&dev->lock);
> > + return IIO_VAL_FRACTIONAL;
> > + }
> > + }
> > +
> > + ret = -EINVAL;
> > +
> > +unlock_fail:
> > + mutex_unlock(&dev->lock);
> > + return ret;
> > +
> > +}
> > +
> > +static ssize_t max9611_shunt_resistor_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
> > +
> > + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
> > +}
> > +
> > +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
> > + max9611_shunt_resistor_show, NULL, 0);
> > +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
> > + max9611_shunt_resistor_show, NULL, 0);
> > +
> > +static struct attribute *max9611_attributes[] = {
> > + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
> > + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group max9611_attribute_group = {
> > + .attrs = max9611_attributes,
> > +};
> > +
> > +static const struct iio_info indio_info = {
> > + .driver_module = THIS_MODULE,
> > + .read_raw = max9611_read_raw,
> > + .attrs = &max9611_attribute_group,
> > +};
> > +
> > +static int max9611_init(struct max9611_dev *max9611)
> > +{
> > + struct i2c_client *client = max9611->i2c_client;
> > + u16 regval;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> > + dev_err(max9611->dev,
> > + "No smbus support in I2c adapter: aborting probe.\n");
> This isn't necessarily an accurate message. I2c adapter might support
> smbus_read_byte only rather than word reads for example.
>
> Maybe make it more explict as to what we need?
Chopped away some details to make it fit in 80 columns.. I'll make it
more verbose...
> > + return -EINVAL;
> > + }
> > +
> > + /* Configure MUX to read temperature */
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
> > + return ret;
> > + }
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL2, 0);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL2, 0);
> > + return ret;
> > + }
> > + usleep_range(1000, 2000);
> > +
> > + /* Make sure die temperature is in range to test communications. */
> > + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
> > + MAX9611_REG_TEMP_DATA);
> > + if (ret < 0) {
> > + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> > + MAX9611_REG_TEMP_DATA);
> > + return ret;
> > + }
> > + regval = ret & ~TEMP_MASK;
> > +
> > + if ((regval > TEMP_MAX_RAW_POS &&
> > + regval < TEMP_MIN_RAW_NEG) ||
> > + regval > TEMP_MAX_RAW_NEG) {
> > + dev_err(max9611->dev,
> > + "Invalid value received from ADC 0x%4x: aborting\n",
> > + regval);
> > + return -EIO;
> > + }
> > +
> > + /* Mux shall be zeroed back before applying other configurations */
> > + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> > + MAX9611_REG_CTRL1, 0);
> > + if (ret) {
> > + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> > + MAX9611_REG_CTRL1, 0);
> > + return ret;
> > + }
> > + usleep_range(1000, 2000);
> > +
> > + return 0;
> > +}
> > +
> > +static int max9611_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + const char * const shunt_res_prop = "shunt-resistor-uohm";
> > + struct device_node *of_node = client->dev.of_node;
> > + struct max9611_dev *max9611;
> > + struct iio_dev *indio_dev;
> > + unsigned int of_shunt;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
> > + if (IS_ERR(indio_dev))
> > + return PTR_ERR(indio_dev);
> > +
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + max9611 = iio_priv(indio_dev);
> > + max9611->dev = &client->dev;
> > + max9611->i2c_client = client;
> > + mutex_init(&max9611->lock);
> > +
> > + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> > + if (ret) {
> > + dev_err(&client->dev,
> > + "Missing %s property for %s node\n",
> > + shunt_res_prop, of_node->full_name);
> > + return ret;
> > + }
> > + max9611->shunt_resistor_uohm = of_shunt;
> > +
> > + ret = max9611_init(max9611);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->dev.of_node = client->dev.of_node;
> > + indio_dev->name = client->dev.of_node->name;
> What's this going to give for the name? Name in IIO is supposed to
> be an indication of the part rather than anything more explicit.
> That's not easily obtained from device tree directly...
>
I used the one coming from device tree as otherwise device entries
have the same name, and I wanted to have it to inclued the unit
address (eg. adc@7c and not just adc)
But from you comment I guess it's fine just adc, so I'll change this
back to v1).
Thanks
j
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &indio_info;
> > + indio_dev->channels = max9611_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
> > +
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id max9611_of_table[] = {
> > + {.compatible = "maxim,max9611"},
> > + {.compatible = "maxim,max9612"},
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, max9611_of_table);
> > +
> > +static struct i2c_driver max9611_driver = {
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = max9611_of_table,
> > + },
> > + .probe = max9611_probe,
> > +};
> > +module_i2c_driver(max9611_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
> > +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> > +MODULE_LICENSE("GPL v2");
> >
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-25 17:21 ` jacopo
@ 2017-03-25 17:37 ` Jonathan Cameron
-1 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2017-03-25 17:37 UTC (permalink / raw)
To: jacopo
Cc: Jacopo Mondi, geert, wsa+renesas, magnus.damm, laurent.pinchart,
knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio,
linux-renesas-soc, devicetree
On 25/03/17 17:21, jacopo wrote:
> Hi Jonathan,
> thanks for review
>
> On Sat, Mar 25, 2017 at 03:45:05PM +0000, Jonathan Cameron wrote:
>> On 24/03/17 15:28, Jacopo Mondi wrote:
>>> Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
>>> with 12-bits ADC interface.
>>>
>>> Datasheet publicly available at:
>>> https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> A few more little things inline. Coming together nicely.
>>
>> The channel set here is just odd enough that it might aid review to have
>> a quick listing of the resulting sysfs entries. One or two authors do
>> this an it is always useful for a quick sanity check.
>>
>> Perhaps even a set of typical values. Put this below the --- as we don't
>> need it in the git history, only to assist lazy reviewers like me ;)
>>
>
> I included the output of iio_info in the cover letter, I can add some
> more info here for sure!
Just goes to show I don't always remember to check cover letters ;)
>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> drivers/iio/adc/Kconfig | 10 +
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 601 insertions(+)
>>> create mode 100644 drivers/iio/adc/max9611.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index dedae7a..82f2e7b8 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -354,6 +354,16 @@ config MAX1363
>>> To compile this driver as a module, choose M here: the module will be
>>> called max1363.
>>>
>>> +config MAX9611
>>> + tristate "Maxim max9611/max9612 ADC driver"
>>> + depends on I2C
>>> + help
>>> + Say yes here to build support for Maxim max9611/max9612 current sense
>>> + amplifier with 12-bits ADC interface.
>>> +
>>> + To compile this driver as a module, choose M here: the module will be
>>> + called max9611.
>>> +
>>> config MCP320X
>>> tristate "Microchip Technology MCP3x01/02/04/08"
>>> depends on SPI
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index d001262..149f979 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>>> obj-$(CONFIG_MAX1027) += max1027.o
>>> obj-$(CONFIG_MAX11100) += max11100.o
>>> obj-$(CONFIG_MAX1363) += max1363.o
>>> +obj-$(CONFIG_MAX9611) += max9611.o
>>> obj-$(CONFIG_MCP320X) += mcp320x.o
>>> obj-$(CONFIG_MCP3422) += mcp3422.o
>>> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>>> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
>>> new file mode 100644
>>> index 0000000..61566ec
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/max9611.c
>>> @@ -0,0 +1,590 @@
>>> +/*
>>> + * iio/adc/max9611.c
>>> + *
>>> + * Maxim max9611/max9612 high side current sense amplifier with
>>> + * 12-bit ADC interface.
>>> + *
>>> + * Copyright (C) 2017 Jacopo Mondi
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +/*
>>> + * This driver supports input common-mode voltage, current-sense
>>> + * amplifier with programmable gains and die temperature reading from
>>> + * Maxim max9611/max9612.
>>> + *
>>> + * Op-amp, analog comparator, and watchdog functionalities are not
>>> + * supported by this driver.
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/module.h>
>>> +
>>> +#define DRIVER_NAME "max9611"
>>> +
>>> +/* max9611 register addresses */
>>> +#define MAX9611_REG_CSA_DATA 0x00
>>> +#define MAX9611_REG_RS_DATA 0x02
>>> +#define MAX9611_REG_TEMP_DATA 0x08
>>> +#define MAX9611_REG_CTRL1 0x0a
>>> +#define MAX9611_REG_CTRL2 0x0b
>>> +
>>> +/* max9611 REG1 mux configuration options */
>>> +#define MAX9611_MUX_MASK 0x07
>>> +#define MAX9611_MUX_SENSE_1x 0x00
>>> +#define MAX9611_MUX_SENSE_4x 0x01
>>> +#define MAX9611_MUX_SENSE_8x 0x02
>>> +#define MAX9611_INPUT_VOLT 0x03
>>> +#define MAX9611_MUX_TEMP 0x06
>>> +
>>> +/* max9611 voltage (both csa and input) helper macros */
>>> +#define MAX9611_VOLTAGE_SHIFT 0x04
>>> +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
>>> +
>>> +/*
>>> + * max9611 current sense amplifier voltage output:
>>> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
>>> + *
>>> + * GAIN LSB (nV) OFFSET (LSB steps)
>>> + * 1x 107500 1
>>> + * 4x 26880 1
>>> + * 8x 13440 3
>>> + *
>>> + * The complete formula to calculate current sense voltage is:
>>> + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
>>> + */
>>> +#define CSA_VOLT_1x_LSB_nV 107500
>>> +#define CSA_VOLT_4x_LSB_nV 26880
>>> +#define CSA_VOLT_8x_LSB_nV 13440
>>> +
>>> +#define CSA_VOLT_1x_OFFS_RAW 1
>>> +#define CSA_VOLT_4x_OFFS_RAW 1
>>> +#define CSA_VOLT_8x_OFFS_RAW 3
>>> +
>>> +/*
>>> + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
>>> + *
>>> + * The complete formula to calculate input common voltage is:
>>> + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
>>> + */
>>> +#define CIM_VOLTAGE_LSB_mV 14
>>> +#define CIM_VOLTAGE_OFFSET_RAW 1
>>> +
>>> +/*
>>> + * max9611 temperature reading: LSB is 0.48 degrees Celsius
>>> + *
>>> + * The complete formula to calculate temperature is:
>>> + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
>>> + */
>> I'd prefer these defines to be prefixed with MAX9611_
>> Easiest to just do the lot. Some of these are 'standard' enough
>> the might well clash with something that turns up in an included header
>> at somepoint in the future.
>>
>>> +#define TEMP_SHIFT 0x07
>>> +#define TEMP_MAX_RAW_POS 0x7f80
>>> +#define TEMP_MAX_RAW_NEG 0xff80
>>> +#define TEMP_MIN_RAW_NEG 0xd980
>>> +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
>>> +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
>>> +#define TEMP_LSB_mC 480
>>> +#define TEMP_SCALE_NUM 1000
>>> +#define TEMP_SCALE_DIV 2083
>>> +
>>> +struct max9611_dev {
>>> + struct device *dev;
>>> + struct i2c_client *i2c_client;
>>> + struct mutex lock;
>>> + unsigned int shunt_resistor_uohm;
>>> +};
>>> +
>>> +enum max9611_conf_ids {
>>> + CONF_SENSE_1x,
>>> + CONF_SENSE_4x,
>>> + CONF_SENSE_8x,
>>> + CONF_IN_VOLT,
>>> + CONF_TEMP,
>>> +};
>>> +
>>> +/**
>>> + * max9611_mux_conf - associate ADC mux configuration with register address
>>> + * where data shall be read from
>>> + */
>>> +static unsigned int max9611_mux_conf[][2] = {
>>> + /* CONF_SENSE_1x */
>>> + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_SENSE_4x */
>>> + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_SENSE_8x */
>>> + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_IN_VOLT */
>>> + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
>>> + /* CONF_TEMP */
>>> + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
>>> +};
>>> +
>>> +enum max9611_csa_gain {
>>> + CSA_GAIN_1x,
>>> + CSA_GAIN_4x,
>>> + CSA_GAIN_8x,
>>> +};
>>> +
>>> +enum max9611_csa_gain_params {
>>> + CSA_GAIN_LSB_nV,
>>> + CSA_GAIN_OFFS_RAW,
>>> +};
>>> +
>>> +/**
>>> + * max9611_csa_gain_conf - associate gain multiplier with LSB and
>>> + * offset values.
>>> + *
>>> + * Group together parameters associated with configurable gain
>>> + * on current sense amplifier path to ADC interface.
>>> + * Current sense read routine adjusts gain until it gets a meaningful
>>> + * value; use this structure to retrieve the correct LSB and offset values.
>>> + */
>>> +static unsigned int max9611_gain_conf[][2] = {
>>> + { /* [0] CSA_GAIN_1x */
>>> + CSA_VOLT_1x_LSB_nV,
>>> + CSA_VOLT_1x_OFFS_RAW,
>>> + },
>>> + { /* [1] CSA_GAIN_4x */
>>> + CSA_VOLT_4x_LSB_nV,
>>> + CSA_VOLT_4x_OFFS_RAW,
>>> + },
>>> + { /* [2] CSA_GAIN_8x */
>>> + CSA_VOLT_8x_LSB_nV,
>>> + CSA_VOLT_8x_OFFS_RAW,
>>> + },
>>> +};
>>> +
>>> +enum max9611_chan_addrs {
>>> + MAX9611_CHAN_VOLTAGE_INPUT,
>>> + MAX9611_CHAN_VOLTAGE_SENSE,
>>> + MAX9611_CHAN_TEMPERATURE,
>>> + MAX9611_CHAN_CURRENT_LOAD,
>>> + MAX9611_CHAN_POWER_LOAD,
>>> +};
>>> +
>>> +static struct iio_chan_spec max9611_channels[] = {
>>> + {
>>> + .type = IIO_TEMP,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE),
>>> + .address = MAX9611_CHAN_TEMPERATURE,
>>> + },
>>> + {
>>> + .type = IIO_VOLTAGE,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>> + BIT(IIO_CHAN_INFO_OFFSET),
>>> + .address = MAX9611_CHAN_VOLTAGE_INPUT,
>>> + .indexed = 1,
>>> + .channel = 1,
>>> + },
>>> + {
>>> + .type = IIO_VOLTAGE,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_VOLTAGE_SENSE,
>>> + .indexed = 1,
>>> + .channel = 0,
>> Unusual to have the channels in here other than in channel order...
>>> + },
>>> + {
>>> + .type = IIO_CURRENT,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_CURRENT_LOAD,
>>> + },
>>> + {
>>> + .type = IIO_POWER,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_POWER_LOAD
>>> + },
>>> +};
>>> +
>>> +/**
>>> + * max9611_read_single() - read a single vale from ADC interface
>> value
>>> + *
>>> + * Data registers are 16 bit long, spread between two 8 bit registers
>>> + * with consecutive addresses.
>>> + * Configure ADC mux first, then read register at address "reg_addr".
>>> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
>>> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>>> + *
>>> + * @max9611: max9611 device
>>> + * @selector: index for mux and register configuration
>>> + * @raw_val: the value returned from ADC
>>> + */
>>> +static int max9611_read_single(struct max9611_dev *max9611,
>>> + enum max9611_conf_ids selector,
>>> + u16 *raw_val)
>>> +{
>>> + int ret;
>>> +
>>> + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
>>> + u8 reg_addr = max9611_mux_conf[selector][1];
>>> +
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, mux_conf);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, mux_conf);
>>> + return ret;
>>> + }
>>> +
>>> + /*
>>> + * need a delay here to make register configuration
>>> + * stabilize. 1 msec at least, from empirical testing.
>>> + */
>>> + usleep_range(1000, 2000);
>>> +
>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
>>> + if (ret < 0) {
>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>> + reg_addr);
>>> + return ret;
>>> + }
>>> + *raw_val = ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * max9611_read_csa_voltage() - read current sense amplifier output voltage
>>> + *
>>> + * Current sense amplifier output voltage is read through a configurable
>>> + * 1x, 4x or 8x gain.
>>> + * Start with plain 1x gain, and adjust gain control properly until a
>>> + * meaningful value is read from ADC output.
>>> + *
>>> + * @max9611: max9611 device
>>> + * @adc_raw: raw value read from ADC output
>>> + * @csa_gain: gain configuration option selector
>>> + */
>>> +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
>>> + u16 *adc_raw,
>>> + enum max9611_csa_gain *csa_gain)
>>> +{
>>> + enum max9611_conf_ids gain_selectors[] = {
>>> + CONF_SENSE_1x,
>>> + CONF_SENSE_4x,
>>> + CONF_SENSE_8x
>>> + };
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
>>> + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (*adc_raw > 0) {
>>> + *csa_gain = gain_selectors[i];
>>> + return 0;
>>> + }
>>> + }
>>> +
>>> + return -EIO;
>>> +}
>>> +
>>> +static int max9611_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct max9611_dev *dev = iio_priv(indio_dev);
>>> + enum max9611_csa_gain gain_selector;
>>> + unsigned int *csa_gain;
>>> + u16 adc_data;
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + mutex_lock(&dev->lock);
>>> +
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_TEMPERATURE:
>>> + ret = max9611_read_single(dev, CONF_TEMP,
>>> + &adc_data);
>>> + if (ret)
>> I'm not personally keen on jumping out of deep indentations like this
>> just save on repeating one line. I'd pull the unlock back here and return
>> directly as I feel it'll improve readability.
>
> I'll change it, with this and Peter's suggestion on this function in
> his review.
>
>> Actually come to think of it, why does the lock need to be held for
>> the next line anyway? adc_data is on the stack so doesn't matter if we
>> have concurrent readers, once the i2c transaction is finished.
>> Just unlock before checking ret.
>
> I naively overvalued 'style' (ret assignment and check in two
> consecutive lines) over practicality... I'll change this
>
>>> + goto unlock_fail;
>>> +
>>> + *val = TEMP_RAW(adc_data);
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_INT;
>>> +
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>> + &adc_data);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data);
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + case IIO_CHAN_INFO_OFFSET:
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + *val = CIM_VOLTAGE_OFFSET_RAW;
>>> +
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_TEMPERATURE:
>>> + *val = TEMP_SCALE_NUM;
>>> + *val2 = TEMP_SCALE_DIV;
>>> +
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + *val = CIM_VOLTAGE_LSB_mV;
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + case IIO_CHAN_INFO_PROCESSED:
>>> + mutex_lock(&dev->lock);
>>> +
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_VOLTAGE_SENSE:
>>> + /*
>>> + * processed (mV): (raw - offset) * LSB (nV) / 10^6
>>> + *
>>> + * Even if max9611 can output raw csa voltage readings,
>>> + * use a produced value as scale depends on gain.
>>> + */
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV];
>>> + *val2 = 1000000;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_CURRENT_LOAD:
>>> + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV];
>>> + *val2 = dev->shunt_resistor_uohm;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_POWER_LOAD:
>>> + /*
>>> + * processed (mW): Vin (mV) * Vcsa (uV) /
>>> + * Rshunt (uOhm)
>>> + */
>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>> + &adc_data);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + CIM_VOLTAGE_LSB_mV;
>>> +
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + /* divide by 10^3 here to avoid 32bit overflow */
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV] / 1000;
>>> + *val2 = dev->shunt_resistor_uohm;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> + }
>>> + }
>>> +
>>> + ret = -EINVAL;
>>> +
>>> +unlock_fail:
>>> + mutex_unlock(&dev->lock);
>>> + return ret;
>>> +
>>> +}
>>> +
>>> +static ssize_t max9611_shunt_resistor_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
>>> +
>>> + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
>>> + max9611_shunt_resistor_show, NULL, 0);
>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
>>> + max9611_shunt_resistor_show, NULL, 0);
>>> +
>>> +static struct attribute *max9611_attributes[] = {
>>> + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
>>> + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group max9611_attribute_group = {
>>> + .attrs = max9611_attributes,
>>> +};
>>> +
>>> +static const struct iio_info indio_info = {
>>> + .driver_module = THIS_MODULE,
>>> + .read_raw = max9611_read_raw,
>>> + .attrs = &max9611_attribute_group,
>>> +};
>>> +
>>> +static int max9611_init(struct max9611_dev *max9611)
>>> +{
>>> + struct i2c_client *client = max9611->i2c_client;
>>> + u16 regval;
>>> + int ret;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_WRITE_BYTE |
>>> + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
>>> + dev_err(max9611->dev,
>>> + "No smbus support in I2c adapter: aborting probe.\n");
>> This isn't necessarily an accurate message. I2c adapter might support
>> smbus_read_byte only rather than word reads for example.
>>
>> Maybe make it more explict as to what we need?
>
> Chopped away some details to make it fit in 80 columns.. I'll make it
> more verbose...
>
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Configure MUX to read temperature */
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>> + return ret;
>>> + }
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL2, 0);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL2, 0);
>>> + return ret;
>>> + }
>>> + usleep_range(1000, 2000);
>>> +
>>> + /* Make sure die temperature is in range to test communications. */
>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
>>> + MAX9611_REG_TEMP_DATA);
>>> + if (ret < 0) {
>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>> + MAX9611_REG_TEMP_DATA);
>>> + return ret;
>>> + }
>>> + regval = ret & ~TEMP_MASK;
>>> +
>>> + if ((regval > TEMP_MAX_RAW_POS &&
>>> + regval < TEMP_MIN_RAW_NEG) ||
>>> + regval > TEMP_MAX_RAW_NEG) {
>>> + dev_err(max9611->dev,
>>> + "Invalid value received from ADC 0x%4x: aborting\n",
>>> + regval);
>>> + return -EIO;
>>> + }
>>> +
>>> + /* Mux shall be zeroed back before applying other configurations */
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, 0);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, 0);
>>> + return ret;
>>> + }
>>> + usleep_range(1000, 2000);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int max9611_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + const char * const shunt_res_prop = "shunt-resistor-uohm";
>>> + struct device_node *of_node = client->dev.of_node;
>>> + struct max9611_dev *max9611;
>>> + struct iio_dev *indio_dev;
>>> + unsigned int of_shunt;
>>> + int ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
>>> + if (IS_ERR(indio_dev))
>>> + return PTR_ERR(indio_dev);
>>> +
>>> + i2c_set_clientdata(client, indio_dev);
>>> +
>>> + max9611 = iio_priv(indio_dev);
>>> + max9611->dev = &client->dev;
>>> + max9611->i2c_client = client;
>>> + mutex_init(&max9611->lock);
>>> +
>>> + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
>>> + if (ret) {
>>> + dev_err(&client->dev,
>>> + "Missing %s property for %s node\n",
>>> + shunt_res_prop, of_node->full_name);
>>> + return ret;
>>> + }
>>> + max9611->shunt_resistor_uohm = of_shunt;
>>> +
>>> + ret = max9611_init(max9611);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + indio_dev->dev.parent = &client->dev;
>>> + indio_dev->dev.of_node = client->dev.of_node;
>>> + indio_dev->name = client->dev.of_node->name;
>> What's this going to give for the name? Name in IIO is supposed to
>> be an indication of the part rather than anything more explicit.
>> That's not easily obtained from device tree directly...
>>
>
> I used the one coming from device tree as otherwise device entries
> have the same name, and I wanted to have it to inclued the unit
> address (eg. adc@7c and not just adc)
> But from you comment I guess it's fine just adc, so I'll change this
> back to v1).
Should be the part number - so max9611 or similar..
You can query the device node details directly if you need to identify
which is which.
>
> Thanks
> j
>
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->info = &indio_info;
>>> + indio_dev->channels = max9611_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
>>> +
>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +static const struct of_device_id max9611_of_table[] = {
>>> + {.compatible = "maxim,max9611"},
>>> + {.compatible = "maxim,max9612"},
>>> + { },
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
>>> +
>>> +static struct i2c_driver max9611_driver = {
>>> + .driver = {
>>> + .name = DRIVER_NAME,
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = max9611_of_table,
>>> + },
>>> + .probe = max9611_probe,
>>> +};
>>> +module_i2c_driver(max9611_driver);
>>> +
>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
@ 2017-03-25 17:37 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2017-03-25 17:37 UTC (permalink / raw)
To: jacopo
Cc: Jacopo Mondi, geert-Td1EMuHUCqxL1ZNQvxDV9g,
wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 25/03/17 17:21, jacopo wrote:
> Hi Jonathan,
> thanks for review
>
> On Sat, Mar 25, 2017 at 03:45:05PM +0000, Jonathan Cameron wrote:
>> On 24/03/17 15:28, Jacopo Mondi wrote:
>>> Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
>>> with 12-bits ADC interface.
>>>
>>> Datasheet publicly available at:
>>> https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
>> A few more little things inline. Coming together nicely.
>>
>> The channel set here is just odd enough that it might aid review to have
>> a quick listing of the resulting sysfs entries. One or two authors do
>> this an it is always useful for a quick sanity check.
>>
>> Perhaps even a set of typical values. Put this below the --- as we don't
>> need it in the git history, only to assist lazy reviewers like me ;)
>>
>
> I included the output of iio_info in the cover letter, I can add some
> more info here for sure!
Just goes to show I don't always remember to check cover letters ;)
>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> drivers/iio/adc/Kconfig | 10 +
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/max9611.c | 590 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 601 insertions(+)
>>> create mode 100644 drivers/iio/adc/max9611.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index dedae7a..82f2e7b8 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -354,6 +354,16 @@ config MAX1363
>>> To compile this driver as a module, choose M here: the module will be
>>> called max1363.
>>>
>>> +config MAX9611
>>> + tristate "Maxim max9611/max9612 ADC driver"
>>> + depends on I2C
>>> + help
>>> + Say yes here to build support for Maxim max9611/max9612 current sense
>>> + amplifier with 12-bits ADC interface.
>>> +
>>> + To compile this driver as a module, choose M here: the module will be
>>> + called max9611.
>>> +
>>> config MCP320X
>>> tristate "Microchip Technology MCP3x01/02/04/08"
>>> depends on SPI
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index d001262..149f979 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>>> obj-$(CONFIG_MAX1027) += max1027.o
>>> obj-$(CONFIG_MAX11100) += max11100.o
>>> obj-$(CONFIG_MAX1363) += max1363.o
>>> +obj-$(CONFIG_MAX9611) += max9611.o
>>> obj-$(CONFIG_MCP320X) += mcp320x.o
>>> obj-$(CONFIG_MCP3422) += mcp3422.o
>>> obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>>> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
>>> new file mode 100644
>>> index 0000000..61566ec
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/max9611.c
>>> @@ -0,0 +1,590 @@
>>> +/*
>>> + * iio/adc/max9611.c
>>> + *
>>> + * Maxim max9611/max9612 high side current sense amplifier with
>>> + * 12-bit ADC interface.
>>> + *
>>> + * Copyright (C) 2017 Jacopo Mondi
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +/*
>>> + * This driver supports input common-mode voltage, current-sense
>>> + * amplifier with programmable gains and die temperature reading from
>>> + * Maxim max9611/max9612.
>>> + *
>>> + * Op-amp, analog comparator, and watchdog functionalities are not
>>> + * supported by this driver.
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/module.h>
>>> +
>>> +#define DRIVER_NAME "max9611"
>>> +
>>> +/* max9611 register addresses */
>>> +#define MAX9611_REG_CSA_DATA 0x00
>>> +#define MAX9611_REG_RS_DATA 0x02
>>> +#define MAX9611_REG_TEMP_DATA 0x08
>>> +#define MAX9611_REG_CTRL1 0x0a
>>> +#define MAX9611_REG_CTRL2 0x0b
>>> +
>>> +/* max9611 REG1 mux configuration options */
>>> +#define MAX9611_MUX_MASK 0x07
>>> +#define MAX9611_MUX_SENSE_1x 0x00
>>> +#define MAX9611_MUX_SENSE_4x 0x01
>>> +#define MAX9611_MUX_SENSE_8x 0x02
>>> +#define MAX9611_INPUT_VOLT 0x03
>>> +#define MAX9611_MUX_TEMP 0x06
>>> +
>>> +/* max9611 voltage (both csa and input) helper macros */
>>> +#define MAX9611_VOLTAGE_SHIFT 0x04
>>> +#define MAX9611_VOLTAGE_RAW(_r) ((_r) >> MAX9611_VOLTAGE_SHIFT)
>>> +
>>> +/*
>>> + * max9611 current sense amplifier voltage output:
>>> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
>>> + *
>>> + * GAIN LSB (nV) OFFSET (LSB steps)
>>> + * 1x 107500 1
>>> + * 4x 26880 1
>>> + * 8x 13440 3
>>> + *
>>> + * The complete formula to calculate current sense voltage is:
>>> + * (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
>>> + */
>>> +#define CSA_VOLT_1x_LSB_nV 107500
>>> +#define CSA_VOLT_4x_LSB_nV 26880
>>> +#define CSA_VOLT_8x_LSB_nV 13440
>>> +
>>> +#define CSA_VOLT_1x_OFFS_RAW 1
>>> +#define CSA_VOLT_4x_OFFS_RAW 1
>>> +#define CSA_VOLT_8x_OFFS_RAW 3
>>> +
>>> +/*
>>> + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
>>> + *
>>> + * The complete formula to calculate input common voltage is:
>>> + * (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
>>> + */
>>> +#define CIM_VOLTAGE_LSB_mV 14
>>> +#define CIM_VOLTAGE_OFFSET_RAW 1
>>> +
>>> +/*
>>> + * max9611 temperature reading: LSB is 0.48 degrees Celsius
>>> + *
>>> + * The complete formula to calculate temperature is:
>>> + * ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
>>> + */
>> I'd prefer these defines to be prefixed with MAX9611_
>> Easiest to just do the lot. Some of these are 'standard' enough
>> the might well clash with something that turns up in an included header
>> at somepoint in the future.
>>
>>> +#define TEMP_SHIFT 0x07
>>> +#define TEMP_MAX_RAW_POS 0x7f80
>>> +#define TEMP_MAX_RAW_NEG 0xff80
>>> +#define TEMP_MIN_RAW_NEG 0xd980
>>> +#define TEMP_MASK ((1 << TEMP_SHIFT) - 1)
>>> +#define TEMP_RAW(_r) ((_r) >> TEMP_SHIFT)
>>> +#define TEMP_LSB_mC 480
>>> +#define TEMP_SCALE_NUM 1000
>>> +#define TEMP_SCALE_DIV 2083
>>> +
>>> +struct max9611_dev {
>>> + struct device *dev;
>>> + struct i2c_client *i2c_client;
>>> + struct mutex lock;
>>> + unsigned int shunt_resistor_uohm;
>>> +};
>>> +
>>> +enum max9611_conf_ids {
>>> + CONF_SENSE_1x,
>>> + CONF_SENSE_4x,
>>> + CONF_SENSE_8x,
>>> + CONF_IN_VOLT,
>>> + CONF_TEMP,
>>> +};
>>> +
>>> +/**
>>> + * max9611_mux_conf - associate ADC mux configuration with register address
>>> + * where data shall be read from
>>> + */
>>> +static unsigned int max9611_mux_conf[][2] = {
>>> + /* CONF_SENSE_1x */
>>> + { MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_SENSE_4x */
>>> + { MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_SENSE_8x */
>>> + { MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
>>> + /* CONF_IN_VOLT */
>>> + { MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
>>> + /* CONF_TEMP */
>>> + { MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
>>> +};
>>> +
>>> +enum max9611_csa_gain {
>>> + CSA_GAIN_1x,
>>> + CSA_GAIN_4x,
>>> + CSA_GAIN_8x,
>>> +};
>>> +
>>> +enum max9611_csa_gain_params {
>>> + CSA_GAIN_LSB_nV,
>>> + CSA_GAIN_OFFS_RAW,
>>> +};
>>> +
>>> +/**
>>> + * max9611_csa_gain_conf - associate gain multiplier with LSB and
>>> + * offset values.
>>> + *
>>> + * Group together parameters associated with configurable gain
>>> + * on current sense amplifier path to ADC interface.
>>> + * Current sense read routine adjusts gain until it gets a meaningful
>>> + * value; use this structure to retrieve the correct LSB and offset values.
>>> + */
>>> +static unsigned int max9611_gain_conf[][2] = {
>>> + { /* [0] CSA_GAIN_1x */
>>> + CSA_VOLT_1x_LSB_nV,
>>> + CSA_VOLT_1x_OFFS_RAW,
>>> + },
>>> + { /* [1] CSA_GAIN_4x */
>>> + CSA_VOLT_4x_LSB_nV,
>>> + CSA_VOLT_4x_OFFS_RAW,
>>> + },
>>> + { /* [2] CSA_GAIN_8x */
>>> + CSA_VOLT_8x_LSB_nV,
>>> + CSA_VOLT_8x_OFFS_RAW,
>>> + },
>>> +};
>>> +
>>> +enum max9611_chan_addrs {
>>> + MAX9611_CHAN_VOLTAGE_INPUT,
>>> + MAX9611_CHAN_VOLTAGE_SENSE,
>>> + MAX9611_CHAN_TEMPERATURE,
>>> + MAX9611_CHAN_CURRENT_LOAD,
>>> + MAX9611_CHAN_POWER_LOAD,
>>> +};
>>> +
>>> +static struct iio_chan_spec max9611_channels[] = {
>>> + {
>>> + .type = IIO_TEMP,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE),
>>> + .address = MAX9611_CHAN_TEMPERATURE,
>>> + },
>>> + {
>>> + .type = IIO_VOLTAGE,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>> + BIT(IIO_CHAN_INFO_OFFSET),
>>> + .address = MAX9611_CHAN_VOLTAGE_INPUT,
>>> + .indexed = 1,
>>> + .channel = 1,
>>> + },
>>> + {
>>> + .type = IIO_VOLTAGE,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_VOLTAGE_SENSE,
>>> + .indexed = 1,
>>> + .channel = 0,
>> Unusual to have the channels in here other than in channel order...
>>> + },
>>> + {
>>> + .type = IIO_CURRENT,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_CURRENT_LOAD,
>>> + },
>>> + {
>>> + .type = IIO_POWER,
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + .address = MAX9611_CHAN_POWER_LOAD
>>> + },
>>> +};
>>> +
>>> +/**
>>> + * max9611_read_single() - read a single vale from ADC interface
>> value
>>> + *
>>> + * Data registers are 16 bit long, spread between two 8 bit registers
>>> + * with consecutive addresses.
>>> + * Configure ADC mux first, then read register at address "reg_addr".
>>> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
>>> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>>> + *
>>> + * @max9611: max9611 device
>>> + * @selector: index for mux and register configuration
>>> + * @raw_val: the value returned from ADC
>>> + */
>>> +static int max9611_read_single(struct max9611_dev *max9611,
>>> + enum max9611_conf_ids selector,
>>> + u16 *raw_val)
>>> +{
>>> + int ret;
>>> +
>>> + u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
>>> + u8 reg_addr = max9611_mux_conf[selector][1];
>>> +
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, mux_conf);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, mux_conf);
>>> + return ret;
>>> + }
>>> +
>>> + /*
>>> + * need a delay here to make register configuration
>>> + * stabilize. 1 msec at least, from empirical testing.
>>> + */
>>> + usleep_range(1000, 2000);
>>> +
>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
>>> + if (ret < 0) {
>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>> + reg_addr);
>>> + return ret;
>>> + }
>>> + *raw_val = ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * max9611_read_csa_voltage() - read current sense amplifier output voltage
>>> + *
>>> + * Current sense amplifier output voltage is read through a configurable
>>> + * 1x, 4x or 8x gain.
>>> + * Start with plain 1x gain, and adjust gain control properly until a
>>> + * meaningful value is read from ADC output.
>>> + *
>>> + * @max9611: max9611 device
>>> + * @adc_raw: raw value read from ADC output
>>> + * @csa_gain: gain configuration option selector
>>> + */
>>> +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
>>> + u16 *adc_raw,
>>> + enum max9611_csa_gain *csa_gain)
>>> +{
>>> + enum max9611_conf_ids gain_selectors[] = {
>>> + CONF_SENSE_1x,
>>> + CONF_SENSE_4x,
>>> + CONF_SENSE_8x
>>> + };
>>> + unsigned int i;
>>> + int ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
>>> + ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (*adc_raw > 0) {
>>> + *csa_gain = gain_selectors[i];
>>> + return 0;
>>> + }
>>> + }
>>> +
>>> + return -EIO;
>>> +}
>>> +
>>> +static int max9611_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct max9611_dev *dev = iio_priv(indio_dev);
>>> + enum max9611_csa_gain gain_selector;
>>> + unsigned int *csa_gain;
>>> + u16 adc_data;
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + mutex_lock(&dev->lock);
>>> +
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_TEMPERATURE:
>>> + ret = max9611_read_single(dev, CONF_TEMP,
>>> + &adc_data);
>>> + if (ret)
>> I'm not personally keen on jumping out of deep indentations like this
>> just save on repeating one line. I'd pull the unlock back here and return
>> directly as I feel it'll improve readability.
>
> I'll change it, with this and Peter's suggestion on this function in
> his review.
>
>> Actually come to think of it, why does the lock need to be held for
>> the next line anyway? adc_data is on the stack so doesn't matter if we
>> have concurrent readers, once the i2c transaction is finished.
>> Just unlock before checking ret.
>
> I naively overvalued 'style' (ret assignment and check in two
> consecutive lines) over practicality... I'll change this
>
>>> + goto unlock_fail;
>>> +
>>> + *val = TEMP_RAW(adc_data);
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_INT;
>>> +
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>> + &adc_data);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data);
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + case IIO_CHAN_INFO_OFFSET:
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + *val = CIM_VOLTAGE_OFFSET_RAW;
>>> +
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_TEMPERATURE:
>>> + *val = TEMP_SCALE_NUM;
>>> + *val2 = TEMP_SCALE_DIV;
>>> +
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_VOLTAGE_INPUT:
>>> + *val = CIM_VOLTAGE_LSB_mV;
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + case IIO_CHAN_INFO_PROCESSED:
>>> + mutex_lock(&dev->lock);
>>> +
>>> + switch (chan->address) {
>>> + case MAX9611_CHAN_VOLTAGE_SENSE:
>>> + /*
>>> + * processed (mV): (raw - offset) * LSB (nV) / 10^6
>>> + *
>>> + * Even if max9611 can output raw csa voltage readings,
>>> + * use a produced value as scale depends on gain.
>>> + */
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV];
>>> + *val2 = 1000000;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_CURRENT_LOAD:
>>> + /* processed (mA): Vcsa (nV) / Rshunt (uOhm) */
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV];
>>> + *val2 = dev->shunt_resistor_uohm;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> +
>>> + case MAX9611_CHAN_POWER_LOAD:
>>> + /*
>>> + * processed (mW): Vin (mV) * Vcsa (uV) /
>>> + * Rshunt (uOhm)
>>> + */
>>> + ret = max9611_read_single(dev, CONF_IN_VOLT,
>>> + &adc_data);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + adc_data -= CIM_VOLTAGE_OFFSET_RAW;
>>> + *val = MAX9611_VOLTAGE_RAW(adc_data) *
>>> + CIM_VOLTAGE_LSB_mV;
>>> +
>>> + ret = max9611_read_csa_voltage(dev, &adc_data,
>>> + &gain_selector);
>>> + if (ret)
>>> + goto unlock_fail;
>>> +
>>> + csa_gain = max9611_gain_conf[gain_selector];
>>> +
>>> + /* divide by 10^3 here to avoid 32bit overflow */
>>> + adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
>>> + *val *= MAX9611_VOLTAGE_RAW(adc_data) *
>>> + csa_gain[CSA_GAIN_LSB_nV] / 1000;
>>> + *val2 = dev->shunt_resistor_uohm;
>>> +
>>> + mutex_unlock(&dev->lock);
>>> + return IIO_VAL_FRACTIONAL;
>>> + }
>>> + }
>>> +
>>> + ret = -EINVAL;
>>> +
>>> +unlock_fail:
>>> + mutex_unlock(&dev->lock);
>>> + return ret;
>>> +
>>> +}
>>> +
>>> +static ssize_t max9611_shunt_resistor_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
>>> +
>>> + return sprintf(buf, "%d\n", max9611->shunt_resistor_uohm);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_power, 0444,
>>> + max9611_shunt_resistor_show, NULL, 0);
>>> +static IIO_DEVICE_ATTR(in_shunt_resistor_current, 0444,
>>> + max9611_shunt_resistor_show, NULL, 0);
>>> +
>>> +static struct attribute *max9611_attributes[] = {
>>> + &iio_dev_attr_in_shunt_resistor_power.dev_attr.attr,
>>> + &iio_dev_attr_in_shunt_resistor_current.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group max9611_attribute_group = {
>>> + .attrs = max9611_attributes,
>>> +};
>>> +
>>> +static const struct iio_info indio_info = {
>>> + .driver_module = THIS_MODULE,
>>> + .read_raw = max9611_read_raw,
>>> + .attrs = &max9611_attribute_group,
>>> +};
>>> +
>>> +static int max9611_init(struct max9611_dev *max9611)
>>> +{
>>> + struct i2c_client *client = max9611->i2c_client;
>>> + u16 regval;
>>> + int ret;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_WRITE_BYTE |
>>> + I2C_FUNC_SMBUS_READ_WORD_DATA)) {
>>> + dev_err(max9611->dev,
>>> + "No smbus support in I2c adapter: aborting probe.\n");
>> This isn't necessarily an accurate message. I2c adapter might support
>> smbus_read_byte only rather than word reads for example.
>>
>> Maybe make it more explict as to what we need?
>
> Chopped away some details to make it fit in 80 columns.. I'll make it
> more verbose...
>
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Configure MUX to read temperature */
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, MAX9611_MUX_TEMP);
>>> + return ret;
>>> + }
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL2, 0);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL2, 0);
>>> + return ret;
>>> + }
>>> + usleep_range(1000, 2000);
>>> +
>>> + /* Make sure die temperature is in range to test communications. */
>>> + ret = i2c_smbus_read_word_swapped(max9611->i2c_client,
>>> + MAX9611_REG_TEMP_DATA);
>>> + if (ret < 0) {
>>> + dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
>>> + MAX9611_REG_TEMP_DATA);
>>> + return ret;
>>> + }
>>> + regval = ret & ~TEMP_MASK;
>>> +
>>> + if ((regval > TEMP_MAX_RAW_POS &&
>>> + regval < TEMP_MIN_RAW_NEG) ||
>>> + regval > TEMP_MAX_RAW_NEG) {
>>> + dev_err(max9611->dev,
>>> + "Invalid value received from ADC 0x%4x: aborting\n",
>>> + regval);
>>> + return -EIO;
>>> + }
>>> +
>>> + /* Mux shall be zeroed back before applying other configurations */
>>> + ret = i2c_smbus_write_byte_data(max9611->i2c_client,
>>> + MAX9611_REG_CTRL1, 0);
>>> + if (ret) {
>>> + dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> + MAX9611_REG_CTRL1, 0);
>>> + return ret;
>>> + }
>>> + usleep_range(1000, 2000);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int max9611_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + const char * const shunt_res_prop = "shunt-resistor-uohm";
>>> + struct device_node *of_node = client->dev.of_node;
>>> + struct max9611_dev *max9611;
>>> + struct iio_dev *indio_dev;
>>> + unsigned int of_shunt;
>>> + int ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
>>> + if (IS_ERR(indio_dev))
>>> + return PTR_ERR(indio_dev);
>>> +
>>> + i2c_set_clientdata(client, indio_dev);
>>> +
>>> + max9611 = iio_priv(indio_dev);
>>> + max9611->dev = &client->dev;
>>> + max9611->i2c_client = client;
>>> + mutex_init(&max9611->lock);
>>> +
>>> + ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
>>> + if (ret) {
>>> + dev_err(&client->dev,
>>> + "Missing %s property for %s node\n",
>>> + shunt_res_prop, of_node->full_name);
>>> + return ret;
>>> + }
>>> + max9611->shunt_resistor_uohm = of_shunt;
>>> +
>>> + ret = max9611_init(max9611);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + indio_dev->dev.parent = &client->dev;
>>> + indio_dev->dev.of_node = client->dev.of_node;
>>> + indio_dev->name = client->dev.of_node->name;
>> What's this going to give for the name? Name in IIO is supposed to
>> be an indication of the part rather than anything more explicit.
>> That's not easily obtained from device tree directly...
>>
>
> I used the one coming from device tree as otherwise device entries
> have the same name, and I wanted to have it to inclued the unit
> address (eg. adc@7c and not just adc)
> But from you comment I guess it's fine just adc, so I'll change this
> back to v1).
Should be the part number - so max9611 or similar..
You can query the device node details directly if you need to identify
which is which.
>
> Thanks
> j
>
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->info = &indio_info;
>>> + indio_dev->channels = max9611_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
>>> +
>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +static const struct of_device_id max9611_of_table[] = {
>>> + {.compatible = "maxim,max9611"},
>>> + {.compatible = "maxim,max9612"},
>>> + { },
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
>>> +
>>> +static struct i2c_driver max9611_driver = {
>>> + .driver = {
>>> + .name = DRIVER_NAME,
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = max9611_of_table,
>>> + },
>>> + .probe = max9611_probe,
>>> +};
>>> +module_i2c_driver(max9611_driver);
>>> +
>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
@ 2017-03-26 10:02 ` jacopo
0 siblings, 0 replies; 39+ messages in thread
From: jacopo @ 2017-03-26 10:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jacopo Mondi, geert, wsa+renesas, magnus.damm, laurent.pinchart,
knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio,
linux-renesas-soc, devicetree
Hi Jonathan,
On Sat, Mar 25, 2017 at 05:37:52PM +0000, Jonathan Cameron wrote:
> On 25/03/17 17:21, jacopo wrote:
> > Hi Jonathan,
> > thanks for review
[snip]
> >>> +
> >>> + indio_dev->dev.parent = &client->dev;
> >>> + indio_dev->dev.of_node = client->dev.of_node;
> >>> + indio_dev->name = client->dev.of_node->name;
> >> What's this going to give for the name? Name in IIO is supposed to
> >> be an indication of the part rather than anything more explicit.
> >> That's not easily obtained from device tree directly...
> >>
> >
> > I used the one coming from device tree as otherwise device entries
> > have the same name, and I wanted to have it to inclued the unit
> > address (eg. adc@7c and not just adc)
> > But from you comment I guess it's fine just adc, so I'll change this
> > back to v1).
> Should be the part number - so max9611 or similar..
>
> You can query the device node details directly if you need to identify
> which is which.
That would not help, as I've been suggested to use a generic "adc" in
node name property.
I can hard-code "max9611" here. That would not help with the fact that
two chips will appear in userspace with the same name (and that's why
I wanted to have the unit address).
Otherwise I can do what Quentin is suggesting in his review of
AST2400: have different name for each compatible entry, so that this
will appear as either max9611 or max9612 in userspace
Thanks
j
> >
> > Thanks
> > j
> >
> >>> + indio_dev->modes = INDIO_DIRECT_MODE;
> >>> + indio_dev->info = &indio_info;
> >>> + indio_dev->channels = max9611_channels;
> >>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
> >>> +
> >>> + return devm_iio_device_register(&client->dev, indio_dev);
> >>> +}
> >>> +
> >>> +static const struct of_device_id max9611_of_table[] = {
> >>> + {.compatible = "maxim,max9611"},
> >>> + {.compatible = "maxim,max9612"},
> >>> + { },
> >>> +};
> >>> +
> >>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
> >>> +
> >>> +static struct i2c_driver max9611_driver = {
> >>> + .driver = {
> >>> + .name = DRIVER_NAME,
> >>> + .owner = THIS_MODULE,
> >>> + .of_match_table = max9611_of_table,
> >>> + },
> >>> + .probe = max9611_probe,
> >>> +};
> >>> +module_i2c_driver(max9611_driver);
> >>> +
> >>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
> >>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> >>> +MODULE_LICENSE("GPL v2");
> >>>
> >>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
@ 2017-03-26 10:02 ` jacopo
0 siblings, 0 replies; 39+ messages in thread
From: jacopo @ 2017-03-26 10:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jacopo Mondi, geert-Td1EMuHUCqxL1ZNQvxDV9g,
wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Hi Jonathan,
On Sat, Mar 25, 2017 at 05:37:52PM +0000, Jonathan Cameron wrote:
> On 25/03/17 17:21, jacopo wrote:
> > Hi Jonathan,
> > thanks for review
[snip]
> >>> +
> >>> + indio_dev->dev.parent = &client->dev;
> >>> + indio_dev->dev.of_node = client->dev.of_node;
> >>> + indio_dev->name = client->dev.of_node->name;
> >> What's this going to give for the name? Name in IIO is supposed to
> >> be an indication of the part rather than anything more explicit.
> >> That's not easily obtained from device tree directly...
> >>
> >
> > I used the one coming from device tree as otherwise device entries
> > have the same name, and I wanted to have it to inclued the unit
> > address (eg. adc@7c and not just adc)
> > But from you comment I guess it's fine just adc, so I'll change this
> > back to v1).
> Should be the part number - so max9611 or similar..
>
> You can query the device node details directly if you need to identify
> which is which.
That would not help, as I've been suggested to use a generic "adc" in
node name property.
I can hard-code "max9611" here. That would not help with the fact that
two chips will appear in userspace with the same name (and that's why
I wanted to have the unit address).
Otherwise I can do what Quentin is suggesting in his review of
AST2400: have different name for each compatible entry, so that this
will appear as either max9611 or max9612 in userspace
Thanks
j
> >
> > Thanks
> > j
> >
> >>> + indio_dev->modes = INDIO_DIRECT_MODE;
> >>> + indio_dev->info = &indio_info;
> >>> + indio_dev->channels = max9611_channels;
> >>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
> >>> +
> >>> + return devm_iio_device_register(&client->dev, indio_dev);
> >>> +}
> >>> +
> >>> +static const struct of_device_id max9611_of_table[] = {
> >>> + {.compatible = "maxim,max9611"},
> >>> + {.compatible = "maxim,max9612"},
> >>> + { },
> >>> +};
> >>> +
> >>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
> >>> +
> >>> +static struct i2c_driver max9611_driver = {
> >>> + .driver = {
> >>> + .name = DRIVER_NAME,
> >>> + .owner = THIS_MODULE,
> >>> + .of_match_table = max9611_of_table,
> >>> + },
> >>> + .probe = max9611_probe,
> >>> +};
> >>> +module_i2c_driver(max9611_driver);
> >>> +
> >>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
> >>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> >>> +MODULE_LICENSE("GPL v2");
> >>>
> >>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
2017-03-26 10:02 ` jacopo
@ 2017-03-26 10:23 ` Jonathan Cameron
-1 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2017-03-26 10:23 UTC (permalink / raw)
To: jacopo
Cc: Jacopo Mondi, geert, wsa+renesas, magnus.damm, laurent.pinchart,
knaack.h, lars, pmeerw, robh+dt, mark.rutland, linux-iio,
linux-renesas-soc, devicetree
On 26/03/17 11:02, jacopo wrote:
> Hi Jonathan,
>
> On Sat, Mar 25, 2017 at 05:37:52PM +0000, Jonathan Cameron wrote:
>> On 25/03/17 17:21, jacopo wrote:
>>> Hi Jonathan,
>>> thanks for review
>
> [snip]
>
>>>>> +
>>>>> + indio_dev->dev.parent = &client->dev;
>>>>> + indio_dev->dev.of_node = client->dev.of_node;
>>>>> + indio_dev->name = client->dev.of_node->name;
>>>> What's this going to give for the name? Name in IIO is supposed to
>>>> be an indication of the part rather than anything more explicit.
>>>> That's not easily obtained from device tree directly...
>>>>
>>>
>>> I used the one coming from device tree as otherwise device entries
>>> have the same name, and I wanted to have it to inclued the unit
>>> address (eg. adc@7c and not just adc)
>>> But from you comment I guess it's fine just adc, so I'll change this
>>> back to v1).
>> Should be the part number - so max9611 or similar..
>>
>> You can query the device node details directly if you need to identify
>> which is which.
>
> That would not help, as I've been suggested to use a generic "adc" in
> node name property.
>
> I can hard-code "max9611" here. That would not help with the fact that
> two chips will appear in userspace with the same name (and that's why
> I wanted to have the unit address)
This is just a convenience dating way back before device tree. The one
thing it is still useful for is identifying when a part is different from
what the devicetree suggests - particularly when fairly generic compatibles
are in use but the device has an ID register. In a lot of consumer
boards these chips get changed to 'newer' (i.e. cheaper) versions that
are more or less compatible without any devicetree changes (or
longer ago without it being reflected in difference in the board file).
If you want to know the device tree node it is easily available via
/sys/bus/iio/device/iio:deviceX/uevent for example.
Having just fired up a beaglebone blue I have:
root@beaglebone:/sys/bus/iio/devices/iio:device0# cat uevent
MAJOR=243
MINOR=0
DEVNAME=iio:device0
DEVTYPE=iio_device
OF_NAME=mpu9150
OF_FULLNAME=/ocp/i2c@4819c000/mpu9150@68
OF_COMPATIBLE_0=invensense,mpu9150
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device0# cat name
mpu9150
Which is actually lying as it's a 9250 - my fault in this case as I
wrote the relevant bit of the device tree and it's not mainlined yet.
root@beaglebone:/sys/bus/iio/devices/iio:device1# cat uevent
MAJOR=243
MINOR=1
DEVNAME=iio:device1
DEVTYPE=iio_device
OF_NAME=bmp280
OF_FULLNAME=/ocp/i2c@4819c000/bmp280@76
OF_COMPATIBLE_0=bosch,bmp280
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device1# cat name
bmp280
root@beaglebone:/sys/bus/iio/devices/iio:device2# cat uevent
MAJOR=243
MINOR=2
DEVNAME=iio:device2
DEVTYPE=iio_device
OF_NAME=adc
OF_FULLNAME=/ocp/tscadc@44e0d000/adc
OF_COMPATIBLE_0=ti,am3359-adc
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device2# cat name
TI-am335x-adc
root@beaglebone:/sys/bus/iio/devices/iio:device3# cat uevent
MAJOR=243
MINOR=3
DEVNAME=iio:device3
DEVTYPE=iio_device
OF_NAME=ax8975
OF_FULLNAME=/ocp/i2c@4819c000/mpu9150@68/i2c-gate/ax8975@c
OF_COMPATIBLE_0=ak,ak8975
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device3# cat name
ak8975
So all the info you could possibly want in in userspace is
available without using the name attribute - even down to
the complex nature of the path to that ak8975.
At various points in the past submitters have put drivers
in with the devicetree node or other bus related naming
and we haven't always picked up on it in review.
As we can't break userspace ABI not much we can do about it
now unfortunately.
Mind you, this has reminded me that I need to fix that mpu9150
case above before anyone mainlines the devicetree for the blue.
>
> Otherwise I can do what Quentin is suggesting in his review of
> AST2400: have different name for each compatible entry, so that this
> will appear as either max9611 or max9612 in userspace
>
That's ideal. It should be as specific as we can make it.
> Thanks
> j
>
>>>
>>> Thanks
>>> j
>>>
>>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>>> + indio_dev->info = &indio_info;
>>>>> + indio_dev->channels = max9611_channels;
>>>>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
>>>>> +
>>>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id max9611_of_table[] = {
>>>>> + {.compatible = "maxim,max9611"},
>>>>> + {.compatible = "maxim,max9612"},
>>>>> + { },
>>>>> +};
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
>>>>> +
>>>>> +static struct i2c_driver max9611_driver = {
>>>>> + .driver = {
>>>>> + .name = DRIVER_NAME,
>>>>> + .owner = THIS_MODULE,
>>>>> + .of_match_table = max9611_of_table,
>>>>> + },
>>>>> + .probe = max9611_probe,
>>>>> +};
>>>>> +module_i2c_driver(max9611_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
>>>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver
@ 2017-03-26 10:23 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2017-03-26 10:23 UTC (permalink / raw)
To: jacopo
Cc: Jacopo Mondi, geert-Td1EMuHUCqxL1ZNQvxDV9g,
wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 26/03/17 11:02, jacopo wrote:
> Hi Jonathan,
>
> On Sat, Mar 25, 2017 at 05:37:52PM +0000, Jonathan Cameron wrote:
>> On 25/03/17 17:21, jacopo wrote:
>>> Hi Jonathan,
>>> thanks for review
>
> [snip]
>
>>>>> +
>>>>> + indio_dev->dev.parent = &client->dev;
>>>>> + indio_dev->dev.of_node = client->dev.of_node;
>>>>> + indio_dev->name = client->dev.of_node->name;
>>>> What's this going to give for the name? Name in IIO is supposed to
>>>> be an indication of the part rather than anything more explicit.
>>>> That's not easily obtained from device tree directly...
>>>>
>>>
>>> I used the one coming from device tree as otherwise device entries
>>> have the same name, and I wanted to have it to inclued the unit
>>> address (eg. adc@7c and not just adc)
>>> But from you comment I guess it's fine just adc, so I'll change this
>>> back to v1).
>> Should be the part number - so max9611 or similar..
>>
>> You can query the device node details directly if you need to identify
>> which is which.
>
> That would not help, as I've been suggested to use a generic "adc" in
> node name property.
>
> I can hard-code "max9611" here. That would not help with the fact that
> two chips will appear in userspace with the same name (and that's why
> I wanted to have the unit address)
This is just a convenience dating way back before device tree. The one
thing it is still useful for is identifying when a part is different from
what the devicetree suggests - particularly when fairly generic compatibles
are in use but the device has an ID register. In a lot of consumer
boards these chips get changed to 'newer' (i.e. cheaper) versions that
are more or less compatible without any devicetree changes (or
longer ago without it being reflected in difference in the board file).
If you want to know the device tree node it is easily available via
/sys/bus/iio/device/iio:deviceX/uevent for example.
Having just fired up a beaglebone blue I have:
root@beaglebone:/sys/bus/iio/devices/iio:device0# cat uevent
MAJOR=243
MINOR=0
DEVNAME=iio:device0
DEVTYPE=iio_device
OF_NAME=mpu9150
OF_FULLNAME=/ocp/i2c@4819c000/mpu9150@68
OF_COMPATIBLE_0=invensense,mpu9150
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device0# cat name
mpu9150
Which is actually lying as it's a 9250 - my fault in this case as I
wrote the relevant bit of the device tree and it's not mainlined yet.
root@beaglebone:/sys/bus/iio/devices/iio:device1# cat uevent
MAJOR=243
MINOR=1
DEVNAME=iio:device1
DEVTYPE=iio_device
OF_NAME=bmp280
OF_FULLNAME=/ocp/i2c@4819c000/bmp280@76
OF_COMPATIBLE_0=bosch,bmp280
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device1# cat name
bmp280
root@beaglebone:/sys/bus/iio/devices/iio:device2# cat uevent
MAJOR=243
MINOR=2
DEVNAME=iio:device2
DEVTYPE=iio_device
OF_NAME=adc
OF_FULLNAME=/ocp/tscadc@44e0d000/adc
OF_COMPATIBLE_0=ti,am3359-adc
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device2# cat name
TI-am335x-adc
root@beaglebone:/sys/bus/iio/devices/iio:device3# cat uevent
MAJOR=243
MINOR=3
DEVNAME=iio:device3
DEVTYPE=iio_device
OF_NAME=ax8975
OF_FULLNAME=/ocp/i2c@4819c000/mpu9150@68/i2c-gate/ax8975@c
OF_COMPATIBLE_0=ak,ak8975
OF_COMPATIBLE_N=1
root@beaglebone:/sys/bus/iio/devices/iio:device3# cat name
ak8975
So all the info you could possibly want in in userspace is
available without using the name attribute - even down to
the complex nature of the path to that ak8975.
At various points in the past submitters have put drivers
in with the devicetree node or other bus related naming
and we haven't always picked up on it in review.
As we can't break userspace ABI not much we can do about it
now unfortunately.
Mind you, this has reminded me that I need to fix that mpu9150
case above before anyone mainlines the devicetree for the blue.
>
> Otherwise I can do what Quentin is suggesting in his review of
> AST2400: have different name for each compatible entry, so that this
> will appear as either max9611 or max9612 in userspace
>
That's ideal. It should be as specific as we can make it.
> Thanks
> j
>
>>>
>>> Thanks
>>> j
>>>
>>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>>> + indio_dev->info = &indio_info;
>>>>> + indio_dev->channels = max9611_channels;
>>>>> + indio_dev->num_channels = ARRAY_SIZE(max9611_channels);
>>>>> +
>>>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id max9611_of_table[] = {
>>>>> + {.compatible = "maxim,max9611"},
>>>>> + {.compatible = "maxim,max9612"},
>>>>> + { },
>>>>> +};
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(of, max9611_of_table);
>>>>> +
>>>>> +static struct i2c_driver max9611_driver = {
>>>>> + .driver = {
>>>>> + .name = DRIVER_NAME,
>>>>> + .owner = THIS_MODULE,
>>>>> + .of_match_table = max9611_of_table,
>>>>> + },
>>>>> + .probe = max9611_probe,
>>>>> +};
>>>>> +module_i2c_driver(max9611_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
>>>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 39+ messages in thread