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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 28 Mar 2023 00:14:16 +0530
Naresh Solanki <naresh.solanki@9elements.com> wrote:

> Hi,
> 
> On 26-03-2023 01:06 am, Jonathan Cameron wrote:
> > On Thu, 23 Mar 2023 20:45:48 +0100
> > Naresh Solanki <naresh.solanki@9elements.com> wrote:
> >   
> >> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >>
> >> max597x 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>  
> > 
> > I'm not a fan of wild cards in driver names. This doesn't
> > for example support the max5974, max5971 etc
> > 
> > Much better to name it after one of the supported parts.
> > Obviously can't do much about the mfd driver now, but I'd prefer
> > not to carry that through to the IIO driver if possible.
> > 
> > One concern I have here is that from the max5978 datasheet I see
> > this device supports features that are very much directed at hwmon
> > type usecases.  In particular warning and critical threshold detection.
> > We don't support multiple thresholds (in same direction) for a single
> > channel via IIO.  If you want those features in the future you may want
> > to consider using the hwmon subsystem.
> > 
> > We tend to be flexible with devices that sit near the boundary of IIO
> > and hwmon because we can bridge many of the features using the iio-hwmon
> > bridge driver.  That doesn't work for more complex event handling and
> > I suspect some of the other features this device provides.  
> I believe it is the most appropriate approach for our use case at the 
> moment. If we decide to incorporate more complex event handling or need 
> to support multiple thresholds in the future, we will definitely 
> consider using the hwmon subsystem. Thank for your input.

It's not easy to move a driver (because of need to maintain ABI compatibility
in most cases).  Hence I'd suggest at least CCing the hwmon list and maintainers
on future versions with a cover letter than explains your reasoning on why
this particular support should use IIO.


> > 
> >   
> >> +
> >> +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;
> >> +		*val = (reg_h << 2) | (reg_l & 3);  
> > 
> > I replied late to previous patch, but I'd prefer to see a bulk read if
> > possible.  It might ensure a matched pair, or if not reduce the chance of
> > tearing (when reg_l & 3 transitions from 3 to 0 for example and
> > reg_h & 1 is going from 0 to 1)
> > 
> > You could try a repeated read if the sampling rate is fairly low as
> > simply getting same high bits on either side of the low bit read is probably
> > enough to say tearing didn't happen.  
> Yes. will use something like:
> 		ret = regmap_bulk_read(data->regmap, chan->address - 1, &reg_l, 2);
> 		if (ret < 0)
> 			return ret;
> 		reg_h = reg_l & 0xff;
> 		reg_l = (reg_l >> 8) & 0xff;
> 		*val = (reg_h << 2) | (reg_l & 3);
As you are going to handle them as separate registers (which makes sense under the
circumstances) read into a u8 regs[2] then express this as the following which also
deals with endian issues by make the registering ordering explicit.
*val = (reg[0] << 2) | (reg[1] & 3);

Thanks,

Jonathan