[PATCH 00/14] pinctrl-sx150x: Various bug-fixes and code simplifications

[PATCH 00/14] pinctrl-sx150x: Various bug-fixes and code simplifications

[Andrey Smirnov]

Linus, Neil:

I've had some help and got my hardware setup modified to enable IRQ
functionality testing, so ended up looking at the code of SX150x more
resulting in some code improvements (hopefully) and bugfixes.

There are many small changes each of which is probably better
described by corresponding commit's message, however the most
porminenet changes of the whole patchset are the switch to regmap API
(patches ## 7,8) and reduction of locking (patch # 9)

Please let me know what you think.

Thanks,
Andrey

Andrey Smirnov (14):
  pinctrl-sx150x: Rely on of_modalias_node for OF matching
  pinctrl-sx150x: Add SX1503 specific data
  pinctrl-sx150x: Replace magic number in sx150x_init_hw
  pinctrl-sx150x: Fix incorrect constant in sx150x_init_hw
  pinctrl-sx150x: Move some code out of sx150x_init_hw
  pinctrl-sx150x: Improve sx150x_init_misc for SX1504/5/6
  pinctrl-sx150x: Convert driver to use regmap API
  pinctrl-sx150x: Replace sx150x_*_cfg by means of regmap API
  pinctrl-sx150x: Remove excessive locking
  pinctrl-sx150x: Improve oscio GPIO functions
  pinctrl-sx150x: Simplify interrupt handler
  pinctrl-sx150x: Use handle_bad_irq instead of handle_edge_irq
  pinctrl-sx150x: Remove magic numbers from sx150x_irq_set_type
  pinctrl-sx150x: Remove magic numbers from sx150x_reset

 drivers/pinctrl/pinctrl-sx150x.c | 753 +++++++++++++++++++++------------------
 1 file changed, 416 insertions(+), 337 deletions(-)

-- 
2.5.5

[PATCH 01/14] pinctrl-sx150x: Rely on of_modalias_node for OF matching

[Andrey Smirnov]

None of the OF match table entries contain any compatiblity strings that
could not be matched against using i2c_device_id table above and
of_modalias_node. Besides that entries in OF match table do not cary
proper device variant information which is need by the drive. Those two
facts combined, IMHO, make a compelling case for removal of that code
altogether.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index d2d4211..41b9e6a 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -861,14 +861,6 @@ static const struct i2c_device_id sx150x_id[] = {
 	{}
 };
 
-static const struct of_device_id sx150x_of_match[] = {
-	{ .compatible = "semtech,sx1508q" },
-	{ .compatible = "semtech,sx1509q" },
-	{ .compatible = "semtech,sx1506q" },
-	{ .compatible = "semtech,sx1502q" },
-	{},
-};
-
 static int sx150x_init_io(struct sx150x_pinctrl *pctl, u8 base, u16 cfg)
 {
 	int err = 0;
@@ -1049,7 +1041,6 @@ static int sx150x_probe(struct i2c_client *client,
 static struct i2c_driver sx150x_driver = {
 	.driver = {
 		.name = "sx150x-pinctrl",
-		.of_match_table = of_match_ptr(sx150x_of_match),
 	},
 	.probe    = sx150x_probe,
 	.id_table = sx150x_id,
-- 
2.5.5

[PATCH 02/14] pinctrl-sx150x: Add SX1503 specific data

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 41b9e6a..e0f52e4 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -229,6 +229,29 @@ static const struct sx150x_device_data sx1502q_device_data = {
 	.npins = 8, /* oscio not available */
 };
 
+static const struct sx150x_device_data sx1503q_device_data = {
+	.model = SX150X_123,
+	.reg_pullup	= 0x05,
+	.reg_pulldn	= 0x07,
+	.reg_dir	= 0x03,
+	.reg_data	= 0x01,
+	.reg_irq_mask	= 0x09,
+	.reg_irq_src	= 0x0f,
+	.reg_sense	= 0x07,
+	.pri.x123 = {
+		.reg_pld_mode	= 0x10,
+		.reg_pld_table0	= 0x11,
+		.reg_pld_table1	= 0x12,
+		.reg_pld_table2	= 0x13,
+		.reg_pld_table3	= 0x14,
+		.reg_pld_table4	= 0x15,
+		.reg_advance	= 0xad,
+	},
+	.ngpios	= 16,
+	.pins = sx150x_16_pins,
+	.npins  = 16, /* oscio not available */
+};
+
 static s32 sx150x_i2c_write(struct i2c_client *client, u8 reg, u8 val)
 {
 	s32 err = i2c_smbus_write_byte_data(client, reg, val);
@@ -858,6 +881,7 @@ static const struct i2c_device_id sx150x_id[] = {
 	{"sx1509q", (kernel_ulong_t) &sx1509q_device_data },
 	{"sx1506q", (kernel_ulong_t) &sx1506q_device_data },
 	{"sx1502q", (kernel_ulong_t) &sx1502q_device_data },
+	{"sx1503q", (kernel_ulong_t) &sx1503q_device_data },
 	{}
 };
 
-- 
2.5.5

[PATCH 03/14] pinctrl-sx150x: Replace magic number in sx150x_init_hw

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index e0f52e4..f4481fb 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -42,6 +42,9 @@ enum {
 	SX150X_456,
 	SX150X_789,
 };
+enum {
+	SX150X_789_REG_MISC_AUTOCLEAR_OFF = 1 << 0,
+};
 
 struct sx150x_123_pri {
 	u8 reg_pld_mode;
@@ -925,7 +928,7 @@ static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
 	if (pctl->data->model == SX150X_789)
 		err = sx150x_i2c_write(pctl->client,
 				pctl->data->pri.x789.reg_misc,
-				0x01);
+				SX150X_789_REG_MISC_AUTOCLEAR_OFF);
 	else if (pctl->data->model == SX150X_456)
 		err = sx150x_i2c_write(pctl->client,
 				pctl->data->pri.x456.reg_advance,
-- 
2.5.5

[PATCH 04/14] pinctrl-sx150x: Fix incorrect constant in sx150x_init_hw

[Andrey Smirnov]

According to the datasheet for SX1504/5/6, RegAdvanced's
"Autoclear NINT" bit that turns the feature when set and disables it
when cleared, so writing 0x04 to the register will have the opposite
from desirable effect.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index f4481fb..a38c8fc 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -932,7 +932,7 @@ static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
 	else if (pctl->data->model == SX150X_456)
 		err = sx150x_i2c_write(pctl->client,
 				pctl->data->pri.x456.reg_advance,
-				0x04);
+				0x00);
 	else
 		err = sx150x_i2c_write(pctl->client,
 				pctl->data->pri.x123.reg_advance,
-- 
2.5.5

[PATCH 05/14] pinctrl-sx150x: Move some code out of sx150x_init_hw

[Andrey Smirnov]

Move the code configuring explicit IRQ acking into a standalone function
to declutter sx150x_init_hw a bit and make that code somewhat less
repetitious.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index a38c8fc..4283504 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -914,6 +914,31 @@ static int sx150x_reset(struct sx150x_pinctrl *pctl)
 	return err;
 }
 
+static int sx150x_init_misc(struct sx150x_pinctrl *pctl)
+{
+	u8 reg, value;
+
+	switch (pctl->data->model) {
+	case SX150X_789:
+		reg   = pctl->data->pri.x789.reg_misc;
+		value = SX150X_789_REG_MISC_AUTOCLEAR_OFF;
+		break;
+	case SX150X_456:
+		reg   = pctl->data->pri.x456.reg_advance;
+		value = 0x00;
+		break;
+	case SX150X_123:
+		reg   = pctl->data->pri.x123.reg_advance;
+		value = 0x00;
+		break;
+	default:
+		WARN(1, "Unknown chip model %d\n", pctl->data->model);
+		return -EINVAL;
+	}
+
+	return sx150x_i2c_write(pctl->client, reg, value);
+}
+
 static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
 {
 	int err;
@@ -925,18 +950,7 @@ static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
 			return err;
 	}
 
-	if (pctl->data->model == SX150X_789)
-		err = sx150x_i2c_write(pctl->client,
-				pctl->data->pri.x789.reg_misc,
-				SX150X_789_REG_MISC_AUTOCLEAR_OFF);
-	else if (pctl->data->model == SX150X_456)
-		err = sx150x_i2c_write(pctl->client,
-				pctl->data->pri.x456.reg_advance,
-				0x00);
-	else
-		err = sx150x_i2c_write(pctl->client,
-				pctl->data->pri.x123.reg_advance,
-				0x00);
+	err = sx150x_init_misc(pctl);
 	if (err < 0)
 		return err;
 
-- 
2.5.5

[PATCH 06/14] pinctrl-sx150x: Improve sx150x_init_misc for SX1504/5/6

[Andrey Smirnov]

For Sx1504/5/6 only SX1506 has RegAdvanced, so put some code in place to
account for that.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 4283504..9063424 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -926,6 +926,13 @@ static int sx150x_init_misc(struct sx150x_pinctrl *pctl)
 	case SX150X_456:
 		reg   = pctl->data->pri.x456.reg_advance;
 		value = 0x00;
+
+		/*
+		 * Only SX1506 has RegAdvanced, SX1504/5 are expected
+		 * to initialize this offset to zero
+		 */
+		if (!reg)
+			return 0;
 		break;
 	case SX150X_123:
 		reg   = pctl->data->pri.x123.reg_advance;
-- 
2.5.5

[PATCH 07/14] pinctrl-sx150x: Convert driver to use regmap API

[Andrey Smirnov]

To allow for future code simplification

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 102 +++++++++++++++++++++------------------
 1 file changed, 56 insertions(+), 46 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 9063424..0e7c5fb 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -18,6 +18,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/regmap.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -44,6 +45,7 @@ enum {
 };
 enum {
 	SX150X_789_REG_MISC_AUTOCLEAR_OFF = 1 << 0,
+	SX150X_MAX_REGISTER = 0xad,
 };
 
 struct sx150x_123_pri {
@@ -101,6 +103,7 @@ struct sx150x_pinctrl {
 	struct pinctrl_desc pinctrl_desc;
 	struct gpio_chip gpio;
 	struct irq_chip irq_chip;
+	struct regmap *regmap;
 	struct {
 		int update;
 		u32 sense;
@@ -255,30 +258,6 @@ static const struct sx150x_device_data sx1503q_device_data = {
 	.npins  = 16, /* oscio not available */
 };
 
-static s32 sx150x_i2c_write(struct i2c_client *client, u8 reg, u8 val)
-{
-	s32 err = i2c_smbus_write_byte_data(client, reg, val);
-
-	if (err < 0)
-		dev_warn(&client->dev,
-			"i2c write fail: can't write %02x to %02x: %d\n",
-			val, reg, err);
-	return err;
-}
-
-static s32 sx150x_i2c_read(struct i2c_client *client, u8 reg, u8 *val)
-{
-	s32 err = i2c_smbus_read_byte_data(client, reg);
-
-	if (err >= 0)
-		*val = err;
-	else
-		dev_warn(&client->dev,
-			"i2c read fail: can't read from %02x: %d\n",
-			reg, err);
-	return err;
-}
-
 /*
  * These utility functions solve the common problem of locating and setting
  * configuration bits.  Configuration bits are grouped into registers
@@ -311,30 +290,32 @@ static int sx150x_write_cfg(struct i2c_client *client,
 			    u8 offset, u8 width, u8 reg, u8 val)
 {
 	u8  mask;
-	u8  data;
+	unsigned int data;
 	u8  shift;
 	int err;
+	struct sx150x_pinctrl *pctl = i2c_get_clientdata(client);
 
 	sx150x_find_cfg(offset, width, &reg, &mask, &shift);
-	err = sx150x_i2c_read(client, reg, &data);
+	err = regmap_read(pctl->regmap, reg, &data);
 	if (err < 0)
 		return err;
 
 	data &= ~mask;
 	data |= (val << shift) & mask;
-	return sx150x_i2c_write(client, reg, data);
+	return regmap_write(pctl->regmap, reg, data);
 }
 
 static int sx150x_read_cfg(struct i2c_client *client,
 			   u8 offset, u8 width, u8 reg)
 {
 	u8  mask;
-	u8  data;
+	unsigned int data;
 	u8  shift;
 	int err;
+	struct sx150x_pinctrl *pctl = i2c_get_clientdata(client);
 
 	sx150x_find_cfg(offset, width, &reg, &mask, &shift);
-	err = sx150x_i2c_read(client, reg, &data);
+	err = regmap_read(pctl->regmap, reg, &data);
 	if (err < 0)
 		return err;
 
@@ -462,11 +443,10 @@ static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
 
 	if (sx150x_pin_is_oscio(pctl, offset)) {
-
 		mutex_lock(&pctl->lock);
-		sx150x_i2c_write(pctl->client,
-				       pctl->data->pri.x789.reg_clock,
-				       (value ? 0x1f : 0x10));
+		regmap_write(pctl->regmap,
+			     pctl->data->pri.x789.reg_clock,
+			     (value ? 0x1f : 0x10));
 		mutex_unlock(&pctl->lock);
 	} else {
 		mutex_lock(&pctl->lock);
@@ -566,19 +546,19 @@ static irqreturn_t sx150x_irq_thread_fn(int irq, void *dev_id)
 	unsigned int sub_irq;
 	unsigned int n;
 	s32 err;
-	u8 val;
+	unsigned int val;
 	int i;
 
 	for (i = (pctl->data->ngpios / 8) - 1; i >= 0; --i) {
-		err = sx150x_i2c_read(pctl->client,
-				      pctl->data->reg_irq_src - i,
-				      &val);
+		err = regmap_read(pctl->regmap,
+				  pctl->data->reg_irq_src - i,
+				  &val);
 		if (err < 0)
 			continue;
 
-		err = sx150x_i2c_write(pctl->client,
-				       pctl->data->reg_irq_src - i,
-				       val);
+		err = regmap_write(pctl->regmap,
+				   pctl->data->reg_irq_src - i,
+				   val);
 		if (err < 0)
 			continue;
 
@@ -649,15 +629,15 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 	u32 arg;
 
 	if (sx150x_pin_is_oscio(pctl, pin)) {
-		u8 data;
+		unsigned int data;
 
 		switch (param) {
 		case PIN_CONFIG_DRIVE_PUSH_PULL:
 		case PIN_CONFIG_OUTPUT:
 			mutex_lock(&pctl->lock);
-			ret = sx150x_i2c_read(pctl->client,
-					pctl->data->pri.x789.reg_clock,
-					&data);
+			ret = regmap_read(pctl->regmap,
+					  pctl->data->pri.x789.reg_clock,
+					  &data);
 			mutex_unlock(&pctl->lock);
 
 			if (ret < 0)
@@ -894,7 +874,7 @@ static int sx150x_init_io(struct sx150x_pinctrl *pctl, u8 base, u16 cfg)
 	unsigned int n;
 
 	for (n = 0; err >= 0 && n < (pctl->data->ngpios / 8); ++n)
-		err = sx150x_i2c_write(pctl->client, base - n, cfg >> (n * 8));
+		err = regmap_write(pctl->regmap, base - n, cfg >> (n * 8));
 	return err;
 }
 
@@ -943,7 +923,7 @@ static int sx150x_init_misc(struct sx150x_pinctrl *pctl)
 		return -EINVAL;
 	}
 
-	return sx150x_i2c_write(pctl->client, reg, value);
+	return i2c_smbus_write_byte_data(pctl->client, reg, value);
 }
 
 static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
@@ -987,6 +967,26 @@ static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
 	return 0;
 }
 
+static bool sx150x_reg_volatile(struct device *dev, unsigned int reg)
+{
+	struct sx150x_pinctrl *pctl = i2c_get_clientdata(to_i2c_client(dev));
+
+	return reg == pctl->data->reg_irq_src     ||
+	       reg == pctl->data->reg_irq_src - 1 ||
+	       reg == pctl->data->reg_data        ||
+	       reg == pctl->data->reg_data - 1;
+}
+
+const struct regmap_config sx150x_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.cache_type = REGCACHE_RBTREE,
+
+	.max_register = SX150X_MAX_REGISTER,
+	.volatile_reg = sx150x_reg_volatile,
+};
+
 static int sx150x_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -1006,10 +1006,20 @@ static int sx150x_probe(struct i2c_client *client,
 	if (!pctl)
 		return -ENOMEM;
 
+	i2c_set_clientdata(client, pctl);
+
 	pctl->dev = dev;
 	pctl->client = client;
 	pctl->data = (void *)id->driver_data;
 
+	pctl->regmap = devm_regmap_init_i2c(client, &sx150x_regmap_config);
+	if (IS_ERR(pctl->regmap)) {
+		ret = PTR_ERR(pctl->regmap);
+		dev_err(dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
 	mutex_init(&pctl->lock);
 
 	ret = sx150x_init_hw(pctl);
-- 
2.5.5

[PATCH 08/14] pinctrl-sx150x: Replace sx150x_*_cfg by means of regmap API

[Andrey Smirnov]

The difference between 8 and 16 pin GPIO expanders can be accomodated by
the means of regmap API without resorting to usaing driver-specific
read/write accessors. This change, IMHO, brings the following benefits:

	- Replaces driver's idiosyncratic way of dealing with
	  mult-register fields with regmap API, which, hopefuly,
	  makes the code a bit easier for a new reader to understand

	- Removes various multi-read for-loop register read logic
	  from various places in the code and puts it in a signle
	  place

	- Removes ad-hoc IRQ register caching code in
	  sx150x_irq_bus_sync_unlock, since that functionality is
	  provided by regmap

Besided aforementioned benefits this change also implements necessary
RegSense byte swap necessary for SX1503 and SX1506 variants of the chip.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 527 +++++++++++++++++++++------------------
 1 file changed, 284 insertions(+), 243 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 0e7c5fb..af0fc07 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -105,11 +105,8 @@ struct sx150x_pinctrl {
 	struct irq_chip irq_chip;
 	struct regmap *regmap;
 	struct {
-		int update;
 		u32 sense;
 		u32 masked;
-		u32 dev_sense;
-		u32 dev_masked;
 	} irq;
 	struct mutex lock;
 	const struct sx150x_device_data *data;
@@ -170,16 +167,16 @@ static const struct sx150x_device_data sx1508q_device_data = {
 
 static const struct sx150x_device_data sx1509q_device_data = {
 	.model = SX150X_789,
-	.reg_pullup	= 0x07,
-	.reg_pulldn	= 0x09,
-	.reg_dir	= 0x0f,
-	.reg_data	= 0x11,
-	.reg_irq_mask	= 0x13,
-	.reg_irq_src	= 0x19,
-	.reg_sense	= 0x17,
+	.reg_pullup	= 0x06,
+	.reg_pulldn	= 0x08,
+	.reg_dir	= 0x0e,
+	.reg_data	= 0x10,
+	.reg_irq_mask	= 0x12,
+	.reg_irq_src	= 0x18,
+	.reg_sense	= 0x14,
 	.pri.x789 = {
-		.reg_drain	= 0x0b,
-		.reg_polarity	= 0x0d,
+		.reg_drain	= 0x0a,
+		.reg_polarity	= 0x0c,
 		.reg_clock	= 0x1e,
 		.reg_misc	= 0x1f,
 		.reg_reset	= 0x7d,
@@ -191,20 +188,20 @@ static const struct sx150x_device_data sx1509q_device_data = {
 
 static const struct sx150x_device_data sx1506q_device_data = {
 	.model = SX150X_456,
-	.reg_pullup	= 0x05,
-	.reg_pulldn	= 0x07,
-	.reg_dir	= 0x03,
-	.reg_data	= 0x01,
-	.reg_irq_mask	= 0x09,
-	.reg_irq_src	= 0x0f,
-	.reg_sense	= 0x0d,
+	.reg_pullup	= 0x04,
+	.reg_pulldn	= 0x06,
+	.reg_dir	= 0x02,
+	.reg_data	= 0x00,
+	.reg_irq_mask	= 0x08,
+	.reg_irq_src	= 0x0e,
+	.reg_sense	= 0x0a,
 	.pri.x456 = {
-		.reg_pld_mode	= 0x21,
-		.reg_pld_table0	= 0x23,
-		.reg_pld_table1	= 0x25,
-		.reg_pld_table2	= 0x27,
-		.reg_pld_table3	= 0x29,
-		.reg_pld_table4	= 0x2b,
+		.reg_pld_mode	= 0x20,
+		.reg_pld_table0	= 0x22,
+		.reg_pld_table1	= 0x24,
+		.reg_pld_table2	= 0x26,
+		.reg_pld_table3	= 0x28,
+		.reg_pld_table4	= 0x2a,
 		.reg_advance	= 0xad,
 	},
 	.ngpios	= 16,
@@ -237,20 +234,20 @@ static const struct sx150x_device_data sx1502q_device_data = {
 
 static const struct sx150x_device_data sx1503q_device_data = {
 	.model = SX150X_123,
-	.reg_pullup	= 0x05,
-	.reg_pulldn	= 0x07,
-	.reg_dir	= 0x03,
-	.reg_data	= 0x01,
-	.reg_irq_mask	= 0x09,
-	.reg_irq_src	= 0x0f,
-	.reg_sense	= 0x07,
+	.reg_pullup	= 0x04,
+	.reg_pulldn	= 0x06,
+	.reg_dir	= 0x02,
+	.reg_data	= 0x00,
+	.reg_irq_mask	= 0x08,
+	.reg_irq_src	= 0x0e,
+	.reg_sense	= 0x0a,
 	.pri.x123 = {
-		.reg_pld_mode	= 0x10,
-		.reg_pld_table0	= 0x11,
-		.reg_pld_table1	= 0x12,
-		.reg_pld_table2	= 0x13,
-		.reg_pld_table3	= 0x14,
-		.reg_pld_table4	= 0x15,
+		.reg_pld_mode	= 0x20,
+		.reg_pld_table0	= 0x22,
+		.reg_pld_table1	= 0x24,
+		.reg_pld_table2	= 0x26,
+		.reg_pld_table3	= 0x28,
+		.reg_pld_table4	= 0x2a,
 		.reg_advance	= 0xad,
 	},
 	.ngpios	= 16,
@@ -258,70 +255,6 @@ static const struct sx150x_device_data sx1503q_device_data = {
 	.npins  = 16, /* oscio not available */
 };
 
-/*
- * These utility functions solve the common problem of locating and setting
- * configuration bits.  Configuration bits are grouped into registers
- * whose indexes increase downwards.  For example, with eight-bit registers,
- * sixteen gpios would have their config bits grouped in the following order:
- * REGISTER N-1 [ f e d c b a 9 8 ]
- *          N   [ 7 6 5 4 3 2 1 0 ]
- *
- * For multi-bit configurations, the pattern gets wider:
- * REGISTER N-3 [ f f e e d d c c ]
- *          N-2 [ b b a a 9 9 8 8 ]
- *          N-1 [ 7 7 6 6 5 5 4 4 ]
- *          N   [ 3 3 2 2 1 1 0 0 ]
- *
- * Given the address of the starting register 'N', the index of the gpio
- * whose configuration we seek to change, and the width in bits of that
- * configuration, these functions allow us to locate the correct
- * register and mask the correct bits.
- */
-static inline void sx150x_find_cfg(u8 offset, u8 width,
-				   u8 *reg, u8 *mask, u8 *shift)
-{
-	*reg   -= offset * width / 8;
-	*mask   = (1 << width) - 1;
-	*shift  = (offset * width) % 8;
-	*mask <<= *shift;
-}
-
-static int sx150x_write_cfg(struct i2c_client *client,
-			    u8 offset, u8 width, u8 reg, u8 val)
-{
-	u8  mask;
-	unsigned int data;
-	u8  shift;
-	int err;
-	struct sx150x_pinctrl *pctl = i2c_get_clientdata(client);
-
-	sx150x_find_cfg(offset, width, &reg, &mask, &shift);
-	err = regmap_read(pctl->regmap, reg, &data);
-	if (err < 0)
-		return err;
-
-	data &= ~mask;
-	data |= (val << shift) & mask;
-	return regmap_write(pctl->regmap, reg, data);
-}
-
-static int sx150x_read_cfg(struct i2c_client *client,
-			   u8 offset, u8 width, u8 reg)
-{
-	u8  mask;
-	unsigned int data;
-	u8  shift;
-	int err;
-	struct sx150x_pinctrl *pctl = i2c_get_clientdata(client);
-
-	sx150x_find_cfg(offset, width, &reg, &mask, &shift);
-	err = regmap_read(pctl->regmap, reg, &data);
-	if (err < 0)
-		return err;
-
-	return (data & mask);
-}
-
 static int sx150x_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	return 0;
@@ -367,31 +300,33 @@ static int sx150x_gpio_get_direction(struct gpio_chip *chip,
 				      unsigned int offset)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
-	int status;
+	unsigned int value;
+	int ret;
 
 	if (sx150x_pin_is_oscio(pctl, offset))
 		return false;
 
-	status = sx150x_read_cfg(pctl->client, offset, 1, pctl->data->reg_dir);
-	if (status >= 0)
-		status = !!status;
+	ret = regmap_read(pctl->regmap, pctl->data->reg_dir, &value);
+	if (ret < 0)
+		return ret;
 
-	return status;
+	return !!(value & BIT(offset));
 }
 
 static int sx150x_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
-	int status;
+	unsigned int value;
+	int ret;
 
 	if (sx150x_pin_is_oscio(pctl, offset))
 		return -EINVAL;
 
-	status = sx150x_read_cfg(pctl->client, offset, 1, pctl->data->reg_data);
-	if (status >= 0)
-		status = !!status;
+	ret = regmap_read(pctl->regmap, pctl->data->reg_data, &value);
+	if (ret < 0)
+		return ret;
 
-	return status;
+	return !!(value & BIT(offset));
 }
 
 static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
@@ -408,9 +343,9 @@ static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
 			return 0;
 
 		mutex_lock(&pctl->lock);
-		ret = sx150x_write_cfg(pctl->client, offset, 1,
-				       pctl->data->pri.x789.reg_drain,
-				       0);
+		ret = regmap_write_bits(pctl->regmap,
+					pctl->data->pri.x789.reg_drain,
+					BIT(offset), 0);
 		mutex_unlock(&pctl->lock);
 		if (ret < 0)
 			return ret;
@@ -422,9 +357,9 @@ static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
 			return -ENOTSUPP;
 
 		mutex_lock(&pctl->lock);
-		ret = sx150x_write_cfg(pctl->client, offset, 1,
-				       pctl->data->pri.x789.reg_drain,
-				       1);
+		ret = regmap_write_bits(pctl->regmap,
+					pctl->data->pri.x789.reg_drain,
+					BIT(offset), BIT(offset));
 		mutex_unlock(&pctl->lock);
 		if (ret < 0)
 			return ret;
@@ -437,6 +372,13 @@ static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
 	return 0;
 }
 
+static int __sx150x_gpio_set(struct sx150x_pinctrl *pctl, unsigned int offset,
+			     int value)
+{
+	return regmap_write_bits(pctl->regmap, pctl->data->reg_data,
+				 BIT(offset), value ? BIT(offset) : 0);
+}
+
 static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 			       int value)
 {
@@ -450,9 +392,7 @@ static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 		mutex_unlock(&pctl->lock);
 	} else {
 		mutex_lock(&pctl->lock);
-		sx150x_write_cfg(pctl->client, offset, 1,
-				       pctl->data->reg_data,
-				       (value ? 1 : 0));
+		__sx150x_gpio_set(pctl, offset, value);
 		mutex_unlock(&pctl->lock);
 	}
 }
@@ -467,8 +407,9 @@ static int sx150x_gpio_direction_input(struct gpio_chip *chip,
 		return -EINVAL;
 
 	mutex_lock(&pctl->lock);
-	ret = sx150x_write_cfg(pctl->client, offset, 1,
-				pctl->data->reg_dir, 1);
+	ret = regmap_write_bits(pctl->regmap,
+				pctl->data->reg_dir,
+				BIT(offset), BIT(offset));
 	mutex_unlock(&pctl->lock);
 
 	return ret;
@@ -486,12 +427,11 @@ static int sx150x_gpio_direction_output(struct gpio_chip *chip,
 	}
 
 	mutex_lock(&pctl->lock);
-	status = sx150x_write_cfg(pctl->client, offset, 1,
-				  pctl->data->reg_data,
-				  (value ? 1 : 0));
+	status = __sx150x_gpio_set(pctl, offset, value);
 	if (status >= 0)
-		status = sx150x_write_cfg(pctl->client, offset, 1,
-					  pctl->data->reg_dir, 0);
+		status = regmap_write_bits(pctl->regmap,
+					   pctl->data->reg_dir,
+					   BIT(offset), 0);
 	mutex_unlock(&pctl->lock);
 
 	return status;
@@ -503,8 +443,7 @@ static void sx150x_irq_mask(struct irq_data *d)
 			gpiochip_get_data(irq_data_get_irq_chip_data(d));
 	unsigned int n = d->hwirq;
 
-	pctl->irq.masked |= (1 << n);
-	pctl->irq.update = n;
+	pctl->irq.masked |= BIT(n);
 }
 
 static void sx150x_irq_unmask(struct irq_data *d)
@@ -513,8 +452,7 @@ static void sx150x_irq_unmask(struct irq_data *d)
 			gpiochip_get_data(irq_data_get_irq_chip_data(d));
 	unsigned int n = d->hwirq;
 
-	pctl->irq.masked &= ~(1 << n);
-	pctl->irq.update = n;
+	pctl->irq.masked &= ~BIT(n);
 }
 
 static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
@@ -535,7 +473,6 @@ static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
 
 	pctl->irq.sense &= ~(3UL << (n * 2));
 	pctl->irq.sense |= val << (n * 2);
-	pctl->irq.update = n;
 	return 0;
 }
 
