Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 11 Nov 2018 16:49:28 +0000
Peter Rosin <peda@axentia.se> wrote:

> On 2018-11-11 15:55, Jonathan Cameron wrote:
> > On Tue, 6 Nov 2018 16:37:11 +0000
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> Hi!
> >>
> >> Some comments inline...  
> > A few additions from me.  
> 
> *snip*
> 
> >>> +	data = iio_priv(indio_dev);
> >>> +	spi_set_drvdata(spi, indio_dev);
> >>> +	data->spi = spi;
> >>> +	data->cfg = &mcp41010_cfg[devid];    
> >>
> >> I'm missing a "of_device_get_match_data(dev)" call somewhere in here, see e.g. the
> >> max5481.c file in this directory. Yes, that's missing from the mcp4131 driver as
> >> well, but that's not a valid reason for not doing it here AFAICT...  
> > If you want to use the data elements from the devicetree bindings you'll need
> > to do that.  However, there is an odd path that actually means it will fall back
> > to getting the right thing as you have it here.
> > spi_get_device_id calls spi_match_id which compares the sdev->modalias
> > with the id table names.  modalias has been carefully constructed
> > to be the text after the comma only and as such this works as is.
> > 
> > Perhaps it's neater though to just use the devicetree access functions.  
> 
> Isn't that part about not looking at the vendor-part of the DT-compatible slightly
> fragile? Or will modalias/chip-number clashes be detected by something?
Yes :)

Well actually in this case not so bad.

It will use a match on the of_device_id to find which driver to probe if there is
one. The modalias part is only being abused to then figure out which device we
have within the driver.

I definitely prefer the explicit route.  Just wanted to point out it 'worked'
:)

> 
> Cheers,
> Peter