AW: [PATCH 2/2] iio: adc: Add driver for ADS7128 / ADS7138

[Sperling, Tobias]

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.

> > +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?

> > +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?

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?

> > +static int ads7138_init_hw(struct ads71x8_data *data)
> > +{
> > +	int ret = 0;
> Always set, so don't init here.
> 
> > +
> > +	data->vref_regu = devm_regulator_get(&data->client->dev, "avdd");
> > +	if (IS_ERR(data->vref_regu))
> > +		data->vref_regu = NULL;
> What is intent here? If you don't have a regulator you'll get a stub
> which isn't an error. If you get an error for another reason, something
> is very wrong, so error out. I may just be a deferral though so you
> may end up successfully probing later.

Changed this to devm_regulator_get_optional() as I wanted the supply to be
optionally. If available it will be used to return the actual scale value.