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

[Jonathan Cameron]

	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.

>