Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

2016-03-22 Thread Slawomir Stepien 
On Mar 22, 2016 17:10, Joachim Eastwood wrote:
> Hi Slawomir,
> 
> On 22 March 2016 at 16:44, Slawomir Stepien  wrote:
> > The following functionalities are supported:
> >  - write, read from volatile memory
> >
> > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
> >
> > Signed-off-by: Slawomir Stepien 
> > ---
> 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> 
> Give that you use that you have a some OF stuff in your driver you
> should also include . Same goes for .
> I am sure this builds fine without those includes, but you should
> explicitly include stuff that you use.

Oh yes that's true.

> While you're at it you could also put the includes in alphabetic order.

OK

> > +static int mcp4131_read_raw(struct iio_dev *indio_dev,
> > +   struct iio_chan_spec const *chan,
> > +   int *val, int *val2, long mask)
> > +{
> > +   int err;
> > +   struct mcp4131_data *data = iio_priv(indio_dev);
> > +   int address = chan->channel;
> > +
> > +   switch (mask) {
> > +   case IIO_CHAN_INFO_RAW:
> > +   mutex_lock(>lock);
> > +
> > +   data->buf[0] = (address << MCP4131_WIPER_SHIFT) | 
> > MCP4131_READ;
> > +   data->buf[1] = 0;
> > +
> > +   err = mcp4131_read(data->spi, data->buf, 2);
> > +   if (err) {
> > +   mutex_unlock(>lock);
> > +   return err;
> > +   }
> > +
> > +   /* Error, bad address/command combination */
> > +   if (!MCP4131_CMDERR(data->buf))
> > +   return -EIO;
> 
> Missing mutex unlock here.

Oh ;) I'll fix that.

> > +
> > +   *val = MCP4131_RAW(data->buf);
> > +   mutex_unlock(>lock);
> > +
> > +   return IIO_VAL_INT;
> > +
> > +   case IIO_CHAN_INFO_SCALE:
> > +   *val = 1000 * data->cfg->kohms;
> > +   *val2 = data->cfg->max_pos;
> > +   return IIO_VAL_FRACTIONAL;
> > +   }
> > +
> > +   return -EINVAL;
> > +}
> > +
> > +static int mcp4131_write_raw(struct iio_dev *indio_dev,
> > +struct iio_chan_spec const *chan,
> > +int val, int val2, long mask)
> > +{
> > +   int err;
> > +   struct mcp4131_data *data = iio_priv(indio_dev);
> > +   int address = chan->channel << MCP4131_WIPER_SHIFT;
> > +
> > +   switch (mask) {
> > +   case IIO_CHAN_INFO_RAW:
> > +   if (val > data->cfg->max_pos || val < 0)
> > +   return -EINVAL;
> > +   break;
> > +
> > +   default:
> > +   return -EINVAL;
> > +   }
> > +
> > +   mutex_lock(>lock);
> > +
> > +   data->buf[0] = address << MCP4131_WIPER_SHIFT;
> > +   data->buf[0] |= MCP4131_WRITE | (val >> 8);
> > +   data->buf[1] = val & 0xFF; /* 8 bits here */
> > +
> > +   err = spi_write(data->spi, data->buf, 2);
> > +   if (err) {
> > +   mutex_unlock(>lock);
> > +   return err;
> > +   }
> > +   mutex_unlock(>lock);
> > +
> > +   return 0;
> 
> This last part could be written as:
>   err = spi_write(data->spi, data->buf, 2);
>   mutex_unlock(>lock);
> 
>   return err;
 
OK

> Other than the things I noted driver looks good to me.
> 
> 
> regards,
> Joachim Eastwood

-- 
Slawomir Stepien

Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

2016-03-22 Thread Slawomir Stepien 
On Mar 22, 2016 17:10, Joachim Eastwood wrote:
> Hi Slawomir,
> 
> On 22 March 2016 at 16:44, Slawomir Stepien  wrote:
> > The following functionalities are supported:
> >  - write, read from volatile memory
> >
> > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
> >
> > Signed-off-by: Slawomir Stepien 
> > ---
> 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> 
> Give that you use that you have a some OF stuff in your driver you
> should also include . Same goes for .
> I am sure this builds fine without those includes, but you should
> explicitly include stuff that you use.

Oh yes that's true.

> While you're at it you could also put the includes in alphabetic order.

OK

> > +static int mcp4131_read_raw(struct iio_dev *indio_dev,
> > +   struct iio_chan_spec const *chan,
> > +   int *val, int *val2, long mask)
> > +{
> > +   int err;
> > +   struct mcp4131_data *data = iio_priv(indio_dev);
> > +   int address = chan->channel;
> > +
> > +   switch (mask) {
> > +   case IIO_CHAN_INFO_RAW:
> > +   mutex_lock(>lock);
> > +
> > +   data->buf[0] = (address << MCP4131_WIPER_SHIFT) | 
> > MCP4131_READ;
> > +   data->buf[1] = 0;
> > +
> > +   err = mcp4131_read(data->spi, data->buf, 2);
> > +   if (err) {
> > +   mutex_unlock(>lock);
> > +   return err;
> > +   }
> > +
> > +   /* Error, bad address/command combination */
> > +   if (!MCP4131_CMDERR(data->buf))
> > +   return -EIO;
> 
> Missing mutex unlock here.

Oh ;) I'll fix that.

> > +
> > +   *val = MCP4131_RAW(data->buf);
> > +   mutex_unlock(>lock);
> > +
> > +   return IIO_VAL_INT;
> > +
> > +   case IIO_CHAN_INFO_SCALE:
> > +   *val = 1000 * data->cfg->kohms;
> > +   *val2 = data->cfg->max_pos;
> > +   return IIO_VAL_FRACTIONAL;
> > +   }
> > +
> > +   return -EINVAL;
> > +}
> > +
> > +static int mcp4131_write_raw(struct iio_dev *indio_dev,
> > +struct iio_chan_spec const *chan,
> > +int val, int val2, long mask)
> > +{
> > +   int err;
> > +   struct mcp4131_data *data = iio_priv(indio_dev);
> > +   int address = chan->channel << MCP4131_WIPER_SHIFT;
> > +
> > +   switch (mask) {
> > +   case IIO_CHAN_INFO_RAW:
> > +   if (val > data->cfg->max_pos || val < 0)
> > +   return -EINVAL;
> > +   break;
> > +
> > +   default:
> > +   return -EINVAL;
> > +   }
> > +
> > +   mutex_lock(>lock);
> > +
> > +   data->buf[0] = address << MCP4131_WIPER_SHIFT;
> > +   data->buf[0] |= MCP4131_WRITE | (val >> 8);
> > +   data->buf[1] = val & 0xFF; /* 8 bits here */
> > +
> > +   err = spi_write(data->spi, data->buf, 2);
> > +   if (err) {
> > +   mutex_unlock(>lock);
> > +   return err;
> > +   }
> > +   mutex_unlock(>lock);
> > +
> > +   return 0;
> 
> This last part could be written as:
>   err = spi_write(data->spi, data->buf, 2);
>   mutex_unlock(>lock);
> 
>   return err;
 
OK

> Other than the things I noted driver looks good to me.
> 
> 
> regards,
> Joachim Eastwood

-- 
Slawomir Stepien

Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

2016-03-22 Thread Joachim Eastwood 
Hi Slawomir,

On 22 March 2016 at 16:44, Slawomir Stepien  wrote:
> The following functionalities are supported:
>  - write, read from volatile memory
>
> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
>
> Signed-off-by: Slawomir Stepien 
> ---

> +#include 
> +#include 
> +#include 
> +
> +#include 

Give that you use that you have a some OF stuff in your driver you
should also include . Same goes for .
I am sure this builds fine without those includes, but you should
explicitly include stuff that you use.

While you're at it you could also put the includes in alphabetic order.

> +static int mcp4131_read_raw(struct iio_dev *indio_dev,
> +   struct iio_chan_spec const *chan,
> +   int *val, int *val2, long mask)
> +{
> +   int err;
> +   struct mcp4131_data *data = iio_priv(indio_dev);
> +   int address = chan->channel;
> +
> +   switch (mask) {
> +   case IIO_CHAN_INFO_RAW:
> +   mutex_lock(>lock);
> +
> +   data->buf[0] = (address << MCP4131_WIPER_SHIFT) | 
> MCP4131_READ;
> +   data->buf[1] = 0;
> +
> +   err = mcp4131_read(data->spi, data->buf, 2);
> +   if (err) {
> +   mutex_unlock(>lock);
> +   return err;
> +   }
> +
> +   /* Error, bad address/command combination */
> +   if (!MCP4131_CMDERR(data->buf))
> +   return -EIO;

Missing mutex unlock here.

> +
> +   *val = MCP4131_RAW(data->buf);
> +   mutex_unlock(>lock);
> +
> +   return IIO_VAL_INT;
> +
> +   case IIO_CHAN_INFO_SCALE:
> +   *val = 1000 * data->cfg->kohms;
> +   *val2 = data->cfg->max_pos;
> +   return IIO_VAL_FRACTIONAL;
> +   }
> +
> +   return -EINVAL;
> +}
> +
> +static int mcp4131_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> +   int err;
> +   struct mcp4131_data *data = iio_priv(indio_dev);
> +   int address = chan->channel << MCP4131_WIPER_SHIFT;
> +
> +   switch (mask) {
> +   case IIO_CHAN_INFO_RAW:
> +   if (val > data->cfg->max_pos || val < 0)
> +   return -EINVAL;
> +   break;
> +
> +   default:
> +   return -EINVAL;
> +   }
> +
> +   mutex_lock(>lock);
> +
> +   data->buf[0] = address << MCP4131_WIPER_SHIFT;
> +   data->buf[0] |= MCP4131_WRITE | (val >> 8);
> +   data->buf[1] = val & 0xFF; /* 8 bits here */
> +
> +   err = spi_write(data->spi, data->buf, 2);
> +   if (err) {
> +   mutex_unlock(>lock);
> +   return err;
> +   }
> +   mutex_unlock(>lock);
> +
> +   return 0;

This last part could be written as:
  err = spi_write(data->spi, data->buf, 2);
  mutex_unlock(>lock);

  return err;


Other than the things I noted driver looks good to me.


regards,
Joachim Eastwood

Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

2016-03-22 Thread Joachim Eastwood 
Hi Slawomir,

On 22 March 2016 at 16:44, Slawomir Stepien  wrote:
> The following functionalities are supported:
>  - write, read from volatile memory
>
> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
>
> Signed-off-by: Slawomir Stepien 
> ---

> +#include 
> +#include 
> +#include 
> +
> +#include 

Give that you use that you have a some OF stuff in your driver you
should also include . Same goes for .
I am sure this builds fine without those includes, but you should
explicitly include stuff that you use.

While you're at it you could also put the includes in alphabetic order.

> +static int mcp4131_read_raw(struct iio_dev *indio_dev,
> +   struct iio_chan_spec const *chan,
> +   int *val, int *val2, long mask)
> +{
> +   int err;
> +   struct mcp4131_data *data = iio_priv(indio_dev);
> +   int address = chan->channel;
> +
> +   switch (mask) {
> +   case IIO_CHAN_INFO_RAW:
> +   mutex_lock(>lock);
> +
> +   data->buf[0] = (address << MCP4131_WIPER_SHIFT) | 
> MCP4131_READ;
> +   data->buf[1] = 0;
> +
> +   err = mcp4131_read(data->spi, data->buf, 2);
> +   if (err) {
> +   mutex_unlock(>lock);
> +   return err;
> +   }
> +
> +   /* Error, bad address/command combination */
> +   if (!MCP4131_CMDERR(data->buf))
> +   return -EIO;

Missing mutex unlock here.

> +
> +   *val = MCP4131_RAW(data->buf);
> +   mutex_unlock(>lock);
> +
> +   return IIO_VAL_INT;
> +
> +   case IIO_CHAN_INFO_SCALE:
> +   *val = 1000 * data->cfg->kohms;
> +   *val2 = data->cfg->max_pos;
> +   return IIO_VAL_FRACTIONAL;
> +   }
> +
> +   return -EINVAL;
> +}
> +
> +static int mcp4131_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> +   int err;
> +   struct mcp4131_data *data = iio_priv(indio_dev);
> +   int address = chan->channel << MCP4131_WIPER_SHIFT;
> +
> +   switch (mask) {
> +   case IIO_CHAN_INFO_RAW:
> +   if (val > data->cfg->max_pos || val < 0)
> +   return -EINVAL;
> +   break;
> +
> +   default:
> +   return -EINVAL;
> +   }
> +
> +   mutex_lock(>lock);
> +
> +   data->buf[0] = address << MCP4131_WIPER_SHIFT;
> +   data->buf[0] |= MCP4131_WRITE | (val >> 8);
> +   data->buf[1] = val & 0xFF; /* 8 bits here */
> +
> +   err = spi_write(data->spi, data->buf, 2);
> +   if (err) {
> +   mutex_unlock(>lock);
> +   return err;
> +   }
> +   mutex_unlock(>lock);
> +
> +   return 0;

This last part could be written as:
  err = spi_write(data->spi, data->buf, 2);
  mutex_unlock(>lock);

  return err;


Other than the things I noted driver looks good to me.


regards,
Joachim Eastwood

[PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

2016-03-22 Thread Slawomir Stepien 
The following functionalities are supported:
 - write, read from volatile memory

Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf

Signed-off-by: Slawomir Stepien 
---
Changes since v3:
- Added 'potentiometer' to subject
- Replaced mcp4131_exec with spi_write and mcp4131_read
- Narrowed mutex locking and unlocking places

Changes since v2:
- Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro
- Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned
- Changed mutex locking position
- Replaced the devid with pointer to model configuration
- Removed positive ("Registered" & "Unregistered") dev_info prints
- Removed the mcp4131_remove callback

Changes since v1:
- One line summary changed
- Fixed module name and OF compatible
- Based code on Peter Rosin's code from mcp4531.c
- No additional sysfs attributes
- Deleted not used devid struct memeber
- Changed mcp4131_exec function arguments:
 - write value is now u16
 - no need to pass return buffer array - rx array from mcp4131_data is used
- Added missing new line characters to dev_info
---
 .../bindings/iio/potentiometer/mcp4131.txt |  84 
 drivers/iio/potentiometer/Kconfig  |  18 +
 drivers/iio/potentiometer/Makefile |   1 +
 drivers/iio/potentiometer/mcp4131.c| 492 +
 4 files changed, 595 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
 create mode 100644 drivers/iio/potentiometer/mcp4131.c

diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt 
b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
new file mode 100644
index 000..3ccba16
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
@@ -0,0 +1,84 @@
+* Microchip MCP413X/414X/415X/416X/423X/424X/425X/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,mcp4131-502"
+   "microchip,mcp4131-103"
+   "microchip,mcp4131-503"
+   "microchip,mcp4131-104"
+   "microchip,mcp4132-502"
+   "microchip,mcp4132-103"
+   "microchip,mcp4132-503"
+   "microchip,mcp4132-104"
+   "microchip,mcp4141-502"
+   "microchip,mcp4141-103"
+   "microchip,mcp4141-503"
+   "microchip,mcp4141-104"
+   "microchip,mcp4142-502"
+   "microchip,mcp4142-103"
+   "microchip,mcp4142-503"
+   "microchip,mcp4142-104"
+   "microchip,mcp4151-502"
+   "microchip,mcp4151-103"
+   "microchip,mcp4151-503"
+   "microchip,mcp4151-104"
+   "microchip,mcp4152-502"
+   "microchip,mcp4152-103"
+   "microchip,mcp4152-503"
+   "microchip,mcp4152-104"
+   "microchip,mcp4161-502"
+   "microchip,mcp4161-103"
+   "microchip,mcp4161-503"
+   "microchip,mcp4161-104"
+   "microchip,mcp4162-502"
+   "microchip,mcp4162-103"
+   "microchip,mcp4162-503"
+   "microchip,mcp4162-104"
+   "microchip,mcp4231-502"
+   "microchip,mcp4231-103"
+   "microchip,mcp4231-503"
+   "microchip,mcp4231-104"
+   "microchip,mcp4232-502"
+   "microchip,mcp4232-103"
+   "microchip,mcp4232-503"
+   "microchip,mcp4232-104"
+   "microchip,mcp4241-502"
+   "microchip,mcp4241-103"
+   "microchip,mcp4241-503"
+   "microchip,mcp4241-104"
+   "microchip,mcp4242-502"
+   "microchip,mcp4242-103"
+   "microchip,mcp4242-503"
+   "microchip,mcp4242-104"
+   "microchip,mcp4251-502"
+   "microchip,mcp4251-103"
+   "microchip,mcp4251-503"
+   "microchip,mcp4251-104"
+   "microchip,mcp4252-502"
+   "microchip,mcp4252-103"
+   "microchip,mcp4252-503"
+   "microchip,mcp4252-104"
+

[PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

2016-03-22 Thread Slawomir Stepien 
The following functionalities are supported:
 - write, read from volatile memory

Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf

Signed-off-by: Slawomir Stepien 
---
Changes since v3:
- Added 'potentiometer' to subject
- Replaced mcp4131_exec with spi_write and mcp4131_read
- Narrowed mutex locking and unlocking places

Changes since v2:
- Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro
- Replaced the rx and tx SPI buffers with one buf that is cacheline_aligned
- Changed mutex locking position
- Replaced the devid with pointer to model configuration
- Removed positive ("Registered" & "Unregistered") dev_info prints
- Removed the mcp4131_remove callback

Changes since v1:
- One line summary changed
- Fixed module name and OF compatible
- Based code on Peter Rosin's code from mcp4531.c
- No additional sysfs attributes
- Deleted not used devid struct memeber
- Changed mcp4131_exec function arguments:
 - write value is now u16
 - no need to pass return buffer array - rx array from mcp4131_data is used
- Added missing new line characters to dev_info
---
 .../bindings/iio/potentiometer/mcp4131.txt |  84 
 drivers/iio/potentiometer/Kconfig  |  18 +
 drivers/iio/potentiometer/Makefile |   1 +
 drivers/iio/potentiometer/mcp4131.c| 492 +
 4 files changed, 595 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
 create mode 100644 drivers/iio/potentiometer/mcp4131.c

diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt 
b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
new file mode 100644
index 000..3ccba16
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
@@ -0,0 +1,84 @@
+* Microchip MCP413X/414X/415X/416X/423X/424X/425X/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,mcp4131-502"
+   "microchip,mcp4131-103"
+   "microchip,mcp4131-503"
+   "microchip,mcp4131-104"
+   "microchip,mcp4132-502"
+   "microchip,mcp4132-103"
+   "microchip,mcp4132-503"
+   "microchip,mcp4132-104"
+   "microchip,mcp4141-502"
+   "microchip,mcp4141-103"
+   "microchip,mcp4141-503"
+   "microchip,mcp4141-104"
+   "microchip,mcp4142-502"
+   "microchip,mcp4142-103"
+   "microchip,mcp4142-503"
+   "microchip,mcp4142-104"
+   "microchip,mcp4151-502"
+   "microchip,mcp4151-103"
+   "microchip,mcp4151-503"
+   "microchip,mcp4151-104"
+   "microchip,mcp4152-502"
+   "microchip,mcp4152-103"
+   "microchip,mcp4152-503"
+   "microchip,mcp4152-104"
+   "microchip,mcp4161-502"
+   "microchip,mcp4161-103"
+   "microchip,mcp4161-503"
+   "microchip,mcp4161-104"
+   "microchip,mcp4162-502"
+   "microchip,mcp4162-103"
+   "microchip,mcp4162-503"
+   "microchip,mcp4162-104"
+   "microchip,mcp4231-502"
+   "microchip,mcp4231-103"
+   "microchip,mcp4231-503"
+   "microchip,mcp4231-104"
+   "microchip,mcp4232-502"
+   "microchip,mcp4232-103"
+   "microchip,mcp4232-503"
+   "microchip,mcp4232-104"
+   "microchip,mcp4241-502"
+   "microchip,mcp4241-103"
+   "microchip,mcp4241-503"
+   "microchip,mcp4241-104"
+   "microchip,mcp4242-502"
+   "microchip,mcp4242-103"
+   "microchip,mcp4242-503"
+   "microchip,mcp4242-104"
+   "microchip,mcp4251-502"
+   "microchip,mcp4251-103"
+   "microchip,mcp4251-503"
+   "microchip,mcp4251-104"
+   "microchip,mcp4252-502"
+   "microchip,mcp4252-103"
+   "microchip,mcp4252-503"
+   "microchip,mcp4252-104"
+   "microchip,mcp4261-502"
+