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
