Hi Geert, On Thu, Jan 4, 2024 at 4:25 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > On Thu, Jan 4, 2024 at 4:55 PM Lad, Prabhakar > <prabhakar.csengg@xxxxxxxxx> wrote: > > On Tue, Jan 2, 2024 at 10:18 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > > On Thu, Dec 21, 2023 at 10:04 PM Lad, Prabhakar > > > <prabhakar.csengg@xxxxxxxxx> wrote: > > > > On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > > > > On Fri, Dec 1, 2023 at 2:16 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > > > > > > > Currently we assume all the port pins are sequential ie always PX_0 to > > > > > > PX_n (n=1..7) exist, but on RZ/Five SoC we have additional pins P19_1 to > > > > > > P28_5 which have holes in them, for example only one pin on port19 is > > > > > > available and that is P19_1 and not P19_0. So to handle such cases > > > > > > include pinmap for each port which would indicate the pin availability > > > > > > on each port. As the pincount can be calculated based on pinmap drop this > > > > > > from RZG2L_GPIO_PORT_PACK() macro and update RZG2L_GPIO_PORT_GET_PINCNT() > > > > > > macro. > > > > > > > > > > > > Previously we had a max of 7 pins on each port but on RZ/Five Port-20 > > > > > > has 8 pins, so move the single pin configuration to BIT(63). > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > @@ -80,15 +80,17 @@ > > > > > > * n indicates number of pins in the port, a is the register index > > > > > > * and f is pin configuration capabilities supported. > > > > > > */ > > > > > > -#define RZG2L_GPIO_PORT_PACK(n, a, f) (((n) << 28) | ((a) << 20) | (f)) > > > > > > -#define RZG2L_GPIO_PORT_GET_PINCNT(x) (((x) & GENMASK(30, 28)) >> 28) > > > > > > +#define RZG2L_GPIO_PORT_PACK(n, a, f) (((n) > 0 ? ((u64)(GENMASK_ULL(((n) - 1 + 28), 28))) : 0) | \ > > > > > > > > > > The mask creation can be simplified to > > > > > > > > > > ((1ULL << (n)) - 1) << 28 > > > > > > > > > OK. > > > > > > > > > but see below... > > > > > > > > > > > + ((a) << 20) | (f)) > > > > > > +#define RZG2L_GPIO_PORT_GET_PINMAP(x) (((x) & GENMASK_ULL(35, 28)) >> 28) > > > > > > +#define RZG2L_GPIO_PORT_GET_PINCNT(x) (hweight8(RZG2L_GPIO_PORT_GET_PINMAP((x)))) > > > > > > > > > > I think we've reached the point where it would be easier for the > > > > > casual reviewer to #define PIN_CFG_*_MASK for all fields, and use > > > > > FIELD_{PREP,GET}() to pack resp. extract values. That would also > > > > > make it more obvious which bits are in use, and how many bits are > > > > > still available for future use. > > > To clarify, you mean to define PIN_CFG_*_MASK for all > > PIN_CFG_IOLH_A..PIN_CFG_OEN macros? I ask because we dont extract the > > respective CFG flags in the code. > > The PIN_CFG_IOLH_A..PIN_CFG_OEN macros are single-bit definitions. > I mean to #define PIN_CFG_*_MASK macros for all multi-bit fields, currently > accessed using open-coded GENMASK(). > > You already tried: > > #define RZG2L_GPIO_PORT_PIN_CNT_MASK GENMASK(31, 28) > #define RZG2L_GPIO_PORT_PIN_REG_MASK GENMASK(27, 20) > #define RZG2L_GPIO_PORT_PIN_CFG_MASK GENMASK(19, 0) > > As they actually share the PIN_CFG_* bit space, I'd call them: > > #define PIN_CFG_PIN_CNT_MASK GENMASK(31, 28) > #define PIN_CFG_PIN_REG_MASK GENMASK(27, 20) > #define PIN_CFG_MASK GENMASK(19, 0) > > Also, you already have: > > #define MUX_PIN_ID_MASK GENMASK(15, 0) > #define MUX_FUNC_MASK GENMASK(31, 16) > #define MUX_FUNC_OFFS 16 > > But all of > > #define MUX_FUNC(pinconf) (((pinconf) & MUX_FUNC_MASK) >> > MUX_FUNC_OFFS) > > pins[i] = value & MUX_PIN_ID_MASK; > > can use FIELD_GET(), removing the need for MUX_FUNC_OFFS. > > Also: > > u8 pincount = RZG2L_GPIO_PORT_GET_PINCNT(cfg); > > can become > > u8 pincount = FIELD_GET(PIN_CFG_PIN_CNT_MASK, cfg); > > Same for all the other macros using GENMASK(). > > I hope this makes it more clear what I had in mind? > Thanks! > Thanks for the detailed explanation. I'll get that sorted soon. Cheers, Prabhakar