Message-ID: <CACRpkdYKnFAyq8C5h2=5NQ8AU92RmzShNHd6+=21rWednjv-fA@mail.gmail.com>
Date: Mon, 15 Sep 2025 10:35:37 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Gary Yang <gary.yang@...tech.com>
Cc: "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>, 
	"conor+dt@...nel.org" <conor+dt@...nel.org>, 
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>, 
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
	cix-kernel-upstream <cix-kernel-upstream@...tech.com>
Subject: Re: [v2 1/3] pinctrl: cix: Add pin-controller support for sky1

On Mon, Sep 15, 2025 at 9:09 AM Gary Yang <gary.yang@...tech.com> wrote:

> > Using a NULL func->name to terminate the array looks a bit dangerous.
>
> Checking whether this pointer is a null pointer is generally acceptable. A name maps to a mux value.
> I think that it is safe. Of course, your suggestion is also a good idea. If you think this is not safe, we will
> change codes as your suggestions.

It's OK just a suggestion. There are many ways to do this, first fix
other problems.

There are things in the language and the kernel that can help
you to check boundaries of arrays such as these functions so
you can't write code that index out of range, e.g.

+struct sky1_pin_desc {
+       const struct pinctrl_pin_desc pin;
+       const struct sky1_function_desc functions[4];
+};
+
+struct sky1_pinctrl_soc_info {
+       const struct sky1_pin_desc *pins;
+       unsigned int npins;
+};

It is possible to use a flexible array with the intrinsic
__counted by() here, e.g. instead of:

struct sky1_pin_desc {
     const struct pinctrl_pin_desc pin;
     const struct sky1_function_desc functions[4];

You can use:

+ size_t nfunctions;
+ const struct sky1_function_desc functions[] __counted_by(nfunctions);

If you grep counted_by in the kernel you find many other
examples of how we use this.

But flexible arrays is a bit complicated and dangerous so maybe you
want to avoid it altogether. Also I'm not sure it works when you put
things containing a flexible array into another array... I never tried it.

> > Then you can use nfuncs to iterate over the array of function names, and
> > define a macro like this:
> >
> > #define SKY_PINFUNCTION(_muxval, _functions, _nfunctions)   \
> > (struct sky1_function_desc) {                                  \
> >                 .muxval = (muxval),                        \
> >                 .functions = (_functions),                    \
> >                 .nfuncs = (_nfunctions),                  \
> >         }
> >
> > And then this:
> >
> > +static const struct sky1_pin_desc sky1_pinctrl_s5_pads[] = {
> > > +       {
> > > +               .pin = PINCTRL_PIN(0, "GPIO1"),
> > > +               .functions = {
> > > +                       [0] = {0, "GPIO1"},
> > > +               },
> > > +       },
> > > +       {
> > > +               .pin = PINCTRL_PIN(1, "GPIO2"),
> > > +               .functions = {
> > > +                       [0] = {0, "GPIO2"},
> > > +               },
> >
> > > +       },
> >
> > becomes
> >
> > static const struct sky1_pin_desc sky1_pinctrl_s5_pads[] = {
> >     SKY_PINFUNCTION(PINCTRL_PIN(0, "GPIO1"),  "GPIO1", 1),
> >     SKY_PINFUNCTION(PINCTRL_PIN(1, "GPIO2"),  "GPIO2", 1),
> >
> > I don't know about using the PINCTRL_PIN() macro here though, can't you just
> > put in 0, 1...?
> >
> > Anyway I think you get the idea.
> >
>
> First, let us review the struct sky1_pin_desc, it contains two members, one is the struct pinctrl_pin_desc.
>
> struct pinctrl_pin_desc {
>         unsigned int number;
>         const char *name;
>         void *drv_data;
> };
>
> PINCTRL_PIN is used to initialize this struct in kernel. It locates in include/linux/pinctrl/pinctrl.h
>
> #define PINCTRL_PIN(a, b) { .number = a, .name = b }
>
> PINCTRL_PIN(0, "GPIO1") defines a pin, its number is 0, its name is "GPIO1".

Ah I saw it wrong, sorry :(

You're right about this of course.

But I think you can still use a macro to define the long pin tables?
Albeit macros with flexible arguments is a bit hard to write.
Save it until everything else is working.

> > If this is the implied pattern for this driver, write as a comment to the above
> > function that this pin controller place all pins into a single group with one pin
> > and that this is why this works.
> >
> > The normal (as can be seen from the pin control documentation
> > https://docs.kernel.org/driver-api/pin-control.html ) is to group pins, so e.g.
> >
> > uart0_rx_tx_grp = { pin1, pin2 };
> > i2c0_sda_scl_grp = { pin1, pin2 };
> >
> > Then this is combined with functions such as uart0 and i2c0:
> >
> > function, group
> > ("uart0", uart0_rx_tx_grp)
> > ("i2c0", i2c0_sda_scl_grp)
> >
> > Here you see the two pins are used for uart in the first case and for i2c in the
> > second case, it's the same pins, but members of two different groups, and
> > these groups are then used with a function.
> >
> > The possible functions for a group are then defined somewhere so these
> > settings can be applied.
> >
> > Maybe this pattern is something you have in your driver because the code was
> > copied from some other driver which use one group per pin, it's not certain
> > that this is the best layout for the cix SoC so look it over!
>
> First maybe you ignore that fact the struct sky1_pinctrl_group is different from
> the struct group_desc. It only saves the config of a pin from dts. it doesn't include
> pin function part. As we know, a pin only has a config at the same time. One group map to a pin.
> The pin function part is included in the struct sky1_function_desc. One pin can map
> serval functions. There are four functions on sky1. We define the sky1_gpio_functions array.
> It is used to select the pin functions.
>
> Second, you are right. the pinctrl driver support new scheme is seldom in kernel. You take mt723 as
> an example before. Some codes come from the pinctrl driver on MTK. We modify them to adopt our platform.
> If I misunderstand or miss anything, please let me know.

Yes I can see that the driver is based on the MTK driver, my point is that
make sure you are not following it too closely, because what is good for
the mtk chips is not necessarily good for the cix chips.

But if you feel convenient with one group per pin and you are convinced
this is the best for your driver, go ahead with this scheme!

Yours,
Linus Walleij

Powered by blists - more mailing lists