linux-kernel - Re: [PATCH v5 12/12] pinctrl: Add pin controller driver for AAEON UP boards

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0af29bac-ebe2-4448-b708-f0b9258544e7@bootlin.com>
Date: Wed, 7 May 2025 15:34:12 +0200
From: Thomas Richard <thomas.richard@...tlin.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Bartosz Golaszewski <brgl@...ev.pl>,
 Geert Uytterhoeven <geert+renesas@...der.be>, Kees Cook <kees@...nel.org>,
 Andy Shevchenko <andy@...nel.org>, linux-gpio@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
 DanieleCleri@...on.eu, GaryWang@...on.com.tw, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v5 12/12] pinctrl: Add pin controller driver for AAEON UP
 boards

On 5/7/25 08:57, Andy Shevchenko wrote:
> 
>> +#define UPBOARD_UP_PIN_MUX(bit, data)                          \
>> +       {                                                       \
>> +               .number = UPBOARD_UP_BIT_##bit,                 \
>> +               .name = "PINMUX_"#bit,                          \
>> +               .drv_data = (void *)(data),                     \
> 
> Wondering why we need to cast here and there if currently we all use
> constant driver data. Perhaps enable const for now and if we ever need
> that to be modified by the consumer we add that.

We need to cast as drv_data is not const, so the compiler complains.
Or I can remove const.
I looked at other drivers, some do a cast, others have not const driver
data.

> 
>> +       }
>> +
>> +#define UPBOARD_UP_PIN_FUNC(id, data)                          \
>> +       {                                                       \
>> +               .number = UPBOARD_UP_BIT_##id,                  \
>> +               .name = #id,                                    \
>> +               .drv_data = (void *)(data),                     \
> 
> Ditto.
> 
>> +       }
> 
> ...
> 
>> +static int upboard_pinctrl_register_groups(struct upboard_pinctrl *pctrl)
>> +{
>> +       const struct upboard_pingroup *groups = pctrl->pctrl_data->groups;
>> +       size_t ngroups = pctrl->pctrl_data->ngroups;
>> +       unsigned int i;
>> +       int ret;
>> +
>> +       for (i = 0; i < ngroups; i++) {
>> +               ret = pinctrl_generic_add_group(pctrl->pctldev, groups[i].grp.name,
>> +                                               groups[i].grp.pins, groups[i].grp.npins, pctrl);
> 
>> +               if (ret < 0)
> 
> Why ' < 0' ?

pinctrl_generic_add_group() returns the group selector which can be >=
0. So all values >= 0 are valid. [1]

[1]
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/pinctrl/core.c#L658-L676

> 
>> +                       return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int upboard_pinctrl_register_functions(struct upboard_pinctrl *pctrl)
>> +{
>> +       const struct pinfunction *funcs = pctrl->pctrl_data->funcs;
>> +       size_t nfuncs = pctrl->pctrl_data->nfuncs;
>> +       unsigned int i;
>> +       int ret;
>> +
>> +       for (i = 0; i < nfuncs ; i++) {
>> +               ret = pinmux_generic_add_function(pctrl->pctldev, funcs[i].name,
>> +                                                 funcs[i].groups, funcs[i].ngroups, NULL);
>> +               if (ret < 0)
> 
> Ditto.
> 
>> +                       return ret;
>> +       }
>> +
>> +       return 0;
>> +}
> 
> ...
> 
>> +       board_id = (enum upboard_board_id)(unsigned long)dmi_id->driver_data;
>> +
> 
> Unneeded blank line.
> 
>> +       switch (board_id) {
>> +       case UPBOARD_APL01:
>> +               pctrl->maps = upboard_pinctrl_mapping_apl01;
>> +               pctrl->nmaps = ARRAY_SIZE(upboard_pinctrl_mapping_apl01);
>> +               break;
>> +       default:
>> +               return dev_err_probe(dev, -ENODEV, "Unsupported board\n");
>> +       }
> 
> And actually we can get rid of that train of castings by switching to
> use the info type of the structure
> 
> (namings are just constructed without checking for the better or
> existing ones, choose another if you think they fit)
> 
> struct upboard_pinctrl_map {
>   ... *maps;
>   ... nmaps;
> };
> 
> static const struct upboard_pinctrl_map apl01 = {
>   ...
> };
> 
> ...dmi... = {
>   ...
>   .data = (void *)&apl01,
>   ...
> };
> 
> board_map = (const ...) dmi_id->driver_data;
> 
> ...
> 
> But since DMI will require castings, it's up to you as I don't like
> the idea of having that driver data not to be const in the first
> place.

I definitely prefer your proposal, so we can remove the switch case and
the enum.
I will just split the definition to compute automatically nmaps like this:

static const struct pinctrl_map pinctrl_map_apl01[] = {
        ...
};

static const struct upboard_pinctrl_map upboard_pinctrl_map_apl01 = {
	.maps = &pinctrl_map_apl01[0],
	.nmaps = ARRAY_SIZE(pinctrl_map_apl01),
};

Regards,

Thomas