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

[Thomas Richard <thomas.richard@bootlin.com>]

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