Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fecd3c0d-33bb-d30c-3104-00e3bb36b2a8@linaro.org>
Date:   Fri, 21 Oct 2022 10:29:25 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH net-next v5 5/5] ARM: dts: qcom: ipq4019: Add description
 for the IPQESS Ethernet controller
Content-Language: en-US
To:     Vladimir Oltean <vladimir.oltean@nxp.com>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     "davem@davemloft.net" <davem@davemloft.net>,
        Rob Herring <robh+dt@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "thomas.petazzoni@bootlin.com" <thomas.petazzoni@bootlin.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Luka Perkov <luka.perkov@sartura.hr>,
        Robert Marko <robert.marko@sartura.hr>
References: <20221021124556.100445-1-maxime.chevallier@bootlin.com>
 <20221021124556.100445-1-maxime.chevallier@bootlin.com>
 <20221021124556.100445-6-maxime.chevallier@bootlin.com>
 <20221021124556.100445-6-maxime.chevallier@bootlin.com>
 <20221021142057.zbc3xfny4hfdshei@skbuf>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20221021142057.zbc3xfny4hfdshei@skbuf>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 21/10/2022 10:20, Vladimir Oltean wrote:
> On Fri, Oct 21, 2022 at 02:45:56PM +0200, Maxime Chevallier wrote:
>> @@ -591,6 +592,51 @@ wifi1: wifi@a800000 {
>>  			status = "disabled";
>>  		};
>>  
>> +		gmac: ethernet@c080000 {
> 
> Pretty random ordering in this dts, you'd expect nodes are sorted by
> address...

Good point.

> 
>> +			compatible = "qcom,ipq4019-ess-edma";
>> +			reg = <0xc080000 0x8000>;
>> +			resets = <&gcc ESS_RESET>;
>> +			reset-names = "ess";
>> +			clocks = <&gcc GCC_ESS_CLK>;
>> +			clock-names = "ess";
>> +			interrupts = <GIC_SPI  65 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  66 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  67 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  68 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  69 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  70 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  71 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  72 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  73 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  74 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  75 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  76 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  77 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  78 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  79 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI  80 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 240 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 241 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 242 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 243 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 244 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 245 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 246 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 247 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 248 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 249 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 250 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 251 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 252 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 253 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 254 IRQ_TYPE_EDGE_RISING>,
>> +				     <GIC_SPI 255 IRQ_TYPE_EDGE_RISING>;
> 
> 32 interrupts, and no interrupt-names? :)

There is no requirement for names, because entries must be ordered. Also
Linux driver simply does not use them as it is slower to map, then just
by index. For few other Qualcomm drivers we dropped the names as well.

> 
>> +
>> +			status = "disabled";
>> +
> 
> Could you drop these 2 blank lines? They aren't generally added between
> properties.
> 

... and put status at the end.

Best regards,
Krzysztof