| Message ID | 20210211175140.85391-1-alban.bedel@aerq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpio: pca953x: add support for open drain pins on PCAL6524 | expand |
Hint: don't forget to include reviewers from previous version On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel <alban.bedel@aerq.com> wrote: > > From a quick glance at various datasheets the PCAL6524 and the > PCAL6534 seems to be the only chips in this family 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, PCAL65xx_REGS, 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 configures the OUT_INDCONF register when the GPIO API sets the > drive mode of the pins. ... > +#define PCAL65xx_REGS BIT(10) Can we have it as a _TYPE, please? ... > +#define PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12) IND is a bit ambiguous based on the description below. After you elaborate, I probably can propose better naming. ... > + * - PCAL65xx with individual pin configuration > + * Individual pin output config 0x40 + 12 * bank_size RW Not sure I understand what "individual" means here (no, I haven't looked into the datasheet). ... > + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN) > + val = mask; > + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL) > + val = 0; > + else > + return -EINVAL; Switch-case will look more naturally here (despite being longer in terms of LOCs). ... > +exit: exit_unlock: > + mutex_unlock(&chip->i2c_lock); > + return ret; ... > +#define OF_L65XX(__nrgpio) OF_953X(__nrgpio, PCA_LATCH_INT | PCAL65xx_REGS) When you change to the type, it will go accordingly. Don't add LATCH_INT to the macro.
On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > Hint: don't forget to include reviewers from previous version I added you to the CC list for the new patch, did I miss someone else? > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel <alban.bedel@aerq.com> > wrote: > > From a quick glance at various datasheets the PCAL6524 and the > > PCAL6534 seems to be the only chips in this family 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, PCAL65xx_REGS, 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 configures the OUT_INDCONF register when the GPIO API sets the > > drive mode of the pins. > > ... > > > +#define PCAL65xx_REGS BIT(10) > > Can we have it as a _TYPE, please? Let's please take a closer look at these macros and what they mean. Currently we have 3 possible set of functions that are indicated by setting bits in driver_data using the PCA_xxx macros: - Basic register only: 0 - With interrupt support: PCA_INT - With latching interrupt regs: PCA_INT | PCA_PCAL = PCA_LATCH_INT This patch then add a forth case: - With pin config regs: PCA_INT | PCA_PCAL | $MACRO_WE_ARE_DICUSSING Then there is the PCA953X_TYPE and PCA957X_TYPE macros which indicate the need for a different regmap config and register layout. These are accessed using the PCA_CHIP_TYPE() and are used as an integer value, not as bit-field like the above ones. If we had a struct instead of a packed integer that would be a different field. I'll change it to PCAL65xx_TYPE if you insist, but that seems very wrong to me to use the same naming convention for macros meant for different fields. > > > +#define PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12) > > IND is a bit ambiguous based on the description below. > After you elaborate, I probably can propose better naming. It's derived from the register name in the datasheet which is "Individual pin output port X configuration register". > > + * - PCAL65xx with individual pin configuration > > + * Individual pin output config 0x40 + 12 * bank_size RW > > Not sure I understand what "individual" means here (no, I haven't > looked into the datasheet). "individual" mean that each pin can be configured individually as opposed to other chips that only allow to configure a whole bank of pins. > > + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN) > > + val = mask; > > + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL) > > + val = 0; > > + else > > + return -EINVAL; > > Switch-case will look more naturally here (despite being longer in > terms of LOCs). Ok. > > +exit: > > exit_unlock: Will do. > > + mutex_unlock(&chip->i2c_lock); > > + return ret; > > ... > > > +#define OF_L65XX(__nrgpio) OF_953X(__nrgpio, PCA_LATCH_INT | > > PCAL65xx_REGS) > > When you change to the type, it will go accordingly. Don't add > LATCH_INT to the macro. As explained above all chips with these registers also have the registers indicated by PCA_LATCH_INT. Alban
On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban <alban.bedel@aerq.com> wrote: > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > > Hint: don't forget to include reviewers from previous version > > I added you to the CC list for the new patch, did I miss someone else? Then we are fine, thanks! > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel <alban.bedel@aerq.com> > > wrote: > > > From a quick glance at various datasheets the PCAL6524 and the > > > PCAL6534 seems to be the only chips in this family 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, PCAL65xx_REGS, 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 configures the OUT_INDCONF register when the GPIO API sets the > > > drive mode of the pins. Before continuing on this, have you considered to split this particular chip to a real pin controller and use the existing driver only for GPIO part of the functionality? ... > > > +#define PCAL65xx_REGS BIT(10) > > > > Can we have it as a _TYPE, please? > > Let's please take a closer look at these macros and what they mean. > Currently we have 3 possible set of functions that are indicated by > setting bits in driver_data using the PCA_xxx macros: > > - Basic register only: 0 > - With interrupt support: PCA_INT > - With latching interrupt regs: PCA_INT | PCA_PCAL = PCA_LATCH_INT > > This patch then add a forth case: > > - With pin config regs: PCA_INT | PCA_PCAL | $MACRO_WE_ARE_DICUSSING > > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which indicate > the need for a different regmap config and register layout. Exactly, and you have a different register layout (coincidentally overlaps with the original PCA953x). > These are > accessed using the PCA_CHIP_TYPE() and are used as an integer value, > not as bit-field like the above ones. If we had a struct instead of a > packed integer that would be a different field. How come? It's a bitmask. > I'll change it to PCAL65xx_TYPE if you insist, but that seems very > wrong to me to use the same naming convention for macros meant for > different fields. To me it's the opposite. The 6524 defines a new type (which has some registers others don't have). We even have already definitions of those special registers. I think TYPE is a better approach here. > > > +#define PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12) > > > > IND is a bit ambiguous based on the description below. > > After you elaborate, I probably can propose better naming. > > It's derived from the register name in the datasheet which is > "Individual pin output port X configuration register". Since we have already register definitions, if should follow existing pattern, i.e. OUT_INDCONF.
On Tue, 2021-02-16 at 19:50 +0200, Andy Shevchenko wrote: > On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban <alban.bedel@aerq.com> > wrote: > > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > > > Hint: don't forget to include reviewers from previous version > > > > I added you to the CC list for the new patch, did I miss someone > > else? > > Then we are fine, thanks! > > > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel <alban.bedel@aerq.com > > > > > > > wrote: > > > > From a quick glance at various datasheets the PCAL6524 and the > > > > PCAL6534 seems to be the only chips in this family 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, PCAL65xx_REGS, 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 configures the OUT_INDCONF register when the GPIO API sets > > > > the > > > > drive mode of the pins. > > Before continuing on this, have you considered to split this > particular chip to a real pin controller and use the existing driver > only for GPIO part of the functionality? No, this driver already use the ->set_config() mechanism so adding another property is trivial. On the other hand having a pinctrl driver would be a massive undertaking as the pinctrl API is quiet complex iirc. Furthermore, unless I'm missing something, that would not allow a consumer to request an open drain GPIO which is what I'm after. > > > > +#define PCAL65xx_REGS BIT(10) > > > > > > Can we have it as a _TYPE, please? > > > > Let's please take a closer look at these macros and what they mean. > > Currently we have 3 possible set of functions that are indicated by > > setting bits in driver_data using the PCA_xxx macros: > > > > - Basic register only: 0 > > - With interrupt support: PCA_INT > > - With latching interrupt regs: PCA_INT | PCA_PCAL = PCA_LATCH_INT > > > > This patch then add a forth case: > > > > - With pin config regs: PCA_INT | PCA_PCAL | > > $MACRO_WE_ARE_DICUSSING > > > > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which > > indicate > > the need for a different regmap config and register layout. > > Exactly, and you have a different register layout (coincidentally > overlaps with the original PCA953x). We have 2 layout for the base registers, the "mixed up registers" of the PCA957x and the "standard" of the PCA953x. Then we have the PCALxxxx chips which extend the base PCA953x registers with further registers for better interrupt handling. These are not treated as a new type in the current code, but as an extra feature on top of the PCA953x. The PCAL65xx we are talking about add a further register block, so following the existing concept its not a new layout. > > These are > > accessed using the PCA_CHIP_TYPE() and are used as an integer > > value, > > not as bit-field like the above ones. If we had a struct instead of > > a > > packed integer that would be a different field. > > How come? It's a bitmask. The definitions use BIT() but all evaluations of PCA_CHIP_TYPE() use the equality operator. > > I'll change it to PCAL65xx_TYPE if you insist, but that seems very > > wrong to me to use the same naming convention for macros meant for > > different fields. > > To me it's the opposite. The 6524 defines a new type (which has some > registers others don't have). We even have already definitions of > those special registers. I think TYPE is a better approach here. Let's look at pca953x_writeable_register() which I think clearly show how chips variants are currently handled. This is just one example but the rest of the code follows the same concept. static bool pca953x_writeable_register(struct device *dev, unsigned int reg) { struct pca953x_chip *chip = dev_get_drvdata(dev); u32 bank; if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) { bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG; } else { bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY | PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD; } if (chip->driver_data & PCA_PCAL) bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN | PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK; + if (chip->driver_data & PCAL65xx_REGS) + bank |= PCAL65xx_BANK_INDOUT_CONF; + return pca953x_check_register(chip, reg, bank); } The chip we are talking about further extend the PCAL registers, so it is currently covered by PCA953X_TYPE as base type and has the PCA_PCAL bit set. I really fails to see how this new type would nicely fit in the existing code. > > > > +#define PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12) > > > > > > IND is a bit ambiguous based on the description below. > > > After you elaborate, I probably can propose better naming. > > > > It's derived from the register name in the datasheet which is > > "Individual pin output port X configuration register". > > Since we have already register definitions, if should follow existing > pattern, i.e. OUT_INDCONF. Ok Alban
On Wed, Feb 17, 2021 at 3:11 PM Bedel, Alban <alban.bedel@aerq.com> wrote: > On Tue, 2021-02-16 at 19:50 +0200, Andy Shevchenko wrote: > > On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban <alban.bedel@aerq.com> > > wrote: > > > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > > > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel <alban.bedel@aerq.com > > > > wrote: ... > > Before continuing on this, have you considered to split this > > particular chip to a real pin controller and use the existing driver > > only for GPIO part of the functionality? > > No, this driver already use the ->set_config() mechanism so adding > another property is trivial. On the other hand having a pinctrl driver > would be a massive undertaking as the pinctrl API is quiet complex > iirc. > Furthermore, unless I'm missing something, that would not allow a > consumer to request an open drain GPIO which is what I'm after. Hmm... Linus, is it so? ... > > > > > +#define PCAL65xx_REGS BIT(10) > > > > > > > > Can we have it as a _TYPE, please? > > > > > > Let's please take a closer look at these macros and what they mean. > > > Currently we have 3 possible set of functions that are indicated by > > > setting bits in driver_data using the PCA_xxx macros: > > > > > > - Basic register only: 0 > > > - With interrupt support: PCA_INT > > > - With latching interrupt regs: PCA_INT | PCA_PCAL = PCA_LATCH_INT > > > > > > This patch then add a forth case: > > > > > > - With pin config regs: PCA_INT | PCA_PCAL | > > > $MACRO_WE_ARE_DICUSSING > > > > > > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which > > > indicate > > > the need for a different regmap config and register layout. > > > > Exactly, and you have a different register layout (coincidentally > > overlaps with the original PCA953x). > > We have 2 layout for the base registers, the "mixed up registers" of > the PCA957x and the "standard" of the PCA953x. Then we have the > PCALxxxx chips which extend the base PCA953x registers with further > registers for better interrupt handling. These are not treated as a new > type in the current code, but as an extra feature on top of the > PCA953x. Yes, because they are about interrupts AFAICS. > The PCAL65xx we are talking about add a further register > block, so following the existing concept its not a new layout. Yes, with one important detail, i.e. it extends the "mixed up" registers, it's not a separate "feature" in this sense. The separate "feature" can be, for example, PWM registers. I admit that this most of the angle of preference how to draw a line between the features. I prefer to see it as a type because of two things (in the current code): - OF_9*() macros take only two arguments, second of which is Interrupt related - PCA_INT group of bits is about Interrupt only Your proposal will disrupt the code (more invasive). Actually I prefer to see this chip as a pin controller, but it will be a longer road to pass, I suppose. ... > > > These are > > > accessed using the PCA_CHIP_TYPE() and are used as an integer > > > value, > > > not as bit-field like the above ones. If we had a struct instead of > > > a > > > packed integer that would be a different field. > > > > How come? It's a bitmask. > > The definitions use BIT() but all evaluations of PCA_CHIP_TYPE() use > the equality operator. I don't get how it's related. It's a bitmap and each bit uniquely defines the type. ... > > > I'll change it to PCAL65xx_TYPE if you insist, but that seems very > > > wrong to me to use the same naming convention for macros meant for > > > different fields. > > > > To me it's the opposite. The 6524 defines a new type (which has some > > registers others don't have). We even have already definitions of > > those special registers. I think TYPE is a better approach here. > > Let's look at pca953x_writeable_register() which I think clearly show > how chips variants are currently handled. This is just one example but > the rest of the code follows the same concept. > > static bool pca953x_writeable_register(struct device *dev, unsigned int reg) > { > struct pca953x_chip *chip = dev_get_drvdata(dev); > u32 bank; > > if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) { > bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY | > PCA953x_BANK_CONFIG; > } else { > bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY | > PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD; > } > > if (chip->driver_data & PCA_PCAL) > bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN | > PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK; > > + if (chip->driver_data & PCAL65xx_REGS) > + bank |= PCAL65xx_BANK_INDOUT_CONF; > + > return pca953x_check_register(chip, reg, bank); > } > > The chip we are talking about further extend the PCAL registers, so it > is currently covered by PCA953X_TYPE as base type and has the PCA_PCAL > bit set. I really fails to see how this new type would nicely fit in > the existing code. Use switch-case instead of if-else-if and it will bring you a better picture (not sure about __fallthrough be good or bad here). switch (PCA_CHIP_TYPE(chip->driver_data)) { case PCA6524_TYPE: ... __fallthrough; // perhaps better is to explicitly show what's going on case PCA953X_TYPE: default: // originally default seems PCA957X, but I guess this makes more sense ... break; case PCA957X_TYPE: ... break; }
On Wed, 2021-02-17 at 16:19 +0200, Andy Shevchenko wrote: > On Wed, Feb 17, 2021 at 3:11 PM Bedel, Alban <alban.bedel@aerq.com> > wrote: > > On Tue, 2021-02-16 at 19:50 +0200, Andy Shevchenko wrote: > > > On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban < > > > alban.bedel@aerq.com> > > > wrote: > > > > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > > > > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel < > > > > > alban.bedel@aerq.com > > > > > wrote: > > ... > > > > > > > +#define PCAL65xx_REGS BIT(10) > > > > > > > > > > Can we have it as a _TYPE, please? > > > > > > > > Let's please take a closer look at these macros and what they > > > > mean. > > > > Currently we have 3 possible set of functions that are > > > > indicated by > > > > setting bits in driver_data using the PCA_xxx macros: > > > > > > > > - Basic register only: 0 > > > > - With interrupt support: PCA_INT > > > > - With latching interrupt regs: PCA_INT | PCA_PCAL = > > > > PCA_LATCH_INT > > > > > > > > This patch then add a forth case: > > > > > > > > - With pin config regs: PCA_INT | PCA_PCAL | > > > > $MACRO_WE_ARE_DICUSSING > > > > > > > > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which > > > > indicate > > > > the need for a different regmap config and register layout. > > > > > > Exactly, and you have a different register layout (coincidentally > > > overlaps with the original PCA953x). > > > > We have 2 layout for the base registers, the "mixed up registers" > > of > > the PCA957x and the "standard" of the PCA953x. Then we have the > > PCALxxxx chips which extend the base PCA953x registers with further > > registers for better interrupt handling. These are not treated as a > > new > > type in the current code, but as an extra feature on top of the > > PCA953x. > > Yes, because they are about interrupts AFAICS. This distinction seems arbitrary, each more advanced version of the chip just has more features along with a new register block. > > The PCAL65xx we are talking about add a further register > > block, so following the existing concept its not a new layout. > > Yes, with one important detail, i.e. it extends the "mixed up" > registers, it's not a separate "feature" in this sense. The separate > "feature" can be, for example, PWM registers. I admit that this most > of the angle of preference how to draw a line between the features. > > I prefer to see it as a type because of two things (in the current > code): > - OF_9*() macros take only two arguments, second of which is > Interrupt related > - PCA_INT group of bits is about Interrupt only No, the register set indicated by PCA_PCAL also allow setting pull up/down which is supported by this driver. Furthermore the extra registers of the PCAL65XX also allow configuring edge triggered mode for interrupts. I really don't see why there should be 2 class of features, that only make the code more complex. > Your proposal will disrupt the code (more invasive). I tried to implement what you like to see: 1 file changed, 105 insertions(+), 20 deletions(-) vs my proposal: 1 file changed, 65 insertions(+), 3 deletions(-) Alban
On Wed, Feb 17, 2021 at 06:57:20PM +0000, Bedel, Alban wrote: > On Wed, 2021-02-17 at 16:19 +0200, Andy Shevchenko wrote: > > On Wed, Feb 17, 2021 at 3:11 PM Bedel, Alban <alban.bedel@aerq.com> > > wrote: > > > On Tue, 2021-02-16 at 19:50 +0200, Andy Shevchenko wrote: > > > > On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban < > > > > alban.bedel@aerq.com> > > > > wrote: > > > > > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote: > > > > > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel < > > > > > > alban.bedel@aerq.com > > > > > > wrote: > > > > ... > > > > > > > > > +#define PCAL65xx_REGS BIT(10) > > > > > > > > > > > > Can we have it as a _TYPE, please? > > > > > > > > > > Let's please take a closer look at these macros and what they > > > > > mean. > > > > > Currently we have 3 possible set of functions that are > > > > > indicated by > > > > > setting bits in driver_data using the PCA_xxx macros: > > > > > > > > > > - Basic register only: 0 > > > > > - With interrupt support: PCA_INT > > > > > - With latching interrupt regs: PCA_INT | PCA_PCAL = > > > > > PCA_LATCH_INT > > > > > > > > > > This patch then add a forth case: > > > > > > > > > > - With pin config regs: PCA_INT | PCA_PCAL | > > > > > $MACRO_WE_ARE_DICUSSING > > > > > > > > > > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which > > > > > indicate > > > > > the need for a different regmap config and register layout. > > > > > > > > Exactly, and you have a different register layout (coincidentally > > > > overlaps with the original PCA953x). > > > > > > We have 2 layout for the base registers, the "mixed up registers" > > > of > > > the PCA957x and the "standard" of the PCA953x. Then we have the > > > PCALxxxx chips which extend the base PCA953x registers with further > > > registers for better interrupt handling. These are not treated as a > > > new > > > type in the current code, but as an extra feature on top of the > > > PCA953x. > > > > Yes, because they are about interrupts AFAICS. > > This distinction seems arbitrary, each more advanced version of the > chip just has more features along with a new register block. > > > > The PCAL65xx we are talking about add a further register > > > block, so following the existing concept its not a new layout. > > > > Yes, with one important detail, i.e. it extends the "mixed up" > > registers, it's not a separate "feature" in this sense. The separate > > "feature" can be, for example, PWM registers. I admit that this most > > of the angle of preference how to draw a line between the features. > > > > I prefer to see it as a type because of two things (in the current > > code): > > - OF_9*() macros take only two arguments, second of which is > > Interrupt related > > - PCA_INT group of bits is about Interrupt only > > No, the register set indicated by PCA_PCAL also allow setting pull > up/down which is supported by this driver. Furthermore the extra > registers of the PCAL65XX also allow configuring edge triggered mode > for interrupts. I really don't see why there should be 2 class of > features, that only make the code more complex. > > > Your proposal will disrupt the code (more invasive). > > I tried to implement what you like to see: > > 1 file changed, 105 insertions(+), 20 deletions(-) > > vs my proposal: > > 1 file changed, 65 insertions(+), 3 deletions(-) Do you have any repo to look for both solutions?
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 825b362eb4b7..c5c216ddfe18 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -64,6 +64,7 @@ #define PCA_INT BIT(8) #define PCA_PCAL BIT(9) #define PCA_LATCH_INT (PCA_PCAL | PCA_INT) +#define PCAL65xx_REGS BIT(10) #define PCA953X_TYPE BIT(12) #define PCA957X_TYPE BIT(13) #define PCA_TYPE_MASK GENMASK(15, 12) @@ -88,7 +89,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 | PCAL65xx_REGS, }, { "pcal9535", 16 | PCA953X_TYPE | PCA_LATCH_INT, }, { "pcal9554b", 8 | PCA953X_TYPE | PCA_LATCH_INT, }, { "pcal9555a", 16 | PCA953X_TYPE | PCA_LATCH_INT, }, @@ -265,6 +266,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 PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12) /* * We care about the following registers: @@ -288,6 +292,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 + * + * - PCAL65xx 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 +344,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 & PCAL65xx_REGS) + bank |= PCAL65xx_BANK_INDOUT_CONF; + return pca953x_check_register(chip, reg, bank); } @@ -359,6 +370,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 & PCAL65xx_REGS) + bank |= PCAL65xx_BANK_INDOUT_CONF; + return pca953x_check_register(chip, reg, bank); } @@ -618,6 +632,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_indconf_reg = pca953x_recalc_addr(chip, PCAL6524_OUT_INDCONF, + offset); + u8 out_conf_reg = pca953x_recalc_addr(chip, PCAL953X_OUT_CONF, 0); + u8 mask = BIT(offset % BANK_SZ); + unsigned int out_conf; + int ret; + u8 val; + + /* configuration requires the PCAL65xx extended registers */ + if (!(chip->driver_data & PCAL65xx_REGS)) + 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 +681,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; } @@ -1232,6 +1289,7 @@ static int pca953x_resume(struct device *dev) /* convenience to stop overlong match-table lines */ #define OF_953X(__nrgpio, __int) (void *)(__nrgpio | PCA953X_TYPE | __int) #define OF_957X(__nrgpio, __int) (void *)(__nrgpio | PCA957X_TYPE | __int) +#define OF_L65XX(__nrgpio) OF_953X(__nrgpio, PCA_LATCH_INT | PCAL65xx_REGS) static const struct of_device_id pca953x_dt_ids[] = { { .compatible = "nxp,pca6416", .data = OF_953X(16, PCA_INT), }, @@ -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_L65XX(24), }, { .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), },
From a quick glance at various datasheets the PCAL6524 and the PCAL6534 seems to be the only chips in this family 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, PCAL65xx_REGS, 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 configures the OUT_INDCONF register when the GPIO API sets the drive mode of the pins. Signed-off-by: Alban Bedel <alban.bedel@aerq.com> --- v2: - Rename the feature flag to PCAL65xx_REGS as there is at least the PCAL6534 which also have this feature. - Fixed the typos in the log message. - Fixed the code style issues. --- drivers/gpio/gpio-pca953x.c | 64 +++++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 3 deletions(-)