Re: [PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:51:28PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Move I2C-specific code into its own file and rely on regmap to access
> > registers. The core code provides access to x, y, z and scale readings.
> 
> Portion of minor comments.
> 
> > +config ADXL345_I2C
> > +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
> > Driver"
> 
> > depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
> 
> I'm wondering if it works in a form of
> 
> depends on INPUT_ADXL34X=n
> 

Yes it works. Will change it into this form.

> > +int adxl345_common_probe(struct device *dev, struct regmap *regmap,
> > +const char *name);
> > +int adxl345_common_remove(struct device *dev);
> 
> I think a "common" word is redundant.
> 

OK. I will use "core" instead.

> > - * IIO driver for ADXL345
> > - * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
> > - * 0x53 (ALT ADDRESS pin grounded)
> 
> > + * IIO core driver for ADXL345
> 
> Should not it be at the beginning of header comment?
> 

Ack.

> > +static int adxl345_i2c_probe(struct i2c_client *client,
> > +const struct i2c_device_id *id)
> > +{
> 
> > +   struct regmap *regmap;
> > +   const char *name = NULL;
> 
> Reverse tree order, please.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _i2c_regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing i2c regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> > +   return PTR_ERR(regmap);
> > +   }
> > +
> 
> > +   if (id)
> > +   name = id->name;
> > +
> > +   return adxl345_common_probe(>dev, regmap, name);
> 
> Do you need temporary variable?
> 
> return adxl345_probe(>dev, regmap, id ? id->name : NULL);
> 

Will remove it and use the form you suggest.

Thanks,
Eva

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Eva Rachel Retuya]

On Fri, Mar 03, 2017 at 06:51:28PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  
> wrote:
> > Move I2C-specific code into its own file and rely on regmap to access
> > registers. The core code provides access to x, y, z and scale readings.
> 
> Portion of minor comments.
> 
> > +config ADXL345_I2C
> > +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
> > Driver"
> 
> > depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
> 
> I'm wondering if it works in a form of
> 
> depends on INPUT_ADXL34X=n
> 

Yes it works. Will change it into this form.

> > +int adxl345_common_probe(struct device *dev, struct regmap *regmap,
> > +const char *name);
> > +int adxl345_common_remove(struct device *dev);
> 
> I think a "common" word is redundant.
> 

OK. I will use "core" instead.

> > - * IIO driver for ADXL345
> > - * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
> > - * 0x53 (ALT ADDRESS pin grounded)
> 
> > + * IIO core driver for ADXL345
> 
> Should not it be at the beginning of header comment?
> 

Ack.

> > +static int adxl345_i2c_probe(struct i2c_client *client,
> > +const struct i2c_device_id *id)
> > +{
> 
> > +   struct regmap *regmap;
> > +   const char *name = NULL;
> 
> Reverse tree order, please.
> 
> > +
> > +   regmap = devm_regmap_init_i2c(client, _i2c_regmap_config);
> > +   if (IS_ERR(regmap)) {
> > +   dev_err(>dev, "Error initializing i2c regmap: %d\n",
> > +   (int)PTR_ERR(regmap));
> > +   return PTR_ERR(regmap);
> > +   }
> > +
> 
> > +   if (id)
> > +   name = id->name;
> > +
> > +   return adxl345_common_probe(>dev, regmap, name);
> 
> Do you need temporary variable?
> 
> return adxl345_probe(>dev, regmap, id ? id->name : NULL);
> 

Will remove it and use the form you suggest.

Thanks,
Eva

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  wrote:
> Move I2C-specific code into its own file and rely on regmap to access
> registers. The core code provides access to x, y, z and scale readings.

Portion of minor comments.

> +config ADXL345_I2C
> +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
> Driver"

> depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)

I'm wondering if it works in a form of

depends on INPUT_ADXL34X=n

> +int adxl345_common_probe(struct device *dev, struct regmap *regmap,
> +const char *name);
> +int adxl345_common_remove(struct device *dev);

I think a "common" word is redundant.

> - * IIO driver for ADXL345
> - * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
> - * 0x53 (ALT ADDRESS pin grounded)

> + * IIO core driver for ADXL345

Should not it be at the beginning of header comment?

> +static int adxl345_i2c_probe(struct i2c_client *client,
> +const struct i2c_device_id *id)
> +{

> +   struct regmap *regmap;
> +   const char *name = NULL;

Reverse tree order, please.

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

> +   if (id)
> +   name = id->name;
> +
> +   return adxl345_common_probe(>dev, regmap, name);

Do you need temporary variable?

return adxl345_probe(>dev, regmap, id ? id->name : NULL);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Andy Shevchenko]

