Re: [PATCH 1/2] pinctrl/nomadik: add STn8815 ASIC support

[Stephen Warren <swarren@wwwdotorg.org>]

On 06/12/2012 05:28 AM, Linus Walleij wrote:
> On Fri, Jun 8, 2012 at 6:39 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 06/08/2012 03:07 AM, Linus Walleij wrote:
>>> This adds support for the STN8815 ASIC for the Nomadik pin
>>> controller.
>>
>>> diff --git a/drivers/pinctrl/pinctrl-nomadik.c b/drivers/pinctrl/pinctrl-nomadik.c
>>
>>> @@ -1717,6 +1717,8 @@ static int __devinit nmk_pinctrl_probe(struct platform_device *pdev)
>>>                       of_match_device(nmk_pinctrl_match, &pdev->dev)->data;
>>>
>>>       /* Poke in other ASIC variants here */
>>> +     if (version == PINCTRL_NMK_STN8815)
>>> +             nmk_pinctrl_stn8815_init(&npct->soc);
>>>       if (version == PINCTRL_NMK_DB8500)
>>>               nmk_pinctrl_db8500_init(&npct->soc);
>>
>> One comment that came up in other reviews is that we shouldn't have a
>> single driver that switches on the SoC type it's running on and then
>> dispatches to different ${soc}_init() functions, but rather should have
>> multiple separate drivers, where each probe calls some utility function
>> with the appropriate SoC parameterization structures/tables.
> 
> I discussed this with Arnd, the patch doesn't give the context of where this
> identifier comes from:
> 
> static const struct platform_device_id nmk_pinctrl_id[] = {
>         { "pinctrl-stn8815", PINCTRL_NMK_STN8815 },
>         { "pinctrl-db8500", PINCTRL_NMK_DB8500 },
> };
> 
> static struct platform_driver nmk_pinctrl_driver = {
>         .driver = {
>                 .owner = THIS_MODULE,
>                 .name = "pinctrl-nomadik",
>                 .of_match_table = nmk_pinctrl_match,
>         },
>         .probe = nmk_pinctrl_probe,
>         .id_table = nmk_pinctrl_id,
> };
> 
> And the probe looks like so:
> 
> static int __devinit nmk_pinctrl_probe(struct platform_device *pdev)
> {
>         const struct platform_device_id *platid = platform_get_device_id(pdev);
> (...)
>         if (platid)
>                 version = platid->driver_data;
> 
> So the same driver handles several platform device names, then
> the name is used to select a variant. IIRC I asked Arnd about this
> and he preferred this, and I was told by Mark Brown in the past
> to do things this way (c.f. drivers/mfs/ab8500-core.c).

Hmm. I believe that's the exact opposite of what Arnd said during review
of the SPEAr pinctrl driver! Maybe there's some other aspect of the
design that caused this scheme to be perceived as worse for SPEAr,
beyond just this simple issue.

Still, I only raised this comment because I recalled it being made on
the SPEAr review. I'm not particularly attached to doing it one way or
the other, so don't count this sub-thread as NAKing the patch or
anything like that.

> Doing it the other way is also possible but leads to a proliferation
> of probe() calls and struct platform_driver blocks, and result in
> more lines of code.
> 
> Both ways have precedents in the kernel so I actually think both
> are OK, there is two ways to skin this cat simply and I'm not that
> sensitive to which one is used.
> 
> Yours,
> Linus Walleij