Re: [PATCH 3/4] iio: adc: Add support for the GE HealthCare PMC ADC

[Jonathan Cameron]

On Wed, 2 Oct 2024 10:23:24 +0200
Herve Codina <herve.codina@xxxxxxxxxxx> wrote:

> Hi Jonathan,
> 
> On Tue, 1 Oct 2024 20:24:30 +0100
> Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> 
> > On Tue,  1 Oct 2024 09:46:17 +0200
> > Herve Codina <herve.codina@xxxxxxxxxxx> wrote:
> >   
> > > The GE HealthCare PMC Analog to Digital Converter (ADC) is a 16-Channel
> > > (voltage and current), 16-Bit ADC with an I2C Interface.
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>    
> > 
> > Just one thing to add to David's review.
> > 
> > I'm going to guess this isn't a general purpose ADC? Can you share any info
> > on what sort of device it is used in?
> > 
> > No problem if not - I'm just curious as I've not seen GE HealthCare I2C parts
> > before!  
> 
> I cannot tell about the product it is used in :(
> One sure thing I can say is that the component itself is not available off
> the shelf, and is fully designed to be used in a specific product.
> 
> >   
> > > diff --git a/drivers/iio/adc/gehc-pmc-adc.c b/drivers/iio/adc/gehc-pmc-adc.c
> > > new file mode 100644
> > > index 000000000000..c46c2fb84d35
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/gehc-pmc-adc.c
> > > @@ -0,0 +1,233 @@    
> > 
> >   
> > > +
> > > +static int pmc_adc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > > +			    int *val, int *val2, long mask)
> > > +{
> > > +	struct pmc_adc *pmc_adc = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = pmc_adc_read_raw_ch(pmc_adc, chan->address, val);
> > > +		if (ret)
> > > +			return ret;
> > > +		return IIO_VAL_INT;
> > > +
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		*val = 1; /* Raw values are directly read in mV or mA */    
> > 
> > Drop this scale and make the channels processed. That saves userspace even applying  
> 
> I thought that scale was mandatory.
> 
> From the userspace, offset is clearly optional
>   https://elixir.bootlin.com/linux/v6.11/source/Documentation/ABI/testing/sysfs-bus-iio#L458
> But nothing about a default value is mentioned in the scale description
>   https://elixir.bootlin.com/linux/v6.11/source/Documentation/ABI/testing/sysfs-bus-iio#L515
It doesn't get applied to _PROCESSED attributes (see ABI for _input attributes which
simply doesn't say to apply anything.

There is a corner case where _OFFSET is provided but not _SCALE and hence the channel
is _RAW.  In that case I'd say _SCALE is optional, but I'm not sure we've ever seen
it in reality!  The more common case (though still rare) is like this one where 
the reading is in the base units of the ABI so _input is the away to go.
This is fairly ancient ABI lifted from hwmon.


> 
> > the *1 this indicates.  Rare to find a device that outputs in our base units
> > but might as well take advantage of one that does :)  
> 
> Yes, the device is a custom designed device and it has a fully knowledge (by
> design) of the board it is soldered on. As I was involved in the communication
> protocol definition, units were chosen to fit well with IIO.

Nice :)
> 
> >   
> > > +		return IIO_VAL_INT;
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}    
> 
> Best regards,
> Hervé