[PATCH 0/3] pinctrl: sx150x fixes for the 4-pin chips

[PATCH 0/3] pinctrl: sx150x fixes for the 4-pin chips

[Peter Rosin]

Hi Linus!

I found some more issues when reading the sx150x code.

Only the first patch actually fixes a problem. I didn't want to add a
Fixes tag, since I don't know if commits on the pinctrl devel branch
are considered stable? Or if you perhaps want to squash this patch with
the patch it fixes?

Anyway, at your discretion, this could be added to patch #1:

Fixes: 4f5ac8cf0a11 ("pinctrl: sx150x: add support for sx1501, sx1504, sx1505, sx1507")

The other two patches are almost entirely cosmetic.

Cheers,
Peter

Peter Rosin (3):
  pinctrl: sx150x: access the correct bits in the 4-bit regs of
    sx150[147]
  pinctrl: sx150x: rename 'reg_advance' to 'reg_advanced'
  pinctrl: sx150x: handle missing 'advanced' reg in sx1504 and sx1505

 drivers/pinctrl/pinctrl-sx150x.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

-- 
2.1.4

[PATCH 1/3] pinctrl: sx150x: access the correct bits in the 4-bit regs of sx150[147]

[Peter Rosin]

The code assumes 8-bit or 16-bit width registers, but three of the
chips (sx1501/sx1504/sx1507) are 4-bit. So, try to handle 4-bit chips as
well, they leave the high part of each register unused.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-sx150x.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 1a1c8b51a992..a121819ffc92 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2010, Code Aurora Forum. All rights reserved.
  *
  * Driver for Semtech SX150X I2C GPIO Expanders
+ * The handling of the 4-bit chips (SX1501/SX1504/SX1507) is untested.
  *
  * Author: Gregory Bean <gbean@codeaurora.org>
  *
