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
Agreed. A separate ID will tidy this up and may provide a transition path
to let you do the other parts in multiples steps.
>
> >
> > 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
Perfect.
> >
> > 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?
Yes, though hopefully we can also provide a _SCALE to give userspace the
information it needs.
>
