Re: [PATCH v1] iio: adc: intel_mrfld_adc: Add Basin Cove ADC driver

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 19 Mar 2019 23:25:07 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Mar 19, 2019 at 08:02:15PM +0100, Tomasz Duszynski wrote:
> > On Tue, Mar 19, 2019 at 12:37:28AM +0200, Andy Shevchenko wrote:  
> > > On Mon, Mar 18, 2019 at 09:52:42PM +0100, Tomasz Duszynski wrote:  
> > > > On Mon, Mar 18, 2019 at 12:51:03PM +0300, Andy Shevchenko wrote:  
> 
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/driver.h>
> > > > > +#include <linux/iio/machine.h>  
> 
> > > > Alphabetical order is preferred.  
> > >
> > > Ah, you meant iio.h vs. driver.h?
> > >  
> > 
> > Yes.  
> 
> It won't compile in alphabetical order.

That's a little worrying as this should definitely not be order dependant.
What was the error? 

Ah, I'm guessing struct iio_dev not defined.
If you would like to put a patch in to add that forward definition in driver.h
that would be great.  Its an omission rather than intentional.

Any chance of getting the whole MFD including this buildable with COMPILE_TEST?
Otherwise coverage likely to be somewhat reduced (I probably won't build it
normally for starters!)

Jonathan

> 
> > > > > +	u8 buf[2];
> > > > > +	int ret;  
> > >  
> > > > > +	ret = regmap_bulk_read(regmap, chan->address, buf, 2);
> > > > > +	if (ret)
> > > > > +		goto done;
> > > > > +
> > > > > +	*result = (buf[0] << 8) | buf[1];  
> > > >
> > > > be/le16_to_cpu() will do it for you.  
> > >
> > > I think get_unaligned_le16() will be better here. Otherwise we need to define
> > > __le16 variable and cast around it.
> > >  
> > 
> > I was thinking about be16_to_cpu(*(__be16 *)buf).
> > Given this is local array and you do not do any pointer arithmetic
> > before casting I would be surprised if that caused unaligned access.  
> 
> Explicit casting to bitwise types seems weird to me, that's why get_unaligned looks better.
> And no, there is no unaligned access per se.
>