Message-ID:
 <BE1P281MB2420B7C7A95A913B486A2228EFF42@BE1P281MB2420.DEUP281.PROD.OUTLOOK.COM>
Date: Tue, 4 Feb 2025 15:50:18 +0000
From: "Sperling, Tobias" <Tobias.Sperling@...ting.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, "linux-iio@...r.kernel.org"
	<linux-iio@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] iio: adc: Add driver for ADS7128 / ADS7138

Just one more question, as I've received a mail from the kernel test robot.
It reported two warnings regarding following unused functions
ads71x8_runtime_suspend() and ads71x8_runtime_resume().
I guess this is due PM not being used necessarily. So do I still have to use
#ifdef CONFIG_PM or what did I miss there?

static int ads71x8_runtime_suspend(struct device *dev)
{
        struct iio_dev *indio_dev = dev_get_drvdata(dev);
        struct ads71x8_data *data = iio_priv(indio_dev);

        return ads71x8_set_conv_mode(data, ADS71x8_MODE_MANUAL);
}

static int ads71x8_runtime_resume(struct device *dev)
{
        struct iio_dev *indio_dev = dev_get_drvdata(dev);
        struct ads71x8_data *data = iio_priv(indio_dev);

        return ads71x8_set_conv_mode(data, ADS71x8_MODE_AUTO);
}

static const struct dev_pm_ops ads71x8_pm_ops = {
        SET_RUNTIME_PM_OPS(ads71x8_runtime_suspend, ads71x8_runtime_resume, NULL)
};

....

static struct i2c_driver ads71x8_driver = {
        .driver = {
                .name = "ads7138",
                .of_match_table = ads71x8_of_match,
                .pm = pm_ptr(&ads71x8_pm_ops),
        },
        .id_table = ads71x8_device_ids,
        .probe = ads71x8_probe,
};
module_i2c_driver(ads71x8_driver);


