Re: [PATCH v1] staging: iio: ad7816: add iio interface

From: JuenKit Yip
Date: Sun Jul 16 2023 - 20:40:24 EST


在 2023/7/16 21:40, Jonathan Cameron 写道:
On Sun, 9 Jul 2023 00:29:58 +0800
JuenKit Yip <JuenKit_Yip@xxxxxxxxxxx> wrote:

add iio interface for 4 channels, replacing the previous sysfs
interface

Signed-off-by: JuenKit Yip <JuenKit_Yip@xxxxxxxxxxx>
Hi JuenKit,

Great to see some work on this driver.

A few comments inline. Mostly they are about the fact we can't unwind
this part of the interface without dealing with the other use of the existing 'channel'
attribute.

This is a great start, but need to jump forwards a few more steps so that
we don't accidentally reduce or confuse the existing functionality.


---
drivers/staging/iio/adc/ad7816.c | 122 +++++++++++++++----------------
1 file changed, 59 insertions(+), 63 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 6c14d7bcdd67..8af117b6ae11 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -162,64 +162,17 @@ static ssize_t ad7816_show_available_modes(struct device *dev,
static IIO_DEVICE_ATTR(available_modes, 0444, ad7816_show_available_modes,
NULL, 0);
-static ssize_t ad7816_show_channel(struct device *dev,
- struct device_attribute *attr,
- char *buf)
+static int ad7816_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val,
+ int *val2,
+ long m)
Probably better to rewrap this to get closer to the 80 char line length.
ack

{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct ad7816_chip_info *chip = iio_priv(indio_dev);
-
- return sprintf(buf, "%d\n", chip->channel_id);
-}
-
-static ssize_t ad7816_store_channel(struct device *dev,
- struct device_attribute *attr,
- const char *buf,
- size_t len)
-{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct ad7816_chip_info *chip = iio_priv(indio_dev);
- unsigned long data;
- int ret;
-
- ret = kstrtoul(buf, 10, &data);
- if (ret)
- return ret;
-
- if (data > AD7816_CS_MAX && data != AD7816_CS_MASK) {
- dev_err(&chip->spi_dev->dev, "Invalid channel id %lu for %s.\n",
- data, indio_dev->name);
- return -EINVAL;
- } else if (strcmp(indio_dev->name, "ad7818") == 0 && data > 1) {
- dev_err(&chip->spi_dev->dev,
- "Invalid channel id %lu for ad7818.\n", data);
- return -EINVAL;
- } else if (strcmp(indio_dev->name, "ad7816") == 0 && data > 0) {
- dev_err(&chip->spi_dev->dev,
- "Invalid channel id %lu for ad7816.\n", data);
- return -EINVAL;
- }
-
- chip->channel_id = data;
-
- return len;
-}
-
-static IIO_DEVICE_ATTR(channel, 0644,
- ad7816_show_channel,
- ad7816_store_channel,
- 0);
-
-static ssize_t ad7816_show_value(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct ad7816_chip_info *chip = iio_priv(indio_dev);
u16 data;
- s8 value;
int ret;
+ chip->channel_id = (u8)chan->channel;
Can we keep the channel_id local?
It is used for over temperature detection (OTI) but that needs separating out.

ack, maybe need a another commit.

channel_id may be removed from ad7816_chip_info


Given you'll be breaking that connection I think you need to deal with
both the main attributes and the event ones in a single go. Thus removing
any hidden usage of the last channel touched like you have here.


ret = ad7816_spi_read(chip, &data);
if (ret)
return -EIO;
@@ -227,22 +180,21 @@ static ssize_t ad7816_show_value(struct device *dev,
data >>= AD7816_VALUE_OFFSET;
if (chip->channel_id == 0) {
- value = (s8)((data >> AD7816_TEMP_FLOAT_OFFSET) - 103);
- data &= AD7816_TEMP_FLOAT_MASK;
- if (value < 0)
- data = BIT(AD7816_TEMP_FLOAT_OFFSET) - data;
- return sprintf(buf, "%d.%.2d\n", value, data * 25);
+ *val = (s8)((data >> AD7816_TEMP_FLOAT_OFFSET) - 103);
Use masks and FIELD_GET() though that change perhaps belongs in a separate patch set.
ack
+ *val2 = (data & AD7816_TEMP_FLOAT_MASK) * 25;
+ if (*val < 0)
+ *val2 = BIT(AD7816_TEMP_FLOAT_OFFSET) - *val2;
+ return IIO_VAL_INT_PLUS_MICRO;
}
- return sprintf(buf, "%u\n", data);
-}
-static IIO_DEVICE_ATTR(value, 0444, ad7816_show_value, NULL, 0);
+ *val = data;
+
+ return IIO_VAL_INT;
+}
static struct attribute *ad7816_attributes[] = {
&iio_dev_attr_available_modes.dev_attr.attr,
&iio_dev_attr_mode.dev_attr.attr,
- &iio_dev_attr_channel.dev_attr.attr,
- &iio_dev_attr_value.dev_attr.attr,
NULL,
};
@@ -341,10 +293,47 @@ static const struct attribute_group ad7816_event_attribute_group = {
};
static const struct iio_info ad7816_info = {
+ .read_raw = ad7816_read_raw,
.attrs = &ad7816_attribute_group,
.event_attrs = &ad7816_event_attribute_group,
};
+static const struct iio_chan_spec ad7816_channels[] = {
+ {
+ .type = IIO_TEMP,
+ .indexed = 1,
+ .channel = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ },
+};
+
+static const struct iio_chan_spec ad7817_channels[] = {
+ {
+ .type = IIO_TEMP,
+ .indexed = 1,
+ .channel = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
This would require the reading presented to be in the units defined by
the ABI (Documentation/ABI/testing/sysfs-bus-iio)
Can you confirm that these are all correct?
I will upload test report

Note it is very unusual for an IIO driver to present all processed channels.
Superficially it looks like there might be some appropriate conversions done
for the temperature channels for them to be in the right units, but nothing
at all is done to the voltage channels...

In fact, I hope to set voltage channel to RAW, and leave conversion to users.

Is it a good idea?


+ },
+ {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .channel = 1,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ },
+ {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .channel = 2,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ },
+ {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .channel = 3,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ },
+};
+
/*
* device probe and remove
*/
@@ -367,6 +356,13 @@ static int ad7816_probe(struct spi_device *spi_dev)
chip->oti_data[i] = 203;
chip->id = spi_get_device_id(spi_dev)->driver_data;
+ if (chip->id == ID_AD7816) {
+ indio_dev->channels = ad7816_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ad7816_channels);
+ } else {
+ indio_dev->channels = ad7817_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ad7817_channels);
+ }
chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_OUT_HIGH);
if (IS_ERR(chip->rdwr_pin)) {
ret = PTR_ERR(chip->rdwr_pin);