[PATCH v2 0/5] gpio: pca953x: code refactoring

[PATCH v2 0/5] gpio: pca953x: code refactoring

[Bartosz Golaszewski]

I'm working on converting the pca953x driver to using regmap, but since
it's not a trivial task I figured I'd post a couple refactoring patches
I did so far for 4.9.

The first patch just fixes a couple coding style issues. The second
removes a couple unnecessary switches. Last three refactor the
read/write_regs functions to avoid if-elses by using function pointers
to smaller, specialized routines.

Tested with pca9534 and pca9535 chips.

v1 -> v2:
- constified the offset structures in patch 2/5

Bartosz Golaszewski (5):
  gpio: pca953x: coding style fixes
  gpio: pca953x: code shrink
  gpio: pca953x: refactor pca953x_write_regs()
  gpio: pca953x: remove an unused variable
  gpio: pca953x: refactor pca953x_read_regs()

 drivers/gpio/gpio-pca953x.c | 270 ++++++++++++++++++++++----------------------
 1 file changed, 136 insertions(+), 134 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] gpio: pca953x: coding style fixes

[Bartosz Golaszewski]

pca953x_gpio_set_multiple() has some coding style issues that make it
harder to read. Tweak the code a bit.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 02f2a56..2312f8d 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -355,13 +355,13 @@ exit:
 }
 
 static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
-		unsigned long *mask, unsigned long *bits)
+				      unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 reg_val[MAX_BANK];
-	int ret, offset = 0;
+	int ret, bank, offset = 0;
 	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