@@ -1088,7 +1089,7 @@ static int sx150x_regmap_reg_write(void *context, unsigned int reg,
 
 	val = sx150x_maybe_swizzle(pctl, reg, val);
 
-	n = width - 8;
+	n = (width - 1) & ~7;
 	do {
 		const u8 byte = (val >> n) & 0xff;
 
-- 
2.1.4

[PATCH 2/3] pinctrl: sx150x: rename 'reg_advance' to 'reg_advanced'

[Peter Rosin]

This matches the datasheets and is less confusing since the register
has nothing to with advancing anything.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-sx150x.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index a121819ffc92..37b8737e61a9 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -60,7 +60,7 @@ struct sx150x_123_pri {
 	u8 reg_pld_table2;
 	u8 reg_pld_table3;
 	u8 reg_pld_table4;
-	u8 reg_advance;
+	u8 reg_advanced;
 };
 
 struct sx150x_456_pri {
@@ -70,7 +70,7 @@ struct sx150x_456_pri {
 	u8 reg_pld_table2;
 	u8 reg_pld_table3;
 	u8 reg_pld_table4;
-	u8 reg_advance;
+	u8 reg_advanced;
 };
 
 struct sx150x_789_pri {
@@ -170,7 +170,7 @@ static const struct sx150x_device_data sx1501q_device_data = {
 		.reg_pld_mode	= 0x10,
 		.reg_pld_table0	= 0x11,
 		.reg_pld_table2	= 0x13,
-		.reg_advance	= 0xad,
+		.reg_advanced	= 0xad,
 	},
 	.ngpios	= 4,
 	.pins = sx150x_4_pins,
@@ -193,7 +193,7 @@ static const struct sx150x_device_data sx1502q_device_data = {
 		.reg_pld_table2	= 0x13,
 		.reg_pld_table3	= 0x14,
 		.reg_pld_table4	= 0x15,
-		.reg_advance	= 0xad,
+		.reg_advanced	= 0xad,
 	},
 	.ngpios	= 8,
 	.pins = sx150x_8_pins,
@@ -216,7 +216,7 @@ static const struct sx150x_device_data sx1503q_device_data = {
 		.reg_pld_table2	= 0x26,
 		.reg_pld_table3	= 0x28,
 		.reg_pld_table4	= 0x2a,
-		.reg_advance	= 0xad,
+		.reg_advanced	= 0xad,
 	},
 	.ngpios	= 16,
 	.pins = sx150x_16_pins,
@@ -280,7 +280,7 @@ static const struct sx150x_device_data sx1506q_device_data = {
 		.reg_pld_table2	= 0x26,
 		.reg_pld_table3	= 0x28,
 		.reg_pld_table4	= 0x2a,
-		.reg_advance	= 0xad,
+		.reg_advanced	= 0xad,
 	},
 	.ngpios	= 16,
 	.pins = sx150x_16_pins,
@@ -900,7 +900,7 @@ static int sx150x_init_misc(struct sx150x_pinctrl *pctl)
 		value = SX150X_789_REG_MISC_AUTOCLEAR_OFF;
 		break;
 	case SX150X_456:
-		reg   = pctl->data->pri.x456.reg_advance;
+		reg   = pctl->data->pri.x456.reg_advanced;
 		value = 0x00;
 
 		/*
@@ -911,7 +911,7 @@ static int sx150x_init_misc(struct sx150x_pinctrl *pctl)
 			return 0;
 		break;
 	case SX150X_123:
-		reg   = pctl->data->pri.x123.reg_advance;
+		reg   = pctl->data->pri.x123.reg_advanced;
 		value = 0x00;
 		break;
 	default:
@@ -964,10 +964,10 @@ static int sx150x_regmap_reg_width(struct sx150x_pinctrl *pctl,
 		     reg == data->pri.x789.reg_reset))
 		   ||
 		   (data->model == SX150X_123 &&
-		    reg == data->pri.x123.reg_advance)
+		    reg == data->pri.x123.reg_advanced)
 		   ||
 		   (data->model == SX150X_456 &&
-		    reg == data->pri.x456.reg_advance)) {
+		    reg == data->pri.x456.reg_advanced)) {
 		return 8;
 	} else {
 		return data->ngpios;
-- 
2.1.4

[PATCH 3/3] pinctrl: sx150x: handle missing 'advanced' reg in sx1504 and sx1505

[Peter Rosin]

This fixes a problem where sx150x_regmap_reg_width() returns 8 for the
data register (reg 0) for sx1504 where it should return 4, and return
a correct 8 for sx1505 but for the wrong reason (both chips lack the
'advanced' register). This is not a real problem, since nothing depends
on the function returning 4 or 8, and certainly not if it is returning
8 for the wrong reason. But fix this to avoid nasty surprises down the
line.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-sx150x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
index 37b8737e61a9..bdb7ea3d9911 100644
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -967,6 +967,7 @@ static int sx150x_regmap_reg_width(struct sx150x_pinctrl *pctl,
 		    reg == data->pri.x123.reg_advanced)
 		   ||
 		   (data->model == SX150X_456 &&
+		    data->pri.x456.reg_advanced &&
 		    reg == data->pri.x456.reg_advanced)) {
 		return 8;
 	} else {
-- 
2.1.4

Re: [PATCH 0/3] pinctrl: sx150x fixes for the 4-pin chips

[Peter Rosin]

On 2016-12-02 11:51, Peter Rosin wrote:
> Anyway, at your discretion, this could be added to patch #1:
> 
> Fixes: 4f5ac8cf0a11 ("pinctrl: sx150x: add support for sx1501, sx1504, sx1505, sx1507")

I'm apparently bad at copying, this should be:
Fixes: 4f5ac8cf0a11 ("pinctrl: sx150x: add support for sx1501, sx1504, sx1505 and sx1507")

Since I obviously typed this manually, I can note that I did check that
the shortened hash is correct.

Cheers,
Peter

Re: [PATCH 1/3] pinctrl: sx150x: access the correct bits in the 4-bit regs of sx150[147]

[Linus Walleij]

On Fri, Dec 2, 2016 at 11:51 AM, Peter Rosin <peda@axentia.se> wrote:

> The code assumes 8-bit or 16-bit width registers, but three of the
> chips (sx1501/sx1504/sx1507) are 4-bit. So, try to handle 4-bit chips as
> well, they leave the high part of each register unused.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl: sx150x: rename 'reg_advance' to 'reg_advanced'

[Linus Walleij]

On Fri, Dec 2, 2016 at 11:51 AM, Peter Rosin <peda@axentia.se> wrote:

> This matches the datasheets and is less confusing since the register
> has nothing to with advancing anything.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 3/3] pinctrl: sx150x: handle missing 'advanced' reg in sx1504 and sx1505

[Linus Walleij]

On Fri, Dec 2, 2016 at 11:51 AM, Peter Rosin <peda@axentia.se> wrote:

> This fixes a problem where sx150x_regmap_reg_width() returns 8 for the
> data register (reg 0) for sx1504 where it should return 4, and return
> a correct 8 for sx1505 but for the wrong reason (both chips lack the
> 'advanced' register). This is not a real problem, since nothing depends
> on the function returning 4 or 8, and certainly not if it is returning
> 8 for the wrong reason. But fix this to avoid nasty surprises down the
> line.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 0/3] pinctrl: sx150x fixes for the 4-pin chips

[Linus Walleij]

On Fri, Dec 2, 2016 at 11:51 AM, Peter Rosin <peda@axentia.se> wrote:

> I found some more issues when reading the sx150x code.

Thanks, I applied all for v4.10.

> Only the first patch actually fixes a problem. I didn't want to add a
> Fixes tag, since I don't know if commits on the pinctrl devel branch
> are considered stable? Or if you perhaps want to squash this patch with
> the patch it fixes?

Nah I just applied them.

This kernel cycle moves the driver and rewrites it and everything
and a bit of fuzz should be expected so last minute changes
should be regarded as business as usual.

I'm just happy about all the people that paid so much attention
to this driver in the v4.10 cycle :D

Yours,
Linus Walleij