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@xxxxxxxxx> 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]
