| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DB4PR10MB626128364C9D170DFFAD0A48923BA@DB4PR10MB6261.EURPRD10.PROD.OUTLOOK.COM>
Date: Mon, 17 Jul 2023 08:40:02 +0800
From: JuenKit Yip <JuenKit_Yip@...mail.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
gregkh@...uxfoundation.org, linux-iio@...r.kernel.org,
linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] staging: iio: ad7816: add iio interface
在 2023/7/16 21:40, Jonathan Cameron 写道:
> On Sun, 9 Jul 2023 00:29:58 +0800
> JuenKit Yip <JuenKit_Yip@...mail.com> wrote:
>
>> add iio interface for 4 channels, replacing the previous sysfs
>> interface
>>
>> Signed-off-by: JuenKit Yip <JuenKit_Yip@...mail.com>
> 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);
>
Powered by blists - more mailing lists