pinctrl: cy8c95x0: Bugfixes and cleanups
Subsystems (3)
cover pinctrl: cy8c95x0: Bugfixes and cleanups
This a set of the bugfixes and cleanups I have collected so far while testing the driver on Intel Galileo Gen 1 last year.
Andy Shevchenko (16):
pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware
pinctrl: cy8c95x0: Rename PWMSEL to SELPWM
pinctrl: cy8c95x0: Enable regmap locking for debug
pinctrl: cy8c95x0: Fix off-by-one in the regmap range settings
pinctrl: cy8c95x0: Remove incorrectly set fields in regmap
configuration
pinctrl: cy8c95x0: Avoid accessing reserved registers
pinctrl: cy8c95x0: Use better bitmap APIs where appropriate
pinctrl: cy8c95x0; Switch to use for_each_set_clump8()
pinctrl: cy8c95x0: Transform to cy8c95x0_regmap_read_bits()
pinctrl: cy8c95x0: Remove redundant check in
cy8c95x0_regmap_update_bits_base()
pinctrl: cy8c95x0: Replace 'return ret' by 'return 0' in some cases
pinctrl: cy8c95x0: Initialise boolean variable with boolean values
pinctrl: cy8c95x0: Get rid of cy8c95x0_pinmux_direction() forward
declaration
pinctrl: cy8c95x0: Drop unneeded casting
pinctrl: cy8c95x0: Separate EEPROM related register definitios
pinctrl: cy8c95x0: Fix comment style
drivers/pinctrl/pinctrl-cy8c95x0.c | 235 ++++++++++++++--------------- 1 file changed, 113 insertions(+), 122 deletions(-)
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 1/16 pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware
Som of the platforms may connect the INT pin via inversion logic effectively make the triggering to be active-low.
Remove explicit trigger flag to respect the settings from firmware.
Without this change even idling chip produces spurious interrupts and kernel disables the line in the result:
irq 33: nobody cared (try booting with the "irqpoll" option)
CPU: 0 UID: 0 PID: 125 Comm: irq/33-i2c-INT3 Not tainted 6.12.0-00236-g8b874ed11dae #64 Hardware name: Intel Corp. QUARK/Galileo, BIOS 0x01000900 01/01/2014 ... handlers: [<86e86bea>] irq_default_primary_handler threaded [<d153e44a>] cy8c95x0_irq_handler [pinctrl_cy8c95x0] Disabling IRQ #33
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +1 -1
@@ -1347,7 +1347,7 @@ static int cy8c95x0_irq_setup(struct cy8c95x0_pinctrl *chip, int irq) ret =devm_request_threaded_irq (chip->dev, irq,NULL , cy8c95x0_irq_handler, -IRQF_ONESHOT |IRQF_SHARED |IRQF_TRIGGER_HIGH , +IRQF_ONESHOT |IRQF_SHARED ,dev_name (chip->dev), chip);if (ret) {dev_err (chip->dev,"failed to request irq %d\n" , irq);
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
Re: [PATCH v1 01/16] pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware
Quoted 15 lines · Andy Shevchenko
Som of the platforms may connect the INT pin via inversion logic effectively make the triggering to be active-low.
Remove explicit trigger flag to respect the settings from firmware.Without this change even idling chip produces spurious interrupts and kernel disables the line in the result:
irq 33: nobody cared (try booting with the "irqpoll" option)
CPU: 0 UID: 0 PID: 125 Comm: irq/33-i2c-INT3 Not tainted 6.12.0-00236-g8b874ed11dae #64 Hardware name: Intel Corp. QUARK/Galileo, BIOS 0x01000900 01/01/2014 ... handlers: [<86e86bea>] irq_default_primary_handler threaded [<d153e44a>] cy8c95x0_irq_handler [pinctrl_cy8c95x0] Disabling IRQ #33
Quoted 2 lines
Fixes e6cbbe42944d ("pinctrl: Add Cypress cy8c95x0 support")Signed-off-by Andy Shevchenko <[email protected]>
Patch applied for fixes! (Speling mistake on first line fixed in the process.)
Yours,
Linus Walleij
Re: [PATCH v1 01/16] pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware
Quoted 18 lines · Linus Walleij
Quoted 15 lines · Andy Shevchenko
Som of the platforms may connect the INT pin via inversion logic effectively make the triggering to be active-low.
Remove explicit trigger flag to respect the settings from firmware.Without this change even idling chip produces spurious interrupts and kernel disables the line in the result:
irq 33: nobody cared (try booting with the "irqpoll" option)
CPU: 0 UID: 0 PID: 125 Comm: irq/33-i2c-INT3 Not tainted 6.12.0-00236-g8b874ed11dae #64 Hardware name: Intel Corp. QUARK/Galileo, BIOS 0x01000900 01/01/2014 ... handlers: [<86e86bea>] irq_default_primary_handler threaded [<d153e44a>] cy8c95x0_irq_handler [pinctrl_cy8c95x0] Disabling IRQ #33
Quoted 4 lines
Quoted 2 lines
Fixes e6cbbe42944d ("pinctrl: Add Cypress cy8c95x0 support")Signed-off-by Andy Shevchenko <[email protected]>Patch applied for fixes! (Speling mistake on first line fixed in the process.)
Thanks! Any plans to take the rest of the fixes (except the one which Patrick rightfully objected on)?
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH 2/16 pinctrl: cy8c95x0: Rename PWMSEL to SELPWM
In reply to Andy ShevchenkoThere are two registers in the hardware, one, "Select PWM", is per-port configuration enabling PWM function instead of GPIO. The other one is "PWM Select" is per-PWM selector to configure PWM itself. Original code uses abbreviation of the latter to describe the former. Rename it to follow the datasheet.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +7 -7
@@ -42,7 +42,7 @@#define CY8C95X0_PORTSEL 0x18/* Port settings, write PORTSEL first */ #define CY8C95X0_INTMASK 0x19 -#define CY8C95X0_PWMSEL 0x1A +#define CY8C95X0_SELPWM 0x1A#define CY8C95X0_INVERT 0x1B#define CY8C95X0_DIRECTION 0x1C/* Drive mode register change state on writing '1' */
@@ -365,8 +365,8 @@ static bool cy8c95x0_volatile_register(struct device *dev, unsigned int reg) case CY8C95X0_INPUT_(0) ... CY8C95X0_INPUT_(7): case CY8C95X0_INTSTATUS_(0) ... CY8C95X0_INTSTATUS_(7):case CY8C95X0_INTMASK : +case CY8C95X0_SELPWM :case CY8C95X0_INVERT : -case CY8C95X0_PWMSEL :case CY8C95X0_DIRECTION :case CY8C95X0_DRV_PU :case CY8C95X0_DRV_PD :
@@ -395,7 +395,7 @@ static bool cy8c95x0_muxed_register(unsigned int reg) {switch (reg) {case CY8C95X0_INTMASK : -case CY8C95X0_PWMSEL : +case CY8C95X0_SELPWM :case CY8C95X0_INVERT :case CY8C95X0_DIRECTION :case CY8C95X0_DRV_PU :
@@ -789,7 +789,7 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip, reg =CY8C95X0_DIRECTION ;break ;case PIN_CONFIG_MODE_PWM : - reg =CY8C95X0_PWMSEL ; + reg =CY8C95X0_SELPWM ;break ;case PIN_CONFIG_OUTPUT : reg =CY8C95X0_OUTPUT ;
@@ -868,7 +868,7 @@ static int cy8c95x0_gpio_set_pincfg(struct cy8c95x0_pinctrl *chip, reg =CY8C95X0_DRV_PP_FAST ;break ;case PIN_CONFIG_MODE_PWM : - reg =CY8C95X0_PWMSEL ; + reg =CY8C95X0_SELPWM ;break ;case PIN_CONFIG_OUTPUT_ENABLE :return cy8c95x0_pinmux_direction (chip, off, !arg);
@@ -1153,7 +1153,7 @@ static void cy8c95x0_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *bitmap_zero (mask,MAX_LINE );__set_bit (pin, mask); -if (cy8c95x0_read_regs_mask (chip,CY8C95X0_PWMSEL , pwm, mask)) { +if (cy8c95x0_read_regs_mask (chip,CY8C95X0_SELPWM , pwm, mask)) {seq_puts (s,"not available" );return ; }
@@ -1198,7 +1198,7 @@ static int cy8c95x0_set_mode(struct cy8c95x0_pinctrl *chip, unsigned int off, bou8 port =cypress_get_port (chip, off);u8 bit =cypress_get_pin_mask (chip, off); -return cy8c95x0_regmap_write_bits (chip,CY8C95X0_PWMSEL , port, bit, mode ? bit :0 ); +return cy8c95x0_regmap_write_bits (chip,CY8C95X0_SELPWM , port, bit, mode ? bit :0 ); } static int cy8c95x0_pinmux_mode(struct cy8c95x0_pinctrl *chip,
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 3/16 pinctrl: cy8c95x0: Enable regmap locking for debug
In reply to Andy ShevchenkoWhen regmap locking is disabled, debugfs is also disabled. Enable locking for debug when CONFIG_DEBUG_PINCTRL is set.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/pinctrl/pinctrl-cy8c95x0.c +4 -0
@@ -466,7 +466,11 @@ static const struct regmap_config cy8c9520_i2c_regmap = { .max_register = 0, /* Updated at runtime */ .num_reg_defaults_raw = 0, /* Updated at runtime */ .use_single_read = true, /* Workaround for regcache bug */ +#if IS_ENABLED (CONFIG_DEBUG_PINCTRL ) + .disable_locking = false, +#else .disable_locking = true, +#endif }; static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 4/16 pinctrl: cy8c95x0: Fix off-by-one in the regmap range settings
In reply to Andy ShevchenkoThe range_max is inclusive, so we need to use the number of the last accessible register address.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +3 -3
@@ -1442,15 +1442,15 @@ static int cy8c95x0_probe(struct i2c_client *client)switch (chip->tpin) {case 20 :strscpy (chip->name, cy8c95x0_id[0 ].name); - regmap_range_conf.range_max =CY8C95X0_VIRTUAL +3 *MUXED_STRIDE ; + regmap_range_conf.range_max =CY8C95X0_VIRTUAL +3 *MUXED_STRIDE -1 ;break ;case 40 :strscpy (chip->name, cy8c95x0_id[1 ].name); - regmap_range_conf.range_max =CY8C95X0_VIRTUAL +6 *MUXED_STRIDE ; + regmap_range_conf.range_max =CY8C95X0_VIRTUAL +6 *MUXED_STRIDE -1 ;break ;case 60 :strscpy (chip->name, cy8c95x0_id[2 ].name); - regmap_range_conf.range_max =CY8C95X0_VIRTUAL +8 *MUXED_STRIDE ; + regmap_range_conf.range_max =CY8C95X0_VIRTUAL +8 *MUXED_STRIDE -1 ;break ;default :return -ENODEV ;
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 5/16 pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration
In reply to Andy ShevchenkoWe don't provide defaults for the regmap, we shouldn't provide the number of them either.
Remove incorrectly set fields in regmap configuration.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 2 -- 1 file changed, 2 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +0 -2
@@ -464,7 +464,6 @@ static const struct regmap_config cy8c9520_i2c_regmap = { .ranges = NULL, /* Updated at runtime */ .num_ranges = 1, .max_register = 0, /* Updated at runtime */ - .num_reg_defaults_raw = 0, /* Updated at runtime */ .use_single_read = true,/* Workaround for regcache bug */ #if IS_ENABLED(CONFIG_DEBUG_PINCTRL) .disable_locking = false,
@@ -1475,7 +1474,6 @@ static int cy8c95x0_probe(struct i2c_client *client)memcpy (®map_conf, &cy8c9520_i2c_regmap,sizeof (regmap_conf)); regmap_conf.ranges = ®map_range_conf; regmap_conf.max_register = regmap_range_conf.range_max; - regmap_conf.num_reg_defaults_raw = regmap_range_conf.range_max; chip->regmap =devm_regmap_init_i2c (client, ®map_conf);if (IS_ERR (chip->regmap))
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
Patrick Rudolph
maintainerRe: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration
Hi Andy,
Quoted 3 lines · Andy Shevchenko
We don't provide defaults for the regmap, we shouldn't provide the number of them either.
The intention is to read back the defaults from HW to initialize the regmap cache.
The defaults are applied at each POR from the device's internal EEPROM. See regcache_hw_init() for details.
Quoted 32 lines
Remove incorrectly set fields in regmap configuration.
Fixes 8670de9fae49 ("pinctrl: cy8c95x0: Use regmap ranges")Signed-off-by Andy Shevchenko <[email protected]>--- drivers/pinctrl/pinctrl-cy8c95x0.c | 2 -- 1 file changed, 2 deletions(-)drivers/pinctrl/pinctrl-cy8c95x0.c +0
-3@@ -464,7 +464,6 @@ static const struct regmap_config cy8c9520_i2c_regmap = { .ranges = NULL, /* Updated at runtime */ .num_ranges = 1, .max_register = 0, /* Updated at runtime */ - .num_reg_defaults_raw = 0, /* Updated at runtime */ .use_single_read = true,/* Workaround for regcache bug */ #if IS_ENABLED(CONFIG_DEBUG_PINCTRL) .disable_locking = false,@@ -1475,7 +1474,6 @@ static int cy8c95x0_probe(struct i2c_client *client)memcpy (®map_conf, &cy8c9520_i2c_regmap,sizeof (regmap_conf)); regmap_conf.ranges = ®map_range_conf; regmap_conf.max_register = regmap_range_conf.range_max; - regmap_conf.num_reg_defaults_raw = regmap_range_conf.range_max; chip->regmap =devm_regmap_init_i2c (client, ®map_conf);if (IS_ERR (chip->regmap)) --2.43.0.rc1.1336.g36b5255a03ac
Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration
Quoted 10 lines · Patrick Rudolph
Hi Andy,
Quoted 3 lines · Andy Shevchenko
We don't provide defaults for the regmap, we shouldn't provide the number of them either.
The intention is to read back the defaults from HW to initialize the regmap cache.
The defaults are applied at each POR from the device's internal EEPROM. Seeregcache_hw_init()for details.
Yes, I was looking a lot and that code and it doesn't work as intended. I can reproduce the issue, but it's real issue and I don't think we need to read back anything from the HW forcibly. It will be done naturally, no?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration
Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · Patrick Rudolph
Hi Andy,
Quoted 3 lines · Andy Shevchenko
We don't provide defaults for the regmap, we shouldn't provide the number of them either.
Quoted 8 lines
Quoted 4 lines
The intention is to read back the defaults from HW to initialize the regmap cache.
The defaults are applied at each POR from the device's internal EEPROM. Seeregcache_hw_init()for details.Yes, I was looking a lot and that code and it doesn't work as intended. I can reproduce the issue, but it's real issue and I don't think we need to read back anything from the HW forcibly. It will be done naturally, no?
I think I now remember, it has a NULL pointer dereference.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Andy Shevchenko
Quoted 10 lines · Patrick Rudolph
Hi Andy,
Quoted 3 lines · Andy Shevchenko
We don't provide defaults for the regmap, we shouldn't provide the number of them either.
The intention is to read back the defaults from HW to initialize the regmap cache.
The defaults are applied at each POR from the device's internal EEPROM. Seeregcache_hw_init()for details.
Quoted 5 lines
Quoted 3 lines
Yes, I was looking a lot and that code and it doesn't work as intended. I can reproduce the issue, but it's real issue and I don't think we need to read back anything from the HW forcibly. It will be done naturally, no?
I think I now remember, it has a NULL pointer dereference.
Hmm... I have just tested and no issues found so far. I'm now puzzled what was that when I tested this the first time a few months ago...
Okay, let's say I will drop this patch in v2. Can you look at the rest of the series?
Thanks for review!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration
In reply to Andy ShevchenkoQuoted 12 lines · Andy Shevchenko
Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Patrick Rudolph
Quoted 3 lines · Andy Shevchenko
We don't provide defaults for the regmap, we shouldn't provide the number of them either.
The intention is to read back the defaults from HW to initialize the regmap cache.
The defaults are applied at each POR from the device's internal EEPROM. Seeregcache_hw_init()for details.
Quoted 5 lines
Quoted 3 lines
Yes, I was looking a lot and that code and it doesn't work as intended. I can reproduce the issue, but it's real issue and I don't think we need to read back anything from the HW forcibly. It will be done naturally, no?
I think I now remember, it has a NULL pointer dereference.
FWIW, okay, it was induced by a fix in regmap core, so this NULL pointer dereference is for real, but it's a regression in the regmap core.
For the reference read this thread:
[email protected].
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH 6/16 pinctrl: cy8c95x0: Avoid accessing reserved registers
In reply to Andy ShevchenkoThe checks for vrtual registers in the cy8c95x0_readable_register() and cy8c95x0_writeable_register() are not aligned and broken.
Fix that by explicitly avoiding reserved registers to be accessed.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +10 -6
@@ -328,14 +328,14 @@ static int cypress_get_pin_mask(struct cy8c95x0_pinctrl *chip, unsigned int pin)static bool cy8c95x0_readable_register (struct device *dev,unsigned int reg) {/* -* Only 12 registers are present per port (see Table 6 in the -* datasheet). +* Only 12 registers are present per port (see Table 6 in the datasheet). */ -if (reg >=CY8C95X0_VIRTUAL && (reg %MUXED_STRIDE ) <12 ) -return true; +if (reg >=CY8C95X0_VIRTUAL && (reg %MUXED_STRIDE ) >=12 ) +return false;switch (reg) { case 0x24 ... 0x27: + case 0x31 ... 0x3f:return false;default :return true;
@@ -344,8 +344,11 @@ static bool cy8c95x0_readable_register(struct device *dev, unsigned int reg)static bool cy8c95x0_writeable_register (struct device *dev,unsigned int reg) { -if (reg >=CY8C95X0_VIRTUAL ) -return true; +/* +* Only 12 registers are present per port (see Table 6 in the datasheet). +*/ +if (reg >=CY8C95X0_VIRTUAL && (reg %MUXED_STRIDE ) >=12 ) +return false;switch (reg) { case CY8C95X0_INPUT_(0) ... CY8C95X0_INPUT_(7):
@@ -353,6 +356,7 @@ static bool cy8c95x0_writeable_register(struct device *dev, unsigned int reg)case CY8C95X0_DEVID :return false; case 0x24 ... 0x27: + case 0x31 ... 0x3f:return false;default :return true;
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 7/16 pinctrl: cy8c95x0: Use better bitmap APIs where appropriate
In reply to Andy ShevchenkoThere are bitmap_gather() and bitmap_scatter() that are factually reimplemented in the driver. Use better bitmap APIs where appropriate.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 33 +++++++++++------------------- 1 file changed, 12 insertions(+), 21 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +12 -21
@@ -137,7 +137,7 @@ static const struct dmi_system_id cy8c95x0_dmi_acpi_irq_info[] = {* @irq_trig_low: I/O bits affected by a low voltage level * @irq_trig_high: I/O bits affected by a high voltage level * @push_pull: I/O bits configured as push pull driver -* @shiftmask: Mask used to compensate for Gport2 width +* @map: Mask used to compensate for Gport2 width * @nport: Number of Gports in this chip * @gpio_chip: gpiolib chip * @driver_data: private driver data
@@ -158,7 +158,7 @@ struct cy8c95x0_pinctrl {DECLARE_BITMAP (irq_trig_low,MAX_LINE );DECLARE_BITMAP (irq_trig_high,MAX_LINE );DECLARE_BITMAP (push_pull,MAX_LINE ); -DECLARE_BITMAP (shiftmask,MAX_LINE ); +DECLARE_BITMAP (map,MAX_LINE );unsigned int nport;struct gpio_chip gpio_chip;unsigned long driver_data;
@@ -621,13 +621,8 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,int ret;/* Add the 4 bit gap of Gport2 */ -bitmap_andnot (tmask, mask, chip->shiftmask,MAX_LINE ); -bitmap_shift_left (tmask, tmask,4 ,MAX_LINE ); -bitmap_replace (tmask, tmask, mask, chip->shiftmask,BANK_SZ *3 ); - -bitmap_andnot (tval, val, chip->shiftmask,MAX_LINE ); -bitmap_shift_left (tval, tval,4 ,MAX_LINE ); -bitmap_replace (tval, tval, val, chip->shiftmask,BANK_SZ *3 ); +bitmap_scatter (tmask, mask, chip->map,MAX_LINE ); +bitmap_scatter (tval, val, chip->map,MAX_LINE );for (unsigned int i =0 ; i < chip->nport; i++) {/* Skip over unused banks */
@@ -652,19 +647,13 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg, {DECLARE_BITMAP (tmask,MAX_LINE );DECLARE_BITMAP (tval,MAX_LINE ); -DECLARE_BITMAP (tmp,MAX_LINE );int read_val;u8 bits;int ret;/* Add the 4 bit gap of Gport2 */ -bitmap_andnot (tmask, mask, chip->shiftmask,MAX_LINE ); -bitmap_shift_left (tmask, tmask,4 ,MAX_LINE ); -bitmap_replace (tmask, tmask, mask, chip->shiftmask,BANK_SZ *3 ); - -bitmap_andnot (tval, val, chip->shiftmask,MAX_LINE ); -bitmap_shift_left (tval, tval,4 ,MAX_LINE ); -bitmap_replace (tval, tval, val, chip->shiftmask,BANK_SZ *3 ); +bitmap_scatter (tmask, mask, chip->map,MAX_LINE ); +bitmap_scatter (tval, val, chip->map,MAX_LINE );for (unsigned int i =0 ; i < chip->nport; i++) {/* Skip over unused banks */
@@ -684,8 +673,7 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg, }/* Fill the 4 bit gap of Gport2 */ -bitmap_shift_right (tmp, tval,4 ,MAX_LINE ); -bitmap_replace (val, tmp, tval, chip->shiftmask,MAX_LINE ); +bitmap_gather (tval, val, chip->map,MAX_LINE );return 0 ; }
@@ -1484,8 +1472,11 @@ static int cy8c95x0_probe(struct i2c_client *client)return PTR_ERR (chip->regmap);bitmap_zero (chip->push_pull,MAX_LINE ); -bitmap_zero (chip->shiftmask,MAX_LINE ); -bitmap_set (chip->shiftmask,0 ,20 ); + +/* Setup HW pins mapping */ +bitmap_fill (chip->map,MAX_LINE ); +bitmap_clear (chip->map,20 ,4 ); +mutex_init (&chip->i2c_lock);if (dmi_first_match (cy8c95x0_dmi_acpi_irq_info)) {
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 8/16 pinctrl: cy8c95x0; Switch to use for_each_set_clump8()
In reply to Andy Shevchenkofor_each_set_clump8() has embedded check for unset clump to skip. Switch driver to use for_each_set_clump8().
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +9 -15
@@ -616,21 +616,18 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg, {DECLARE_BITMAP (tmask,MAX_LINE );DECLARE_BITMAP (tval,MAX_LINE ); +unsigned long bits, offset;int write_val; -u8 bits;int ret;/* Add the 4 bit gap of Gport2 */ bitmap_scatter (tmask, mask, chip->map,MAX_LINE );bitmap_scatter (tval, val, chip->map,MAX_LINE ); -for (unsigned int i =0 ; i < chip->nport; i++) { -/* Skip over unused banks */ - bits =bitmap_get_value8 (tmask, i *BANK_SZ ); -if (!bits) -continue ; +for_each_set_clump8 (offset, bits, tmask, chip->tpin) { +unsigned int i = offset /8 ; - write_val =bitmap_get_value8 (tval, i *BANK_SZ ); + write_val =bitmap_get_value8 (tval, offset); ret =cy8c95x0_regmap_update_bits (chip, reg, i, bits, write_val);if (ret <0 ) {
@@ -647,19 +644,16 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg, {DECLARE_BITMAP (tmask,MAX_LINE );DECLARE_BITMAP (tval,MAX_LINE ); +unsigned long bits, offset;int read_val; -u8 bits;int ret;/* Add the 4 bit gap of Gport2 */ bitmap_scatter (tmask, mask, chip->map,MAX_LINE );bitmap_scatter (tval, val, chip->map,MAX_LINE ); -for (unsigned int i =0 ; i < chip->nport; i++) { -/* Skip over unused banks */ - bits =bitmap_get_value8 (tmask, i *BANK_SZ ); -if (!bits) -continue ; +for_each_set_clump8 (offset, bits, tmask, chip->tpin) { +unsigned int i = offset /8 ; ret =cy8c95x0_regmap_read (chip, reg, i, &read_val);if (ret <0 ) {
@@ -668,8 +662,8 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg, } read_val &= bits; - read_val |=bitmap_get_value8 (tval, i *BANK_SZ ) & ~bits; -bitmap_set_value8 (tval, read_val, i *BANK_SZ ); + read_val |=bitmap_get_value8 (tval, offset) & ~bits; +bitmap_set_value8 (tval, read_val, offset); }/* Fill the 4 bit gap of Gport2 */
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 9/16 pinctrl: cy8c95x0: Transform to cy8c95x0_regmap_read_bits()
In reply to Andy ShevchenkoThe returned value of cy8c95x0_regmap_read() is used always with a bitmask being applied. Move that bitmasking code into the function. At the same time transform it to cy8c95x0_regmap_read_bits() which will be in align with the write and update counterparts.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 45 +++++++++++++++++------------- 1 file changed, 25 insertions(+), 20 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +25 -20
@@ -574,12 +574,13 @@ static int cy8c95x0_regmap_update_bits(struct cy8c95x0_pinctrl *chip, unsigned i }/** -* cy8c95x0_regmap_read() - reads a register using the regmap cache +* cy8c95x0_regmap_read_bits() - reads a register using the regmap cache * @chip: The pinctrl to work on * @reg: The register to read from. Can be direct access or muxed register. * @port: The port to be used for muxed registers or quick path direct access * registers. Otherwise unused. -* @read_val: Value read from hardware or cache +* @mask: Bitmask to apply +* @val: Value read from hardware or cache * * This function handles the register reads from the direct access registers and * the muxed registers while caching all register accesses, internally handling
@@ -589,10 +590,12 @@ static int cy8c95x0_regmap_update_bits(struct cy8c95x0_pinctrl *chip, unsigned i* * Return: 0 for successful request, else a corresponding error value */ -static int cy8c95x0_regmap_read (struct cy8c95x0_pinctrl *chip,unsigned int reg, -unsigned int port,unsigned int *read_val) +static int cy8c95x0_regmap_read_bits (struct cy8c95x0_pinctrl *chip,unsigned int reg, +unsigned int port,unsigned int mask,unsigned int *val) { -int off, ret; +unsigned int off; +unsigned int tmp; +int ret;/* Registers behind the PORTSEL mux have their own range in regmap */ if (cy8c95x0_muxed_register (reg)) {
@@ -604,11 +607,14 @@ static int cy8c95x0_regmap_read(struct cy8c95x0_pinctrl *chip, unsigned int reg,else off = reg; } -guard (mutex)(&chip->i2c_lock); - ret =regmap_read (chip->regmap, off, read_val); +scoped_guard (mutex, &chip->i2c_lock) + ret = regmap_read(chip->regmap, off, &tmp); +if (ret) +return ret; -return ret; + *val = tmp & mask; +return 0 ; } static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
@@ -645,7 +651,7 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,DECLARE_BITMAP (tmask,MAX_LINE );DECLARE_BITMAP (tval,MAX_LINE );unsigned long bits, offset; -int read_val; +unsigned int read_val;int ret;/* Add the 4 bit gap of Gport2 */
@@ -655,13 +661,12 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,for_each_set_clump8 (offset, bits, tmask, chip->tpin) {unsigned int i = offset /8 ; - ret =cy8c95x0_regmap_read (chip, reg, i, &read_val); + ret =cy8c95x0_regmap_read_bits (chip, reg, i, bits, &read_val);if (ret <0 ) {dev_err (chip->dev,"failed reading register %d, port %u: err %d\n" , reg, i, ret);return ret; } - read_val &= bits; read_val |=bitmap_get_value8 (tval, offset) & ~bits;bitmap_set_value8 (tval, read_val, offset); }
@@ -698,10 +703,10 @@ static int cy8c95x0_gpio_get_value(struct gpio_chip *gc, unsigned int off)struct cy8c95x0_pinctrl *chip =gpiochip_get_data (gc);u8 port =cypress_get_port (chip, off);u8 bit =cypress_get_pin_mask (chip, off); -u32 reg_val; +unsigned int reg_val;int ret; - ret =cy8c95x0_regmap_read (chip,CY8C95X0_INPUT , port, ®_val); + ret =cy8c95x0_regmap_read_bits (chip,CY8C95X0_INPUT , port, bit, ®_val);if (ret <0 ) {/* * NOTE:
@@ -712,7 +717,7 @@ static int cy8c95x0_gpio_get_value(struct gpio_chip *gc, unsigned int off)return 0 ; } -return !!(reg_val & bit); +return reg_val ?1 :0 ; } static void cy8c95x0_gpio_set_value(struct gpio_chip *gc, unsigned int off,
@@ -730,14 +735,14 @@ static int cy8c95x0_gpio_get_direction(struct gpio_chip *gc, unsigned int off)struct cy8c95x0_pinctrl *chip =gpiochip_get_data (gc);u8 port =cypress_get_port (chip, off);u8 bit =cypress_get_pin_mask (chip, off); -u32 reg_val; +unsigned int reg_val;int ret; - ret =cy8c95x0_regmap_read (chip,CY8C95X0_DIRECTION , port, ®_val); + ret =cy8c95x0_regmap_read_bits (chip,CY8C95X0_DIRECTION , port, bit, ®_val);if (ret <0 )return ret; -if (reg_val & bit) +if (reg_val)return GPIO_LINE_DIRECTION_IN ;return GPIO_LINE_DIRECTION_OUT ;
@@ -750,8 +755,8 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,enum pin_config_param param =pinconf_to_config_param (*config);u8 port =cypress_get_port (chip, off);u8 bit =cypress_get_pin_mask (chip, off); +unsigned int reg_val;unsigned int reg; -u32 reg_val;u16 arg =0 ;int ret;
@@ -808,11 +813,11 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,* Writing 1 to one of the drive mode registers will automatically * clear conflicting set bits in the other drive mode registers. */ - ret =cy8c95x0_regmap_read (chip, reg, port, ®_val); + ret =cy8c95x0_regmap_read_bits (chip, reg, port, bit, ®_val);if (ret <0 )return ret; -if (reg_val & bit) +if (reg_val) arg =1 ;if (param ==PIN_CONFIG_OUTPUT_ENABLE ) arg = !arg;
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 10/16 pinctrl: cy8c95x0: Remove redundant check in cy8c95x0_regmap_update_bits_base()
In reply to Andy ShevchenkoThe function is never called with the PORTSEL register in the argument. Drop unneeded check, but rescue a comment. While at it, drop inline and allow any compiler to choose better stragy (note, that inline in C code is only a recomendation to most of the modern compilers anyway).
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +5 -11
@@ -476,20 +476,14 @@ static const struct regmap_config cy8c9520_i2c_regmap = {#endif }; -static inline int cy8c95x0_regmap_update_bits_base (struct cy8c95x0_pinctrl *chip, -unsigned int reg, -unsigned int port, -unsigned int mask, -unsigned int val, -bool *change,bool async, -bool force) +/* Caller should never modify PORTSEL directly */ +static int cy8c95x0_regmap_update_bits_base (struct cy8c95x0_pinctrl *chip, +unsigned int reg,unsigned int port, +unsigned int mask,unsigned int val, +bool *change,bool async,bool force) {int ret, off, i; -/* Caller should never modify PORTSEL directly */ -if (reg ==CY8C95X0_PORTSEL ) -return -EINVAL ; -/* Registers behind the PORTSEL mux have their own range in regmap */ if (cy8c95x0_muxed_register (reg)) { off =CY8C95X0_MUX_REGMAP_TO_OFFSET (reg, port);
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 11/16 pinctrl: cy8c95x0: Replace 'return ret' by 'return 0' in some cases
In reply to Andy ShevchenkoWhen it's known that the returned value can't be non-zero, use 'return 0' explicitly.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +3 -3
@@ -516,7 +516,7 @@ static int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,regcache_cache_only (chip->regmap, false); } -return ret; +return 0 ; }/**
@@ -1285,7 +1285,7 @@ static int cy8c95x0_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin, unsigned long *configs, unsigned int num_configs) { struct cy8c95x0_pinctrl *chip = pinctrl_dev_get_drvdata(pctldev); -int ret =0 ; +int ret;int i;for (i =0 ; i < num_configs; i++) {
@@ -1294,7 +1294,7 @@ static int cy8c95x0_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,return ret; } -return ret; +return 0 ; }static const struct pinconf_ops cy8c95x0_pinconf_ops = {
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 12/16 pinctrl: cy8c95x0: Initialise boolean variable with boolean values
In reply to Andy ShevchenkoThe 'ret' variable in cy8c95x0_irq_handler() is defined as bool, but is intialised with integers. Avoid implicit castings and initialise boolean variable with boolean values.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +2 -2
@@ -1075,7 +1075,7 @@ static irqreturn_t cy8c95x0_irq_handler(int irq, void *devid)if (!ret)return IRQ_RETVAL (0 ); - ret =0 ; + ret = false;for_each_set_bit (level, pending,MAX_LINE ) {/* Already accounted for 4bit gap in GPort2 */ nested_irq =irq_find_mapping (gc->irq.domain, level);
@@ -1094,7 +1094,7 @@ static irqreturn_t cy8c95x0_irq_handler(int irq, void *devid)else handle_nested_irq (nested_irq); - ret =1 ; + ret = true; }return IRQ_RETVAL (ret);
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 13/16 pinctrl: cy8c95x0: Get rid of cy8c95x0_pinmux_direction() forward declaration
In reply to Andy ShevchenkoThe function is used before being defined. Just move it up enough to get rid of forward declaration.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 54 ++++++++++++++---------------- 1 file changed, 25 insertions(+), 29 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +25 -29
@@ -310,9 +310,6 @@ static const char * const cy8c95x0_groups[] = { "gp77", }; -static int cy8c95x0_pinmux_direction (struct cy8c95x0_pinctrl *chip, -unsigned int pin,bool input); -static inline u8 cypress_get_port (struct cy8c95x0_pinctrl *chip,unsigned int pin) {/* Account for GPORT2 which only has 4 bits */
@@ -671,6 +668,31 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,return 0 ; } +static int cy8c95x0_pinmux_direction (struct cy8c95x0_pinctrl *chip,unsigned int pin,bool input) +{ +u8 port =cypress_get_port (chip, pin); +u8 bit =cypress_get_pin_mask (chip, pin); +int ret; + + ret =cy8c95x0_regmap_write_bits (chip,CY8C95X0_DIRECTION , port, bit, input ? bit :0 ); +if (ret) +return ret; + +/* +* Disable driving the pin by forcing it to HighZ. Only setting +* the direction register isn't sufficient in Push-Pull mode. +*/ +if (input &&test_bit (pin, chip->push_pull)) { + ret =cy8c95x0_regmap_write_bits (chip,CY8C95X0_DRV_HIZ , port, bit, bit); +if (ret) +return ret; + +__clear_bit (pin, chip->push_pull); + } + +return 0 ; +} +static int cy8c95x0_gpio_direction_input (struct gpio_chip *gc,unsigned int off) {return pinctrl_gpio_direction_input (gc, off);
@@ -1228,32 +1250,6 @@ static int cy8c95x0_gpio_request_enable(struct pinctrl_dev *pctldev,return cy8c95x0_set_mode (chip, pin, false); } -static int cy8c95x0_pinmux_direction (struct cy8c95x0_pinctrl *chip, -unsigned int pin,bool input) -{ -u8 port =cypress_get_port (chip, pin); -u8 bit =cypress_get_pin_mask (chip, pin); -int ret; - - ret =cy8c95x0_regmap_write_bits (chip,CY8C95X0_DIRECTION , port, bit, input ? bit :0 ); -if (ret) -return ret; - -/* -* Disable driving the pin by forcing it to HighZ. Only setting -* the direction register isn't sufficient in Push-Pull mode. -*/ -if (input &&test_bit (pin, chip->push_pull)) { - ret =cy8c95x0_regmap_write_bits (chip,CY8C95X0_DRV_HIZ , port, bit, bit); -if (ret) -return ret; - -__clear_bit (pin, chip->push_pull); - } - -return 0 ; -} -static int cy8c95x0_gpio_set_direction (struct pinctrl_dev *pctldev,struct pinctrl_gpio_range *range,unsigned int pin,bool input)
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 14/16 pinctrl: cy8c95x0: Drop unneeded casting
In reply to Andy ShevchenkoThe 'arg' variable in cy8c95x0_gpio_get_pincfg() is already type of u16. No need to cast it, so drop unneeded casting.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +1 -1
@@ -838,7 +838,7 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,if (param ==PIN_CONFIG_OUTPUT_ENABLE ) arg = !arg; - *config =pinconf_to_config_packed (param, (u16 )arg); + *config =pinconf_to_config_packed (param, arg);return 0 ; }
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 15/16 pinctrl: cy8c95x0: Separate EEPROM related register definitios
In reply to Andy ShevchenkoCurrently it's not easy to see at a glance the group of the registers that are per port. Add a blank line and a comment to make it better.
Also add a missing definition for one of the EEPROM related registers.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/pinctrl/pinctrl-cy8c95x0.c +4 -0
@@ -40,6 +40,7 @@/* Port Select configures the port */ #define CY8C95X0_PORTSEL 0x18 +/* Port settings, write PORTSEL first */ #define CY8C95X0_INTMASK 0x19#define CY8C95X0_SELPWM 0x1A
@@ -53,6 +54,9 @@#define CY8C95X0_DRV_PP_FAST 0x21#define CY8C95X0_DRV_PP_SLOW 0x22#define CY8C95X0_DRV_HIZ 0x23 + +/* Internal device configuration */ +#define CY8C95X0_ENABLE_WDE 0x2D#define CY8C95X0_DEVID 0x2E#define CY8C95X0_WATCHDOG 0x2F#define CY8C95X0_COMMAND 0x30
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac
PATCH 16/16 pinctrl: cy8c95x0: Fix comment style
In reply to Andy ShevchenkoOne comment style is not aligned with the rest. Fix that.
--- drivers/pinctrl/pinctrl-cy8c95x0.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +2 -1
@@ -501,7 +501,8 @@ static int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,if (ret <0 )return ret; -/* Mimic what hardware does and update the cache when a WC bit is written. +/* +* Mimic what hardware does and update the cache when a WC bit is written. * Allows to mark the registers as non-volatile and reduces I/O cycles. */ if (cy8c95x0_wc_register (reg) && (mask & val)) {
Signature · 1 line
2.43.0.rc1.1336.g36b5255a03ac