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

[PATCH v3 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

v2 -> v3:
- modified the order of the patches so that minor coding style fixes
  no longer create noise for the later changes
- moved the **_write_regs_16() assignments to where other variants
  are assigned

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

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

-- 
2.7.4

[PATCH v3 1/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 02f2a56..d47e6f9 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, offset = 0;
+	int ret;
 	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
 	int bank;
 
-	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++) {
@@ -383,7 +354,9 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 			reg_val[bank] = (reg_val[bank] & ~bankmask) | bankval;
 		}
 	}
-	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;
 
@@ -515,7 +488,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 */
@@ -540,15 +513,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;
 
@@ -608,20 +573,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;
 
@@ -684,6 +642,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;
@@ -709,6 +669,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 v3 2/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 | 73 ++++++++++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index d47e6f9..b3020ee 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;
@@ -754,6 +762,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)
+		chip->write_regs = pca953x_write_regs_8;
+	else if (chip->gpio_chip.ngpio >= 24)
+		chip->write_regs = pca953x_write_regs_24;
+	else
+		chip->write_regs = chip->chip_type == PCA953X_TYPE ?
+					pca953x_write_regs_16 :
+					pca957x_write_regs_16;
+
 	if (chip->chip_type == PCA953X_TYPE)
 		ret = device_pca953x_init(chip, invert);
 	else
-- 
2.7.4

[PATCH v3 3/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 | 56 +++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index b3020ee..018bd18 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -135,6 +135,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,
@@ -220,24 +221,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;
@@ -762,14 +780,18 @@ 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;
-	else
+		chip->read_regs = pca953x_read_regs_24;
+	} else {
 		chip->write_regs = chip->chip_type == PCA953X_TYPE ?
 					pca953x_write_regs_16 :
 					pca957x_write_regs_16;
+		chip->read_regs = pca953x_read_regs_16;
+	}
 
 	if (chip->chip_type == PCA953X_TYPE)
 		ret = device_pca953x_init(chip, invert);
-- 
2.7.4

[PATCH v3 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 | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 018bd18..b08ed52 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;
@@ -771,8 +770,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.
@@ -787,13 +784,14 @@ static int pca953x_probe(struct i2c_client *client,
 		chip->write_regs = pca953x_write_regs_24;
 		chip->read_regs = pca953x_read_regs_24;
 	} else {
-		chip->write_regs = chip->chip_type == PCA953X_TYPE ?
-					pca953x_write_regs_16 :
-					pca957x_write_regs_16;
+		if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
+			chip->write_regs = pca953x_write_regs_16;
+		else
+			chip->write_regs = pca957x_write_regs_16;
 		chip->read_regs = pca953x_read_regs_16;
 	}
 
-	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 v3 5/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 b08ed52..bbec5d7 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -360,25 +360,26 @@ 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;
+	int ret, bank;
 	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
-	int bank;
+	unsigned int bankmask, bankval;
 
 	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,
 					     chip->offset->output << bank_shift,
 					     NBANK(chip), reg_val);
-- 
2.7.4

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

[Andy Shevchenko]

On Wed, 2016-09-07 at 15:37 +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 | 56 +++++++++++++++++++++++++++++++-----
> ---------
>  1 file changed, 39 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index b3020ee..018bd18 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -135,6 +135,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,
> @@ -220,24 +221,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;

It's probably of out scope of this series, but looks like

if (ret < 0)
 return ret;

*val = ret;
return 0 (?);

> @@ -762,14 +780,18 @@ 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;
> -	else
> +		chip->read_regs = pca953x_read_regs_24;
> +	} else {
>  		chip->write_regs = chip->chip_type == PCA953X_TYPE ?
>  					pca953x_write_regs_16 :
>  					pca957x_write_regs_16;
> +		chip->read_regs = pca953x_read_regs_16;
> +	}

Would you move {} to the previous patch?

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

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

[Andy Shevchenko]

On Wed, 2016-09-07 at 15:37 +0200, Bartosz Golaszewski wrote:
> The chip_type variable in struct pca953x_chip is no longer required.
> 
> Remove it.
> 
> -		chip->write_regs = chip->chip_type == PCA953X_TYPE ?
> -					pca953x_write_regs_16 :
> -					pca957x_write_regs_16;
> +		if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE)
> +			chip->write_regs = pca953x_write_regs_16;
> +		else
> +			chip->write_regs = pca957x_write_regs_16;
>  		chip->read_regs = pca953x_read_regs_16;

I would also move this to previous patch.

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

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

[Andy Shevchenko]

On Wed, 2016-09-07 at 15:37 +0200, Bartosz Golaszewski wrote:
> 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 b08ed52..bbec5d7 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -360,25 +360,26 @@ 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;
> +	int ret, bank;
>  	int bank_shift = fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
> -	int bank;
> +	unsigned int bankmask, bankval;

I doubt it's the best representation. Can we use reversed xmas tree?
Also, I would not unify ret and bank on the same line.


>  
>  	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,
>  					     chip->offset->output <<
> bank_shift,
>  					     NBANK(chip), reg_val);

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

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

[Andy Shevchenko]

On Wed, 2016-09-07 at 15:37 +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.

Still few style comments, otherwise

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Tested with pca9534 and pca9535 chips.
> 
> v1 -> v2:
> - constified the offset structures in patch 2/5
> 
> v2 -> v3:
> - modified the order of the patches so that minor coding style fixes
>   no longer create noise for the later changes
> - moved the **_write_regs_16() assignments to where other variants
>   are assigned
> 
> Bartosz Golaszewski (5):
>   gpio: pca953x: code shrink
>   gpio: pca953x: refactor pca953x_write_regs()
>   gpio: pca953x: refactor pca953x_read_regs()
>   gpio: pca953x: remove an unused variable
>   gpio: pca953x: coding style fixes
> 
>  drivers/gpio/gpio-pca953x.c | 268 ++++++++++++++++++++++-------------
> ---------
>  1 file changed, 134 insertions(+), 134 deletions(-)
> 

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

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

[Bartosz Golaszewski]

2016-09-07 15:56 GMT+02:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Wed, 2016-09-07 at 15:37 +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 | 56 +++++++++++++++++++++++++++++++-----
>> ---------
>>  1 file changed, 39 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index b3020ee..018bd18 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -135,6 +135,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,
>> @@ -220,24 +221,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;
>
> It's probably of out scope of this series, but looks like
>
> if (ret < 0)
>  return ret;
>
> *val = ret;
> return 0 (?);
>

I'll look into it.

Thanks,
Bartosz