Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>> 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@xxxxxxxxxx>");
>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux