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

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.

Best Regards,
Changhuang
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv