Re: [RFC/RFT PATCH 1/3] Input: ad7879 - convert to use regmap

2017-02-20 Thread Michael Hennerich 

On 18.02.2017 00:31, Dmitry Torokhov wrote:

Instead of rolling our own infrastructure to provide uniform access to I2C
and SPI buses, let's switch to using regmap.

Signed-off-by: Dmitry Torokhov 
---

Michael,

I am a bit (actually a lot) confused how this driver was working with SPI
and Blackfin, which is, as far as I understand, little-endian, because from
my reading of the spec the data on wire is big-endian. If this is not
correct we need to change spi regmap config to be:

static const struct regmap_config ad7879_spi_regmap_config = {
.reg_bits = 16,
.val_bits = 16,
.max_register = 15,
.read_flag_mask = AD7879_CMD_MAGIC | AD7879_CMD_READ,
.write_flag_mask = AD7879_CMD_MAGIC,
.reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
.val_format_endian_default = REGMAP_ENDIAN_LITTLE,
};

Also, the original ad7879_spi_xfer was funky to tell the least. We
allocated n+2 spi_transfer structures, then smashed the beginning of the
first one with u16 command, then basically "restored" it???

Anyway, if you could try it out (and fix ;) ), that would be great.


Hi Dimitry,

Thanks for doing this.
Overall looks great, but i definitely need to test this.
Your right this xfer functions looks awkward.
I'll review and get back to you.



--
Greetings,
Michael

--
Analog Devices GmbH  Otl-Aicher Strasse 60-64  80807 München
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Peter Kolberg, Ali Raza Husain, Eileen Wynne

Re: [RFC/RFT PATCH 1/3] Input: ad7879 - convert to use regmap

2017-02-20 Thread Michael Hennerich 

On 18.02.2017 00:31, Dmitry Torokhov wrote:

Instead of rolling our own infrastructure to provide uniform access to I2C
and SPI buses, let's switch to using regmap.

Signed-off-by: Dmitry Torokhov 
---

Michael,

I am a bit (actually a lot) confused how this driver was working with SPI
and Blackfin, which is, as far as I understand, little-endian, because from
my reading of the spec the data on wire is big-endian. If this is not
correct we need to change spi regmap config to be:

static const struct regmap_config ad7879_spi_regmap_config = {
.reg_bits = 16,
.val_bits = 16,
.max_register = 15,
.read_flag_mask = AD7879_CMD_MAGIC | AD7879_CMD_READ,
.write_flag_mask = AD7879_CMD_MAGIC,
.reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
.val_format_endian_default = REGMAP_ENDIAN_LITTLE,
};

Also, the original ad7879_spi_xfer was funky to tell the least. We
allocated n+2 spi_transfer structures, then smashed the beginning of the
first one with u16 command, then basically "restored" it???

Anyway, if you could try it out (and fix ;) ), that would be great.


Hi Dimitry,

Thanks for doing this.
Overall looks great, but i definitely need to test this.
Your right this xfer functions looks awkward.
I'll review and get back to you.



--
Greetings,
Michael

--
Analog Devices GmbH  Otl-Aicher Strasse 60-64  80807 München
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Peter Kolberg, Ali Raza Husain, Eileen Wynne

[RFC/RFT PATCH 1/3] Input: ad7879 - convert to use regmap

2017-02-17 Thread Dmitry Torokhov 
Instead of rolling our own infrastructure to provide uniform access to I2C
and SPI buses, let's switch to using regmap.

Signed-off-by: Dmitry Torokhov 
---

Michael,

I am a bit (actually a lot) confused how this driver was working with SPI
and Blackfin, which is, as far as I understand, little-endian, because from
my reading of the spec the data on wire is big-endian. If this is not
correct we need to change spi regmap config to be:

static const struct regmap_config ad7879_spi_regmap_config = {
.reg_bits = 16,
.val_bits = 16,
.max_register = 15,
.read_flag_mask = AD7879_CMD_MAGIC | AD7879_CMD_READ,
.write_flag_mask = AD7879_CMD_MAGIC,
.reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
.val_format_endian_default = REGMAP_ENDIAN_LITTLE,
};

Also, the original ad7879_spi_xfer was funky to tell the least. We
allocated n+2 spi_transfer structures, then smashed the beginning of the
first one with u16 command, then basically "restored" it???

Anyway, if you could try it out (and fix ;) ), that would be great.

Thanks.

 drivers/input/touchscreen/ad7879-i2c.c |  50 ---
 drivers/input/touchscreen/ad7879-spi.c | 112 ++---
 drivers/input/touchscreen/ad7879.c |  46 +-
 drivers/input/touchscreen/ad7879.h |  12 +---
 4 files changed, 63 insertions(+), 157 deletions(-)

diff --git a/drivers/input/touchscreen/ad7879-i2c.c 
b/drivers/input/touchscreen/ad7879-i2c.c
index 58f72e0246ab..25aa9b89a6aa 100644
--- a/drivers/input/touchscreen/ad7879-i2c.c
+++ b/drivers/input/touchscreen/ad7879-i2c.c
@@ -12,53 +12,23 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "ad7879.h"
 
 #define AD7879_DEVID   0x79/* AD7879-1/AD7889-1 */
 
-/* All registers are word-sized.
- * AD7879 uses a high-byte first convention.
- */
-static int ad7879_i2c_read(struct device *dev, u8 reg)
-{
-   struct i2c_client *client = to_i2c_client(dev);
-
-   return i2c_smbus_read_word_swapped(client, reg);
-}
-
-static int ad7879_i2c_multi_read(struct device *dev,
-u8 first_reg, u8 count, u16 *buf)
-{
-   struct i2c_client *client = to_i2c_client(dev);
-   u8 idx;
-
-   i2c_smbus_read_i2c_block_data(client, first_reg, count * 2, (u8 *)buf);
-
-   for (idx = 0; idx < count; ++idx)
-   buf[idx] = swab16(buf[idx]);
-
-   return 0;
-}
-
-static int ad7879_i2c_write(struct device *dev, u8 reg, u16 val)
-{
-   struct i2c_client *client = to_i2c_client(dev);
-
-   return i2c_smbus_write_word_swapped(client, reg, val);
-}
-
-static const struct ad7879_bus_ops ad7879_i2c_bus_ops = {
-   .bustype= BUS_I2C,
-   .read   = ad7879_i2c_read,
-   .multi_read = ad7879_i2c_multi_read,
-   .write  = ad7879_i2c_write,
+static const struct regmap_config ad7879_i2c_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 16,
+   .max_register = 15,
 };
 
 static int ad7879_i2c_probe(struct i2c_client *client,
  const struct i2c_device_id *id)
 {
struct ad7879 *ts;
+   struct regmap *regmap;
 
if (!i2c_check_functionality(client->adapter,
 I2C_FUNC_SMBUS_WORD_DATA)) {
@@ -66,8 +36,12 @@ static int ad7879_i2c_probe(struct i2c_client *client,
return -EIO;
}
 
-   ts = ad7879_probe(>dev, AD7879_DEVID, client->irq,
- _i2c_bus_ops);
+   regmap = devm_regmap_init_i2c(client, _i2c_regmap_config);
+   if (IS_ERR(regmap))
+   return PTR_ERR(regmap);
+
+   ts = ad7879_probe(>dev, regmap, client->irq,
+ BUS_I2C, AD7879_DEVID);
if (IS_ERR(ts))
return PTR_ERR(ts);
 
diff --git a/drivers/input/touchscreen/ad7879-spi.c 
b/drivers/input/touchscreen/ad7879-spi.c
index d42b6b9af191..e9535aacaabf 100644
--- a/drivers/input/touchscreen/ad7879-spi.c
+++ b/drivers/input/touchscreen/ad7879-spi.c
@@ -11,109 +11,29 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "ad7879.h"
 
 #define AD7879_DEVID   0x7A/* AD7879/AD7889 */
 
 #define MAX_SPI_FREQ_HZ  500
-#define AD7879_CMD_MAGIC 0xE000
-#define AD7879_CMD_READ  (1 << 10)
-#define AD7879_CMD(reg)  (AD7879_CMD_MAGIC | ((reg) & 0xF))
-#define AD7879_WRITECMD(reg) (AD7879_CMD(reg))
-#define AD7879_READCMD(reg)  (AD7879_CMD(reg) | AD7879_CMD_READ)
-
-/*
- * ad7879_read/write are only used for initial setup and for sysfs controls.
- * The main traffic is done in ad7879_collect().
- */
-
-static int ad7879_spi_xfer(struct spi_device *spi,
-  u16 cmd, u8 count, u16 *tx_buf, u16 *rx_buf)
-{
-   struct spi_message msg;
-   struct spi_transfer *xfers;
-   void *spi_data;
-   u16 *command;
-   u16 *_rx_buf = _rx_buf; /* shut gcc up */
-   u8 idx;
-   int ret;
-
-

[RFC/RFT PATCH 1/3] Input: ad7879 - convert to use regmap

2017-02-17 Thread Dmitry Torokhov 
Instead of rolling our own infrastructure to provide uniform access to I2C
and SPI buses, let's switch to using regmap.

Signed-off-by: Dmitry Torokhov 
---

Michael,

I am a bit (actually a lot) confused how this driver was working with SPI
and Blackfin, which is, as far as I understand, little-endian, because from
my reading of the spec the data on wire is big-endian. If this is not
correct we need to change spi regmap config to be:

static const struct regmap_config ad7879_spi_regmap_config = {
.reg_bits = 16,
.val_bits = 16,
.max_register = 15,
.read_flag_mask = AD7879_CMD_MAGIC | AD7879_CMD_READ,
.write_flag_mask = AD7879_CMD_MAGIC,
.reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
.val_format_endian_default = REGMAP_ENDIAN_LITTLE,
};

Also, the original ad7879_spi_xfer was funky to tell the least. We
allocated n+2 spi_transfer structures, then smashed the beginning of the
first one with u16 command, then basically "restored" it???

Anyway, if you could try it out (and fix ;) ), that would be great.

Thanks.

 drivers/input/touchscreen/ad7879-i2c.c |  50 ---
 drivers/input/touchscreen/ad7879-spi.c | 112 ++---
 drivers/input/touchscreen/ad7879.c |  46 +-
 drivers/input/touchscreen/ad7879.h |  12 +---
 4 files changed, 63 insertions(+), 157 deletions(-)

diff --git a/drivers/input/touchscreen/ad7879-i2c.c 
b/drivers/input/touchscreen/ad7879-i2c.c
index 58f72e0246ab..25aa9b89a6aa 100644
--- a/drivers/input/touchscreen/ad7879-i2c.c
+++ b/drivers/input/touchscreen/ad7879-i2c.c
@@ -12,53 +12,23 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "ad7879.h"
 
 #define AD7879_DEVID   0x79/* AD7879-1/AD7889-1 */
 
-/* All registers are word-sized.
- * AD7879 uses a high-byte first convention.
- */
-static int ad7879_i2c_read(struct device *dev, u8 reg)
-{
-   struct i2c_client *client = to_i2c_client(dev);
-
-   return i2c_smbus_read_word_swapped(client, reg);
-}
-
-static int ad7879_i2c_multi_read(struct device *dev,
-u8 first_reg, u8 count, u16 *buf)
-{
-   struct i2c_client *client = to_i2c_client(dev);
-   u8 idx;
-
-   i2c_smbus_read_i2c_block_data(client, first_reg, count * 2, (u8 *)buf);
-
-   for (idx = 0; idx < count; ++idx)
-   buf[idx] = swab16(buf[idx]);
-
-   return 0;
-}
-
-static int ad7879_i2c_write(struct device *dev, u8 reg, u16 val)
-{
-   struct i2c_client *client = to_i2c_client(dev);
-
-   return i2c_smbus_write_word_swapped(client, reg, val);
-}
-
-static const struct ad7879_bus_ops ad7879_i2c_bus_ops = {
-   .bustype= BUS_I2C,
-   .read   = ad7879_i2c_read,
-   .multi_read = ad7879_i2c_multi_read,
-   .write  = ad7879_i2c_write,
+static const struct regmap_config ad7879_i2c_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 16,
+   .max_register = 15,
 };
 
 static int ad7879_i2c_probe(struct i2c_client *client,
  const struct i2c_device_id *id)
 {
struct ad7879 *ts;
+   struct regmap *regmap;
 
if (!i2c_check_functionality(client->adapter,
 I2C_FUNC_SMBUS_WORD_DATA)) {
@@ -66,8 +36,12 @@ static int ad7879_i2c_probe(struct i2c_client *client,
return -EIO;
}
 
-   ts = ad7879_probe(>dev, AD7879_DEVID, client->irq,
- _i2c_bus_ops);
+   regmap = devm_regmap_init_i2c(client, _i2c_regmap_config);
+   if (IS_ERR(regmap))
+   return PTR_ERR(regmap);
+
+   ts = ad7879_probe(>dev, regmap, client->irq,
+ BUS_I2C, AD7879_DEVID);
if (IS_ERR(ts))
return PTR_ERR(ts);
 
diff --git a/drivers/input/touchscreen/ad7879-spi.c 
b/drivers/input/touchscreen/ad7879-spi.c
index d42b6b9af191..e9535aacaabf 100644
--- a/drivers/input/touchscreen/ad7879-spi.c
+++ b/drivers/input/touchscreen/ad7879-spi.c
@@ -11,109 +11,29 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "ad7879.h"
 
 #define AD7879_DEVID   0x7A/* AD7879/AD7889 */
 
 #define MAX_SPI_FREQ_HZ  500
-#define AD7879_CMD_MAGIC 0xE000
-#define AD7879_CMD_READ  (1 << 10)
-#define AD7879_CMD(reg)  (AD7879_CMD_MAGIC | ((reg) & 0xF))
-#define AD7879_WRITECMD(reg) (AD7879_CMD(reg))
-#define AD7879_READCMD(reg)  (AD7879_CMD(reg) | AD7879_CMD_READ)
-
-/*
- * ad7879_read/write are only used for initial setup and for sysfs controls.
- * The main traffic is done in ad7879_collect().
- */
-
-static int ad7879_spi_xfer(struct spi_device *spi,
-  u16 cmd, u8 count, u16 *tx_buf, u16 *rx_buf)
-{
-   struct spi_message msg;
-   struct spi_transfer *xfers;
-   void *spi_data;
-   u16 *command;
-   u16 *_rx_buf = _rx_buf; /* shut gcc up */
-   u8 idx;
-   int ret;
-
-   xfers = spi_data =