On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <alban.be...@aerq.com> wrote: > > From a quick glance at various datasheet the PCAL6524 seems to be the > only chip in this familly that support setting the drive mode of > single pins. Other chips either don't support it at all, or can only > set the drive mode of whole banks, which doesn't map to the GPIO API. > > Add a new flag, PCAL6524, to mark chips that have the extra registers > needed for this feature. Then mark the needed register banks as > readable and writable, here we don't set OUT_CONF as writable, > although it is, as we only need to read it. Finally add a function > that configure the OUT_INDCONF register when the GPIO API set the > drive mode of the pins. > > Signed-off-by: Alban Bedel <alban.be...@aerq.com> > --- > drivers/gpio/gpio-pca953x.c | 64 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 61 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c > index 825b362eb4b7..db0b3dab1490 100644 > --- a/drivers/gpio/gpio-pca953x.c > +++ b/drivers/gpio/gpio-pca953x.c > @@ -64,6 +64,8 @@ > #define PCA_INT BIT(8) > #define PCA_PCAL BIT(9) > #define PCA_LATCH_INT (PCA_PCAL | PCA_INT) > +#define PCAL6524 BIT(10)
Maybe call it PCAL6524_TYPE for consistency with the ones below? > + No need for this newline. > #define PCA953X_TYPE BIT(12) > #define PCA957X_TYPE BIT(13) > #define PCA_TYPE_MASK GENMASK(15, 12) > @@ -88,7 +90,7 @@ static const struct i2c_device_id pca953x_id[] = { > { "pca9698", 40 | PCA953X_TYPE, }, > > { "pcal6416", 16 | PCA953X_TYPE | PCA_LATCH_INT, }, > - { "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT, }, > + { "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT | PCAL6524, }, > { "pcal9535", 16 | PCA953X_TYPE | PCA_LATCH_INT, }, > { "pcal9554b", 8 | PCA953X_TYPE | PCA_LATCH_INT, }, > { "pcal9555a", 16 | PCA953X_TYPE | PCA_LATCH_INT, }, > @@ -265,6 +267,9 @@ static int pca953x_bank_shift(struct pca953x_chip *chip) > #define PCAL9xxx_BANK_PULL_SEL BIT(8 + 4) > #define PCAL9xxx_BANK_IRQ_MASK BIT(8 + 5) > #define PCAL9xxx_BANK_IRQ_STAT BIT(8 + 6) > +#define PCAL9xxx_BANK_OUT_CONF BIT(8 + 7) > + No need for the newline here either. > +#define PCAL6524_BANK_INDOUT_CONF BIT(8 + 12) > > /* > * We care about the following registers: > @@ -288,6 +293,10 @@ static int pca953x_bank_shift(struct pca953x_chip *chip) > * Pull-up/pull-down select reg 0x40 + 4 * bank_size RW > * Interrupt mask register 0x40 + 5 * bank_size RW > * Interrupt status register 0x40 + 6 * bank_size R > + * Output port configuration 0x40 + 7 * bank_size R > + * > + * - PCAL6524 with individual pin configuration > + * Individual pin output config 0x40 + 12 * bank_size RW > * > * - Registers with bit 0x80 set, the AI bit > * The bit is cleared and the registers fall into one of the > @@ -336,9 +345,12 @@ static bool pca953x_readable_register(struct device > *dev, unsigned int reg) > if (chip->driver_data & PCA_PCAL) { > bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN | > PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK | > - PCAL9xxx_BANK_IRQ_STAT; > + PCAL9xxx_BANK_IRQ_STAT | PCAL9xxx_BANK_OUT_CONF; > } > > + if (chip->driver_data & PCAL6524) > + bank |= PCAL6524_BANK_INDOUT_CONF; > + > return pca953x_check_register(chip, reg, bank); > } > > @@ -359,6 +371,9 @@ static bool pca953x_writeable_register(struct device > *dev, unsigned int reg) > bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN | > PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK; > > + if (chip->driver_data & PCAL6524) > + bank |= PCAL6524_BANK_INDOUT_CONF; > + > return pca953x_check_register(chip, reg, bank); > } > > @@ -618,6 +633,46 @@ static int pca953x_gpio_set_pull_up_down(struct > pca953x_chip *chip, > return ret; > } > > +static int pcal6524_gpio_set_drive_mode(struct pca953x_chip *chip, > + unsigned int offset, > + unsigned long config) > +{ > + u8 out_conf_reg = pca953x_recalc_addr( > + chip, PCAL953X_OUT_CONF, 0); This line fits within the 80 characters limit. > + u8 out_indconf_reg = pca953x_recalc_addr( > + chip, PCAL6524_OUT_INDCONF, offset); And this could be broken like this: u8 out_indconf_reg = pca953x_recalc_addr(chip, PCAL6524_OUT_INDCONF, offset); Which is visually more pleasing. > + u8 mask = BIT(offset % BANK_SZ), val; > + unsigned int out_conf; > + int ret; > + > + /* configuration requires PCAL6524 extended registers */ > + if (!(chip->driver_data & PCAL6524)) > + return -ENOTSUPP; > + > + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN) > + val = mask; > + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL) > + val = 0; > + else > + return -EINVAL; > + > + mutex_lock(&chip->i2c_lock); > + > + /* Invert the value if ODENn is set */ > + ret = regmap_read(chip->regmap, out_conf_reg, &out_conf); > + if (ret) > + goto exit; > + if (out_conf & BIT(offset / BANK_SZ)) > + val ^= mask; > + > + /* Configure the drive mode */ > + ret = regmap_write_bits(chip->regmap, out_indconf_reg, mask, val); > + > +exit: > + mutex_unlock(&chip->i2c_lock); > + return ret; > +} > + > static int pca953x_gpio_set_config(struct gpio_chip *gc, unsigned int offset, > unsigned long config) > { > @@ -627,6 +682,9 @@ static int pca953x_gpio_set_config(struct gpio_chip *gc, > unsigned int offset, > case PIN_CONFIG_BIAS_PULL_UP: > case PIN_CONFIG_BIAS_PULL_DOWN: > return pca953x_gpio_set_pull_up_down(chip, offset, config); > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + return pcal6524_gpio_set_drive_mode(chip, offset, config); > default: > return -ENOTSUPP; > } > @@ -1251,7 +1309,7 @@ static const struct of_device_id pca953x_dt_ids[] = { > { .compatible = "nxp,pca9698", .data = OF_953X(40, 0), }, > > { .compatible = "nxp,pcal6416", .data = OF_953X(16, PCA_LATCH_INT), }, > - { .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_LATCH_INT), }, > + { .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_LATCH_INT | > PCAL6524), }, > { .compatible = "nxp,pcal9535", .data = OF_953X(16, PCA_LATCH_INT), }, > { .compatible = "nxp,pcal9554b", .data = OF_953X( 8, PCA_LATCH_INT), > }, > { .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_LATCH_INT), > }, > -- > 2.25.1 > Bart