Re: [PATCH v6 03/21] dt-bindings: pinctrl: Add starfive,jhb100-sys0-pinctrl

[Changhuang Liang]

Hi, Krzysztof,

> On 30/08/2026 10:57, Changhuang Liang wrote:
> > Hi, Krzysztof
> >
> > Thanks for the review.
> >
> >> On 30/08/2026 10:42, Changhuang Liang wrote:
> >>> Hi, Krzysztof
> >>>
> >>> Thanks for the review.
> >>>
> >>>> On 30/08/2026 03:29, Changhuang Liang wrote:
> >>>>> Hi, Krzysztof
> >>>>>
> >>>>> Thanks for the review.
> >>>>>
> >>>>>> On 29/08/2026 04:57, Changhuang Liang wrote:
> >>>>>>> +#ifndef __DT_BINDINGS_PINCTRL_STARFIVE_JHB100_H__
> >>>>>>> +#define __DT_BINDINGS_PINCTRL_STARFIVE_JHB100_H__
> >>>>>>> +
> >>>>>>> +/* sys0 pad numbers */
> >>>>>>> +#define PADNUM_SYS0_GPIO_A0				0
> >>>>>>> +#define PADNUM_SYS0_GPIO_A1				1
> >>>>>>> +#define PADNUM_SYS0_GPIO_A2				2
> >>>>>>> +#define PADNUM_SYS0_GPIO_A3				3
> >>>>>>
> >>>>>> I could not find how your driver uses these, so are you sure it's
> >>>>>> a binding constant for the SW?
> >>>>>
> >>>>> Yes, these are not used in the driver, but in PATCH 6/8/10/12,
> >>>>> etc., some definitions for other GPIOs are still used. Therefore,
> >>>>> I have uniformly defined them in the binding, and they will be
> >>>>> provided to the
> >>>> device tree for use later.
> >>>>
> >>>> I am confused. The driver in these patches 6/8/10/12 does include
> >>>> the header but does not use them. Why do you include unused
> >>>> headers? Or what is exactly the use?
> >>>
> >>> I'm sorry, I missed one thing. Actually, the current patches
> >> 4/6/8/10/12/14/16/18/20 all use this:
> >>>
> >>> For example, in patch 4:
> >>>
> >>> static const struct jhb100_pinctrl_func_maps jhb100_func_maps_sys0[] =
> {
> >>> 	{ .func = "auxpwrgood",		.val = 1 },
> >>> 	{ .func = "gpio",		.val = 0,
> >>> 	  .max_pin =
> >> JHB100_FUNC_MAPS_MAX_PIN(PADNUM_SYS0_GPIO_A2) },
> >>> 	{ .func = "gpio",		.val = 1,
> >>> 	  .max_pin =
> >> JHB100_FUNC_MAPS_MAX_PIN(PADNUM_SYS0_GPIO_A3) },
> >>> 	{ .func = "hbled",		.val = 1 },
> >>> 	{ .func = "pe2rst_out",		.val = 0 },
> >>> };
> >>>
> >>> These definitions are used in `struct jhb100_pinctrl_func_maps` or
> >>> `struct
> >> pinvref_desc`.
> >>
> >> OK, I was looking for PADNUM_SYS0_GPIO_A0.
> >>
> >> Why do you need unused constants? The constants in binding headers
> >> are interface between DTS and drivers, not hardware values. You do
> >> not need to store there hardware values, so what is the benefit of
> >> defining PADNUM_SYS0_GPIO_A0? Why can't PADNUM_SYS0_GPIO_A2
> start at
> >> index 0?
> >
> > Here, it is also provided for use by the device tree later. For example:
> >
> > 	auxpowergood0_pins: auxpowergood0-grp {
> > 		auxpowergood0-pins {
> > 			pins = <PADNUM_SYS0_GPIO_A0>;
> > 			function = "auxpwrgood";
> > 			input-enable;
> > 		};
> > 	};
> >
> > 	auxpowergood1_pins: auxpowergood1-grp {
> > 		auxpowergood1-pins {
> > 			pins = <PADNUM_SYS0_GPIO_A1>;
> > 			function = "auxpwrgood";
> > 			input-enable;
> > 		};
> > 	};
> >
> > The pins property corresponds to the specific GPIO offset, so
> PADNUM_SYS0_GPIO_A2 cannot start at index 0.
> 
> Did you even read my reply? Or you just respond whatever to tick the
> checkbox of responding to maintainer?
> 
> AGAIN:
> "You do not need to store there hardware values"
> 
> Do you understand the term ABI between software and DTS? Is in this term
> anything about your actual hardware numbers? No.

You are right, and I now see I was answering a different question than the one
You asked. 

So I'd like to double-check: in the next v8 version, I'll place the currently unmodified 
`starfive,jhb100-pinctrl.h` under `arch/riscv/boot/dts/starfive/`. For the macro 
definitions used by the driver (e.g., `PADNUM_SYS0_GPIO_A2`, `PADNUM_SYS0_GPIO_A3`), 
I'm not entirely sure where to put those driver-used definitions now. My plan is to place 
them directly into the relevant domain (`sys0`) file `pinctrl-starfive-jhb100-<domain>.c`. 
Would this change be acceptable?

Best Regards,
Changhuang