On Tue, Feb 28, 2017 at 4:37 AM, Eva Rachel Retuya  wrote:
> Move I2C-specific code into its own file and rely on regmap to access
> registers. The core code provides access to x, y, z and scale readings.

Portion of minor comments.

> +config ADXL345_I2C
> +   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
> Driver"

> depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)

I'm wondering if it works in a form of

depends on INPUT_ADXL34X=n

> +int adxl345_common_probe(struct device *dev, struct regmap *regmap,
> +const char *name);
> +int adxl345_common_remove(struct device *dev);

I think a "common" word is redundant.

> - * IIO driver for ADXL345
> - * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
> - * 0x53 (ALT ADDRESS pin grounded)

> + * IIO core driver for ADXL345

Should not it be at the beginning of header comment?

> +static int adxl345_i2c_probe(struct i2c_client *client,
> +const struct i2c_device_id *id)
> +{

> +   struct regmap *regmap;
> +   const char *name = NULL;

Reverse tree order, please.

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

> +   if (id)
> +   name = id->name;
> +
> +   return adxl345_common_probe(>dev, regmap, name);

Do you need temporary variable?

return adxl345_probe(>dev, regmap, id ? id->name : NULL);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Eva Rachel Retuya]

Move I2C-specific code into its own file and rely on regmap to access
registers. The core code provides access to x, y, z and scale readings.

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

