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@xxxxxxxx> > 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@xxxxxxxx> > > > 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@xxxxxxxx > > > > > 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
Attachment:
signature.asc
Description: This is a digitally signed message part
