On Sat, 17 Nov 2018 20:31:30 +0000 Chris Coffey <cmc@xxxxxxxxxxxxx> wrote: > 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"? If we are going to explicitly support he of_device_get_match_data path then we should do the names as well in v3. Just end up with an odd half measure otherwise! Jonathan > > [snip]
