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

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

[Andrey Smirnov]

Linus, Neil:

Here's the second version of sx150x patches, it's all pretty much the
same code with the following differences:

     - Commit removing OF tree match tables changed to code properly
       matching aginst OF data
     - Added REGMAP dependency in Kconfig
     - Documented "semtech,sx1503q" compatibility string

I also added Neil's Tested-bys and Acked-bys to the patches that
didn't change since v1.

Let me know if I missed someting.

Thank you,
Andrey Smirnov

Andrey Smirnov (15):
  pinctrl-sx150x: Improve OF device matching code
  pinctrl-sx150x: Add SX1503 specific data
  bindings: pinctrl-sx150x: Document SX1503 compatibility string
  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

 .../devicetree/bindings/pinctrl/pinctrl-sx150x.txt |   3 +-
 drivers/pinctrl/Kconfig                            |   1 +
 drivers/pinctrl/pinctrl-sx150x.c                   | 766 ++++++++++++---------
 3 files changed, 433 insertions(+), 337 deletions(-)

-- 
2.5.5

[PATCH v2 01/15] pinctrl-sx150x: Improve OF device matching code

[Andrey Smirnov]

Add proper device specific information to of_device_id table of the
driver and add code to match against and fetch said data from it.

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

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index d2d4211..0523f5a 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -25,6 +25,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/gpio.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinconf.h>
@@ -862,10 +863,10 @@ 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" },
+	{ .compatible = "semtech,sx1508q", .data = &sx1508q_device_data },
+	{ .compatible = "semtech,sx1509q", .data = &sx1509q_device_data },
+	{ .compatible = "semtech,sx1506q", .data = &sx1506q_device_data },
+	{ .compatible = "semtech,sx1502q", .data = &sx1502q_device_data },
 	{},
 };
 
@@ -956,9 +957,6 @@ static int sx150x_probe(struct i2c_client *client,
 	struct sx150x_pinctrl *pctl;
 	int ret;
 
-	if (!id->driver_data)
-		return -EINVAL;
-
 	if (!i2c_check_functionality(client->adapter, i2c_funcs))
 		return -ENOSYS;
 
@@ -968,7 +966,14 @@ static int sx150x_probe(struct i2c_client *client,
 
 	pctl->dev = dev;
 	pctl->client = client;
-	pctl->data = (void *)id->driver_data;
+
+	if (dev->of_node)
+		pctl->data = of_device_get_match_data(dev);
+	else
+		pctl->data = (struct sx150x_device_data *)id->driver_data;
+
+	if (!pctl->data)
+		return -EINVAL;
 
 	mutex_init(&pctl->lock);
 
-- 
2.5.5

Re: [PATCH v2 01/15] pinctrl-sx150x: Improve OF device matching code

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Add proper device specific information to of_device_id table of the
> driver and add code to match against and fetch said data from it.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied. Excellent work!

Yours,
Linus Walleij

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

[Andrey Smirnov]

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/pinctrl-sx150x.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 0523f5a..9466777 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -230,6 +230,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);
@@ -859,6 +882,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 },
 	{}
 };
 
@@ -867,6 +891,7 @@ static const struct of_device_id sx150x_of_match[] = {
 	{ .compatible = "semtech,sx1509q", .data = &sx1509q_device_data },
 	{ .compatible = "semtech,sx1506q", .data = &sx1506q_device_data },
 	{ .compatible = "semtech,sx1502q", .data = &sx1502q_device_data },
+	{ .compatible = "semtech,sx1503q", .data = &sx1503q_device_data },
 	{},
 };
 
-- 
2.5.5

Re: [PATCH v2 02/15] pinctrl-sx150x: Add SX1503 specific data

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 03/15] bindings: pinctrl-sx150x: Document SX1503 compatibility string

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-sx150x.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-sx150x.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-sx150x.txt
index c293c8a..25b4ec8 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-sx150x.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-sx150x.txt
@@ -9,7 +9,8 @@ Required properties:
 			"semtech,sx1506q",
 			"semtech,sx1508q",
 			"semtech,sx1509q",
-			"semtech,sx1502q".
+			"semtech,sx1502q",
+			"semtech,sx1503q".
 
 - reg: The I2C slave address for this device.
 
-- 
2.5.5

Re: [PATCH v2 03/15] bindings: pinctrl-sx150x: Document SX1503 compatibility string

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied.

Yours,
Linus Walleij

[PATCH v2 04/15] pinctrl-sx150x: Replace magic number in sx150x_init_hw

[Andrey Smirnov]

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 9466777..3ccd048 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -43,6 +43,9 @@ enum {
 	SX150X_456,
 	SX150X_789,
 };
+enum {
+	SX150X_789_REG_MISC_AUTOCLEAR_OFF = 1 << 0,
+};
 
 struct sx150x_123_pri {
 	u8 reg_pld_mode;
@@ -935,7 +938,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

Re: [PATCH v2 04/15] pinctrl-sx150x: Replace magic number in sx150x_init_hw

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 05/15] 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.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 3ccd048..0e3e1da 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -942,7 +942,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

Re: [PATCH v2 05/15] pinctrl-sx150x: Fix incorrect constant in sx150x_init_hw

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> 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.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 06/15] 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.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 0e3e1da..27194e7 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -924,6 +924,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;
@@ -935,18 +960,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

Re: [PATCH v2 06/15] pinctrl-sx150x: Move some code out of sx150x_init_hw

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> 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.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 07/15] 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.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 27194e7..d51ec73 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -936,6 +936,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

