Re: [PATCHv2 4/4] dt-bindings: net: dsa: add new MT7530 binding to support MT7621

[Greg Ungerer <gerg@kernel.org>]

Hi Andrew,

On 17/1/19 2:12 am, Andrew Lunn wrote:
> On Wed, Jan 16, 2019 at 11:14:30PM +1000, Greg Ungerer wrote:
>> Hi Andrew,
>>
>> On 15/1/19 11:18 pm, Andrew Lunn wrote:
>> [snip]
>>> As i said, it is a bit messy. I would probably have a section:
>>>
>>> Required properties
>>>
>>> which lists all common required properties. And then a section
>>>
>>> Required properties mediatek,mt7530
>>>
>>> With those which are required by that device.
>>
>>
>> Ok, I see what you mean.
>> The simplest approach is something like this:
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
>> index aa3527f71fdc..47aa205ee0bd 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
>> +++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
>> @@ -3,12 +3,16 @@ Mediatek MT7530 Ethernet switch
>>   Required properties:
>> -- compatible: Must be compatible = "mediatek,mt7530";
>> +- compatible: may be compatible = "mediatek,mt7530"
>> +	or compatible = "mediatek,mt7621"
>>   - #address-cells: Must be 1.
>>   - #size-cells: Must be 0.
>>   - mediatek,mcm: Boolean; if defined, indicates that either MT7530 is the part
>>   	on multi-chip module belong to MT7623A has or the remotely standalone
>>   	chip as the function MT7623N reference board provided for.
>> +
>> +If compatible mediatek,mt7530 is set then the following properties are required
>> +
>>   - core-supply: Phandle to the regulator node necessary for the core power.
>>   - io-supply: Phandle to the regulator node necessary for the I/O power.
>>   	See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
> 
> Yes, that looks good.
> 
>>
>>
>> That leaves the existing if/else with other bindings in place after this.
>>
>> Maybe I am mis-reading the mt7530.c code but I am not entirely sure that
>> "reset-gpios" is actually required for the mediatek,mcm not defined case -
>> it is optional. (And I actually don't define it for my mt7621 case where
>> mediatek,mcm is not defined).
> 
> The gpio seems to be optional in general, any only used for
> device other than mediatek,mcm.
> 
> The reset controller is required for mediatek,mcm, but not others.
> 
> So it looks like the binding document needs a few fixes :-(

Yep.. Thanks for the feedback though. I'll roll a v3.

Regards
Greg