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

[Chris Coffey <cmc@babblebit.net>]

On Fri, Nov 16, 2018 at 06:10:43PM +0000, Jonathan Cameron wrote:
> On Wed, 14 Nov 2018 11:30:15 +0100
> Slawomir Stepien <sst@poczta.fm> wrote:
> 
> > On lis 14, 2018 09:52, Chris Coffey wrote:
> > > This patch adds driver support for the Microchip MCP41xxx/42xxx family
> > > of digital potentiometers:
> > > 
> > > DEVICE      Wipers  Positions   Resistance (kOhm)
> > > MCP41010    1       256         10
> > > MCP41050    1       256         50
> > > MCP41100    1       256         100
> > > MCP42010    2       256         10
> > > MCP42050    2       256         50
> > > MCP42100    2       256         100
> > > 
> > > Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf  
> > 
> > Hi
> > 
> > Some hints inline.
> A few minor comments from me.
> 
> Thanks,
> 
> Jonathan
> 

Thank you for the review; I have one question below.

Thanks,
Chris

[snip]

> > > +static int mcp41010_probe(struct spi_device *spi)
> > > +{
> > > +	int err;
> > > +	struct device *dev = &spi->dev;
> > > +	unsigned long devid = spi_get_device_id(spi)->driver_data;  
> > 
> > I guess the calculation of devid value can now be done only when
> > of_device_get_match_data() did not return config.
> > 
> > > +	struct mcp41010_data *data;
> > > +	struct iio_dev *indio_dev;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	data = iio_priv(indio_dev);
> > > +	spi_set_drvdata(spi, indio_dev);
> > > +	data->spi = spi;
> > > +	data->cfg = of_device_get_match_data(&spi->dev);
> > > +	if (!data->cfg)
> > > +		data->cfg = &mcp41010_cfg[devid];
> > > +
> > > +	mutex_init(&data->lock);
> > > +
> > > +	indio_dev->dev.parent = dev;
> > > +	indio_dev->info = &mcp41010_info;
> > > +	indio_dev->channels = mcp41010_channels;
> > > +	indio_dev->num_channels = data->cfg->wipers;
> > > +	indio_dev->name = spi_get_device_id(spi)->name;
> 
> It is a bit odd to use the of match for the data, but
> then get the name always from the spi_device_id table.
> We probably need a separate source for the names such
> as in the config structure.
> 

I see what you mean. Is this something you'd like me to do for v3 (add
the names to the config struct), or were you thinking more in the
abstract, "this is something we should consider doing in the future"?

[snip]