Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 05/27/2015 11:52 AM, Geert Uytterhoeven wrote:

>>>> The  pin array handled by sh_pfc_map_pins() may contain holes
>>>> representing
>>>> non- existing pins. We have to first count the valid pins in order to
>>>> calculate the size  of the memory to be allocated, then to skip over the
>>>> non-existing pins when  initializing the allocated arrays, and then to
>>>> return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
>>>> success.

>>>> As we have to touch devm_kzalloc() calls anyway, use more fitting
>>>> devm_kcalloc() instead which additionally checks the array size. And
>>>> since
>>>> PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed
>>>> out 'pmx->configs' array.

>>> I agree with this optimization, but I think it deserves a comment in the
>>> source code that explicitly mentions PINMUX_TYPE_NONE, to make sure any
>>> later
>>> rework changing the PINMUX_TYPE_NONE value would catch this.

>>     Note that this is not just "drove by" optimization, I was trying to avoid
>> the very need to explicitly initialize 'pmx->configs' array to
>> PINMUX_TYPE_NONE...

>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> [...]

>>>> @@ -622,7 +628,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
>>>>          pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
>>>>          pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
>>>>          pmx->pctl_desc.pins = pmx->pins;
>>>> -       pmx->pctl_desc.npins = pfc->info->nr_pins;
>>>> +       pmx->pctl_desc.npins = ret;
>>>>
>>>>          pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
>>>>          if (pmx->pctl == NULL)

>>> Shouldn't you also fix sh_pfc_init_ranges() ? It includes the following
>>> code
>>> that doesn't seem to handle holes properly.

>>>           for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
>>>                   if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin -
>>> 1)
>>>                           nr_ranges++;
>>>           }

>>> Please make sure that sh_pfc_get_pin_index() doesn't start blowing up
>>> afterwards though.

>>     I think I'm seeing a problem with this function (and it's not due to my
>> changes). Its result is often used to index 'pfc->info->pins' array. While
>> this is working now, when the holes are not yet allowed, it's going to break
>> when we start supporting the holes since that array couldn't be indexed this
>> way anymore (via ranges). This function is good only for 'pmx->configs' and
>> the like where we have already removed the holes. It looks like this holes
>> thing is going to be too complex, so how about leaving things as they are?
>> :-)

> Any conclusion on this?

    I did overestimate the complexity, it seems. :-)
    I have just removed the calls to sh_pfc_get_pin_index() on 
'pfc->info->pins' and its alias.

> If yes, please resend, incl. the r8a7794 pfc patches that depend on it.

    Will repost the patches RSN.

> Gr{oetje,eeting}s,
>                          Geert

WBR, Sergei