Re: [PATCH v4 1/7] dt-bindings: pinctrl: renesas: Add alpha-numerical port support for RZ/V2H

[Rob Herring <robh@xxxxxxxxxx>]

On Tue, Dec 17, 2024 at 12:33:45PM +0000, Biju Das wrote:
> Hi Rob,
> 
> > -----Original Message-----
> > From: Rob Herring <robh@xxxxxxxxxx>
> > Sent: 17 December 2024 11:58
> > Subject: Re: [PATCH v4 1/7] dt-bindings: pinctrl: renesas: Add alpha-numerical port support for RZ/V2H
> > 
> > On Mon, Dec 16, 2024 at 07:53:11PM +0000, Biju Das wrote:
> > > RZ/V2H has ports P0-P9 and PA-PB. Add support for defining
> > > alpha-numerical ports in DT using RZV2H_* macros.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > ---
> > > v3->v4:
> > >  * Added new header file with separate RZV2H_P* definitions.
> > > v3:
> > >  * New patch.
> > > ---
> > >  .../pinctrl/renesas,r9a09g057-pinctrl.h       | 31 +++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >  create mode 100644
> > > include/dt-bindings/pinctrl/renesas,r9a09g057-pinctrl.h
> > >
> > > diff --git a/include/dt-bindings/pinctrl/renesas,r9a09g057-pinctrl.h
> > > b/include/dt-bindings/pinctrl/renesas,r9a09g057-pinctrl.h
> > > new file mode 100644
> > > index 000000000000..9008a7e71609
> > > --- /dev/null
> > > +++ b/include/dt-bindings/pinctrl/renesas,r9a09g057-pinctrl.h
> > > @@ -0,0 +1,31 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > +/*
> > > + * This header provides constants for Renesas RZ/V2H family pinctrl bindings.
> > > + *
> > > + * Copyright (C) 2024 Renesas Electronics Corp.
> > > + *
> > > + */
> > > +
> > > +#ifndef __DT_BINDINGS_RZV2H_PINCTRL_H #define
> > > +__DT_BINDINGS_RZV2H_PINCTRL_H
> > > +
> > > +#include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
> > > +
> > > +/* RZV2H_Px = Offset address of PFC_P_mn  - 0x20 */
> > > +#define RZV2H_P0	0
> > > +#define RZV2H_P1	1
> > > +#define RZV2H_P2	2
> > > +#define RZV2H_P3	3
> > > +#define RZV2H_P4	4
> > > +#define RZV2H_P5	5
> > > +#define RZV2H_P6	6
> > > +#define RZV2H_P7	7
> > > +#define RZV2H_P8	8
> > > +#define RZV2H_P9	9
> > > +#define RZV2H_PA	10
> > > +#define RZV2H_PB	11
> > 
> > I'm not a fan of defines which are just 'FOO_n n'. And these are if you speak hex.
> 
> For RZ/V2H, ports are P{0..9} and P{A..B}, the port name are with in hexa decimal range
> 
> whereas for RZ/G3E SoC,
> 
> Ports are P{0..8},P{A..H},P{J..M},PS
> 
> RZ/V2H and RZ/G3E are similar SoCs.
> 
> > 
> > > +
> > > +#define RZV2H_PORT_PINMUX(b, p, f)	RZG2L_PORT_PINMUX(RZV2H_P##b, p, f)
> > > +#define RZV2H_GPIO(port, pin)		RZG2L_GPIO(RZV2H_P##port, pin)
> > 
> > So the user does RZV2H_GPIO(A, 123) instead of RZV2H_GPIO(0xA, 123)? Not sure the bounds checking the
> > port is worth it. pin or function can still be crap.
> 
> Previously we were using plain number in DT for RZ/V2H, RZG2L_GPIO(10, pin) = RZG2L_GPIO(0xA, pin)
> The port names for RZ/G2L are plane number, whereas for RZ/G3E and RZ/V2H it's alpha numeric.
> 
> Since RZ/V2H ports are within the hexadecimal range, maybe this header file can be dropped
> at least for RZ/V2H??
> 
> Any way we are doing bounds check in driver.

If you are doing it elsewhere, then I guess it is fine for some 
consistency.

Rob