@@ -547,29 +484,20 @@ static irqreturn_t sx150x_irq_thread_fn(int irq, void *dev_id)
 	unsigned int n;
 	s32 err;
 	unsigned int val;
-	int i;
 
-	for (i = (pctl->data->ngpios / 8) - 1; i >= 0; --i) {
-		err = regmap_read(pctl->regmap,
-				  pctl->data->reg_irq_src - i,
-				  &val);
-		if (err < 0)
-			continue;
+	err = regmap_read(pctl->regmap, pctl->data->reg_irq_src, &val);
+	if (err < 0)
+		return IRQ_NONE;
 
-		err = regmap_write(pctl->regmap,
-				   pctl->data->reg_irq_src - i,
-				   val);
-		if (err < 0)
-			continue;
-
-		for (n = 0; n < 8; ++n) {
-			if (val & (1 << n)) {
-				sub_irq = irq_find_mapping(
-						pctl->gpio.irqdomain,
-						(i * 8) + n);
-				handle_nested_irq(sub_irq);
-				++nhandled;
-			}
+	err = regmap_write(pctl->regmap, pctl->data->reg_irq_src, val);
+	if (err < 0)
+		return IRQ_NONE;
+
+	for (n = 0; n < pctl->data->ngpios; ++n) {
+		if (val & BIT(n)) {
+			sub_irq = irq_find_mapping(pctl->gpio.irqdomain, n);
+			handle_nested_irq(sub_irq);
+			++nhandled;
 		}
 	}
 
@@ -588,35 +516,9 @@ static void sx150x_irq_bus_sync_unlock(struct irq_data *d)
 {
 	struct sx150x_pinctrl *pctl =
 			gpiochip_get_data(irq_data_get_irq_chip_data(d));
-	unsigned int n;
-
-	if (pctl->irq.update < 0)
-		goto out;
 
-	n = pctl->irq.update;
-	pctl->irq.update = -1;
-
-	/* Avoid updates if nothing changed */
-	if (pctl->irq.dev_sense == pctl->irq.sense &&
-	    pctl->irq.dev_masked == pctl->irq.masked)
-		goto out;
-
-	pctl->irq.dev_sense = pctl->irq.sense;
-	pctl->irq.dev_masked = pctl->irq.masked;
-
-	if (pctl->irq.masked & (1 << n)) {
-		sx150x_write_cfg(pctl->client, n, 1,
-				 pctl->data->reg_irq_mask, 1);
-		sx150x_write_cfg(pctl->client, n, 2,
-				 pctl->data->reg_sense, 0);
-	} else {
-		sx150x_write_cfg(pctl->client, n, 1,
-				 pctl->data->reg_irq_mask, 0);
-		sx150x_write_cfg(pctl->client, n, 2,
-				 pctl->data->reg_sense,
-				 pctl->irq.sense >> (n * 2));
-	}
-out:
+	regmap_write(pctl->regmap, pctl->data->reg_irq_mask, pctl->irq.masked);
+	regmap_write(pctl->regmap, pctl->data->reg_sense, pctl->irq.sense);
 	mutex_unlock(&pctl->lock);
 }
 
@@ -627,10 +529,9 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 	unsigned int param = pinconf_to_config_param(*config);
 	int ret;
 	u32 arg;
+	unsigned int data;
 
 	if (sx150x_pin_is_oscio(pctl, pin)) {
-		unsigned int data;
-
 		switch (param) {
 		case PIN_CONFIG_DRIVE_PUSH_PULL:
 		case PIN_CONFIG_OUTPUT:
@@ -665,8 +566,10 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 	switch (param) {
 	case PIN_CONFIG_BIAS_PULL_DOWN:
 		mutex_lock(&pctl->lock);
-		ret = sx150x_read_cfg(pctl->client, pin, 1,
-				      pctl->data->reg_pulldn);
+		ret = regmap_read(pctl->regmap,
+				  pctl->data->reg_pulldn,
+				  &data);
+		data &= BIT(pin);
 		mutex_unlock(&pctl->lock);
 
 		if (ret < 0)
@@ -680,8 +583,10 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 
 	case PIN_CONFIG_BIAS_PULL_UP:
 		mutex_lock(&pctl->lock);
-		ret = sx150x_read_cfg(pctl->client, pin, 1,
-				      pctl->data->reg_pullup);
+		ret = regmap_read(pctl->regmap,
+				  pctl->data->reg_pullup,
+				  &data);
+		data &= BIT(pin);
 		mutex_unlock(&pctl->lock);
 
 		if (ret < 0)
@@ -698,14 +603,16 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 			return -ENOTSUPP;
 
 		mutex_lock(&pctl->lock);
-		ret = sx150x_read_cfg(pctl->client, pin, 1,
-				      pctl->data->pri.x789.reg_drain);
+		ret = regmap_read(pctl->regmap,
+				  pctl->data->pri.x789.reg_drain,
+				  &data);
+		data &= BIT(pin);
 		mutex_unlock(&pctl->lock);
 
 		if (ret < 0)
 			return ret;
 
-		if (!ret)
+		if (!data)
 			return -EINVAL;
 
 		arg = 1;
@@ -716,14 +623,16 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 			arg = true;
 		else {
 			mutex_lock(&pctl->lock);
-			ret = sx150x_read_cfg(pctl->client, pin, 1,
-					      pctl->data->pri.x789.reg_drain);
+			ret = regmap_read(pctl->regmap,
+					  pctl->data->pri.x789.reg_drain,
+					  &data);
+			data &= BIT(pin);
 			mutex_unlock(&pctl->lock);
 
 			if (ret < 0)
 				return ret;
 
-			if (ret)
+			if (data)
 				return -EINVAL;
 
 			arg = 1;
@@ -784,15 +693,17 @@ static int sx150x_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 		case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
 		case PIN_CONFIG_BIAS_DISABLE:
 			mutex_lock(&pctl->lock);
-			ret = sx150x_write_cfg(pctl->client, pin, 1,
-					       pctl->data->reg_pulldn, 0);
+			ret = regmap_write_bits(pctl->regmap,
+						pctl->data->reg_pulldn,
+						BIT(pin), 0);
 			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
 
 			mutex_lock(&pctl->lock);
-			ret = sx150x_write_cfg(pctl->client, pin, 1,
-					       pctl->data->reg_pullup, 0);
+			ret = regmap_write_bits(pctl->regmap,
+						pctl->data->reg_pullup,
+						BIT(pin), 0);
 			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
@@ -801,9 +712,9 @@ static int sx150x_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 
 		case PIN_CONFIG_BIAS_PULL_UP:
 			mutex_lock(&pctl->lock);
-			ret = sx150x_write_cfg(pctl->client, pin, 1,
-					       pctl->data->reg_pullup,
-					       1);
+			ret = regmap_write_bits(pctl->regmap,
+						pctl->data->reg_pullup,
+						BIT(pin), BIT(pin));
 			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
@@ -812,9 +723,9 @@ static int sx150x_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 
 		case PIN_CONFIG_BIAS_PULL_DOWN:
 			mutex_lock(&pctl->lock);
-			ret = sx150x_write_cfg(pctl->client, pin, 1,
-					       pctl->data->reg_pulldn,
-					       1);
+			ret = regmap_write_bits(pctl->regmap,
+						pctl->data->reg_pulldn,
+						BIT(pin), BIT(pin));
 			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
@@ -868,16 +779,6 @@ static const struct i2c_device_id sx150x_id[] = {
 	{}
 };
 
-static int sx150x_init_io(struct sx150x_pinctrl *pctl, u8 base, u16 cfg)
-{
-	int err = 0;
-	unsigned int n;
-
-	for (n = 0; err >= 0 && n < (pctl->data->ngpios / 8); ++n)
-		err = regmap_write(pctl->regmap, base - n, cfg >> (n * 8));
-	return err;
-}
-
 static int sx150x_reset(struct sx150x_pinctrl *pctl)
 {
 	int err;
@@ -923,11 +824,16 @@ static int sx150x_init_misc(struct sx150x_pinctrl *pctl)
 		return -EINVAL;
 	}
 
-	return i2c_smbus_write_byte_data(pctl->client, reg, value);
+	return regmap_write(pctl->regmap, reg, value);
 }
 
 static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
 {
+	const u8 reg[] = {
+		[SX150X_789] = pctl->data->pri.x789.reg_polarity,
+		[SX150X_456] = pctl->data->pri.x456.reg_pld_mode,
+		[SX150X_123] = pctl->data->pri.x123.reg_pld_mode,
+	};
 	int err;
 
 	if (pctl->data->model == SX150X_789 &&
@@ -942,28 +848,165 @@ static int sx150x_init_hw(struct sx150x_pinctrl *pctl)
 		return err;
 
 	/* Set all pins to work in normal mode */
-	if (pctl->data->model == SX150X_789) {
-		err = sx150x_init_io(pctl,
-				pctl->data->pri.x789.reg_polarity,
-				0);
-		if (err < 0)
-			return err;
-	} else if (pctl->data->model == SX150X_456) {
-		/* Set all pins to work in normal mode */
-		err = sx150x_init_io(pctl,
-				pctl->data->pri.x456.reg_pld_mode,
-				0);
-		if (err < 0)
-			return err;
+	return regmap_write(pctl->regmap, reg[pctl->data->model], 0);
+}
+
+static int sx150x_regmap_reg_width(struct sx150x_pinctrl *pctl,
+				   unsigned int reg)
+{
+	const struct sx150x_device_data *data = pctl->data;
+
+	if (reg == data->reg_sense) {
+		/*
+		 * RegSense packs two bits of configuration per GPIO,
+		 * so we'd need to read twice as many bits as there
+		 * are GPIO in our chip
+		 */
+		return 2 * data->ngpios;
+	} else if ((data->model == SX150X_789 &&
+		    (reg == data->pri.x789.reg_misc ||
+		     reg == data->pri.x789.reg_clock ||
+		     reg == data->pri.x789.reg_reset))
+		   ||
+		   (data->model == SX150X_123 &&
+		    reg == data->pri.x123.reg_advance)
+		   ||
+		   (data->model == SX150X_456 &&
+		    reg == data->pri.x456.reg_advance)) {
+		return 8;
 	} else {
-		/* Set all pins to work in normal mode */
-		err = sx150x_init_io(pctl,
-				pctl->data->pri.x123.reg_pld_mode,
-				0);
-		if (err < 0)
-			return err;
+		return data->ngpios;
+	}
+}
+
+static unsigned int sx150x_maybe_swizzle(struct sx150x_pinctrl *pctl,
+					 unsigned int reg, unsigned int val)
+{
+	unsigned int a, b;
+	const struct sx150x_device_data *data = pctl->data;
+
+	/*
+	 * Whereas SX1509 presents RegSense in a simple layout as such:
+	 *	reg     [ f f e e d d c c ]
+	 *	reg + 1 [ b b a a 9 9 8 8 ]
+	 *	reg + 2 [ 7 7 6 6 5 5 4 4 ]
+	 *	reg + 3 [ 3 3 2 2 1 1 0 0 ]
+	 *
+	 * SX1503 and SX1506 deviate from that data layout, instead storing
+	 * thier contents as follows:
+	 *
+	 *	reg     [ f f e e d d c c ]
+	 *	reg + 1 [ 7 7 6 6 5 5 4 4 ]
+	 *	reg + 2 [ b b a a 9 9 8 8 ]
+	 *	reg + 3 [ 3 3 2 2 1 1 0 0 ]
+	 *
+	 * so, taking that into account, we swap two
+	 * inner bytes of a 4-byte result
+	 */
+
+	if (reg == data->reg_sense &&
+	    data->ngpios == 16 &&
+	    (data->model == SX150X_123 ||
+	     data->model == SX150X_456)) {
+		a = val & 0x00ff0000;
+		b = val & 0x0000ff00;
+
+		val &= 0xff0000ff;
+		val |= b << 8;
+		val |= a >> 8;
 	}
 
+	return val;
+}
+
+/*
+ * In order to mask the differences between 16 and 8 bit expander
+ * devices we set up a sligthly ficticious regmap that pretends to be
+ * a set of 32-bit (to accomodate RegSenseLow/RegSenseHigh
+ * pair/quartet) registers and transparently reconstructs those
+ * registers via multiple I2C/SMBus reads
+ *
+ * This way the rest of the driver code, interfacing with the chip via
+ * regmap API, can work assuming that each GPIO pin is represented by
+ * a group of bits at an offset proportioan to GPIO number within a
+ * given register.
+ *
+ */
+static int sx150x_regmap_reg_read(void *context, unsigned int reg,
+				  unsigned int *result)
+{
+	int ret, n;
+	struct sx150x_pinctrl *pctl = context;
+	struct i2c_client *i2c = pctl->client;
+	const int width = sx150x_regmap_reg_width(pctl, reg);
+	unsigned int idx, val;
+
+	/*
+	 * There are four potential cases coverd by this function:
+	 *
+	 * 1) 8-pin chip, single configuration bit register
+	 *
+	 *	This is trivial the code below just needs to read:
+	 *		reg  [ 7 6 5 4 3 2 1 0 ]
+	 *
+	 * 2) 8-pin chip, double configuration bit register (RegSense)
+	 *
+	 *	The read will be done as follows:
+	 *		reg      [ 7 7 6 6 5 5 4 4 ]
+	 *		reg + 1  [ 3 3 2 2 1 1 0 0 ]
+	 *
+	 * 3) 16-pin chip, single configuration bit register
+	 *
+	 *	The read will be done as follows:
+	 *		reg     [ f e d c b a 9 8 ]
+	 *		reg + 1 [ 7 6 5 4 3 2 1 0 ]
+	 *
+	 * 4) 16-pin chip, double configuration bit register (RegSense)
+	 *
+	 *	The read will be done as follows:
+	 *		reg     [ f f e e d d c c ]
+	 *		reg + 1 [ b b a a 9 9 8 8 ]
+	 *		reg + 2 [ 7 7 6 6 5 5 4 4 ]
+	 *		reg + 3 [ 3 3 2 2 1 1 0 0 ]
+	 */
+
+	for (n = width, val = 0, idx = reg; n > 0; n -= 8, idx++) {
+		val <<= 8;
+
+		ret = i2c_smbus_read_byte_data(i2c, idx);
+		if (ret < 0)
+			return ret;
+
+		val |= ret;
+	}
+
+	*result = sx150x_maybe_swizzle(pctl, reg, val);
+
+	return 0;
+}
+
+static int sx150x_regmap_reg_write(void *context, unsigned int reg,
+				   unsigned int val)
+{
+	int ret, n;
+	struct sx150x_pinctrl *pctl = context;
+	struct i2c_client *i2c = pctl->client;
+	const int width = sx150x_regmap_reg_width(pctl, reg);
+
+	val = sx150x_maybe_swizzle(pctl, reg, val);
+
+	n = width - 8;
+	do {
+		const u8 byte = (val >> n) & 0xff;
+
+		ret = i2c_smbus_write_byte_data(i2c, reg, byte);
+		if (ret < 0)
+			return ret;
+
+		reg++;
+		n -= 8;
+	} while (n >= 0);
+
 	return 0;
 }
 
@@ -971,18 +1014,18 @@ static bool sx150x_reg_volatile(struct device *dev, unsigned int reg)
 {
 	struct sx150x_pinctrl *pctl = i2c_get_clientdata(to_i2c_client(dev));
 
-	return reg == pctl->data->reg_irq_src     ||
-	       reg == pctl->data->reg_irq_src - 1 ||
-	       reg == pctl->data->reg_data        ||
-	       reg == pctl->data->reg_data - 1;
+	return reg == pctl->data->reg_irq_src || reg == pctl->data->reg_data;
 }
 
 const struct regmap_config sx150x_regmap_config = {
 	.reg_bits = 8,
-	.val_bits = 8,
+	.val_bits = 32,
 
 	.cache_type = REGCACHE_RBTREE,
 
+	.reg_read = sx150x_regmap_reg_read,
+	.reg_write = sx150x_regmap_reg_write,
+
 	.max_register = SX150X_MAX_REGISTER,
 	.volatile_reg = sx150x_reg_volatile,
 };
@@ -1012,7 +1055,8 @@ static int sx150x_probe(struct i2c_client *client,
 	pctl->client = client;
 	pctl->data = (void *)id->driver_data;
 
-	pctl->regmap = devm_regmap_init_i2c(client, &sx150x_regmap_config);
+	pctl->regmap = devm_regmap_init(dev, NULL, pctl,
+					&sx150x_regmap_config);
 	if (IS_ERR(pctl->regmap)) {
 		ret = PTR_ERR(pctl->regmap);
 		dev_err(dev, "Failed to allocate register map: %d\n",
@@ -1058,9 +1102,6 @@ static int sx150x_probe(struct i2c_client *client,
 
 		pctl->irq.masked = ~0;
 		pctl->irq.sense = 0;
-		pctl->irq.dev_masked = ~0;
-		pctl->irq.dev_sense = 0;
-		pctl->irq.update = -1;
 
 		ret = gpiochip_irqchip_add(&pctl->gpio,
 					   &pctl->irq_chip, 0,
-- 
2.5.5

[PATCH 09/14] pinctrl-sx150x: Remove excessive locking

[Andrey Smirnov]

Gpiochip and irqchip aspects of this driver do not access any shared
registers on the chip itself and atomicity of various regmap operations
is ensured by that API's implementation, so there doesn't seem to be a
reason to hold the lock in as many places as it is held now.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 70 +++++++++-------------------------------
 1 file changed, 16 insertions(+), 54 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index af0fc07..8d0fd4b 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -342,13 +342,9 @@ static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
 		    sx150x_pin_is_oscio(pctl, offset))
 			return 0;
 
-		mutex_lock(&pctl->lock);
 		ret = regmap_write_bits(pctl->regmap,
 					pctl->data->pri.x789.reg_drain,
 					BIT(offset), 0);
-		mutex_unlock(&pctl->lock);
-		if (ret < 0)
-			return ret;
 		break;
 
 	case LINE_MODE_OPEN_DRAIN:
@@ -356,20 +352,16 @@ static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
 		    sx150x_pin_is_oscio(pctl, offset))
 			return -ENOTSUPP;
 
-		mutex_lock(&pctl->lock);
 		ret = regmap_write_bits(pctl->regmap,
 					pctl->data->pri.x789.reg_drain,
 					BIT(offset), BIT(offset));
-		mutex_unlock(&pctl->lock);
-		if (ret < 0)
-			return ret;
 		break;
-
 	default:
-		return -ENOTSUPP;
+		ret = -ENOTSUPP;
+		break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int __sx150x_gpio_set(struct sx150x_pinctrl *pctl, unsigned int offset,
@@ -384,57 +376,46 @@ static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
 
-	if (sx150x_pin_is_oscio(pctl, offset)) {
-		mutex_lock(&pctl->lock);
+	if (sx150x_pin_is_oscio(pctl, offset))
 		regmap_write(pctl->regmap,
 			     pctl->data->pri.x789.reg_clock,
 			     (value ? 0x1f : 0x10));
-		mutex_unlock(&pctl->lock);
-	} else {
-		mutex_lock(&pctl->lock);
+	else
 		__sx150x_gpio_set(pctl, offset, value);
-		mutex_unlock(&pctl->lock);
-	}
+
 }
 
 static int sx150x_gpio_direction_input(struct gpio_chip *chip,
 				      unsigned int offset)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
-	int ret;
 
 	if (sx150x_pin_is_oscio(pctl, offset))
 		return -EINVAL;
 
-	mutex_lock(&pctl->lock);
-	ret = regmap_write_bits(pctl->regmap,
-				pctl->data->reg_dir,
-				BIT(offset), BIT(offset));
-	mutex_unlock(&pctl->lock);
-
-	return ret;
+	return regmap_write_bits(pctl->regmap,
+				 pctl->data->reg_dir,
+				 BIT(offset), BIT(offset));
 }
 
 static int sx150x_gpio_direction_output(struct gpio_chip *chip,
 				       unsigned int offset, int value)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
-	int status;
+	int ret;
 
 	if (sx150x_pin_is_oscio(pctl, offset)) {
 		sx150x_gpio_set(chip, offset, value);
 		return 0;
 	}
 
-	mutex_lock(&pctl->lock);
-	status = __sx150x_gpio_set(pctl, offset, value);
-	if (status >= 0)
-		status = regmap_write_bits(pctl->regmap,
-					   pctl->data->reg_dir,
-					   BIT(offset), 0);
-	mutex_unlock(&pctl->lock);
+	ret = __sx150x_gpio_set(pctl, offset, value);
+	if (ret < 0)
+		return ret;
 
-	return status;
+	return regmap_write_bits(pctl->regmap,
+				 pctl->data->reg_dir,
+				 BIT(offset), 0);
 }
 
 static void sx150x_irq_mask(struct irq_data *d)
@@ -535,12 +516,9 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 		switch (param) {
 		case PIN_CONFIG_DRIVE_PUSH_PULL:
 		case PIN_CONFIG_OUTPUT:
-			mutex_lock(&pctl->lock);
 			ret = regmap_read(pctl->regmap,
 					  pctl->data->pri.x789.reg_clock,
 					  &data);
-			mutex_unlock(&pctl->lock);
-
 			if (ret < 0)
 				return ret;
 
@@ -565,12 +543,10 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_PULL_DOWN:
-		mutex_lock(&pctl->lock);
 		ret = regmap_read(pctl->regmap,
 				  pctl->data->reg_pulldn,
 				  &data);
 		data &= BIT(pin);
-		mutex_unlock(&pctl->lock);
 
 		if (ret < 0)
 			return ret;
@@ -582,12 +558,10 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 		break;
 
 	case PIN_CONFIG_BIAS_PULL_UP:
-		mutex_lock(&pctl->lock);
 		ret = regmap_read(pctl->regmap,
 				  pctl->data->reg_pullup,
 				  &data);
 		data &= BIT(pin);
-		mutex_unlock(&pctl->lock);
 
 		if (ret < 0)
 			return ret;
@@ -602,12 +576,10 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 		if (pctl->data->model != SX150X_789)
 			return -ENOTSUPP;
 
-		mutex_lock(&pctl->lock);
 		ret = regmap_read(pctl->regmap,
 				  pctl->data->pri.x789.reg_drain,
 				  &data);
 		data &= BIT(pin);
-		mutex_unlock(&pctl->lock);
 
 		if (ret < 0)
 			return ret;
@@ -622,12 +594,10 @@ static int sx150x_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 		if (pctl->data->model != SX150X_789)
 			arg = true;
 		else {
-			mutex_lock(&pctl->lock);
 			ret = regmap_read(pctl->regmap,
 					  pctl->data->pri.x789.reg_drain,
 					  &data);
 			data &= BIT(pin);
-			mutex_unlock(&pctl->lock);
 
 			if (ret < 0)
 				return ret;
@@ -692,41 +662,33 @@ static int sx150x_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 		switch (param) {
 		case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
 		case PIN_CONFIG_BIAS_DISABLE:
-			mutex_lock(&pctl->lock);
 			ret = regmap_write_bits(pctl->regmap,
 						pctl->data->reg_pulldn,
 						BIT(pin), 0);
-			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
 
-			mutex_lock(&pctl->lock);
 			ret = regmap_write_bits(pctl->regmap,
 						pctl->data->reg_pullup,
 						BIT(pin), 0);
-			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
 
 			break;
 
 		case PIN_CONFIG_BIAS_PULL_UP:
-			mutex_lock(&pctl->lock);
 			ret = regmap_write_bits(pctl->regmap,
 						pctl->data->reg_pullup,
 						BIT(pin), BIT(pin));
-			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
 
 			break;
 
 		case PIN_CONFIG_BIAS_PULL_DOWN:
-			mutex_lock(&pctl->lock);
 			ret = regmap_write_bits(pctl->regmap,
 						pctl->data->reg_pulldn,
 						BIT(pin), BIT(pin));
-			mutex_unlock(&pctl->lock);
 			if (ret < 0)
 				return ret;
 
-- 
2.5.5

[PATCH 10/14] pinctrl-sx150x: Improve oscio GPIO functions

[Andrey Smirnov]

Move actual code that configures oscio pin into a separate function and
use it instead of calling sx150x_gpio_set to avoid calling
sx150x_pin_is_oscio twice and correctly propagte error code in
sx150x_gpio_direction_output.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 8d0fd4b..d2e2b13 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -371,15 +371,21 @@ static int __sx150x_gpio_set(struct sx150x_pinctrl *pctl, unsigned int offset,
 				 BIT(offset), value ? BIT(offset) : 0);
 }
 
+static int sx150x_gpio_oscio_set(struct sx150x_pinctrl *pctl,
+				 int value)
+{
+	return regmap_write(pctl->regmap,
+			    pctl->data->pri.x789.reg_clock,
+			    (value ? 0x1f : 0x10));
+}
+
 static void sx150x_gpio_set(struct gpio_chip *chip, unsigned int offset,
 			       int value)
 {
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
 
 	if (sx150x_pin_is_oscio(pctl, offset))
-		regmap_write(pctl->regmap,
-			     pctl->data->pri.x789.reg_clock,
-			     (value ? 0x1f : 0x10));
+		sx150x_gpio_oscio_set(pctl, value);
 	else
 		__sx150x_gpio_set(pctl, offset, value);
 
@@ -404,10 +410,8 @@ static int sx150x_gpio_direction_output(struct gpio_chip *chip,
 	struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
 	int ret;
 
-	if (sx150x_pin_is_oscio(pctl, offset)) {
-		sx150x_gpio_set(chip, offset, value);
-		return 0;
-	}
+	if (sx150x_pin_is_oscio(pctl, offset))
+		return sx150x_gpio_oscio_set(pctl, value);
 
 	ret = __sx150x_gpio_set(pctl, offset, value);
 	if (ret < 0)
-- 
2.5.5

[PATCH 11/14] pinctrl-sx150x: Simplify interrupt handler

[Andrey Smirnov]

Make use of for_each_set_bit macro and reduce boilerplate code.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index d2e2b13..741981d 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -464,11 +464,9 @@ static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
 static irqreturn_t sx150x_irq_thread_fn(int irq, void *dev_id)
 {
 	struct sx150x_pinctrl *pctl = (struct sx150x_pinctrl *)dev_id;
-	unsigned int nhandled = 0;
-	unsigned int sub_irq;
-	unsigned int n;
-	s32 err;
+	unsigned long n, status;
 	unsigned int val;
+	int err;
 
 	err = regmap_read(pctl->regmap, pctl->data->reg_irq_src, &val);
 	if (err < 0)
@@ -478,15 +476,11 @@ static irqreturn_t sx150x_irq_thread_fn(int irq, void *dev_id)
 	if (err < 0)
 		return IRQ_NONE;
 
-	for (n = 0; n < pctl->data->ngpios; ++n) {
-		if (val & BIT(n)) {
-			sub_irq = irq_find_mapping(pctl->gpio.irqdomain, n);
-			handle_nested_irq(sub_irq);
-			++nhandled;
-		}
-	}
+	status = val;
+	for_each_set_bit(n, &status, pctl->data->ngpios)
+		handle_nested_irq(irq_find_mapping(pctl->gpio.irqdomain, n));
 
-	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
+	return IRQ_HANDLED;
 }
 
 static void sx150x_irq_bus_lock(struct irq_data *d)
-- 
2.5.5

[PATCH 12/14] pinctrl-sx150x: Use handle_bad_irq instead of handle_edge_irq

[Andrey Smirnov]

Althought the function passed as a "handler" during GPIO chip
instantiation is not going to ever be called, specifying handle_edge_irq
there makes for a rather confusing read, both because no "ack" callback
in specified for irqchip and because there's no acking action is
necessary.

Specify handle_bad_irq instead a make a note of the situation. This
commit should be a no-op behaviour wise.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 741981d..31ed7e3 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -1063,9 +1063,20 @@ static int sx150x_probe(struct i2c_client *client,
 		pctl->irq.masked = ~0;
 		pctl->irq.sense = 0;
 
+		/*
+		 * Because sx150x_irq_threaded_fn invokes all of the
+		 * nested interrrupt handlers via handle_nested_irq,
+		 * any "handler" passed to gpiochip_irqchip_add()
+		 * below is going to be ignored, so the choice of the
+		 * function does not matter that much.
+		 *
+		 * We set it to handle_bad_irq to avoid confusion,
+		 * plus it will be instantly noticeable if it is ever
+		 * called (should not happen)
+		 */
 		ret = gpiochip_irqchip_add(&pctl->gpio,
 					   &pctl->irq_chip, 0,
-					   handle_edge_irq, IRQ_TYPE_NONE);
+					   handle_bad_irq, IRQ_TYPE_NONE);
 		if (ret) {
 			dev_err(dev, "could not connect irqchip to gpiochip\n");
 			return ret;
-- 
2.5.5

[PATCH 13/14] pinctrl-sx150x: Remove magic numbers from sx150x_irq_set_type

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 31ed7e3..13afcb9 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -46,6 +46,8 @@ enum {
 enum {
 	SX150X_789_REG_MISC_AUTOCLEAR_OFF = 1 << 0,
 	SX150X_MAX_REGISTER = 0xad,
+	SX150X_IRQ_TYPE_EDGE_RISING = 0x1,
+	SX150X_IRQ_TYPE_EDGE_FALLING = 0x2,
 };
 
 struct sx150x_123_pri {
@@ -440,6 +442,21 @@ static void sx150x_irq_unmask(struct irq_data *d)
 	pctl->irq.masked &= ~BIT(n);
 }
 
+static void sx150x_irq_set_sense(struct sx150x_pinctrl *pctl,
+				 unsigned int line, unsigned int sense)
+{
+	/*
+	 * Every interrupt line is represented by two bits shifted
+	 * proportionally to the line number
+	 */
+	const unsigned int n = line * 2;
+	const unsigned int mask = ~((SX150X_IRQ_TYPE_EDGE_RISING |
+				     SX150X_IRQ_TYPE_EDGE_FALLING) << n);
+
+	pctl->irq.sense &= mask;
+	pctl->irq.sense |= sense << n;
+}
+
 static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
 {
 	struct sx150x_pinctrl *pctl =
@@ -452,12 +469,11 @@ static int sx150x_irq_set_type(struct irq_data *d, unsigned int flow_type)
 	n = d->hwirq;
 
 	if (flow_type & IRQ_TYPE_EDGE_RISING)
-		val |= 0x1;
+		val |= SX150X_IRQ_TYPE_EDGE_RISING;
 	if (flow_type & IRQ_TYPE_EDGE_FALLING)
-		val |= 0x2;
+		val |= SX150X_IRQ_TYPE_EDGE_FALLING;
 
-	pctl->irq.sense &= ~(3UL << (n * 2));
-	pctl->irq.sense |= val << (n * 2);
+	sx150x_irq_set_sense(pctl, n, val);
 	return 0;
 }
 
-- 
2.5.5

[PATCH 14/14] pinctrl-sx150x: Remove magic numbers from sx150x_reset

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 13afcb9..580a6de 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -48,6 +48,8 @@ enum {
 	SX150X_MAX_REGISTER = 0xad,
 	SX150X_IRQ_TYPE_EDGE_RISING = 0x1,
 	SX150X_IRQ_TYPE_EDGE_FALLING = 0x2,
+	SX150X_789_RESET_KEY1 = 0x12,
+	SX150X_789_RESET_KEY2 = 0x34,
 };
 
 struct sx150x_123_pri {
@@ -761,13 +763,13 @@ static int sx150x_reset(struct sx150x_pinctrl *pctl)
 
 	err = i2c_smbus_write_byte_data(pctl->client,
 					pctl->data->pri.x789.reg_reset,
-					0x12);
+					SX150X_789_RESET_KEY1);
 	if (err < 0)
 		return err;
 
 	err = i2c_smbus_write_byte_data(pctl->client,
 					pctl->data->pri.x789.reg_reset,
-					0x34);
+					SX150X_789_RESET_KEY2);
 	return err;
 }
 
-- 
2.5.5

Re: [PATCH 00/14] pinctrl-sx150x: Various bug-fixes and code simplifications

[Neil Armstrong]

On 11/01/2016 04:57 PM, Andrey Smirnov wrote:
> Linus, Neil:
> 
> I've had some help and got my hardware setup modified to enable IRQ
> functionality testing, so ended up looking at the code of SX150x more
> resulting in some code improvements (hopefully) and bugfixes.
> 
> There are many small changes each of which is probably better
> described by corresponding commit's message, however the most
> porminenet changes of the whole patchset are the switch to regmap API
> (patches ## 7,8) and reduction of locking (patch # 9)
> 
> Please let me know what you think.
> 
> Thanks,
> Andrey
> 
> Andrey Smirnov (14):
>   pinctrl-sx150x: Rely on of_modalias_node for OF matching
>   pinctrl-sx150x: Add SX1503 specific data
>   pinctrl-sx150x: Replace magic number in sx150x_init_hw
>   pinctrl-sx150x: Fix incorrect constant in sx150x_init_hw
>   pinctrl-sx150x: Move some code out of sx150x_init_hw
>   pinctrl-sx150x: Improve sx150x_init_misc for SX1504/5/6
>   pinctrl-sx150x: Convert driver to use regmap API
>   pinctrl-sx150x: Replace sx150x_*_cfg by means of regmap API
>   pinctrl-sx150x: Remove excessive locking
>   pinctrl-sx150x: Improve oscio GPIO functions
>   pinctrl-sx150x: Simplify interrupt handler
>   pinctrl-sx150x: Use handle_bad_irq instead of handle_edge_irq
>   pinctrl-sx150x: Remove magic numbers from sx150x_irq_set_type
>   pinctrl-sx150x: Remove magic numbers from sx150x_reset
> 
>  drivers/pinctrl/pinctrl-sx150x.c | 753 +++++++++++++++++++++------------------
>  1 file changed, 416 insertions(+), 337 deletions(-)
> 

Hi Andrey,

This is good, you went faster than me !

Small point, could you add Kconfig dependency on REGMAP ?

I will try out this patchset and hopefully get you a Tested-by in the next few days.

Neil

Re: [PATCH 00/14] pinctrl-sx150x: Various bug-fixes and code simplifications

[Neil Armstrong]

On 11/02/2016 12:01 PM, Neil Armstrong wrote:
> On 11/01/2016 04:57 PM, Andrey Smirnov wrote:
>> Linus, Neil:
>>
>> I've had some help and got my hardware setup modified to enable IRQ
>> functionality testing, so ended up looking at the code of SX150x more
>> resulting in some code improvements (hopefully) and bugfixes.
>>
>> There are many small changes each of which is probably better
>> described by corresponding commit's message, however the most
>> porminenet changes of the whole patchset are the switch to regmap API
>> (patches ## 7,8) and reduction of locking (patch # 9)
>>
>> Please let me know what you think.
>>
>> Thanks,
>> Andrey
>>
>> Andrey Smirnov (14):
>>   pinctrl-sx150x: Rely on of_modalias_node for OF matching
>>   pinctrl-sx150x: Add SX1503 specific data
>>   pinctrl-sx150x: Replace magic number in sx150x_init_hw
>>   pinctrl-sx150x: Fix incorrect constant in sx150x_init_hw
>>   pinctrl-sx150x: Move some code out of sx150x_init_hw
>>   pinctrl-sx150x: Improve sx150x_init_misc for SX1504/5/6
>>   pinctrl-sx150x: Convert driver to use regmap API
>>   pinctrl-sx150x: Replace sx150x_*_cfg by means of regmap API
>>   pinctrl-sx150x: Remove excessive locking
>>   pinctrl-sx150x: Improve oscio GPIO functions
>>   pinctrl-sx150x: Simplify interrupt handler
>>   pinctrl-sx150x: Use handle_bad_irq instead of handle_edge_irq
>>   pinctrl-sx150x: Remove magic numbers from sx150x_irq_set_type
>>   pinctrl-sx150x: Remove magic numbers from sx150x_reset
>>
>>  drivers/pinctrl/pinctrl-sx150x.c | 753 +++++++++++++++++++++------------------
>>  1 file changed, 416 insertions(+), 337 deletions(-)
>>
> 
> Hi Andrey,
> 
> This is good, you went faster than me !
> 
> Small point, could you add Kconfig dependency on REGMAP ?
> 
> I will try out this patchset and hopefully get you a Tested-by in the next few days.
> 
> Neil
> 

Great, Successfully worked on 4.9-rc2 on my BeagleBone black installation with a SX1509.
I got some rising and falling interrupts using gpio-event-mon.

Small NIT: please add the sx1503 entry in the Kconfig desc and in the pinctrl-sx150x.txt bindings.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>

With the Kconfig and bindings changes :
Acked-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks,
Neil

Re: [PATCH 00/14] pinctrl-sx150x: Various bug-fixes and code simplifications

[Andrey Smirnov]

>>
>> Hi Andrey,
>>
>> This is good, you went faster than me !
>>
>> Small point, could you add Kconfig dependency on REGMAP ?

Good catch! Will fix in v2 of the set.

>>
>> I will try out this patchset and hopefully get you a Tested-by in the next few days.
>>
>> Neil
>>
>
> Great, Successfully worked on 4.9-rc2 on my BeagleBone black installation with a SX1509.
> I got some rising and falling interrupts using gpio-event-mon.
>
> Small NIT: please add the sx1503 entry in the Kconfig desc and in the pinctrl-sx150x.txt bindings.
>

Oops, completely forgot I about that, thanks for noticing! Will add in v2.

> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
>
> With the Kconfig and bindings changes :
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
>

Thank you!

Andrey Smirnov

Re: [PATCH 00/14] pinctrl-sx150x: Various bug-fixes and code simplifications

[Linus Walleij]

On Thu, Nov 3, 2016 at 11:22 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:

>>> This is good, you went faster than me !
>>>
>>> Small point, could you add Kconfig dependency on REGMAP ?
>
> Good catch! Will fix in v2 of the set.

OK I wait for a v2 with the ACK/test tags then I'll apply that.

Yours,
Linus Walleij

Re: [PATCH 01/14] pinctrl-sx150x: Rely on of_modalias_node for OF matching

[Linus Walleij]

On Tue, Nov 1, 2016 at 4:57 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> None of the OF match table entries contain any compatiblity strings that
> could not be matched against using i2c_device_id table above and
> of_modalias_node. Besides that entries in OF match table do not cary
> proper device variant information which is need by the drive. Those two
> facts combined, IMHO, make a compelling case for removal of that code
> altogether.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
(...)
> -static const struct of_device_id sx150x_of_match[] = {
> -       { .compatible = "semtech,sx1508q" },
> -       { .compatible = "semtech,sx1509q" },
> -       { .compatible = "semtech,sx1506q" },
> -       { .compatible = "semtech,sx1502q" },
> -       {},
> -};

I'm a bit hesitant about this since we should ideally first match on the
compatible string for any device. We have tried to alleviate the situation
in I2C devices but it has been a bit so-so.

It would be best if we make a separate patch after this tjat adds it
back, set the variant data also in the .data of the match and
use of_device_get_match_data().

It's no strong preference: I will still apply this patch set because
it is overall very very good.

Yours,
Linus Walleij

Re: [PATCH 01/14] pinctrl-sx150x: Rely on of_modalias_node for OF matching

[Andrey Smirnov]

On Fri, Nov 4, 2016 at 5:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Nov 1, 2016 at 4:57 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
>> None of the OF match table entries contain any compatiblity strings that
>> could not be matched against using i2c_device_id table above and
>> of_modalias_node. Besides that entries in OF match table do not cary
>> proper device variant information which is need by the drive. Those two
>> facts combined, IMHO, make a compelling case for removal of that code
>> altogether.
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> (...)
>> -static const struct of_device_id sx150x_of_match[] = {
>> -       { .compatible = "semtech,sx1508q" },
>> -       { .compatible = "semtech,sx1509q" },
>> -       { .compatible = "semtech,sx1506q" },
>> -       { .compatible = "semtech,sx1502q" },
>> -       {},
>> -};
>
> I'm a bit hesitant about this since we should ideally first match on the
> compatible string for any device. We have tried to alleviate the situation
> in I2C devices but it has been a bit so-so.
>

Ah, good to know. Let's do it that way then.

> It would be best if we make a separate patch after this tjat adds it
> back, set the variant data also in the .data of the match and
> use of_device_get_match_data().

Do you prefer it as a separate patch, or, instead, should I change
this patch of the series to do what you describe? I'd be happy to do
either and it seems like it would be a trivial change so it should
invalidate any of the testing done by Neil.

Thanks,
Andrey

Re: [PATCH 01/14] pinctrl-sx150x: Rely on of_modalias_node for OF matching

[Linus Walleij]

On Fri, Nov 4, 2016 at 9:09 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Fri, Nov 4, 2016 at 5:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Nov 1, 2016 at 4:57 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>>
>>> None of the OF match table entries contain any compatiblity strings that
>>> could not be matched against using i2c_device_id table above and
>>> of_modalias_node. Besides that entries in OF match table do not cary
>>> proper device variant information which is need by the drive. Those two
>>> facts combined, IMHO, make a compelling case for removal of that code
>>> altogether.
>>>
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> (...)
>>> -static const struct of_device_id sx150x_of_match[] = {
>>> -       { .compatible = "semtech,sx1508q" },
>>> -       { .compatible = "semtech,sx1509q" },
>>> -       { .compatible = "semtech,sx1506q" },
>>> -       { .compatible = "semtech,sx1502q" },
>>> -       {},
>>> -};
>>
>> I'm a bit hesitant about this since we should ideally first match on the
>> compatible string for any device. We have tried to alleviate the situation
>> in I2C devices but it has been a bit so-so.
>>
>
> Ah, good to know. Let's do it that way then.
>
>> It would be best if we make a separate patch after this tjat adds it
>> back, set the variant data also in the .data of the match and
>> use of_device_get_match_data().
>
> Do you prefer it as a separate patch, or, instead, should I change
> this patch of the series to do what you describe? I'd be happy to do
> either and it seems like it would be a trivial change so it should
> invalidate any of the testing done by Neil.

Yeah it would ideally be a modification of this patch.

Whatever is easiest for you to do!

BTW this is a great patch set and I'm very grateful for yours+Neils
combines contributions on getting this driver into shape.

Yours,
Linus Walleij