> On Mon, 3 Feb 2025 16:57:19 +0000
> "Sperling, Tobias" <Tobias.Sperling@...ting.com> wrote:
>
> > Hi Jonathan,
> > thanks for the great feedback, I tried to improve all the mentioned things, just
> > some comments/questions inline.
> >
> > Regards,
> > Tobi
> >
> > > > +static const struct ads71x8_freq_bits ads71x8_samp_freq[] = {
> > > > + {163, 0x1F}, {244, 0x1E}, {326, 0x1D}, {488, 0x1C}, {651, 0x1B},
> > > > + {977, 0x1A}, {1302, 0x19}, {1953, 0x18}, {2604, 0x17}, {3906, 0x16},
> > > > + {5208, 0x15}, {7813, 0x14}, {10417, 0x13}, {15625, 0x12}, {20833, 0x11},
> > > > + {31250, 0x10}, {41667, 0x09}, {62500, 0x08}, {83333, 0x07},
> > > > + {125000, 0x06}, {166667, 0x05}, {250000, 0x04}, {333333, 0x03},
> > > > + {500000, 0x02}, {666667, 0x01}, {1000000, 0x0}
> > > Format this as something like.
> > >     { 163, 0x1F }, { 244, 0x1E }, { 326, 0x1D }, { 488, 0x1C },
> > >     { 651, 0x1B }, { 977, 0x1A }, { 1302, 0x19 }, { 1953, 0x18 },
> > >
> > > So with more spaces and with a power of 2 entries on each line to make it
> easy
> > > for people to work out the matching.
> > >
> > > Once you use read_avail as requested below, you may well just want to use
> > > the index of the array for the second field and have a simple array of value
> > > assuming no holes that I'm missing.
> >
> > There would have been some holes, as some register values lead to the same
> frequency.
> > I just changed this to repeat these values then in the list. Should be fine now
> and the
> > array's index can be used now.
>
> Ok.  Generally when that happens we don't export repeats in read_avail.
>
> So may be back to this approach :(

Ok I will adapt.

> >
> > > > +static ssize_t ads71x8_read_stats(struct iio_dev *indio_dev, uintptr_t priv,
> > > > + const struct iio_chan_spec *chan, char *buf)
> > > > +{
> > > > + struct ads71x8_data *data = iio_priv(indio_dev);
> > > > + int ret;
> > > > + u8 values[2];
> > > > +
> > > > + switch (priv) {
> > > > + case ADS71x8_STATS_MIN:
> > > > +         ret = ads71x8_i2c_read_block(data->client,
> > > > +                 ADS71x8_REG_MIN_LSB_CH(chan->channel), values,
> > > > +                 ARRAY_SIZE(values));
> > > > +         if (ret < 0)
> > > > +                 return ret;
> > > > +         break;
> > > > + case ADS71x8_STATS_MAX:
> > > > +         ret = ads71x8_i2c_read_block(data->client,
> > > > +                 ADS71x8_REG_MAX_LSB_CH(chan->channel), values,
> > > > +                 ARRAY_SIZE(values));
> > > > +         if (ret < 0)
> > > > +                 return ret;
> > > > +         break;
> > > > + default:
> > > > +         return -EINVAL;
> > > > + }
> > > > +
> > > > + return sprintf(buf, "%d\n", ((values[1] << 8) | values[0]));
> > >
> > > I've no ideas what this is, so needs docs.
> >
> > See comment below regarding custom ABI.
> >
> > > That last bit is a get_unaligned_le16() though so use that to make it
> > > explicit what is going on.
> >
> > > > +};
> > > > +
> > > > +static const struct attribute_group ads71x8_attribute_group = {
> > > > + .attrs = ads71x8_attributes,
> > > > +};
> > > > +
> > > > +static const struct iio_info ti_ads71x8_info = {
> > > > + .attrs = &ads71x8_attribute_group,
> > > > + .read_raw = &ads71x8_read_raw,
> > > > + .write_raw = &ads71x8_write_raw,
> > > > + .read_event_value = &ads71x8_read_event,
> > > > + .write_event_value = &ads71x8_write_event,
> > > > + .read_event_config = &ads71x8_read_event_config,
> > > > + .write_event_config = &ads71x8_write_event_config,
> > > Definitely worth thinking about whether the device can be used to
> > > some degree at least without interrupts.  It is annoyingly common
> > > for board designers to not wire them.
> > >
> > > If it is easy to support (without events) from the start that
> > > is a nice to have. If more complex we can leave it until we know
> > > of actual hardware.
> >
> > In general, this driver could be used without interrupts. What remains
> > is the reading of the ADC values, which probably is sufficient most of
> > the time.
> > Is this what you had in mind?
>
> Yes.

I'll also change this, to make the interrupt optional.

> >
> > > > +static const struct iio_chan_spec_ext_info ads71x8_ext_info[] = {
> > > > + {"stats_min", IIO_SEPARATE, ads71x8_read_stats, NULL,
> > > ADS71x8_STATS_MIN},
> > > > + {"stats_max", IIO_SEPARATE, ads71x8_read_stats, NULL,
> > > ADS71x8_STATS_MAX},
> > > > + {},
> > >     { "stats_min", ...
> > >     { }
> > >
> > > No comma for terminating entries as we don't want it to be easy to add more
> > > after them.
> > >
> > > However, the fields in this structure are non obvious, so
> > >     {
> > >             .name = "stats_min",
> > > etc
> > > preferred.
> > >
> > > This is custom ABI, so I'd expect to see a file under
> > > Documentation/ABI/testing/sysfs-bus-iio-*
> > > that explains what these are.
> > >
> > > Adding custom ABI however is a hard thing, so provide plenty of information
> > > to see if these are justified or not.
> > > Superficially they sound like debugfs things rather than suitable for sysfs.
> >
> > In the current configuration the IC is automatically making some statistics about
> > the minimal and maximum value that were seen on each channel, which can be
> > read back by this ABI.
> > This as quick info, do you think it makes sense to add this as custom ABI?
>
> For max we do have existing ABI peak and trough (only one user of that)
> https://elixir.bootl/
> in.com%2Flinux%2Fv6.13.1%2Fsource%2FDocumentation%2FABI%2Ftesting%2Fs
> ysfs-bus-
> iio%23L363&data=05%7C02%7C%7Cc0561925fc764bf0d29008dd452a3fcd%7Cfe
> 3606fad3974238999768dcd7851f64%7C1%7C0%7C638742769956935518%7CU
> nknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCI
> sIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata
> =sL3aAnBltvf9J7Cn0dgBXUujyc%2B%2BnGxU%2F5%2F%2FMlnmfWQ%3D&reser
> ved=0
> Would those work for you?

Yes, looks like this will work for me. Thanks.

> >
> > Otherwise, making this part of the debugfs, I guess you are talking about
> > granting access via debugfs_reg_access of the iio_info, don't you?
> > And this then also needs docu in "Documentation/ABI/testing/debugfs-bus-iio-
> *",
> > doesn't it?
>
> This doesn't really feel like a feature intended for debug, so better to
> use main ABI. If we need to add something we can.
>
>
> Jonathan
>

Regards and thanks in advance.
Tobi

Powered by blists - more mailing lists