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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you for the review! A few comments inline.

On Tue, Nov 06, 2018 at 04:37:11PM +0000, Peter Rosin wrote:
> Hi!
> 
> Some comments inline...
> 
> On 2018-11-06 12:31, Chris Coffey wrote:
> > This patch adds driver support for the Microchip MCP41xxx/42xxx family
> > of digital potentiometers:
> > 

[snip]

> > +
> > +	data->buf[0] = MCP41010_WIPER_ENABLE << channel;
> > +	data->buf[0] |= MCP41010_WRITE;
> 
> Will this not clobber the other channel for mcp42xxx chips???
> 

I see you followed up on this in another message, so I'll respond there.

[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;
> > +	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 = &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...
> 
> Cheers,
> Peter
> 

Ah, interesting; I was unaware of of_device_get_match_data(). I'll add
that for v2.

Thanks again,
Chris




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux