* [PATCH v3 0/5] gpio: pca953x: code refactoring
@ 2016-09-07 13:37 Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 1/5] gpio: pca953x: code shrink Bartosz Golaszewski
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2016-09-07 13:37 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/5] gpio: pca953x: code shrink
2016-09-07 13:37 [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski
@ 2016-09-07 13:37 ` Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 2/5] gpio: pca953x: refactor pca953x_write_regs() Bartosz Golaszewski
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2016-09-07 13:37 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML, 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, ®_val, off);
+ ret = pca953x_read_single(chip, chip->offset->input, ®_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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/5] gpio: pca953x: refactor pca953x_write_regs()
2016-09-07 13:37 [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 1/5] gpio: pca953x: code shrink Bartosz Golaszewski
@ 2016-09-07 13:37 ` Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs() Bartosz Golaszewski
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2016-09-07 13:37 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs()
2016-09-07 13:37 [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 1/5] gpio: pca953x: code shrink Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 2/5] gpio: pca953x: refactor pca953x_write_regs() Bartosz Golaszewski
@ 2016-09-07 13:37 ` Bartosz Golaszewski
2016-09-07 13:56 ` Andy Shevchenko
2016-09-07 13:37 ` [PATCH v3 4/5] gpio: pca953x: remove an unused variable Bartosz Golaszewski
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Bartosz Golaszewski @ 2016-09-07 13:37 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 4/5] gpio: pca953x: remove an unused variable
2016-09-07 13:37 [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski
` (2 preceding siblings ...)
2016-09-07 13:37 ` [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs() Bartosz Golaszewski
@ 2016-09-07 13:37 ` Bartosz Golaszewski
2016-09-07 13:58 ` Andy Shevchenko
2016-09-07 13:37 ` [PATCH v3 5/5] gpio: pca953x: coding style fixes Bartosz Golaszewski
2016-09-07 14:00 ` [PATCH v3 0/5] gpio: pca953x: code refactoring Andy Shevchenko
5 siblings, 1 reply; 11+ messages in thread
From: Bartosz Golaszewski @ 2016-09-07 13:37 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 5/5] gpio: pca953x: coding style fixes
2016-09-07 13:37 [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski
` (3 preceding siblings ...)
2016-09-07 13:37 ` [PATCH v3 4/5] gpio: pca953x: remove an unused variable Bartosz Golaszewski
@ 2016-09-07 13:37 ` Bartosz Golaszewski
2016-09-07 14:00 ` Andy Shevchenko
2016-09-07 14:00 ` [PATCH v3 0/5] gpio: pca953x: code refactoring Andy Shevchenko
5 siblings, 1 reply; 11+ messages in thread
From: Bartosz Golaszewski @ 2016-09-07 13:37 UTC (permalink / raw)
To: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs()
2016-09-07 13:37 ` [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs() Bartosz Golaszewski
@ 2016-09-07 13:56 ` Andy Shevchenko
2016-09-07 14:51 ` Bartosz Golaszewski
0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2016-09-07 13:56 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/5] gpio: pca953x: remove an unused variable
2016-09-07 13:37 ` [PATCH v3 4/5] gpio: pca953x: remove an unused variable Bartosz Golaszewski
@ 2016-09-07 13:58 ` Andy Shevchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2016-09-07 13:58 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 5/5] gpio: pca953x: coding style fixes
2016-09-07 13:37 ` [PATCH v3 5/5] gpio: pca953x: coding style fixes Bartosz Golaszewski
@ 2016-09-07 14:00 ` Andy Shevchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2016-09-07 14:00 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/5] gpio: pca953x: code refactoring
2016-09-07 13:37 [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski
` (4 preceding siblings ...)
2016-09-07 13:37 ` [PATCH v3 5/5] gpio: pca953x: coding style fixes Bartosz Golaszewski
@ 2016-09-07 14:00 ` Andy Shevchenko
5 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2016-09-07 14:00 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot, Vignesh R,
Yong Li, Geert Uytterhoeven
Cc: linux-gpio, LKML
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs()
2016-09-07 13:56 ` Andy Shevchenko
@ 2016-09-07 14:51 ` Bartosz Golaszewski
0 siblings, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2016-09-07 14:51 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Alexandre Courbot, Vignesh R, Yong Li,
Geert Uytterhoeven, linux-gpio, LKML
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-09-07 14:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 13:37 [PATCH v3 0/5] gpio: pca953x: code refactoring Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 1/5] gpio: pca953x: code shrink Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 2/5] gpio: pca953x: refactor pca953x_write_regs() Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 3/5] gpio: pca953x: refactor pca953x_read_regs() Bartosz Golaszewski
2016-09-07 13:56 ` Andy Shevchenko
2016-09-07 14:51 ` Bartosz Golaszewski
2016-09-07 13:37 ` [PATCH v3 4/5] gpio: pca953x: remove an unused variable Bartosz Golaszewski
2016-09-07 13:58 ` Andy Shevchenko
2016-09-07 13:37 ` [PATCH v3 5/5] gpio: pca953x: coding style fixes Bartosz Golaszewski
2016-09-07 14:00 ` Andy Shevchenko
2016-09-07 14:00 ` [PATCH v3 0/5] gpio: pca953x: code refactoring Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).