On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > On Wed, 16 Mar 2016, Slawomir Stepien wrote: > > > The following functionalities are supported: > > - write, read from volatile and non volatile memory > > - increase and decrease commands > > - read from status register > > - write and read to tcon register > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059b.pdf Thank you for all your comments. > the driver naming is a mess: the subject says MCP414X, the file name is > mcp41xx, the DT compatible string says mcp4113x -- this does not match OK. I will change that to mcp414x in version 2. > plenty of the private API, some of which seems to be debug only? > what is really needed to interact with a poti? I wanted to export both the non volatile and volatile memory addresses for wiper position access. That is bare minimum for the poti to operate. But I also wanted to export additional features of this chip. That is way there is increase and decrease API, and STATUS and TCON register access. The memory_map API is a way to access all the not used by chip memory addresses. This API I think could be deleted. But I still think that some people might find it useful. > comments below > > > +File: /sys/bus/iio/devices/../out_resistanceN_raw > > this is already described in sysfs-bus-iio Should I leave it and add reference to sysfs-bus-iio or delete it completely? > > +struct mcp41xx_cfg { > > + unsigned long int devid; > > devid is not set/used That's true. Will fix it in v2. > > +static int mcp41xx_exec(struct mcp41xx_data *data, > > + u8 addr, u8 cmd, > > + int *trans, size_t n) > > should trans really be int, not u8? There is a need for 9 bit value write/read. I used int just for my convenience. However there is one piece of code missing now I see. I should check the value of tans[0] to see if it's > 256 or lower then 0. Will add it in v2. Using u8 will force additional bit operations. I could try using u16 to still have the option of parsing user input as 9 bit value (eg. 256 position). > > +{ > > + int err; > > + struct spi_device *spi = data->spi; > > + > > + /* Two bytes are needed for the response */ > > + if (n != 2) > > + return -EINVAL; > > why pass n if it is always 2? Will fix in v2. > > + err = devm_iio_device_register(&spi->dev, indio_dev); > > + if (err) { > > + dev_info(&spi->dev, "Unable to register %s", indio_dev->name); > > \n missing > > > + return err; > > + } > > + > > + dev_info(&spi->dev, "Registered %s", indio_dev->name); > > \n > > > + > > + return 0; > > +} > > + > > +static int mcp41xx_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct mcp41xx_data *data = iio_priv(indio_dev); > > + > > + mutex_destroy(&data->lock); > > + devm_iio_device_unregister(&spi->dev, indio_dev); > > + kfree(mcp41xx_attributes); > > + > > + dev_info(&spi->dev, "Unregistered %s", indio_dev->name); Also \n is missing here. Will fix in v2. -- Slawomir Stepien -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