-	int bank;
+	unsigned int bankmask, bankval;
 
 	switch (chip->chip_type) {
 	case PCA953X_TYPE:
@@ -374,15 +374,16 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 
 	memcpy(reg_val, chip->reg_output, NBANK(chip));
 	mutex_lock(&chip->i2c_lock);
-	for(bank=0; bank<NBANK(chip); bank++) {
-		unsigned bankmask = mask[bank / sizeof(*mask)] >>
-				    ((bank % sizeof(*mask)) * 8);
-		if(bankmask) {
-			unsigned bankval  = bits[bank / sizeof(*bits)] >>
-					    ((bank % sizeof(*bits)) * 8);
+	for (bank = 0; bank < NBANK(chip); bank++) {
+		bankmask = mask[bank / sizeof(*mask)] >>
+			   ((bank % sizeof(*mask)) * 8);
+		if (bankmask) {
+			bankval = bits[bank / sizeof(*bits)] >>
+				  ((bank % sizeof(*bits)) * 8);
 			reg_val[bank] = (reg_val[bank] & ~bankmask) | bankval;
 		}
 	}
+
 	ret = i2c_smbus_write_i2c_block_data(chip->client, offset << bank_shift, NBANK(chip), reg_val);
 	if (ret)
 		goto exit;
-- 
2.7.4

[PATCH v2 2/5] gpio: pca953x: code shrink

[Bartosz Golaszewski]

There are multiple places in the driver code where a
switch (chip->chip_type) is used to determine the proper register
offset.

Unduplicate the code by adding a simple structure holding the possible
offsets that differ between the pca953x and pca957x chip families and
use it to avoid the checks.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 122 +++++++++++++++-----------------------------
 1 file changed, 42 insertions(+), 80 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 2312f8d..195944b 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -94,6 +94,24 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
 
 #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
 
+struct pca953x_offset {
+	int direction;
+	int output;
+	int input;
+};
+
+static const struct pca953x_offset pca953x_offsets = {
+	.direction = PCA953X_DIRECTION,
+	.output = PCA953X_OUTPUT,
+	.input = PCA953X_INPUT,
+};
+
+static const struct pca953x_offset pca957x_offsets = {
+	.direction = PCA957X_CFG,
+	.output = PCA957X_OUT,
+	.input = PCA957X_IN,
+};
+
 struct pca953x_chip {
 	unsigned gpio_start;
 	u8 reg_output[MAX_BANK];
@@ -113,6 +131,8 @@ struct pca953x_chip {
 	const char *const *names;
 	int	chip_type;
 	unsigned long driver_data;
+
+	const struct pca953x_offset *offset;
 };
 
 static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
@@ -222,20 +242,12 @@ static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 reg_val;
-	int ret, offset = 0;
+	int ret;
 
 	mutex_lock(&chip->i2c_lock);
 	reg_val = chip->reg_direction[off / BANK_SZ] | (1u << (off % BANK_SZ));
 
-	switch (chip->chip_type) {
-	case PCA953X_TYPE:
-		offset = PCA953X_DIRECTION;
-		break;
-	case PCA957X_TYPE:
-		offset = PCA957X_CFG;
-		break;
-	}
-	ret = pca953x_write_single(chip, offset, reg_val, off);
+	ret = pca953x_write_single(chip, chip->offset->direction, reg_val, off);
 	if (ret)
 		goto exit;
 
@@ -250,7 +262,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 reg_val;
-	int ret, offset = 0;
+	int ret;
 
 	mutex_lock(&chip->i2c_lock);
 	/* set output level */
@@ -261,15 +273,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 		reg_val = chip->reg_output[off / BANK_SZ]
 			& ~(1u << (off % BANK_SZ));
 
-	switch (chip->chip_type) {
-	case PCA953X_TYPE:
-		offset = PCA953X_OUTPUT;
-		break;
-	case PCA957X_TYPE:
-		offset = PCA957X_OUT;
-		break;
-	}
-	ret = pca953x_write_single(chip, offset, reg_val, off);
+	ret = pca953x_write_single(chip, chip->offset->output, reg_val, off);
 	if (ret)
 		goto exit;
 
@@ -277,15 +281,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 
 	/* then direction */
 	reg_val = chip->reg_direction[off / BANK_SZ] & ~(1u << (off % BANK_SZ));
-	switch (chip->chip_type) {
-	case PCA953X_TYPE:
-		offset = PCA953X_DIRECTION;
-		break;
-	case PCA957X_TYPE:
-		offset = PCA957X_CFG;
-		break;
-	}
-	ret = pca953x_write_single(chip, offset, reg_val, off);
+	ret = pca953x_write_single(chip, chip->offset->direction, reg_val, off);
 	if (ret)
 		goto exit;
 
@@ -299,18 +295,10 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u32 reg_val;
-	int ret, offset = 0;
+	int ret;
 
 	mutex_lock(&chip->i2c_lock);
-	switch (chip->chip_type) {
-	case PCA953X_TYPE:
-		offset = PCA953X_INPUT;
-		break;
-	case PCA957X_TYPE:
-		offset = PCA957X_IN;
-		break;
-	}
-	ret = pca953x_read_single(chip, offset, &reg_val, off);
+	ret = pca953x_read_single(chip, chip->offset->input, &reg_val, off);
 	mutex_unlock(&chip->i2c_lock);
 	if (ret < 0) {
 		/* NOTE:  diagnostic already emitted; that's all we should
@@ -327,7 +315,7 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 reg_val;
-	int ret, offset = 0;
+	int ret;
 
 	mutex_lock(&chip->i2c_lock);
 	if (val)
@@ -337,15 +325,7 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 		reg_val = chip->reg_output[off / BANK_SZ]
 			& ~(1u << (off % BANK_SZ));
 
-	switch (chip->chip_type) {
-	case PCA953X_TYPE:
-		offset = PCA953X_OUTPUT;
-		break;
-	case PCA957X_TYPE:
-		offset = PCA957X_OUT;
-		break;
-	}
-	ret = pca953x_write_single(chip, offset, reg_val, off);
+	ret = pca953x_write_single(chip, chip->offset->output, reg_val, off);
 	if (ret)
 		goto exit;
 
@@ -359,19 +339,10 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 reg_val[MAX_BANK];
-	int ret, bank, offset = 0;
+	int ret, bank;
 	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
 	unsigned int bankmask, bankval;
 
-	switch (chip->chip_type) {
-	case PCA953X_TYPE:
-		offset = PCA953X_OUTPUT;
-		break;
-	case PCA957X_TYPE:
-		offset = PCA957X_OUT;
-		break;
-	}
-
 	memcpy(reg_val, chip->reg_output, NBANK(chip));
 	mutex_lock(&chip->i2c_lock);
 	for (bank = 0; bank < NBANK(chip); bank++) {
@@ -384,7 +355,9 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 		}
 	}
 
-	ret = i2c_smbus_write_i2c_block_data(chip->client, offset << bank_shift, NBANK(chip), reg_val);
+	ret = i2c_smbus_write_i2c_block_data(chip->client,
+					     chip->offset->output << bank_shift,
+					     NBANK(chip), reg_val);
 	if (ret)
 		goto exit;
 
@@ -516,7 +489,7 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
 	bool pending_seen = false;
 	bool trigger_seen = false;
 	u8 trigger[MAX_BANK];
-	int ret, i, offset = 0;
+	int ret, i;
 
 	if (chip->driver_data & PCA_PCAL) {
 		/* Read the current interrupt status from the device */
@@ -541,15 +514,7 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
 		return pending_seen;
 	}
 
-	switch (chip->chip_type) {
-	case PCA953X_TYPE:
-		offset = PCA953X_INPUT;
-		break;
-	case PCA957X_TYPE:
-		offset = PCA957X_IN;
-		break;
-	}
-	ret = pca953x_read_regs(chip, offset, cur_stat);
+	ret = pca953x_read_regs(chip, chip->offset->input, cur_stat);
 	if (ret)
 		return false;
 
@@ -609,20 +574,13 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 			     int irq_base)
 {
 	struct i2c_client *client = chip->client;
-	int ret, i, offset = 0;
+	int ret, i;
 
 	if (client->irq && irq_base != -1
 			&& (chip->driver_data & PCA_INT)) {
 
-		switch (chip->chip_type) {
-		case PCA953X_TYPE:
-			offset = PCA953X_INPUT;
-			break;
-		case PCA957X_TYPE:
-			offset = PCA957X_IN;
-			break;
-		}
-		ret = pca953x_read_regs(chip, offset, chip->irq_stat);
+		ret = pca953x_read_regs(chip,
+					chip->offset->input, chip->irq_stat);
 		if (ret)
 			return ret;
 
@@ -685,6 +643,8 @@ static int device_pca953x_init(struct pca953x_chip *chip, u32 invert)
 	int ret;
 	u8 val[MAX_BANK];
 
+	chip->offset = &pca953x_offsets;
+
 	ret = pca953x_read_regs(chip, PCA953X_OUTPUT, chip->reg_output);
 	if (ret)
 		goto out;
@@ -710,6 +670,8 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
 	int ret;
 	u8 val[MAX_BANK];
 
+	chip->offset = &pca957x_offsets;
+
 	ret = pca953x_read_regs(chip, PCA957X_OUT, chip->reg_output);
 	if (ret)
 		goto out;
-- 
2.7.4

[PATCH v2 3/5] gpio: pca953x: refactor pca953x_write_regs()

[Bartosz Golaszewski]

Avoid the unnecessary if-else in pca953x_write_regs() by splitting
the routine into smaller, specialized functions and calling the right
one via a function pointer held in struct pca953x_chip.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 75 ++++++++++++++++++++++++++++-----------------
 1 file changed, 47 insertions(+), 28 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 195944b..6b62898 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -133,6 +133,8 @@ struct pca953x_chip {
 	unsigned long driver_data;
 
 	const struct pca953x_offset *offset;
+
+	int (*write_regs)(struct pca953x_chip *, int, u8 *);
 };
 
 static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
@@ -172,38 +174,44 @@ static int pca953x_write_single(struct pca953x_chip *chip, int reg, u32 val,
 	return 0;
 }
 
-static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_write_regs_8(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	int ret = 0;
+	return i2c_smbus_write_byte_data(chip->client, reg, *val);
+}
 
-	if (chip->gpio_chip.ngpio <= 8)
-		ret = i2c_smbus_write_byte_data(chip->client, reg, *val);
-	else if (chip->gpio_chip.ngpio >= 24) {
-		int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
-		ret = i2c_smbus_write_i2c_block_data(chip->client,
-					(reg << bank_shift) | REG_ADDR_AI,
-					NBANK(chip), val);
-	} else {
-		switch (chip->chip_type) {
-		case PCA953X_TYPE: {
-			__le16 word = cpu_to_le16(get_unaligned((u16 *)val));
+static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
+{
+	__le16 word = cpu_to_le16(get_unaligned((u16 *)val));
 
-			ret = i2c_smbus_write_word_data(chip->client, reg << 1,
-							(__force u16)word);
-			break;
-		}
-		case PCA957X_TYPE:
-			ret = i2c_smbus_write_byte_data(chip->client, reg << 1,
-							val[0]);
-			if (ret < 0)
-				break;
-			ret = i2c_smbus_write_byte_data(chip->client,
-							(reg << 1) + 1,
-							val[1]);
-			break;
-		}
-	}
+	return i2c_smbus_write_word_data(chip->client,
+					 reg << 1, (__force u16)word);
+}
+
+static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
+{
+	int ret;
 
+	ret = i2c_smbus_write_byte_data(chip->client, reg << 1, val[0]);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(chip->client, (reg << 1) + 1, val[1]);
+}
+
+static int pca953x_write_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
+{
+	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+
+	return i2c_smbus_write_i2c_block_data(chip->client,
+					      (reg << bank_shift) | REG_ADDR_AI,
+					      NBANK(chip), val);
+}
+
+static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
+{
+	int ret = 0;
+
+	ret = chip->write_regs(chip, reg, val);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed writing register\n");
 		return ret;
@@ -645,6 +653,9 @@ static int device_pca953x_init(struct pca953x_chip *chip, u32 invert)
 
 	chip->offset = &pca953x_offsets;
 
+	if (!chip->write_regs)
+		chip->write_regs = pca953x_write_regs_16;
+
 	ret = pca953x_read_regs(chip, PCA953X_OUTPUT, chip->reg_output);
 	if (ret)
 		goto out;
@@ -672,6 +683,9 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
 
 	chip->offset = &pca957x_offsets;
 
+	if (!chip->write_regs)
+		chip->write_regs = pca957x_write_regs_16;
+
 	ret = pca953x_read_regs(chip, PCA957X_OUT, chip->reg_output);
 	if (ret)
 		goto out;
@@ -755,6 +769,11 @@ static int pca953x_probe(struct i2c_client *client,
 	 */
 	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
 
+	if (chip->gpio_chip.ngpio <= 8)
+		chip->write_regs = pca953x_write_regs_8;
+	else if (chip->gpio_chip.ngpio >= 24)
+		chip->write_regs = pca953x_write_regs_24;
+
 	if (chip->chip_type == PCA953X_TYPE)
 		ret = device_pca953x_init(chip, invert);
 	else
-- 
2.7.4

[PATCH v2 4/5] gpio: pca953x: remove an unused variable

[Bartosz Golaszewski]

The chip_type variable in struct pca953x_chip is no longer required.

Remove it.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 6b62898..00bb2ea 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -129,7 +129,6 @@ struct pca953x_chip {
 	struct i2c_client *client;
 	struct gpio_chip gpio_chip;
 	const char *const *names;
-	int	chip_type;
 	unsigned long driver_data;
 
 	const struct pca953x_offset *offset;
@@ -760,8 +759,6 @@ static int pca953x_probe(struct i2c_client *client,
 		}
 	}
 
-	chip->chip_type = PCA_CHIP_TYPE(chip->driver_data);
-
 	mutex_init(&chip->i2c_lock);
 
 	/* initialize cached registers from their original values.
@@ -774,7 +771,7 @@ static int pca953x_probe(struct i2c_client *client,
 	else if (chip->gpio_chip.ngpio >= 24)
 		chip->write_regs = pca953x_write_regs_24;
 
-	if (chip->chip_type == PCA953X_TYPE)
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
 		ret = device_pca953x_init(chip, invert);
 	else
 		ret = device_pca957x_init(chip, invert);
-- 
2.7.4

[PATCH v2 5/5] gpio: pca953x: refactor pca953x_read_regs()

[Bartosz Golaszewski]

Avoid the unnecessary if-else in pca953x_read_regs() by spltting the
routine into smaller, specialized functions and calling the right one
via a function pointer held in struct pca953x.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-pca953x.c | 55 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 00bb2ea..e417c42 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -134,6 +134,7 @@ struct pca953x_chip {
 	const struct pca953x_offset *offset;
 
 	int (*write_regs)(struct pca953x_chip *, int, u8 *);
+	int (*read_regs)(struct pca953x_chip *, int, u8 *);
 };
 
 static int pca953x_read_single(struct pca953x_chip *chip, int reg, u32 *val,
@@ -219,24 +220,41 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 	return 0;
 }
 
-static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_read_regs_8(struct pca953x_chip *chip, int reg, u8 *val)
 {
 	int ret;
 
-	if (chip->gpio_chip.ngpio <= 8) {
-		ret = i2c_smbus_read_byte_data(chip->client, reg);
-		*val = ret;
-	} else if (chip->gpio_chip.ngpio >= 24) {
-		int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+	ret = i2c_smbus_read_byte_data(chip->client, reg);
+	*val = ret;
 
-		ret = i2c_smbus_read_i2c_block_data(chip->client,
-					(reg << bank_shift) | REG_ADDR_AI,
-					NBANK(chip), val);
-	} else {
-		ret = i2c_smbus_read_word_data(chip->client, reg << 1);
-		val[0] = (u16)ret & 0xFF;
-		val[1] = (u16)ret >> 8;
-	}
+	return ret;
+}
+
+static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_data(chip->client, reg << 1);
+	val[0] = (u16)ret & 0xFF;
+	val[1] = (u16)ret >> 8;
+
+	return ret;
+}
+
+static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg, u8 *val)
+{
+	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
+
+	return i2c_smbus_read_i2c_block_data(chip->client,
+					     (reg << bank_shift) | REG_ADDR_AI,
+					     NBANK(chip), val);
+}
+
+static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
+{
+	int ret;
+
+	ret = chip->read_regs(chip, reg, val);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed reading register\n");
 		return ret;
@@ -766,10 +784,15 @@ static int pca953x_probe(struct i2c_client *client,
 	 */
 	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
 
-	if (chip->gpio_chip.ngpio <= 8)
+	if (chip->gpio_chip.ngpio <= 8) {
 		chip->write_regs = pca953x_write_regs_8;
-	else if (chip->gpio_chip.ngpio >= 24)
+		chip->read_regs = pca953x_read_regs_8;
+	} else if (chip->gpio_chip.ngpio >= 24) {
 		chip->write_regs = pca953x_write_regs_24;
+		chip->read_regs = pca953x_read_regs_24;
+	} else {
+		chip->read_regs = pca953x_read_regs_16;
+	}
 
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
 		ret = device_pca953x_init(chip, invert);
-- 
2.7.4

Re: [PATCH v2 1/5] gpio: pca953x: coding style fixes

[Andy Shevchenko]

On Wed, 2016-09-07 at 11:24 +0200, Bartosz Golaszewski wrote:
> pca953x_gpio_set_multiple() has some coding style issues that make it
> harder to read. Tweak the code a bit.
> 

I would suggest to make this a last patch in the series. Rationale is
that you might have changed same lines by functional changes and the
first patch in such cases just makes a noise. More important, easier way
to back port if anyone is interested in that.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 5/5] gpio: pca953x: refactor pca953x_read_regs()

[Andy Shevchenko]

On Wed, 2016-09-07 at 11:24 +0200, Bartosz Golaszewski wrote:
> Avoid the unnecessary if-else in pca953x_read_regs() by spltting the
> routine into smaller, specialized functions and calling the right one
> via a function pointer held in struct pca953x.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 55 ++++++++++++++++++++++++++++++++--
> -----------
>  1 file changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 00bb2ea..e417c42 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -134,6 +134,7 @@ struct pca953x_chip {
>  	const struct pca953x_offset *offset;
>  
>  	int (*write_regs)(struct pca953x_chip *, int, u8 *);
> +	int (*read_regs)(struct pca953x_chip *, int, u8 *);
>  };
>  
>  static int pca953x_read_single(struct pca953x_chip *chip, int reg,
> u32 *val,
> @@ -219,24 +220,41 @@ static int pca953x_write_regs(struct
> pca953x_chip *chip, int reg, u8 *val)
>  	return 0;
>  }
>  
> -static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8
> *val)
> +static int pca953x_read_regs_8(struct pca953x_chip *chip, int reg, u8
> *val)
>  {
>  	int ret;
>  
> -	if (chip->gpio_chip.ngpio <= 8) {
> -		ret = i2c_smbus_read_byte_data(chip->client, reg);
> -		*val = ret;
> -	} else if (chip->gpio_chip.ngpio >= 24) {
> -		int bank_shift = fls((chip->gpio_chip.ngpio - 1) /
> BANK_SZ);
> +	ret = i2c_smbus_read_byte_data(chip->client, reg);
> +	*val = ret;
>  
> -		ret = i2c_smbus_read_i2c_block_data(chip->client,
> -					(reg << bank_shift) |
> REG_ADDR_AI,
> -					NBANK(chip), val);
> -	} else {
> -		ret = i2c_smbus_read_word_data(chip->client, reg <<
> 1);
> -		val[0] = (u16)ret & 0xFF;
> -		val[1] = (u16)ret >> 8;
> -	}
> +	return ret;
> +}
> +
> +static int pca953x_read_regs_16(struct pca953x_chip *chip, int reg,
> u8 *val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_word_data(chip->client, reg << 1);
> +	val[0] = (u16)ret & 0xFF;
> +	val[1] = (u16)ret >> 8;
> +
> +	return ret;
> +}
> +
> +static int pca953x_read_regs_24(struct pca953x_chip *chip, int reg,
> u8 *val)
> +{
> +	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> +
> +	return i2c_smbus_read_i2c_block_data(chip->client,
> +					     (reg << bank_shift) |
> REG_ADDR_AI,
> +					     NBANK(chip), val);
> +}
> +
> +static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8
> *val)
> +{
> +	int ret;
> +
> +	ret = chip->read_regs(chip, reg, val);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev, "failed reading
> register\n");
>  		return ret;
> @@ -766,10 +784,15 @@ static int pca953x_probe(struct i2c_client
> *client,
>  	 */
>  	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
>  
> -	if (chip->gpio_chip.ngpio <= 8)
> +	if (chip->gpio_chip.ngpio <= 8) {
>  		chip->write_regs = pca953x_write_regs_8;
> -	else if (chip->gpio_chip.ngpio >= 24)
> +		chip->read_regs = pca953x_read_regs_8;
> +	} else if (chip->gpio_chip.ngpio >= 24) {
>  		chip->write_regs = pca953x_write_regs_24;
> +		chip->read_regs = pca953x_read_regs_24;
> +	} else {
> +		chip->read_regs = pca953x_read_regs_16;
> +	}

For sake of consolidation stuff can we move write_regs_16 variants here?
It might require to refactor patch 3 as well


>  
>  	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
>  		ret = device_pca953x_init(chip, invert);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 4/5] gpio: pca953x: remove an unused variable

[Andy Shevchenko]

On Wed, 2016-09-07 at 11:24 +0200, Bartosz Golaszewski wrote:
> The chip_type variable in struct pca953x_chip is no longer required.
> 
> Remove it.

Would it be patch 4 in the series?

> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 6b62898..00bb2ea 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -129,7 +129,6 @@ struct pca953x_chip {
>  	struct i2c_client *client;
>  	struct gpio_chip gpio_chip;
>  	const char *const *names;
> -	int	chip_type;
>  	unsigned long driver_data;
>  
>  	const struct pca953x_offset *offset;
> @@ -760,8 +759,6 @@ static int pca953x_probe(struct i2c_client
> *client,
>  		}
>  	}
>  
> -	chip->chip_type = PCA_CHIP_TYPE(chip->driver_data);
> -
>  	mutex_init(&chip->i2c_lock);
>  
>  	/* initialize cached registers from their original values.
> @@ -774,7 +771,7 @@ static int pca953x_probe(struct i2c_client
> *client,
>  	else if (chip->gpio_chip.ngpio >= 24)
>  		chip->write_regs = pca953x_write_regs_24;
>  
> -	if (chip->chip_type == PCA953X_TYPE)
> +	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
>  		ret = device_pca953x_init(chip, invert);
>  	else
>  		ret = device_pca957x_init(chip, invert);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 0/5] gpio: pca953x: code refactoring

[Andy Shevchenko]

On Wed, 2016-09-07 at 11:24 +0200, Bartosz Golaszewski wrote:
> I'm working on converting the pca953x driver to using regmap, but
> since
> it's not a trivial task I figured I'd post a couple refactoring
> patches
> I did so far for 4.9.
> 
> The first patch just fixes a couple coding style issues. The second
> removes a couple unnecessary switches. Last three refactor the
> read/write_regs functions to avoid if-elses by using function pointers
> to smaller, specialized routines.
> 
> Tested with pca9534 and pca9535 chips.

Looks good to me overall, but please address my comments first.

> 
> v1 -> v2:
> - constified the offset structures in patch 2/5
> 
> Bartosz Golaszewski (5):
>   gpio: pca953x: coding style fixes
>   gpio: pca953x: code shrink
>   gpio: pca953x: refactor pca953x_write_regs()
>   gpio: pca953x: remove an unused variable
>   gpio: pca953x: refactor pca953x_read_regs()
> 
>  drivers/gpio/gpio-pca953x.c | 270 ++++++++++++++++++++++-------------
> ---------
>  1 file changed, 136 insertions(+), 134 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 4/5] gpio: pca953x: remove an unused variable

[Bartosz Golaszewski]

2016-09-07 13:36 GMT+02:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Wed, 2016-09-07 at 11:24 +0200, Bartosz Golaszewski wrote:
>> The chip_type variable in struct pca953x_chip is no longer required.
>>
>> Remove it.
>
> Would it be patch 4 in the series?

Hi Andy,

I'm afraid I don't understand the question. Could you elaborate?

Thanks,
Bartosz

Re: [PATCH v2 4/5] gpio: pca953x: remove an unused variable

[Andy Shevchenko]

On Wed, 2016-09-07 at 13:40 +0200, Bartosz Golaszewski wrote:
> 2016-09-07 13:36 GMT+02:00 Andy Shevchenko <andriy.shevchenko@linux.in
> tel.com>:
> > 
> > On Wed, 2016-09-07 at 11:24 +0200, Bartosz Golaszewski wrote:
> > > 
> > > The chip_type variable in struct pca953x_chip is no longer
> > > required.
> > > 
> > > Remove it.
> > 
> > Would it be patch 4 in the series?
> 
> Hi Andy,
> 
> I'm afraid I don't understand the question. Could you elaborate?

Same as for patch 1. This is not critical to have it before 5.
So, after rearrangement it would be 4th followed by 5th (aka patch 1 in
current series).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy