On Mon, 3 Feb 2025 16:57:19 +0000
"Sperling, Tobias" <Tobias.Sperling@xxxxxxxxxxx> 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 :(
>
> > > +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.
>
> > > +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.bootlin.com/linux/v6.13.1/source/Documentation/ABI/testing/sysfs-bus-iio#L363
Would those work for you?
>
> 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
