Re: [PATCH 1/2] iio: max597x: Add support for max597x

[Lars-Peter Clausen <lars@xxxxxxxxxx>]

Hi,

This looks really good. A few minor comments inline.

On 3/22/23 05:43, Naresh Solanki wrote:
> [...]
> +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);
> +	unsigned int reg_l, reg_h;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(data->regmap, chan->address, &reg_l);
> +		if (ret < 0)
> +			return ret;
> +		ret = regmap_read(data->regmap, chan->address - 1, &reg_h);
> +		if (ret < 0)
> +			return ret;
Is there any chance of a race condition of getting inconsistent data 
when splitting this over two reads? I.e. registers being updated with 
new values in between the two reads.
> +		*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):
> +			fallthrough;

`fallthrough` should not be needed for multiple case statements right on top of each other with no code in between. Same below

> +		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;
ADC_MASK should really have a MAX5970_ prefix, but I guess it is defined 
in max597x.h
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX5970_REG_VOLTAGE_L(0):
> +			fallthrough;
> +		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;
> +}
> [..]
> +static int max597x_iio_probe(struct platform_device *pdev)
> +{
> +	struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
> +	struct i2c_client *i2c = to_i2c_client(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;
> +
> +	/* registering iio */
> +	indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
For the devm allocations we should be using &pdev->dev and not the I2C 
device, since this is the device to which the allocations belong and 
where they should be freed when the device is removed.
> +	if (!indio_dev) {
> +		dev_err(&i2c->dev, "failed allocating iio device\n");
Consider using dev_err_probe() for error message printing. This will 
give a consistent formatting of the messages. Also again use &pdev->dev 
instead of I2C device to get the right device listed in the error messages.
> +		return -ENOMEM;
> +	}
> +	indio_dev->name = dev_name(&i2c->dev);
The IIO ABI wants the type of the chip for the name. E.g. "max5970", 
using dev_name() of the parent I2C device will result in something else.
> [...]