Changes from v3:
* Revert to explicit and separate I2C and SPI configuration
* Add OF match table, make it enumerable in ACPI environment (Andy's suggestion)

 drivers/iio/accel/Kconfig   | 11 +++-
 drivers/iio/accel/Makefile  |  3 +-
 drivers/iio/accel/adxl345.h | 18 ++
 drivers/iio/accel/{adxl345.c => adxl345_core.c} | 55 -
 drivers/iio/accel/adxl345_i2c.c | 78 +
 5 files changed, 117 insertions(+), 48 deletions(-)
 create mode 100644 drivers/iio/accel/adxl345.h
 rename drivers/iio/accel/{adxl345.c => adxl345_core.c} (78%)
 create mode 100644 drivers/iio/accel/adxl345_i2c.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 26b8614..9f5a889 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -6,16 +6,21 @@
 menu "Accelerometers"
 
 config ADXL345
-   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer Driver"
+   tristate
+
+config ADXL345_I2C
+   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
Driver"
depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
depends on I2C
+   select ADXL345
select REGMAP_I2C
help
  Say Y here if you want to build support for the Analog Devices
  ADXL345 3-axis digital accelerometer.
 
- To compile this driver as a module, choose M here: the
- module will be called adxl345.
+ To compile this driver as a module, choose M here: the module
+ will be called adxl345_i2c and you will also get adxl345_core
+ for the core module.
 
 config BMA180
tristate "Bosch BMA180/BMA250 3-Axis Accelerometer Driver"
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 618488d..3f4a6d6 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -3,7 +3,8 @@
 #
 
 # When adding new entries keep the list in alphabetical order
-obj-$(CONFIG_ADXL345) += adxl345.o
+obj-$(CONFIG_ADXL345) += adxl345_core.o
+obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
 obj-$(CONFIG_BMA180) += bma180.o
 obj-$(CONFIG_BMA220) += bma220_spi.o
 obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
new file mode 100644
index 000..fca3e25
--- /dev/null
+++ b/drivers/iio/accel/adxl345.h
@@ -0,0 +1,18 @@
+/*
+ * ADXL345 3-Axis Digital Accelerometer
+ *
+ * Copyright (c) 2017 Eva Rachel Retuya 
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License. See the file COPYING in the main
+ * directory of this archive for more details.
+ */
+
+#ifndef _ADXL345_H_
+#define _ADXL345_H_
+
+int adxl345_common_probe(struct device *dev, struct regmap *regmap,
+const char *name);
+int adxl345_common_remove(struct device *dev);
+
+#endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345.c b/drivers/iio/accel/adxl345_core.c
similarity index 78%
rename from drivers/iio/accel/adxl345.c
rename to drivers/iio/accel/adxl345_core.c
index bec8bec..9dea6a5 100644
--- a/drivers/iio/accel/adxl345.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -7,17 +7,16 @@
  * the GNU General Public License. See the file COPYING in the main
  * directory of this archive for more details.
  *
- * IIO driver for ADXL345
- * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
- * 0x53 (ALT ADDRESS pin grounded)
+ * IIO core driver for ADXL345
  */
 
-#include 
 #include 
 #include 
 
 #include 
 
+#include "adxl345.h"
+
 #define ADXL345_REG_DEVID  0x00
 #define ADXL345_REG_POWER_CTL  0x2D
 #define ADXL345_REG_DATA_FORMAT0x31
@@ -50,11 +49,6 @@ struct adxl345_data {
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,  \
@@ -107,25 +101,14 @@ static const struct iio_info adxl345_info = {
.read_raw   = adxl345_read_raw,
 };
 
-static int adxl345_probe(struct i2c_client *client,
-const struct i2c_device_id *id)
+int adxl345_common_probe(struct device *dev, struct regmap *regmap,
+const char *name)
 {
struct adxl345_data *data;
struct iio_dev *indio_dev;
-   struct device *dev;
-   struct regmap *regmap;
int ret;
u32 regval;
 

[PATCH v5 3/4] iio: accel: adxl345: Split driver into core and I2C

[Eva Rachel Retuya]

Move I2C-specific code into its own file and rely on regmap to access
registers. The core code provides access to x, y, z and scale readings.

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

Changes from v3:
* Revert to explicit and separate I2C and SPI configuration
* Add OF match table, make it enumerable in ACPI environment (Andy's suggestion)

 drivers/iio/accel/Kconfig   | 11 +++-
 drivers/iio/accel/Makefile  |  3 +-
 drivers/iio/accel/adxl345.h | 18 ++
 drivers/iio/accel/{adxl345.c => adxl345_core.c} | 55 -
 drivers/iio/accel/adxl345_i2c.c | 78 +
 5 files changed, 117 insertions(+), 48 deletions(-)
 create mode 100644 drivers/iio/accel/adxl345.h
 rename drivers/iio/accel/{adxl345.c => adxl345_core.c} (78%)
 create mode 100644 drivers/iio/accel/adxl345_i2c.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 26b8614..9f5a889 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -6,16 +6,21 @@
 menu "Accelerometers"
 
 config ADXL345
-   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer Driver"
+   tristate
+
+config ADXL345_I2C
+   tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C 
Driver"
depends on !(INPUT_ADXL34X=y || INPUT_ADXL34X=m)
depends on I2C
+   select ADXL345
select REGMAP_I2C
help
  Say Y here if you want to build support for the Analog Devices
  ADXL345 3-axis digital accelerometer.
 
- To compile this driver as a module, choose M here: the
- module will be called adxl345.
+ To compile this driver as a module, choose M here: the module
+ will be called adxl345_i2c and you will also get adxl345_core
+ for the core module.
 
 config BMA180
tristate "Bosch BMA180/BMA250 3-Axis Accelerometer Driver"
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 618488d..3f4a6d6 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -3,7 +3,8 @@
 #
 
 # When adding new entries keep the list in alphabetical order
-obj-$(CONFIG_ADXL345) += adxl345.o
+obj-$(CONFIG_ADXL345) += adxl345_core.o
+obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
 obj-$(CONFIG_BMA180) += bma180.o
 obj-$(CONFIG_BMA220) += bma220_spi.o
 obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
new file mode 100644
index 000..fca3e25
--- /dev/null
+++ b/drivers/iio/accel/adxl345.h
@@ -0,0 +1,18 @@
+/*
+ * ADXL345 3-Axis Digital Accelerometer
+ *
+ * Copyright (c) 2017 Eva Rachel Retuya 
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License. See the file COPYING in the main
+ * directory of this archive for more details.
+ */
+
+#ifndef _ADXL345_H_
+#define _ADXL345_H_
+
+int adxl345_common_probe(struct device *dev, struct regmap *regmap,
+const char *name);
+int adxl345_common_remove(struct device *dev);
+
+#endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345.c b/drivers/iio/accel/adxl345_core.c
similarity index 78%
rename from drivers/iio/accel/adxl345.c
rename to drivers/iio/accel/adxl345_core.c
index bec8bec..9dea6a5 100644
--- a/drivers/iio/accel/adxl345.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -7,17 +7,16 @@
  * the GNU General Public License. See the file COPYING in the main
  * directory of this archive for more details.
  *
- * IIO driver for ADXL345
- * 7-bit I2C slave address: 0x1D (ALT ADDRESS pin tied to VDDIO) or
- * 0x53 (ALT ADDRESS pin grounded)
+ * IIO core driver for ADXL345
  */
 
-#include 
 #include 
 #include 
 
 #include 
 
+#include "adxl345.h"
+
 #define ADXL345_REG_DEVID  0x00
 #define ADXL345_REG_POWER_CTL  0x2D
 #define ADXL345_REG_DATA_FORMAT0x31
@@ -50,11 +49,6 @@ struct adxl345_data {
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,  \
@@ -107,25 +101,14 @@ static const struct iio_info adxl345_info = {
.read_raw   = adxl345_read_raw,
 };
 
-static int adxl345_probe(struct i2c_client *client,
-const struct i2c_device_id *id)
+int adxl345_common_probe(struct device *dev, struct regmap *regmap,
+const char *name)
 {
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);
-