linux-kernel - 回复: [v2 1/3] pinctrl: cix: Add pin-controller support for sky1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <PUZPR06MB5887DA8FE0E128993F11E2B3EF16A@PUZPR06MB5887.apcprd06.prod.outlook.com>
Date: Thu, 18 Sep 2025 06:17:28 +0000
From: Gary Yang <gary.yang@...tech.com>
To: Linus Walleij <linus.walleij@...aro.org>
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:
 回复: [v2 1/3] pinctrl: cix: Add pin-controller support for sky1

Hi Linus,

New scheme with macros has verified ok. I just want to confirm with you before submit codes

> EXTERNAL EMAIL
> 
> 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.
> 
In header file:

struct sky1_pin_desc {
        const struct pinctrl_pin_desc pin;
        const char **func_group;
        unsigned int nfunc;
};

#define SKY_PINFUNCTION(_pin, _func)                            \
 (struct sky1_pin_desc) {                                        \
                 .pin = _pin,                                    \
                 .func_group = _func##_group,                    \
                 .nfunc = ARRAY_SIZE(_func##_group),             \
         }

In C file:

static const char *gpio1_group[] = {"GPIO1"};
static const char *gpio2_group[] = {"GPIO2"};

static const struct sky1_pin_desc sky1_pinctrl_s5_pads[] = {
        SKY_PINFUNCTION(PINCTRL_PIN(0, "GPIO1"), gpio1),
        SKY_PINFUNCTION(PINCTRL_PIN(1, "GPIO2"), gpio2),
		.......
};

What's your suggestion? Thanks

> 
> Yours,
> Linus Walleij

Best wishes
Gary