Re: [PATCH v2 07/15] pinctrl-sx150x: Improve sx150x_init_misc for SX1504/5/6

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> For Sx1504/5/6 only SX1506 has RegAdvanced, so put some code in place to
> account for that.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 08/15] pinctrl-sx150x: Convert driver to use regmap API

[Andrey Smirnov]

To allow for future code simplification

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pinctrl/Kconfig          |   1 +
 drivers/pinctrl/pinctrl-sx150x.c | 102 +++++++++++++++++++++------------------
 2 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 801fa8b..666e952 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -171,6 +171,7 @@ config PINCTRL_SX150X
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB_IRQCHIP
+	select REGMAP
 	help
 	  Say yes here to provide support for Semtech SX150x-series I2C
 	  GPIO expanders as pinctrl module.
diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index d51ec73..2eb233f 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>
@@ -45,6 +46,7 @@ enum {
 };
 enum {
 	SX150X_789_REG_MISC_AUTOCLEAR_OFF = 1 << 0,
+	SX150X_MAX_REGISTER = 0xad,
 };
 
 struct sx150x_123_pri {
@@ -102,6 +104,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;
@@ -256,30 +259,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
@@ -312,30 +291,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;
 
@@ -463,11 +444,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);
@@ -567,19 +547,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;
 
@@ -650,15 +630,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)
@@ -904,7 +884,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;
 }
 
@@ -953,7 +933,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)
@@ -997,6 +977,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)
 {
@@ -1013,6 +1013,8 @@ static int sx150x_probe(struct i2c_client *client,
 	if (!pctl)
 		return -ENOMEM;
 
+	i2c_set_clientdata(client, pctl);
+
 	pctl->dev = dev;
 	pctl->client = client;
 
@@ -1024,6 +1026,14 @@ static int sx150x_probe(struct i2c_client *client,
 	if (!pctl->data)
 		return -EINVAL;
 
+	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

Re: [PATCH v2 08/15] pinctrl-sx150x: Convert driver to use regmap API

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> To allow for future code simplification
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 09/15] 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.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 2eb233f..4725fac 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -106,11 +106,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;
@@ -171,16 +168,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,
@@ -192,20 +189,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,
@@ -238,20 +235,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,
@@ -259,70 +256,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;
@@ -368,31 +301,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,
@@ -409,9 +344,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;
@@ -423,9 +358,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;
@@ -438,6 +373,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)
 {
@@ -451,9 +393,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);
 	}
 }
@@ -468,8 +408,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;
@@ -487,12 +428,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;
@@ -504,8 +444,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)
@@ -514,8 +453,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)
@@ -536,7 +474,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;
 }
 
@@ -548,29 +485,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;
 		}
 	}
 
@@ -589,35 +517,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);
 }
 
@@ -628,10 +530,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:
@@ -666,8 +567,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)
@@ -681,8 +584,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)
@@ -699,14 +604,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;
@@ -717,14 +624,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;
@@ -785,15 +694,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;
@@ -802,9 +713,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;
@@ -813,9 +724,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;
@@ -878,16 +789,6 @@ static const struct of_device_id sx150x_of_match[] = {
 	{},
 };
 
-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;
@@ -933,11 +834,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 &&
@@ -952,28 +858,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;
 }
 
@@ -981,18 +1024,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,
 };
@@ -1026,7 +1069,8 @@ static int sx150x_probe(struct i2c_client *client,
 	if (!pctl->data)
 		return -EINVAL;
 
-	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",
@@ -1072,9 +1116,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

Re: [PATCH v2 09/15] pinctrl-sx150x: Replace sx150x_*_cfg by means of regmap API

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> 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.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 10/15] 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.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 4725fac..c339800 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -343,13 +343,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:
@@ -357,20 +353,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,
@@ -385,57 +377,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)
@@ -536,12 +517,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;
 
@@ -566,12 +544,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;
@@ -583,12 +559,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;
@@ -603,12 +577,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;
@@ -623,12 +595,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;
@@ -693,41 +663,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

Re: [PATCH v2 10/15] pinctrl-sx150x: Remove excessive locking

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> 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.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 11/15] 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.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 c339800..f2ec072 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -372,15 +372,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);
 
@@ -405,10 +411,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

Re: [PATCH v2 11/15] pinctrl-sx150x: Improve oscio GPIO functions

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> 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.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

[PATCH v2 12/15] pinctrl-sx150x: Simplify interrupt handler

[Andrey Smirnov]

Make use of for_each_set_bit macro and reduce boilerplate code.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 f2ec072..78e15c9 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -465,11 +465,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)
@@ -479,15 +477,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

Re: [PATCH v2 12/15] pinctrl-sx150x: Simplify interrupt handler

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Make use of for_each_set_bit macro and reduce boilerplate code.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied.

Yours,
Linus Walleij

[PATCH v2 13/15] 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.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 78e15c9..798a8bb 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -1077,9 +1077,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

Re: [PATCH v2 13/15] pinctrl-sx150x: Use handle_bad_irq instead of handle_edge_irq

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> 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.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

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

[Andrey Smirnov]

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 798a8bb..56abe36 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -47,6 +47,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 {
@@ -441,6 +443,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 =
@@ -453,12 +470,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

Re: [PATCH v2 14/15] pinctrl-sx150x: Remove magic numbers from sx150x_irq_set_type

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij

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

[Andrey Smirnov]

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
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 56abe36..dc1341f 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -49,6 +49,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 {
@@ -771,13 +773,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 v2 15/15] pinctrl-sx150x: Remove magic numbers from sx150x_reset

[Linus Walleij]

On Mon, Nov 7, 2016 at 5:53 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Patch applied!

Yours,
Linus Walleij