Re: [PATCH] gpio: pca953x: add support for open drain pins on PCAL6524

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <alban.bedel@xxxxxxxx> 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.bedel@xxxxxxxx>
> ---
>  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