On Tue, 28 Mar 2023 00:14:16 +0530
Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
> >
> >> From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> >>
> >> max597x has 10bit ADC for voltage & current monitoring.
> >> Use iio framework to expose the same in sysfs.
> >>
> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> >
> > 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, ®_l);
> >> + if (ret < 0)
> >> + return ret;
> >> + ret = regmap_read(data->regmap, chan->address - 1, ®_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, ®_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
