Re: [PATCH] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren <swarren@wwwdotorg.org>]

On 07/26/2013 02:44 PM, Dinh Nguyen wrote:
> On Fri, 2013-07-26 at 14:02 -0600, Stephen Warren wrote:
>> On 07/26/2013 01:33 PM, Dinh Nguyen wrote:
>>> On Fri, 2013-07-26 at 11:24 -0600, Stephen Warren wrote:
>>>> On 07/25/2013 04:04 PM, dinguyen@altera.com wrote:
>>>>> From: Dinh Nguyen <dinguyen@altera.com>
>>>>>
>>>>> Add bindings for SD/MMC for SOCFPGA.
>>>>> Add "syscon" to the "altr,sys-mgr" binding.
>>
>>>>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>
>>>>> +Example:
>>>>> +
>>>>> +  The MSHC controller node can be split into two portions, SoC specific and
>>>>> +  board specific portions, as listed below.
>>>>
>>>> That doesn't sound like a good idea. There should be one DT node for
>>>> each logical block. The internal construction of the Linux drivers
>>>> (presumably you have entirely separate code to handle the two nodes in
>>>> Linux so far?) should not influence the DT construction at all.
>>>
>>> In the end, there is only 1 DT node for each logical block:
>>
>> Oh right, I see you were intending to show the distinction between the
>> SoC .dtsi and board .dts file. I hadn't realized that. I don't think
>> it's common to do that in the examples, so I would recommend just
>> merging the whole example together myself.
> 
> I'll merge it.
> 
>>
>>> dwmmc0@ff704000 {
>>> 	compatible = "altr,socfpga-dw-mshc";
>>
>> That should include the baseline synopsis compatible value too.
> 
> We don't need the baseline synopsis compatible because of
> dw_mci_pltfm_register() call.

It's not a matter of whether it's strictly necessary for the SW to work
right now. The compatible property should include entries for everything
that the HW is actually compatible with.

Of course, if a plain driver for the raw synopsis controller/binding
wouldn't actually work on this HW at all, without explicit knowledge of
the extra details of the more HW-specific binding, then that's a good
argument for leaving it out of compatible.