Re: [PATCH] Staging: iio: adt7316: Add regmap support
...
> > > diff --git a/drivers/staging/iio/addac/adt7316.h
> > > b/drivers/staging/iio/addac/adt7316.h
> > > index fd7c5c92b599..2c72cf3f71cd 100644
> > > --- a/drivers/staging/iio/addac/adt7316.h
> > > +++ b/drivers/staging/iio/addac/adt7316.h
> > > @@ -11,16 +11,13 @@
> > >
> > > #include
> > > #include
> > > +#include
> > >
> > > #define ADT7316_REG_MAX_ADDR 0x3F
> > >
> > > -struct adt7316_bus {
> > > - void *client;
> > > - int irq;
> > > - int (*read)(void *client, u8 reg, u8 *data);
> > > - int (*write)(void *client, u8 reg, u8 val);
> > > - int (*multi_read)(void *client, u8 first_reg, u8 count, u8 *data);
> > > - int (*multi_write)(void *client, u8 first_reg, u8 count, u8 *data);
> > > +static const struct regmap_config adt7316_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 10,
>
> I wonder if val_bits should be 8. The driver can read and write 8, 10,
> or 12 bit values. In the 10 and 12 bit cases, the driver currently
> (including with this patch) does two separate reads or writes and expects
> an 8 bit result from each. It then parses these two values to come up with
> the 10 or 12 bit value. I don't think the logic for this calculation is
> known to regmap, so with the current form of the patch, I think val_bits
> should be 8. Maybe there is a better way to do it though?
Good point. I would assume they should be 8 as well.
>
> I would have tested this but I couldn't get the patch to apply. Shreeya,
> I think if you rebase against iio/testing that might take care of it. I
> can then do some testing with v2.
>
Great.
Jonathan
> Jeremy
Re: [PATCH] Staging: iio: adt7316: Add regmap support
On Sat, Jan 05, 2019 at 05:20:37PM +, Jonathan Cameron wrote:
> +CC Jeremy who is also working with this device.
>
> On Sun, 23 Dec 2018 19:32:24 +0530
> Shreeya Patel wrote:
>
> > Both i2c and spi drivers have functions for reading and writing
> > to/from registers. Remove this redundant and common code by using
> > regmap API.
> > Also remove multi_read and multi_write functions from i2c
> > and spi driver as they are not being used anywhere.
> >
> > Signed-off-by: Shreeya Patel
>
> I would have preferred an initial patch that removed the multi_read
> and multi_write first as that would (I assume) be easily separated
> from the 'major' change of moving to regmap. I also suggest
> another section to pull out to a precursor patch below.
>
> The result looks fine to me.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/staging/iio/addac/adt7316-i2c.c | 101 ++--
> > drivers/staging/iio/addac/adt7316-spi.c | 94 +++
> > drivers/staging/iio/addac/adt7316.c | 147
> > drivers/staging/iio/addac/adt7316.h | 15 +--
> > 4 files changed, 103 insertions(+), 254 deletions(-)
> >
> > diff --git a/drivers/staging/iio/addac/adt7316-i2c.c
> > b/drivers/staging/iio/addac/adt7316-i2c.c
> > index ac91163656b5..435b65845174 100644
> > --- a/drivers/staging/iio/addac/adt7316-i2c.c
> > +++ b/drivers/staging/iio/addac/adt7316-i2c.c
> > @@ -12,105 +12,28 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "adt7316.h"
> >
> > -/*
> > - * adt7316 register access by I2C
> > - */
> > -static int adt7316_i2c_read(void *client, u8 reg, u8 *data)
> > -{
> > - struct i2c_client *cl = client;
> > - int ret;
> > -
> > - ret = i2c_smbus_write_byte(cl, reg);
> > - if (ret < 0) {
> > - dev_err(>dev, "I2C fail to select reg\n");
> > - return ret;
> > - }
> > -
> > - ret = i2c_smbus_read_byte(client);
> > -
> > - if (!ret)
> > - return -EIO;
> > -
> > - if (ret < 0) {
> > - dev_err(>dev, "I2C read error\n");
> > - return ret;
> > - }
> > -
> > - *data = ret;
> > -
> > - return 0;
> > -}
> > -
> > -static int adt7316_i2c_write(void *client, u8 reg, u8 data)
> > -{
> > - struct i2c_client *cl = client;
> > - int ret = 0;
> > -
> > - ret = i2c_smbus_write_byte_data(cl, reg, data);
> > - if (ret < 0)
> > - dev_err(>dev, "I2C write error\n");
> > -
> > - return ret;
> > -}
> > -
> > -static int adt7316_i2c_multi_read(void *client, u8 reg, u8 count, u8 *data)
> > -{
> > - struct i2c_client *cl = client;
> > - int i, ret = 0;
> > -
> > - if (count > ADT7316_REG_MAX_ADDR)
> > - count = ADT7316_REG_MAX_ADDR;
> > -
> > - for (i = 0; i < count; i++) {
> > - ret = adt7316_i2c_read(cl, reg, [i]);
> > - if (ret < 0) {
> > - dev_err(>dev, "I2C multi read error\n");
> > - return ret;
> > - }
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -static int adt7316_i2c_multi_write(void *client, u8 reg, u8 count, u8
> > *data)
> > -{
> > - struct i2c_client *cl = client;
> > - int i, ret = 0;
> > -
> > - if (count > ADT7316_REG_MAX_ADDR)
> > - count = ADT7316_REG_MAX_ADDR;
> > -
> > - for (i = 0; i < count; i++) {
> > - ret = adt7316_i2c_write(cl, reg, data[i]);
> > - if (ret < 0) {
> > - dev_err(>dev, "I2C multi write error\n");
> > - return ret;
> > - }
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * device probe and remove
> > */
> > -
> > static int adt7316_i2c_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > - struct adt7316_bus bus = {
> > - .client = client,
> > - .irq = client->irq,
> > - .read = adt7316_i2c_read,
> > - .write = adt7316_i2c_write,
> > - .multi_read = adt7316_i2c_multi_read,
> > - .multi_write = adt7316_i2c_multi_write,
> > - };
> > + struct regmap *regmap;
> > +
> > + regmap = devm_regmap_init_i2c(client, _regmap_config);
> > +
> > + if (IS_ERR(regmap)) {
> > + dev_err(>dev, "Error initializing i2c regmap: %ld\n",
> > + PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> >
> > - return adt7316_probe(>dev, , id->name);
> > + return adt7316_probe(>dev, regmap, id ? id->name : NULL,
> > +client->irq);
> > }
> >
> > static const struct i2c_device_id adt7316_i2c_id[] = {
> > diff --git a/drivers/staging/iio/addac/adt7316-spi.c
> > b/drivers/staging/iio/addac/adt7316-spi.c
> > index e75827e326a6..9e3decb5cb77 100644
> > --- a/drivers/staging/iio/addac/adt7316-spi.c
> > +++ b/drivers/staging/iio/addac/adt7316-spi.c
> > @@ -11,79 +11,12 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "adt7316.h"
> >
> > #define
Re: [PATCH] Staging: iio: adt7316: Add regmap support
+CC Jeremy who is also working with this device.
On Sun, 23 Dec 2018 19:32:24 +0530
Shreeya Patel wrote:
> Both i2c and spi drivers have functions for reading and writing
> to/from registers. Remove this redundant and common code by using
> regmap API.
> Also remove multi_read and multi_write functions from i2c
> and spi driver as they are not being used anywhere.
>
> Signed-off-by: Shreeya Patel
I would have preferred an initial patch that removed the multi_read
and multi_write first as that would (I assume) be easily separated
from the 'major' change of moving to regmap. I also suggest
another section to pull out to a precursor patch below.
The result looks fine to me.
Thanks,
Jonathan
> ---
> drivers/staging/iio/addac/adt7316-i2c.c | 101 ++--
> drivers/staging/iio/addac/adt7316-spi.c | 94 +++
> drivers/staging/iio/addac/adt7316.c | 147
> drivers/staging/iio/addac/adt7316.h | 15 +--
> 4 files changed, 103 insertions(+), 254 deletions(-)
>
> diff --git a/drivers/staging/iio/addac/adt7316-i2c.c
> b/drivers/staging/iio/addac/adt7316-i2c.c
> index ac91163656b5..435b65845174 100644
> --- a/drivers/staging/iio/addac/adt7316-i2c.c
> +++ b/drivers/staging/iio/addac/adt7316-i2c.c
> @@ -12,105 +12,28 @@
> #include
> #include
> #include
> +#include
>
> #include "adt7316.h"
>
> -/*
> - * adt7316 register access by I2C
> - */
> -static int adt7316_i2c_read(void *client, u8 reg, u8 *data)
> -{
> - struct i2c_client *cl = client;
> - int ret;
> -
> - ret = i2c_smbus_write_byte(cl, reg);
> - if (ret < 0) {
> - dev_err(>dev, "I2C fail to select reg\n");
> - return ret;
> - }
> -
> - ret = i2c_smbus_read_byte(client);
> -
> - if (!ret)
> - return -EIO;
> -
> - if (ret < 0) {
> - dev_err(>dev, "I2C read error\n");
> - return ret;
> - }
> -
> - *data = ret;
> -
> - return 0;
> -}
> -
> -static int adt7316_i2c_write(void *client, u8 reg, u8 data)
> -{
> - struct i2c_client *cl = client;
> - int ret = 0;
> -
> - ret = i2c_smbus_write_byte_data(cl, reg, data);
> - if (ret < 0)
> - dev_err(>dev, "I2C write error\n");
> -
> - return ret;
> -}
> -
> -static int adt7316_i2c_multi_read(void *client, u8 reg, u8 count, u8 *data)
> -{
> - struct i2c_client *cl = client;
> - int i, ret = 0;
> -
> - if (count > ADT7316_REG_MAX_ADDR)
> - count = ADT7316_REG_MAX_ADDR;
> -
> - for (i = 0; i < count; i++) {
> - ret = adt7316_i2c_read(cl, reg, [i]);
> - if (ret < 0) {
> - dev_err(>dev, "I2C multi read error\n");
> - return ret;
> - }
> - }
> -
> - return 0;
> -}
> -
> -static int adt7316_i2c_multi_write(void *client, u8 reg, u8 count, u8 *data)
> -{
> - struct i2c_client *cl = client;
> - int i, ret = 0;
> -
> - if (count > ADT7316_REG_MAX_ADDR)
> - count = ADT7316_REG_MAX_ADDR;
> -
> - for (i = 0; i < count; i++) {
> - ret = adt7316_i2c_write(cl, reg, data[i]);
> - if (ret < 0) {
> - dev_err(>dev, "I2C multi write error\n");
> - return ret;
> - }
> - }
> -
> - return 0;
> -}
> -
> /*
> * device probe and remove
> */
> -
> static int adt7316_i2c_probe(struct i2c_client *client,
>const struct i2c_device_id *id)
> {
> - struct adt7316_bus bus = {
> - .client = client,
> - .irq = client->irq,
> - .read = adt7316_i2c_read,
> - .write = adt7316_i2c_write,
> - .multi_read = adt7316_i2c_multi_read,
> - .multi_write = adt7316_i2c_multi_write,
> - };
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_i2c(client, _regmap_config);
> +
> + if (IS_ERR(regmap)) {
> + dev_err(>dev, "Error initializing i2c regmap: %ld\n",
> + PTR_ERR(regmap));
> + return PTR_ERR(regmap);
> + }
>
> - return adt7316_probe(>dev, , id->name);
> + return adt7316_probe(>dev, regmap, id ? id->name : NULL,
> + client->irq);
> }
>
> static const struct i2c_device_id adt7316_i2c_id[] = {
> diff --git a/drivers/staging/iio/addac/adt7316-spi.c
> b/drivers/staging/iio/addac/adt7316-spi.c
> index e75827e326a6..9e3decb5cb77 100644
> --- a/drivers/staging/iio/addac/adt7316-spi.c
> +++ b/drivers/staging/iio/addac/adt7316-spi.c
> @@ -11,79 +11,12 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "adt7316.h"
>
> #define ADT7316_SPI_MAX_FREQ_HZ 500
> -#define ADT7316_SPI_CMD_READ 0x91
> -#define ADT7316_SPI_CMD_WRITE0x90
> -
> -/*
> - * adt7316 register access by SPI
> - */
> -
> -static int adt7316_spi_multi_read(void *client, u8 reg, u8 count, u8 *data)
[PATCH] Staging: iio: adt7316: Add regmap support
Both i2c and spi drivers have functions for reading and writing
to/from registers. Remove this redundant and common code by using
regmap API.
Also remove multi_read and multi_write functions from i2c
and spi driver as they are not being used anywhere.
Signed-off-by: Shreeya Patel
---
drivers/staging/iio/addac/adt7316-i2c.c | 101 ++--
drivers/staging/iio/addac/adt7316-spi.c | 94 +++
drivers/staging/iio/addac/adt7316.c | 147
drivers/staging/iio/addac/adt7316.h | 15 +--
4 files changed, 103 insertions(+), 254 deletions(-)
diff --git a/drivers/staging/iio/addac/adt7316-i2c.c
b/drivers/staging/iio/addac/adt7316-i2c.c
index ac91163656b5..435b65845174 100644
--- a/drivers/staging/iio/addac/adt7316-i2c.c
+++ b/drivers/staging/iio/addac/adt7316-i2c.c
@@ -12,105 +12,28 @@
#include
#include
#include
+#include
#include "adt7316.h"
-/*
- * adt7316 register access by I2C
- */
-static int adt7316_i2c_read(void *client, u8 reg, u8 *data)
-{
- struct i2c_client *cl = client;
- int ret;
-
- ret = i2c_smbus_write_byte(cl, reg);
- if (ret < 0) {
- dev_err(>dev, "I2C fail to select reg\n");
- return ret;
- }
-
- ret = i2c_smbus_read_byte(client);
-
- if (!ret)
- return -EIO;
-
- if (ret < 0) {
- dev_err(>dev, "I2C read error\n");
- return ret;
- }
-
- *data = ret;
-
- return 0;
-}
-
-static int adt7316_i2c_write(void *client, u8 reg, u8 data)
-{
- struct i2c_client *cl = client;
- int ret = 0;
-
- ret = i2c_smbus_write_byte_data(cl, reg, data);
- if (ret < 0)
- dev_err(>dev, "I2C write error\n");
-
- return ret;
-}
-
-static int adt7316_i2c_multi_read(void *client, u8 reg, u8 count, u8 *data)
-{
- struct i2c_client *cl = client;
- int i, ret = 0;
-
- if (count > ADT7316_REG_MAX_ADDR)
- count = ADT7316_REG_MAX_ADDR;
-
- for (i = 0; i < count; i++) {
- ret = adt7316_i2c_read(cl, reg, [i]);
- if (ret < 0) {
- dev_err(>dev, "I2C multi read error\n");
- return ret;
- }
- }
-
- return 0;
-}
-
-static int adt7316_i2c_multi_write(void *client, u8 reg, u8 count, u8 *data)
-{
- struct i2c_client *cl = client;
- int i, ret = 0;
-
- if (count > ADT7316_REG_MAX_ADDR)
- count = ADT7316_REG_MAX_ADDR;
-
- for (i = 0; i < count; i++) {
- ret = adt7316_i2c_write(cl, reg, data[i]);
- if (ret < 0) {
- dev_err(>dev, "I2C multi write error\n");
- return ret;
- }
- }
-
- return 0;
-}
-
/*
* device probe and remove
*/
-
static int adt7316_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
- struct adt7316_bus bus = {
- .client = client,
- .irq = client->irq,
- .read = adt7316_i2c_read,
- .write = adt7316_i2c_write,
- .multi_read = adt7316_i2c_multi_read,
- .multi_write = adt7316_i2c_multi_write,
- };
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, _regmap_config);
+
+ if (IS_ERR(regmap)) {
+ dev_err(>dev, "Error initializing i2c regmap: %ld\n",
+ PTR_ERR(regmap));
+ return PTR_ERR(regmap);
+ }
- return adt7316_probe(>dev, , id->name);
+ return adt7316_probe(>dev, regmap, id ? id->name : NULL,
+client->irq);
}
static const struct i2c_device_id adt7316_i2c_id[] = {
diff --git a/drivers/staging/iio/addac/adt7316-spi.c
b/drivers/staging/iio/addac/adt7316-spi.c
index e75827e326a6..9e3decb5cb77 100644
--- a/drivers/staging/iio/addac/adt7316-spi.c
+++ b/drivers/staging/iio/addac/adt7316-spi.c
@@ -11,79 +11,12 @@
#include
#include
#include
+#include
#include
#include "adt7316.h"
#define ADT7316_SPI_MAX_FREQ_HZ500
-#define ADT7316_SPI_CMD_READ 0x91
-#define ADT7316_SPI_CMD_WRITE 0x90
-
-/*
- * adt7316 register access by SPI
- */
-
-static int adt7316_spi_multi_read(void *client, u8 reg, u8 count, u8 *data)
-{
- struct spi_device *spi_dev = client;
- u8 cmd[2];
- int ret = 0;
-
- if (count > ADT7316_REG_MAX_ADDR)
- count = ADT7316_REG_MAX_ADDR;
-
- cmd[0] = ADT7316_SPI_CMD_WRITE;
- cmd[1] = reg;
-
- ret = spi_write(spi_dev, cmd, 2);
- if (ret < 0) {
- dev_err(_dev->dev, "SPI fail to select reg\n");
- return ret;
- }
-
- cmd[0] = ADT7316_SPI_CMD_READ;
-
- ret = spi_write_then_read(spi_dev, cmd, 1, data, count);
- if (ret < 0) {
- dev_err(_dev->dev, "SPI read data error\n");
