Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c8f709d3-1a8e-c98c-6917-839a0ca4d4ff@9elements.com>
Date:   Tue, 4 Apr 2023 15:37:21 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH v3 1/2] iio: max597x: Add support for max597x
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
References: <20230328094416.3851801-1-Naresh.Solanki@9elements.com>
 <20230402180123.0fc1cfde@jic23-huawei>
Content-Language: en-US
From:   Naresh Solanki <naresh.solanki@9elements.com>
In-Reply-To: <20230402180123.0fc1cfde@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jonathan,

On 02-04-2023 10:31 pm, Jonathan Cameron wrote:
> On Tue, 28 Mar 2023 11:44:14 +0200
> Naresh Solanki <naresh.solanki@9elements.com> wrote:
> 
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> max5970 & max5978 has 10bit ADC for voltage & current
>> monitoring.
>> Use iio framework to expose the same in sysfs.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ...
> 
> --- not ...
> 
> As I mentioned in my reply to v2 thread (which crossed with this v3)
> I'd like this series to be cc'd to the list and maintainers for Hwmon
> with a cover letter explaining the reasoning for it being an IIO driver
> + the restrictions that potentially brings.
Sure.
> 
> A few other comments inline from taking another look.
> 
> Thanks,
> 
> Jonathan
> 
> 
>> Changes in V3:
>> - Use bulk read
>> - Remove line split
>> Changes in V2:
>> - Remove fallthrough
>> - Use pdev->dev instead of i2c->dev
>> - Init indio_dev->name based on device type.
>> ---
>>   drivers/iio/adc/Kconfig       |  15 ++++
>>   drivers/iio/adc/Makefile      |   1 +
>>   drivers/iio/adc/max597x-iio.c | 148 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 164 insertions(+)
>>   create mode 100644 drivers/iio/adc/max597x-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 45af2302be53..69310af5c665 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -735,6 +735,21 @@ config MAX1363
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called max1363.
>>   
>> +config MAX597X_IIO
>> +	tristate "Maxim 597x power switch and monitor"
>> +	depends on I2C && OF
>> +	select MFD_MAX597X
>> +	help
>> +	  This driver enables support for the Maxim 5970 & 5978 smart switch
>> +	  and voltage/current monitoring interface using the Industrial I/O
>> +	  (IIO) framework. The Maxim 597x is a power switch and monitor that can
>> +	  provide voltage and current measurements via the I2C bus. Enabling
>> +	  this driver will allow user space applications to read the voltage
>> +	  and current measurements using IIO interfaces.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called max597x-iio.
>> +
>>   config MAX9611
>>   	tristate "Maxim max9611/max9612 ADC driver"
>>   	depends on I2C
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 36c18177322a..7ec0c2cf7bbb 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
>>   obj-$(CONFIG_MAX11410) += max11410.o
>>   obj-$(CONFIG_MAX1241) += max1241.o
>>   obj-$(CONFIG_MAX1363) += max1363.o
>> +obj-$(CONFIG_MAX597X_IIO) += max597x-iio.o
>>   obj-$(CONFIG_MAX9611) += max9611.o
>>   obj-$(CONFIG_MCP320X) += mcp320x.o
>>   obj-$(CONFIG_MCP3422) += mcp3422.o
>> diff --git a/drivers/iio/adc/max597x-iio.c b/drivers/iio/adc/max597x-iio.c
>> new file mode 100644
>> index 000000000000..f158e49b5a56
>> --- /dev/null
>> +++ b/drivers/iio/adc/max597x-iio.c
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device driver for IIO in MAX5970 and MAX5978 IC
>> + *
>> + * Copyright (c) 2022 9elements GmbH
>> + *
>> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com>
>> + */
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/mfd/max597x.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct max597x_iio {
>> +	struct regmap *regmap;
>> +	int shunt_micro_ohms[MAX5970_NUM_SWITCHES];
>> +	unsigned int irng[MAX5970_NUM_SWITCHES];
>> +	unsigned int mon_rng[MAX5970_NUM_SWITCHES];
>> +};
>> +
>> +#define MAX597X_ADC_CHANNEL(_idx, _type) {			\
>> +	.type = IIO_ ## _type,					\
>> +	.indexed = 1,						\
>> +	.channel = (_idx),					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
>> +			      BIT(IIO_CHAN_INFO_SCALE),		\
>> +	.address = MAX5970_REG_ ## _type ## _L(_idx),		\
>> +}
>> +
>> +static const struct iio_chan_spec max5978_adc_iio_channels[] = {
>> +	MAX597X_ADC_CHANNEL(0, VOLTAGE),
>> +	MAX597X_ADC_CHANNEL(0, CURRENT),
>> +};
>> +
>> +static const struct iio_chan_spec max5970_adc_iio_channels[] = {
>> +	MAX597X_ADC_CHANNEL(0, VOLTAGE),
>> +	MAX597X_ADC_CHANNEL(0, CURRENT),
>> +	MAX597X_ADC_CHANNEL(1, VOLTAGE),
>> +	MAX597X_ADC_CHANNEL(1, CURRENT),
>> +};
>> +
>> +static int max597x_iio_read_raw(struct iio_dev *iio_dev,
>> +				struct iio_chan_spec const *chan,
>> +				int *val, int *val2, long info)
>> +{
>> +	int ret;
>> +	struct max597x_iio *data = iio_priv(iio_dev);
>> +	u16 reg_l, reg_h;
>> +
>> +	switch (info) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = regmap_bulk_read(data->regmap, chan->address - 1, &reg_l, 2);
> 
> This crossed with my reply to the v2 thread.  If reading two separate registers that
> don't directly correspond to a large packed value, read them into an array
> of u8.  Then get the relevant parts from that.
Sure
> 
> The following use is not endian safe (it won't work on a big endian machine)
Sure
> 
>> +		if (ret < 0)
>> +			return ret;
>> +		reg_h = reg_l & 0xff;
>> +		reg_l = (reg_l >> 8) & 0xff;
>> +		*val = (reg_h << 2) | (reg_l & 3);
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->address) {
>> +		case MAX5970_REG_CURRENT_L(0):
>> +		case MAX5970_REG_CURRENT_L(1):
>> +			/* in A, convert to mA */
>> +			*val = data->irng[chan->channel] * 1000;
>> +			*val2 = data->shunt_micro_ohms[chan->channel] * ADC_MASK;
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX5970_REG_VOLTAGE_L(0):
>> +		case MAX5970_REG_VOLTAGE_L(1):
>> +			/* in uV, convert to mV */
>> +			*val = data->mon_rng[chan->channel];
>> +			*val2 = ADC_MASK * 1000;
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>> +		break;
>> +	}
>> +	return -EINVAL;
> 
> I'd prefer this pushed up as a default: in each of the two switch statements.
> Makes it clear to compilers and readers that only listing some case values is
> deliberate.
Sure
> 
>> +}
>> +
>> +static const struct iio_info max597x_adc_iio_info = {
>> +	.read_raw = &max597x_iio_read_raw,
>> +};
>> +
>> +static int max597x_iio_probe(struct platform_device *pdev)
>> +{
>> +	struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
>> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	struct iio_dev *indio_dev;
>> +	struct max597x_iio *priv;
>> +	int ret, i;
>> +
>> +	if (!regmap)
>> +		return -EPROBE_DEFER;
>> +
>> +	if (!max597x || !max597x->num_switches)
>> +		return -EPROBE_DEFER;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
>> +	if (!indio_dev)
>> +		return dev_err_probe(&pdev->dev, -ENOMEM,
>> +				     "failed to allocate iio device\n");
>> +
>> +	indio_dev->info = &max597x_adc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	switch (max597x->num_switches) {
> 
> Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
> Perhaps rename it to type.
Will add a local variable type to track the same within with driver.

> 
> 
>> +	case MAX597x_TYPE_MAX5970:
>> +		indio_dev->channels = max5970_adc_iio_channels;
>> +		indio_dev->num_channels = ARRAY_SIZE(max5970_adc_iio_channels);
>> +		indio_dev->name = "max5970";
>> +		break;
>> +	case MAX597x_TYPE_MAX5978:
>> +		indio_dev->channels = max5978_adc_iio_channels;
>> +		indio_dev->num_channels = ARRAY_SIZE(max5978_adc_iio_channels);
>> +		indio_dev->name = "max5978";
>> +		break;
>> +	}
>> +
>> +	priv = iio_priv(indio_dev);
>> +	priv->regmap = regmap;
>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>> +		priv->irng[i] = max597x->irng[i];
>> +		priv->mon_rng[i] = max597x->mon_rng[i];
>> +		priv->shunt_micro_ohms[i] = max597x->shunt_micro_ohms[i];
>> +	}
>> +
>> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "could not register iio device\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver max597x_iio_driver = {
>> +	.driver = {
>> +		.name = "max597x-iio",
>> +	},
>> +	.probe = max597x_iio_probe,
>> +};
>> +
>> +module_platform_driver(max597x_iio_driver);
>> +
>> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
>> +MODULE_DESCRIPTION("MAX5970_hot-swap controller driver");
>> +MODULE_LICENSE("GPL");
>>
>> base-commit: 368eb79f738a21e16c2bdbcac2444dfa96b01aaa
> 

Regards,
Naresh