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 <[email protected]>
---
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)
+
#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)
+
+#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);
+ u8 out_indconf_reg = pca953x_recalc_addr(
+ chip, PCAL6524_OUT_INDCONF, offset);
+ 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
On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <[email protected]> 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 <[email protected]>
Thats's nice!
> + * Output port configuration 0x40 + 7 * bank_size R
> + *
> + * - PCAL6524 with individual pin configuration
> + * Individual pin output config 0x40 + 12 * bank_size RW
So this will become 0x70? It's a bit hard for me this weird
register layout...
> +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);
> + u8 out_indconf_reg = pca953x_recalc_addr(
> + chip, PCAL6524_OUT_INDCONF, offset);
> + u8 mask = BIT(offset % BANK_SZ), val;
Split to two variable declarations please, this is hard to read.
> + unsigned int out_conf;
> + int ret;
So we set mask to the bit index for the line we want to affect.
> + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN)
> + val = mask;
> + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL)
> + val = 0;
> + else
> + return -EINVAL;
And this makes sense, we set it to 1 to enable open drain.
> + /* 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))
I suppose this could be written if (out_conf & mask)?
> + val ^= mask;
Invert? Why?
The datasheet says:
"If the ODENx bit is set at logic 0 (push-pull), any bit set to logic 1
in the IOCRx register will reverse the output state of that pin only
to open-drain. When ODENx bit is set at logic 1 (open-drain), a
logic 1 in IOCRx will set that pin to push-pull."
So your logic is accounting for the fact that someone go and set
one of the bits in ODENx to 1, but aren't they all by default set
to zero (or should be programmed by the driver to zero)
so that you can control open drain individually here by simply
setting the corresponding bit to 1 for open drain and 0 for
push-pull?
> + /* Configure the drive mode */
> + ret = regmap_write_bits(chip->regmap, out_indconf_reg, mask, val);
Yours,
Linus Walleij
On Tue, 2021-02-02 at 12:42 +0100, Bartosz Golaszewski wrote:
> On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <[email protected]>
> 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 <[email protected]>
> > ---
> > 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?
This is a flag that, like the above ones, indicate the existence of
registers, the types found under indicate a different register layout.
Misusing the naming convention would probably be confusing.
A generic name that don't reference the part type (PCA_???) would
perhaps be better but as I couldn't find any other part with these
registers I left it at that for now.
> > +
>
> No need for this newline.
I'll fix all the style issues you pointed out.
Alban
On Tue, 2021-02-02 at 14:57 +0100, Linus Walleij wrote:
> On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <[email protected]>
> 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 <[email protected]>
>
> Thats's nice!
>
> > + * Output port configuration 0x40 + 7 * bank_size R
> > + *
> > + * - PCAL6524 with individual pin configuration
> > + * Individual pin output config 0x40 + 12 * bank_size RW
>
> So this will become 0x70? It's a bit hard for me this weird
> register layout...
Correct.
> > +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);
> > + u8 out_indconf_reg = pca953x_recalc_addr(
> > + chip, PCAL6524_OUT_INDCONF, offset);
> > + u8 mask = BIT(offset % BANK_SZ), val;
>
> Split to two variable declarations please, this is hard to read.
Will do.
> > + unsigned int out_conf;
> > + int ret;
>
> So we set mask to the bit index for the line we want to affect.
>
> > + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN)
> > + val = mask;
> > + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL)
> > + val = 0;
> > + else
> > + return -EINVAL;
>
> And this makes sense, we set it to 1 to enable open drain.
>
> > + /* 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))
>
> I suppose this could be written if (out_conf & mask)?
No, the mask in ODENn is per bank (group of 8 pins) and not per pin.
> > + val ^= mask;
>
> Invert? Why?
>
> The datasheet says:
>
> "If the ODENx bit is set at logic 0 (push-pull), any bit set to
> logic 1
> in the IOCRx register will reverse the output state of that pin
> only
> to open-drain. When ODENx bit is set at logic 1 (open-drain), a
> logic 1 in IOCRx will set that pin to push-pull."
>
> So your logic is accounting for the fact that someone go and set
> one of the bits in ODENx to 1, but aren't they all by default set
> to zero (or should be programmed by the driver to zero)
> so that you can control open drain individually here by simply
> setting the corresponding bit to 1 for open drain and 0 for
> push-pull?
Yes the ODENx bits are 0 by default, but the bootloader might have
changed them for example. Currently the driver doesn't do any reset so
I think it make sense to correctly handle this case as it doesn't bring
that much extra complexity.
Alban
On Tue, Feb 2, 2021 at 6:45 PM Bedel, Alban <[email protected]> wrote:
> On Tue, 2021-02-02 at 14:57 +0100, Linus Walleij wrote:
> > > + if (out_conf & BIT(offset / BANK_SZ))
> >
> > I suppose this could be written if (out_conf & mask)?
>
> No, the mask in ODENn is per bank (group of 8 pins) and not per pin.
Ah I see it now (confused % for / ...)
> > The datasheet says:
> >
> > "If the ODENx bit is set at logic 0 (push-pull), any bit set to
> > logic 1
> > in the IOCRx register will reverse the output state of that pin
> > only
> > to open-drain. When ODENx bit is set at logic 1 (open-drain), a
> > logic 1 in IOCRx will set that pin to push-pull."
> >
> > So your logic is accounting for the fact that someone go and set
> > one of the bits in ODENx to 1, but aren't they all by default set
> > to zero (or should be programmed by the driver to zero)
> > so that you can control open drain individually here by simply
> > setting the corresponding bit to 1 for open drain and 0 for
> > push-pull?
>
> Yes the ODENx bits are 0 by default, but the bootloader might have
> changed them for example. Currently the driver doesn't do any reset so
> I think it make sense to correctly handle this case as it doesn't bring
> that much extra complexity.
Hm. I guess you're right if that is the style of the driver overall
(don't touch bootloader/reset defaults).
We don't use that style for interrupt registers because we don't
want interrupts to randomly fire, instead we usually disable them
all so the driver and Linux keeps track on what is enabled. But for
bias etc, I guess it is OK. But consider the option of just writing
0 into all the ODENx registers at probe(). I'm not gonna complain
if you really want it like this though.
Yours,
Linus Walleij
On Thu, Jan 28, 2021 at 5:38 PM Alban Bedel <[email protected]> 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
chips
family
> 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
configures
sets
> drive mode of the pins.
Can we actually convert the driver to be a pin control for the starter?
Or split it for some chips to be a pin control, while the rest keeps
just being GPIO.
I would like to avoid more complex changes in it until it's properly
converted to the pin control.
--
With Best Regards,
Andy Shevchenko
On Tue, Feb 2, 2021 at 1:45 PM Bartosz Golaszewski
<[email protected]> wrote:
> On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <[email protected]> wrote:
> >
> > From a quick glance at various datasheet the PCAL6524 seems to be the
Oh, even more typos
datasheets
> > only chip in this familly that support setting the drive mode of
supports
> > 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.
...
> Maybe call it PCAL6524_TYPE for consistency with the ones below?
In case you continue modifying this driver, I agree with Bart on
PCAL6524_TYPE along with new OF_6524() macro.
--
With Best Regards,
Andy Shevchenko