Re: [PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:44:27PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Convert the driver to use regmap instead of I2C-specific functions. This
> > is done in preparation for splitting this driver into core and
> > I2C-specific code as well as introduction of SPI driver.
> >
> > Signed-off-by: Eva Rachel Retuya 
> > Reviewed-by: Andy Shevchenko 
> 
> Okay, my another set of comments.
> > @@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  {
> > struct adxl345_data *data = iio_priv(indio_dev);
> 
> > int ret;
> > +   __le16 regval;
> 
> Reverse tree order, please.
> 
>  __le16 regval;
>  int ret;
> 

Ack.

> > @@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
> >  {
> > struct adxl345_data *data;
> > struct iio_dev *indio_dev;
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > int ret;
> > +   u32 regval;
> 
> Ditto.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> 
> I don't like explicit casting in such cases at all. Why not to use
> %ld? Same for the similar places in the series.
> 

OK. Will do that instead.

> > -   if (ret != ADXL345_DEVID) {
> > -   dev_err(>dev, "Invalid device ID: %d\n", ret);
> > +   if (regval != ADXL345_DEVID) {
> > +   dev_err(dev, "Invalid device ID: %x, expected %x\n",
> > +   regval, ADXL345_DEVID);
> 
> So, originally it was in decimal form. Does hex looks better or what
> is behind the change?

Yes it looks better. I made the change to make it easily seen what it
read in hex compared to the known DEVID.

Thanks,
Eva

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:44:27PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Convert the driver to use regmap instead of I2C-specific functions. This
> > is done in preparation for splitting this driver into core and
> > I2C-specific code as well as introduction of SPI driver.
> >
> > Signed-off-by: Eva Rachel Retuya 
> > Reviewed-by: Andy Shevchenko 
> 
> Okay, my another set of comments.
> > @@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  {
> > struct adxl345_data *data = iio_priv(indio_dev);
> 
> > int ret;
> > +   __le16 regval;
> 
> Reverse tree order, please.
> 
>  __le16 regval;
>  int ret;
> 

Ack.

> > @@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
> >  {
> > struct adxl345_data *data;
> > struct iio_dev *indio_dev;
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > int ret;
> > +   u32 regval;
> 
> Ditto.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> 
> I don't like explicit casting in such cases at all. Why not to use
> %ld? Same for the similar places in the series.
> 

OK. Will do that instead.

> > -   if (ret != ADXL345_DEVID) {
> > -   dev_err(>dev, "Invalid device ID: %d\n", ret);
> > +   if (regval != ADXL345_DEVID) {
> > +   dev_err(dev, "Invalid device ID: %x, expected %x\n",
> > +   regval, ADXL345_DEVID);
> 
> So, originally it was in decimal form. Does hex looks better or what
> is behind the change?

Yes it looks better. I made the change to make it easily seen what it
read in hex compared to the known DEVID.

Thanks,
Eva

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  wrote:
> Convert the driver to use regmap instead of I2C-specific functions. This
> is done in preparation for splitting this driver into core and
> I2C-specific code as well as introduction of SPI driver.
>
> Signed-off-by: Eva Rachel Retuya 
> Reviewed-by: Andy Shevchenko 

Okay, my another set of comments.
> @@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  {
> struct adxl345_data *data = iio_priv(indio_dev);

> int ret;
> +   __le16 regval;

Reverse tree order, please.

 __le16 regval;
 int ret;

> @@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
>  {
> struct adxl345_data *data;
> struct iio_dev *indio_dev;
> +   struct device *dev;
> +   struct regmap *regmap;

> int ret;
> +   u32 regval;

Ditto.

> +
> +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> +   if (IS_ERR(regmap)) {
> +   dev_err(>dev, "Error initializing regmap: %d\n",
> +   (int)PTR_ERR(regmap));

I don't like explicit casting in such cases at all. Why not to use
%ld? Same for the similar places in the series.

> -   if (ret != ADXL345_DEVID) {
> -   dev_err(>dev, "Invalid device ID: %d\n", ret);
> +   if (regval != ADXL345_DEVID) {
> +   dev_err(dev, "Invalid device ID: %x, expected %x\n",
> +   regval, ADXL345_DEVID);

So, originally it was in decimal form. Does hex looks better or what
is behind the change?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  wrote:
> Convert the driver to use regmap instead of I2C-specific functions. This
> is done in preparation for splitting this driver into core and
> I2C-specific code as well as introduction of SPI driver.
>
> Signed-off-by: Eva Rachel Retuya 
> Reviewed-by: Andy Shevchenko 

Okay, my another set of comments.
> @@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  {
> struct adxl345_data *data = iio_priv(indio_dev);

> int ret;
> +   __le16 regval;

Reverse tree order, please.

 __le16 regval;
 int ret;

> @@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
>  {
> struct adxl345_data *data;
> struct iio_dev *indio_dev;
> +   struct device *dev;
> +   struct regmap *regmap;

> int ret;
> +   u32 regval;

Ditto.

> +
> +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> +   if (IS_ERR(regmap)) {
> +   dev_err(>dev, "Error initializing regmap: %d\n",
> +   (int)PTR_ERR(regmap));

I don't like explicit casting in such cases at all. Why not to use
%ld? Same for the similar places in the series.

> -   if (ret != ADXL345_DEVID) {
> -   dev_err(>dev, "Invalid device ID: %d\n", ret);
> +   if (regval != ADXL345_DEVID) {
> +   dev_err(dev, "Invalid device ID: %x, expected %x\n",
> +   regval, ADXL345_DEVID);

So, originally it was in decimal form. Does hex looks better or what
is behind the change?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Eva Rachel Retuya]

Convert the driver to use regmap instead of I2C-specific functions. This
is done in preparation for splitting this driver into core and
I2C-specific code as well as introduction of SPI driver.

Signed-off-by: Eva Rachel Retuya 
Reviewed-by: Andy Shevchenko 
---
Changes from v4:
* Add Andy's Reviewed-by tag

Changes from v3:
* Keep intact I2C client structure which was deleted from v3
* Make use of regmap_get_device to retrieve struct device, use these for
  debugging prints instead of >dev.

 drivers/iio/accel/Kconfig   |  1 +
 drivers/iio/accel/adxl345.c | 66 +
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 2308bac..26b8614 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -9,6 +9,7 @@ config ADXL345
tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer Driver"
depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
depends on I2C
+   select REGMAP_I2C
help
  Say Y here if you want to build support for the Analog Devices
  ADXL345 3-axis digital accelerometer.
diff --git a/drivers/iio/accel/adxl345.c b/drivers/iio/accel/adxl345.c
index c34991f..bec8bec 100644
--- a/drivers/iio/accel/adxl345.c
+++ b/drivers/iio/accel/adxl345.c
@@ -14,6 +14,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -45,10 +46,15 @@
 static const int adxl345_uscale = 38300;
 
 struct adxl345_data {
-   struct i2c_client *client;
+   struct regmap *regmap;
u8 data_range;
 };
 
+static const struct regmap_config adxl345_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 8,
+};
+
 #define ADXL345_CHANNEL(reg, axis) {   \
.type = IIO_ACCEL,  \
.modified = 1,  \
@@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 {
struct adxl345_data *data = iio_priv(indio_dev);
int ret;
+   __le16 regval;
 
switch (mask) {
case IIO_CHAN_INFO_RAW:
@@ -78,11 +85,12 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
 * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
 */
-   ret = i2c_smbus_read_word_data(data->client, chan->address);
+   ret = regmap_bulk_read(data->regmap, chan->address, ,
+  sizeof(regval));
if (ret < 0)
return ret;
 
-   *val = sign_extend32(ret, 12);
+   *val = sign_extend32(le16_to_cpu(regval), 12);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
*val = 0;
@@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
 {
struct adxl345_data *data;
struct iio_dev *indio_dev;
+   struct device *dev;
+   struct regmap *regmap;
int ret;
+   u32 regval;
+
+   regmap = devm_regmap_init_i2c(client, _regmap_config);
+   if (IS_ERR(regmap)) {
+   dev_err(>dev, "Error initializing regmap: %d\n",
+   (int)PTR_ERR(regmap));
+   return PTR_ERR(regmap);
+   }
+
+   dev = regmap_get_device(regmap);
 
-   ret = i2c_smbus_read_byte_data(client, ADXL345_REG_DEVID);
+   ret = regmap_read(regmap, ADXL345_REG_DEVID, );
if (ret < 0) {
-   dev_err(>dev, "Error reading device ID: %d\n", ret);
+   dev_err(dev, "Error reading device ID: %d\n", ret);
return ret;
}
 
-   if (ret != ADXL345_DEVID) {
-   dev_err(>dev, "Invalid device ID: %d\n", ret);
+   if (regval != ADXL345_DEVID) {
+   dev_err(dev, "Invalid device ID: %x, expected %x\n",
+   regval, ADXL345_DEVID);
return -ENODEV;
}
 
-   indio_dev = devm_iio_device_alloc(>dev, sizeof(*data));
+   indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
 
data = iio_priv(indio_dev);
-   i2c_set_clientdata(client, indio_dev);
-   data->client = client;
+   dev_set_drvdata(dev, indio_dev);
+   data->regmap = regmap;
/* Enable full-resolution mode */
data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
 
-   ret = i2c_smbus_write_byte_data(data->client, ADXL345_REG_DATA_FORMAT,
-   data->data_range);
+   ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
+  data->data_range);
if (ret < 0) {
-   dev_err(>dev, "Failed to set data range: %d\n", ret);
+   dev_err(dev, "Failed to set data range: %d\n", ret);
   

[PATCH v5 2/4] iio: accel: adxl345: Use I2C regmap instead of direct I2C access

[Eva Rachel Retuya]

Convert the driver to use regmap instead of I2C-specific functions. This
is done in preparation for splitting this driver into core and
I2C-specific code as well as introduction of SPI driver.

Signed-off-by: Eva Rachel Retuya 
Reviewed-by: Andy Shevchenko 
---
Changes from v4:
* Add Andy's Reviewed-by tag

Changes from v3:
* Keep intact I2C client structure which was deleted from v3
* Make use of regmap_get_device to retrieve struct device, use these for
  debugging prints instead of >dev.

 drivers/iio/accel/Kconfig   |  1 +
 drivers/iio/accel/adxl345.c | 66 +
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 2308bac..26b8614 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -9,6 +9,7 @@ config ADXL345
tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer Driver"
depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
depends on I2C
+   select REGMAP_I2C
help
  Say Y here if you want to build support for the Analog Devices
  ADXL345 3-axis digital accelerometer.
diff --git a/drivers/iio/accel/adxl345.c b/drivers/iio/accel/adxl345.c
index c34991f..bec8bec 100644
--- a/drivers/iio/accel/adxl345.c
+++ b/drivers/iio/accel/adxl345.c
@@ -14,6 +14,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -45,10 +46,15 @@
 static const int adxl345_uscale = 38300;
 
 struct adxl345_data {
-   struct i2c_client *client;
+   struct regmap *regmap;
u8 data_range;
 };
 
+static const struct regmap_config adxl345_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 8,
+};
+
 #define ADXL345_CHANNEL(reg, axis) {   \
.type = IIO_ACCEL,  \
.modified = 1,  \
@@ -70,6 +76,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 {
struct adxl345_data *data = iio_priv(indio_dev);
int ret;
+   __le16 regval;
 
switch (mask) {
case IIO_CHAN_INFO_RAW:
@@ -78,11 +85,12 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
 * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
 */
-   ret = i2c_smbus_read_word_data(data->client, chan->address);
+   ret = regmap_bulk_read(data->regmap, chan->address, ,
+  sizeof(regval));
if (ret < 0)
return ret;
 
-   *val = sign_extend32(ret, 12);
+   *val = sign_extend32(le16_to_cpu(regval), 12);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
*val = 0;
@@ -104,37 +112,50 @@ static int adxl345_probe(struct i2c_client *client,
 {
struct adxl345_data *data;
struct iio_dev *indio_dev;
+   struct device *dev;
+   struct regmap *regmap;
int ret;
+   u32 regval;
+
+   regmap = devm_regmap_init_i2c(client, _regmap_config);
+   if (IS_ERR(regmap)) {
+   dev_err(>dev, "Error initializing regmap: %d\n",
+   (int)PTR_ERR(regmap));
+   return PTR_ERR(regmap);
+   }
+
+   dev = regmap_get_device(regmap);
 
-   ret = i2c_smbus_read_byte_data(client, ADXL345_REG_DEVID);
+   ret = regmap_read(regmap, ADXL345_REG_DEVID, );
if (ret < 0) {
-   dev_err(>dev, "Error reading device ID: %d\n", ret);
+   dev_err(dev, "Error reading device ID: %d\n", ret);
return ret;
}
 
-   if (ret != ADXL345_DEVID) {
-   dev_err(>dev, "Invalid device ID: %d\n", ret);
+   if (regval != ADXL345_DEVID) {
+   dev_err(dev, "Invalid device ID: %x, expected %x\n",
+   regval, ADXL345_DEVID);
return -ENODEV;
}
 
-   indio_dev = devm_iio_device_alloc(>dev, sizeof(*data));
+   indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
 
data = iio_priv(indio_dev);
-   i2c_set_clientdata(client, indio_dev);
-   data->client = client;
+   dev_set_drvdata(dev, indio_dev);
+   data->regmap = regmap;
/* Enable full-resolution mode */
data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
 
-   ret = i2c_smbus_write_byte_data(data->client, ADXL345_REG_DATA_FORMAT,
-   data->data_range);
+   ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
+  data->data_range);
if (ret < 0) {
-   dev_err(>dev, "Failed to set data range: %d\n", ret);
+   dev_err(dev, "Failed to set data range: %d\n", ret);
return ret;
}
 
-