[PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X

2016-03-19 Thread Slawomir Stepien 
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

[PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X

2016-03-19 Thread Slawomir Stepien 
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

Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X

2016-03-19 Thread Peter Meerwald-Stadler 
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

Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X

2016-03-19 Thread Peter Meerwald-Stadler 
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

2016-03-19 Thread Slawomir Stepien 
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(>dev, indio_dev);
> > +   if (err) {
> > +   dev_info(>dev, "Unable to register %s", indio_dev->name);
> 
> \n missing
> 
> > +   return err;
> > +   }
> > +
> > +   dev_info(>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(>lock);
> > +   devm_iio_device_unregister(>dev, indio_dev);
> > +   kfree(mcp41xx_attributes);
> > +
> > +   dev_info(>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

2016-03-19 Thread Slawomir Stepien 
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(>dev, indio_dev);
> > +   if (err) {
> > +   dev_info(>dev, "Unable to register %s", indio_dev->name);
> 
> \n missing
> 
> > +   return err;
> > +   }
> > +
> > +   dev_info(>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(>lock);
> > +   devm_iio_device_unregister(>dev, indio_dev);
> > +   kfree(mcp41xx_attributes);
> > +
> > +   dev_info(>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

2016-03-19 Thread Slawomir Stepien 
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

2016-03-19 Thread Slawomir Stepien 
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

2016-03-19 Thread Slawomir Stepien 
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

2016-03-19 Thread Slawomir Stepien 
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

2016-03-19 Thread Daniel Baluta 
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(>dev, indio_dev);
>> > +   if (err) {
>> > +   dev_info(>dev, "Unable to register %s", indio_dev->name);
>>
>> \n missing
>>
>> > +   return err;
>> > +   }
>> > +
>> > +   dev_info(>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(>lock);
>> > +   devm_iio_device_unregister(>dev, indio_dev);
>> > +   kfree(mcp41xx_attributes);
>> > +
>> > +   dev_info(>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

2016-03-19 Thread Daniel Baluta 
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(>dev, indio_dev);
>> > +   if (err) {
>> > +   dev_info(>dev, "Unable to register %s", indio_dev->name);
>>
>> \n missing
>>
>> > +   return err;
>> > +   }
>> > +
>> > +   dev_info(>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(>lock);
>> > +   devm_iio_device_unregister(>dev, indio_dev);
>> > +   kfree(mcp41xx_attributes);
>> > +
>> > +   dev_info(>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

2016-03-18 Thread Lars-Peter Clausen 
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.

Re: [PATCH] iio: add driver for Microchip MCP414X/416X/424X/426X

2016-03-18 Thread Lars-Peter Clausen 
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.