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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2857 bytes --]

On Wed, Sep 09, 2026 at 01:43:20AM +0000, Changhuang Liang wrote:
> Hi, Conor
> 
> Thanks for the review.
> 
> > On Mon, Aug 31, 2026 at 04:34:56AM -0700, Changhuang Liang wrote:
> > > diff --git a/include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> > > b/include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> > > new file mode 100644
> > > index 000000000000..6d8f5516a178
> > > --- /dev/null
> > > +++ b/include/dt-bindings/pinctrl/starfive,jhb100-pinctrl.h
> > > @@ -0,0 +1,17 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > > +/*
> > > + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> > > + *
> > > + * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > + */
> > > +
> > > +#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
> > 
> > Krzysztof's point [1] about these still stands. Pad indices aren't bindings.
> > Sure, your driver and your dts both might want to use these but that doesn't
> > make them a binding. For that to be the case, they need to effectively be
> > made up numbers - like how clocks are often listed with numbers from 0 into
> > the dozens or hundreds, when that may or may not correlate with actual bits
> > in registers, e.g. indices 0-31 in a clock binding might be in register 1 and then
> > 32-63 are in register 2. There's no need for a binding here to assign meanings
> > to numbers, because the meanings are assigned by the hardware itself - index
> > 0 for SYS0 *is* A0, because that's how the hardware is designed.
> > 
> > Were the numbers to run continuously, so that we had
> > 
> > #define JHB100_PADNUM_A0	0
> > #define JHB100_PADNUM_A1	1
> > #define JHB100_PADNUM_A2	2
> > #define JHB100_PADNUM_A3	3
> > #define JHB100_PADNUM_A4	4
> > and so on down to
> > #define JHB100_PADNUM__D0	1234
> > 
> > then it would be a binding, because we're assigning a meaning to 1234 that's
> > not something determined by the hardware.
> > 
> > FWIW, I'm happy to have the unchanged starfive,jhb100-pinctrl.h sit in
> > arch/riscv/boot/dts/starfive, because the defines are helpful - but as things
> > stand I think Krzysztof is right.
> 
> For the current series, the drivers also use some definitions from the binding. In 
> the next version, should the macro definitions be directly placed in their respective 
> pinctrl-starfive-jhb100-<domain>.c files?

Only the ones that are actually used, which I think is a limited subset
of the definitions in this header. At least, none of the ones that I
looked up were actually used in the drivers, except for the sys0 ones.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]