Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1478307 > unrolled thread

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

Started byBartosz Golaszewski <bgolaszewski@baylibre.com>
First post2016-09-07 15:40 +0200
Last post2016-09-07 16:10 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-07 15:40 +0200
    [PATCH v3 2/5] gpio: pca953x: refactor pca953x_write_regs() Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-07 15:40 +0200
    [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs() Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-07 15:40 +0200
      Re: [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-09-07 16:00 +0200
        Re: [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs() Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-07 17:00 +0200
    [PATCH v3 1/5] gpio: pca953x: code shrink Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-07 15:40 +0200
    Re: [PATCH v3 0/5] gpio: pca953x: code refactoring Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-09-07 16:10 +0200

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

FromBartosz Golaszewski <bgolaszewski@baylibre.com>
Date2016-09-07 15:40 +0200
Subject[PATCH v3 0/5] gpio: pca953x: code refactoring
Message-ID<seLf3-jD-17@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1478308 — [PATCH v3 2/5] gpio: pca953x: refactor pca953x_write_regs()

FromBartosz Golaszewski <bgolaszewski@baylibre.com>
Date2016-09-07 15:40 +0200
Subject[PATCH v3 2/5] gpio: pca953x: refactor pca953x_write_regs()
Message-ID<seLf4-jD-39@gated-at.bofh.it>
In reply to#1478307
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

[toc] | [prev] | [next] | [standalone]

#1478309 — [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs()

FromBartosz Golaszewski <bgolaszewski@baylibre.com>
Date2016-09-07 15:40 +0200
Subject[PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs()
Message-ID<seLf4-jD-43@gated-at.bofh.it>
In reply to#1478307
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

[toc] | [prev] | [next] | [standalone]

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

FromAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Date2016-09-07 16:00 +0200
SubjectRe: [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs()
Message-ID<seLyp-ql-5@gated-at.bofh.it>
In reply to#1478309
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

[toc] | [prev] | [next] | [standalone]

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

FromBartosz Golaszewski <bgolaszewski@baylibre.com>
Date2016-09-07 17:00 +0200
SubjectRe: [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs()
Message-ID<seMuu-13P-45@gated-at.bofh.it>
In reply to#1478318
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

[toc] | [prev] | [next] | [standalone]

#1478310 — [PATCH v3 1/5] gpio: pca953x: code shrink

FromBartosz Golaszewski <bgolaszewski@baylibre.com>
Date2016-09-07 15:40 +0200
Subject[PATCH v3 1/5] gpio: pca953x: code shrink
Message-ID<seLf4-jD-37@gated-at.bofh.it>
In reply to#1478307
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

[toc] | [prev] | [next] | [standalone]

#1478325

FromAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Date2016-09-07 16:10 +0200
Message-ID<seLI5-JF-3@gated-at.bofh.it>
In reply to#1478307
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web