Hi Geert, Thank you for the review. On Wed, Dec 6, 2023 at 1:13 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > 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. > If I use the FIELD_PREP() macro like below I get build issues as below: #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) #define RZG2L_GPIO_PORT_PACK(n, a, f) FIELD_PREP(RZG2L_GPIO_PORT_PIN_CNT_MASK, n) | \ FIELD_PREP(RZG2L_GPIO_PORT_PIN_REG_MASK, a) | \ FIELD_PREP(RZG2L_GPIO_PORT_PIN_CFG_MASK, f) drivers/pinctrl/renesas/pinctrl-rzg2l.c:91:41: note: in expansion of macro 'FIELD_PREP' 91 | FIELD_PREP(RZG2L_GPIO_PORT_PIN_CFG_MASK, f) | ^~~~~~~~~~ drivers/pinctrl/renesas/pinctrl-rzg2l.c:1486:9: note: in expansion of macro 'RZG2L_GPIO_PORT_PACK' 1486 | RZG2L_GPIO_PORT_PACK(6, 0x2a, RZG3S_MPXED_PIN_FUNCS(A)), /* P18 */ | ^~~~~~~~~~~~~~~~~~~~ Do you have any pointers? Cheers, Prabhakar > > > > /* > > - * BIT(31) indicates dedicated pin, p is the register index while > > + * BIT(63) indicates dedicated pin, p is the register index while > > * referencing to SR/IEN/IOLH/FILxx registers, b is the register bits > > * (b * 8) and f is the pin configuration capabilities supported. > > */ > > -#define RZG2L_SINGLE_PIN BIT(31) > > +#define RZG2L_SINGLE_PIN BIT_ULL(63) > > #define RZG2L_SINGLE_PIN_PACK(p, b, f) (RZG2L_SINGLE_PIN | \ > > ((p) << 24) | ((b) << 20) | (f)) > > #define RZG2L_SINGLE_PIN_GET_BIT(x) (((x) & GENMASK(22, 20)) >> 20) > > Likewise. > > > @@ -180,12 +182,12 @@ struct rzg2l_hwcfg { > > > > struct rzg2l_dedicated_configs { > > const char *name; > > - u32 config; > > + u64 config; > > }; > > The rest LGTM. It's a pity we have to switch to 64 bits, but I'm > afraid there is not much we can do about that... > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds