Re: [PATCH RESEND V3 2/2] mmc: sdhci-brcmstb: Add driver for Broadcom BRCMSTB SoCs

[Florian Fainelli <f.fainelli@gmail.com>]

On 07/03/16 13:32, Alan Cooper wrote:
> On Fri, Mar 4, 2016 at 7:48 PM, Kevin Cernekee <cernekee@gmail.com> wrote:
>> On Fri, Mar 4, 2016 at 10:07 AM, Al Cooper <alcooperx@gmail.com> wrote:
>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>> ---
>>>  .../devicetree/bindings/mmc/sdhci-brcmstb.txt      |  40 ++++++
>>>  MAINTAINERS                                        |   7 +
>>>  drivers/mmc/host/Kconfig                           |  11 ++
>>>  drivers/mmc/host/Makefile                          |   1 +
>>>  drivers/mmc/host/sdhci-brcmstb.c                   | 154 +++++++++++++++++++++
>>>  5 files changed, 213 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
>>>  create mode 100644 drivers/mmc/host/sdhci-brcmstb.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt b/Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
>>> new file mode 100644
>>> index 0000000..f8dd6a9d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-brcmstb.txt
>>> @@ -0,0 +1,40 @@
>>> +* BROADCOM BRCMSTB/BMIPS SDHCI Controller
>>> +
>>> +This file documents differences between the core properties in mmc.txt
>>> +and the properties used by the sdhci-brcmstb driver.
>>> +
>>> +Required properties:
>>> +- compatible: "brcm,sdhci-brcmstb"
>>
>> For many of the other SoC drivers, such as drivers/irqchip/irq-bcm*,
>> we've used the compatible string and/or driver name to denote the
>> oldest SoC that is compatible with the driver.  This helps to indicate
>> that the driver is not compatible with previous versions of the core.
>> For instance, the driver you are submitting would be slightly broken
>> on the BCM7468/BCM7208 STB chips, and horrifically broken on BCM7630
>> (which admittedly isn't quite "brcmstb" but uses some of the same
>> drivers).  I assume you don't want to target those ancient parts.
>>
>> When the hardware block changes frequently, we've gone as far as
>> creating a different compatible string for each variant.  For
>> Documentation/devicetree/bindings/net/brcm,bcmgenet.txt we encoded the
>> hardware generation number in the compatible string.
> 
> Something like "brcm,bcm7425-sdhci"?

You certainly know the history more than I do, but that sounds like an
appropriate compatible string here. We could list all chips that have
such a controller and have been quirky, that is how we are supposed to
solve that problem, but that's up to you.

> 
>>
>> [snip]
>>
>>> +static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>> +{
>>> +       struct device_node *dn = pdev->dev.of_node;
>>> +       struct sdhci_host *host;
>>> +       struct sdhci_pltfm_host *pltfm_host;
>>> +       struct clk *clk;
>>> +       int res;
>>> +
>>> +       clk = of_clk_get_by_name(dn, "sw_sdio");
>>> +       if (IS_ERR(clk)) {
>>> +               dev_err(&pdev->dev, "Clock not found in Device Tree\n");
>>> +               clk = NULL;
>>
>> Last time I worked on these chips, they defaulted to "all clocks
>> enabled" in the absence of a clk driver.
>>
>> So, if this is not treated as a fatal condition, maybe it would be
>> better to downgrade it to dev_warn or dev_info.
> 
> Good point, I'll switch it to dev_warn.

Since you are going to revisit this part, the binding should also
document the need for an optional clocks property, since it currently
does not.
-- 
Florian