[PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
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 Signed-off-by: Slawomir Stepien --- .../bindings/iio/potentiometer/mcp41xx.txt | 51 ++ Documentation/iio/mcp41xx.txt | 86 +++ drivers/iio/potentiometer/Kconfig | 18 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/mcp41xx.c| 709 + 5 files changed, 865 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt create mode 100644 Documentation/iio/mcp41xx.txt create mode 100644 drivers/iio/potentiometer/mcp41xx.c diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt new file mode 100644 index 000..6031142 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt @@ -0,0 +1,51 @@ +* Microchip MCP414X/416X/424X/426X Digital Potentiometer driver + +The node for this driver must be a child node of a SPI controller, hence +all mandatory properties described in + +Documentation/devicetree/bindings/spi/spi-bus.txt + +must be specified. + +Required properties: + - compatible: Must be one of the following, depending on the + model: + "microchip,mcp4113x-502" + "microchip,mcp4113x-103" + "microchip,mcp4113x-503" + "microchip,mcp4113x-104" + "microchip,mcp4114x-502" + "microchip,mcp4114x-103" + "microchip,mcp4114x-503" + "microchip,mcp4114x-104" + "microchip,mcp4115x-502" + "microchip,mcp4115x-103" + "microchip,mcp4115x-503" + "microchip,mcp4115x-104" + "microchip,mcp4116x-502" + "microchip,mcp4116x-103" + "microchip,mcp4116x-503" + "microchip,mcp4116x-104" + "microchip,mcp4123x-502" + "microchip,mcp4123x-103" + "microchip,mcp4123x-503" + "microchip,mcp4123x-104" + "microchip,mcp4124x-502" + "microchip,mcp4124x-103" + "microchip,mcp4124x-503" + "microchip,mcp4124x-104" + "microchip,mcp4125x-502" + "microchip,mcp4125x-103" + "microchip,mcp4125x-503" + "microchip,mcp4125x-104" + "microchip,mcp4126x-502" + "microchip,mcp4126x-103" + "microchip,mcp4126x-503" + "microchip,mcp4126x-104" + +Example: +mcp41xx: mcp41xx@0 { + compatible = "mcp416x-502"; + reg = <0>; + spi-max-frequency = <50>; +}; diff --git a/Documentation/iio/mcp41xx.txt b/Documentation/iio/mcp41xx.txt new file mode 100644 index 000..57dc889 --- /dev/null +++ b/Documentation/iio/mcp41xx.txt @@ -0,0 +1,86 @@ +Industrial I/O driver for MCP414X/416X/424X/426X Digital POTs +- + +Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059a.pdf + +sysfs interface +--- +N is the wiper number (0 = first wiper, 1 = second wiper) + +File: /sys/bus/iio/devices/../decr_wiperN +Mode: Write +Description: + Write anything to this file to decrease wiper N position by one step. +Example: + echo > decr_wiper0 + + +File: /sys/bus/iio/devices/../incr_wiperN +Mode: Write +Description: + Write anything to this file to increase wiper N position by one step. +Example: + echo > incr_wiper0 + + +File: /sys/bus/iio/devices/../memory_map +Mode: Read/Write +Description: + Read the whole memory or write value to given memory address. + Read outputs two columns. First column is address and second column is + value at that address. + For write use hex values with leading 0x. First hex is address second + hex is the value. +Example: + cat memory_map + echo "0x00 0x80" > memory_map + + +File: /sys/bus/iio/devices/../nv_wiperN +Mode: Read/Write +Description: + Read or write to non volatile memory of wiper N. + Read outputs decimal value that corresponds to wiper position. + For write use decimal value that corresponds to wiper position. +Example: + cat nv_wiper0 + echo "128" > nv_wiper0 + + +File: /sys/bus/iio/devices/../out_resistance_scale +Mode: Read +Description: + Read the resistance scale of one step
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
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 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 plenty of the private API, some of which seems to be debug only? what is really needed to interact with a poti? comments below > Signed-off-by: Slawomir Stepien > --- > .../bindings/iio/potentiometer/mcp41xx.txt | 51 ++ > Documentation/iio/mcp41xx.txt | 86 +++ > drivers/iio/potentiometer/Kconfig | 18 + > drivers/iio/potentiometer/Makefile | 1 + > drivers/iio/potentiometer/mcp41xx.c| 709 > + > 5 files changed, 865 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt > create mode 100644 Documentation/iio/mcp41xx.txt > create mode 100644 drivers/iio/potentiometer/mcp41xx.c > > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt > b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt > new file mode 100644 > index 000..6031142 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41xx.txt > @@ -0,0 +1,51 @@ > +* Microchip MCP414X/416X/424X/426X Digital Potentiometer driver > + > +The node for this driver must be a child node of a SPI controller, hence > +all mandatory properties described in > + > +Documentation/devicetree/bindings/spi/spi-bus.txt > + > +must be specified. > + > +Required properties: > + - compatible: Must be one of the following, depending on the > + model: > + "microchip,mcp4113x-502" > + "microchip,mcp4113x-103" > + "microchip,mcp4113x-503" > + "microchip,mcp4113x-104" > + "microchip,mcp4114x-502" > + "microchip,mcp4114x-103" > + "microchip,mcp4114x-503" > + "microchip,mcp4114x-104" > + "microchip,mcp4115x-502" > + "microchip,mcp4115x-103" > + "microchip,mcp4115x-503" > + "microchip,mcp4115x-104" > + "microchip,mcp4116x-502" > + "microchip,mcp4116x-103" > + "microchip,mcp4116x-503" > + "microchip,mcp4116x-104" > + "microchip,mcp4123x-502" > + "microchip,mcp4123x-103" > + "microchip,mcp4123x-503" > + "microchip,mcp4123x-104" > + "microchip,mcp4124x-502" > + "microchip,mcp4124x-103" > + "microchip,mcp4124x-503" > + "microchip,mcp4124x-104" > + "microchip,mcp4125x-502" > + "microchip,mcp4125x-103" > + "microchip,mcp4125x-503" > + "microchip,mcp4125x-104" > + "microchip,mcp4126x-502" > + "microchip,mcp4126x-103" > + "microchip,mcp4126x-503" > + "microchip,mcp4126x-104" > + > +Example: > +mcp41xx: mcp41xx@0 { > + compatible = "mcp416x-502"; > + reg = <0>; > + spi-max-frequency = <50>; > +}; > diff --git a/Documentation/iio/mcp41xx.txt b/Documentation/iio/mcp41xx.txt > new file mode 100644 > index 000..57dc889 > --- /dev/null > +++ b/Documentation/iio/mcp41xx.txt > @@ -0,0 +1,86 @@ > +Industrial I/O driver for MCP414X/416X/424X/426X Digital POTs > +- > + > +Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22059a.pdf > + > +sysfs interface > +--- > +N is the wiper number (0 = first wiper, 1 = second wiper) > + > +File:/sys/bus/iio/devices/../decr_wiperN > +Mode:Write > +Description: > + Write anything to this file to decrease wiper N position by one step. > +Example: > + echo > decr_wiper0 > + > + > +File:/sys/bus/iio/devices/../incr_wiperN > +Mode:Write > +Description: > + Write anything to this file to increase wiper N position by one step. > +Example: > + echo > incr_wiper0 > + > + > +File:/sys/bus/iio/devices/../memory_map > +Mode:Read/Write > +Description: > + Read the whole memory or write value to given memory address. > + Read outputs two columns. First column is address and second column is > + value at that address. > + For write use hex values with leading 0x. First hex is address second > + hex is the value. > +Example: > + cat memory_map > +
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
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
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 20:28, Daniel Baluta wrote: > On Wed, Mar 16, 2016 at 6:25 PM, Slawomir Stepien wrote: > > 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. > > Filename shouldn't be generic (e.g ending with xx). It should be a > specific chip name > (a good candidate is the first in alphabetical order), because there > could be chips falling > in the same name category but with a different driver. OK got it. Please wait for the version 2. -- Slawomir Stepien
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Mar 16, 2016 19:24, Lars-Peter Clausen wrote: > On 03/16/2016 05:25 PM, Slawomir Stepien wrote: > > On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: > [...] > >> 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 important part about a framework and the associated device drivers > is to expose the features of a device using a standardized interface so > you can write generic applications/libraries and share infrastructure. > If an application requires device specific knowledge to access the > features of a device you may as well write a userspace driver using i2cdev. > > So when you are introducing new ABI it should at least follow the > standard naming scheme. And also try to think whether this is a feature > that is present in other similar devices and come up with a device > independent way to expose this functionality. > > Let's start with the simple stuff, I don't really see the advantage of > having separate inc/dec controls. This can be handled through the > standard raw attribute. If the newly written value is one off from the > previous one use inc/dec otherwise write it directly. And even then it > might make sense to just ignore that and always write the raw value. I've got your point. The version 2 will use only the IIO facilities. Then I will try to build more based on that. > > 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. > > This sounds more like it should maybe be exposed as a standard EEPROM > device. Not quite familiar with this, but will look into that. Once more thank you for comments. -- Slawomir Stepien
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On Wed, Mar 16, 2016 at 6:25 PM, Slawomir Stepien wrote: > 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. Filename shouldn't be generic (e.g ending with xx). It should be a specific chip name (a good candidate is the first in alphabetical order), because there could be chips falling in the same name category but with a different driver. > >> 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X
On 03/16/2016 05:25 PM, Slawomir Stepien wrote: > On Mar 16, 2016 13:30, Peter Meerwald-Stadler wrote: [...] >> 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 important part about a framework and the associated device drivers is to expose the features of a device using a standardized interface so you can write generic applications/libraries and share infrastructure. If an application requires device specific knowledge to access the features of a device you may as well write a userspace driver using i2cdev. So when you are introducing new ABI it should at least follow the standard naming scheme. And also try to think whether this is a feature that is present in other similar devices and come up with a device independent way to expose this functionality. Let's start with the simple stuff, I don't really see the advantage of having separate inc/dec controls. This can be handled through the standard raw attribute. If the newly written value is one off from the previous one use inc/dec otherwise write it directly. And even then it might make sense to just ignore that and always write the raw value. > 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. This sounds more like it should maybe be exposed as a standard EEPROM device.