[PATCH V2] bcma: use device from DT (brcm, bus-gpio) for SoC GPIO chip

[Rafał Miłecki]

On 30 September 2014 11:37, Arnd Bergmann <arnd at arndb.de> wrote:
> On Sunday 28 September 2014 10:24:01 Rafał Miłecki wrote:
>> @@ -17,4 +33,12 @@ Example:
>>               ranges = <0x00000000 0x18000000 0x00100000>;
>>               #address-cells = <1>;
>>               #size-cells = <1>;
>> +
>> +             chipcommon at 0 {
>> +                     gpio at 0 {
>> +                             compatible = "brcm,bus-gpio";
>> +                             gpio-controller;
>> +                             #gpio-cells = <2>;
>> +                     };
>> +             };
>
> I think you should list the 'reg' property of the chipcommon area
> and make that match the unit address, rather than using '0', mostly
> for consistency.

Do you mean this chipcommon at 0? We propose this foo at offset syntax since
V1 which was posted 1,5 month ago, it's nothing new.


> Can you have multiple gpio areas in chipcommon? If not, I'd probably
> leave out the extra node and mark the chipcommon device itself as
> a 'gpio-controller'. It can also be other things at the same time,
> e.g. 'interrupt-controller' or provide things like pwms or pinctrl
> if that's what the hardware does. No need to have separate nodes
> for those if it's all the same register set.

OK


>> @@ -218,6 +218,11 @@ int bcma_gpio_init(struct bcma_drv_cc *cc)
>>  #if IS_BUILTIN(CONFIG_BCM47XX)
>>       chip->to_irq            = bcma_gpio_to_irq;
>>  #endif
>> +#if IS_BUILTIN(CONFIG_OF)
>> +     if (cc->core->bus->hosttype == BCMA_HOSTTYPE_SOC)
>> +             chip->of_node   = of_find_compatible_node(NULL, NULL,
>> +                                                       "brcm,bus-gpio");
>> +#endif
>
> Just commenting on the general style here, I think you can skip this
> step entirely, as mentioned above.
>
> You should use C syntax here:
>
>         if (IS_BUILTIN(CONFIG_OF) && cc->core->bus->hosttype == BCMA_HOSTTYPE_SOC))
>                 chip->of_node = of_find_compatible_node(NULL, NULL, "brcm,bus-gpio");

OK


> If CONFIG_OF is not set, of_find_compatible_node() is turned into an inline
> function that returns NULL, so you probably don't even need that.

But I need to have of_node defined. Please check out "struct gpio_chip".


> Finally, of_find_compatible_node() is a really slow operation, and "brcm,bus-gpio"
> is a much too generic name. If you need to look for something that is a child
> node, use something based on for_each_available_child_of_node().

Will see what I can do.

-- 
Rafał