Re: [PATCH 2/2] arm64: dts: imx8mm: Add i.MX8M Mini Toradex Verdin based Menlo board

From: Marek Vasut <marex@denx.de>
To: Francesco Dolcini <francesco.dolcini@toradex.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Fabio Estevam <festevam@denx.de>,
	Marcel Ziswiler <marcel.ziswiler@toradex.com>,
	Peng Fan <peng.fan@nxp.com>, Shawn Guo <shawnguo@kernel.org>,
	NXP Linux Team <linux-imx@nxp.com>
Subject: Re: [PATCH 2/2] arm64: dts: imx8mm: Add i.MX8M Mini Toradex Verdin based Menlo board
Date: Wed, 13 Apr 2022 16:14:20 +0200	[thread overview]
Message-ID: <35de4a8f-72c7-020f-4c71-c14d12dbe3b6@denx.de> (raw)
In-Reply-To: <20220413094449.GB118560@francesco-nb.int.toradex.com>

On 4/13/22 11:44, Francesco Dolcini wrote:
> Hello Marek

Hi,

> On Sun, Apr 10, 2022 at 11:37:52PM +0200, Marek Vasut wrote:
>> On 4/10/22 10:46, Francesco Dolcini wrote:
>>> On Fri, Apr 08, 2022 at 05:02:15PM +0200, Marek Vasut wrote:
>>>> On 4/8/22 08:46, Francesco Dolcini wrote:
>>>>>> +	/delete-node/ gpio-keys;
>>>>> would it be better if we had a label in the imx8mm-verdin.dtsi and we
>>>>> could just set status=disabled here?
>>>>
>>>> It would be better if there was Verdin SoM dtsi and Verdin carrier board dts
>>>> which includes the SoM dtsi. Right now, it seems these two things are
>>>> conflated a bit.
>>>>
>>>> There are no GPIO buttons on the Verdin SoM, there are some on the Dahlia
>>>> carrier board I think.
>>>
>>> The GPIO keys, for example the suspend button, are part of Verdin family
>>> SODIMM connector pinout/definition (see related datasheets). In the SoM
>>> dtsi we implement our standard family definition.
>>>
>>> Of course, you are free to redefine this in any way you prefer. In
>>> general the way we envision this is to just enable/disable in the
>>> carrier board or overlay dts, this is the reason I proposed to add
>>> a label there. I do not see any real value on deleting the node at all,
>>> just some potential for additional maintenance burden.
>>
>> There are two reasons for not adding DT nodes for hardware which is not
>> populated:
>> - You are polluting the DT with unused nodes representing hardware which
>>    can never be present on the system, that makes the DT bigger and more
>>    complex, for no reason.
>> - Once DTOs enter the picture, these so far only useless nodes and
>>    properties actively become a problem. You cannot delete either node or
>>    property from a base DT using a DTO, because neither /delete-node/ nor
>>    /delete-property/ can be encoded into the DTO blob .
> 
> Ok, I understand your arguments and I agree they are fully valid.
> We (Toradex) had some discussion about in the past and we still see
> benefit on the way we are doing it never the less.
> 
> - The SoM dtsi representing not only the functionality implemented into
>    the SoM, but the whole connector pinout to the carrier makes very easy
>    to just include a different som.dtsi in the carrier board dts and just
>    switch SoM, for example from a colibri-imx6 to a colibri-imx7.
> - We avoid code duplication
> - Even if the DTO cannot `delete`, it can disable a node. We do our
>    best to have label and keep nodes disabled when functionality is not
>    self-contained in the SoM.

You missed one important problem here, superfluous properties in the SoM DT:

&reg_usb_otg2_vbus {
	/delete-property/ enable-active-high; // <----- HERE
};

You cannot get rid of those using DTOs.

> This is working for us pretty well so far and the majority of the users
> of ours modules rely on that, changing it now would just be too
> disruptive.
> 
> I would propose that you go with the delete-node as you are doing and we
> keep the verdin.dtsi as it is.

Let's do that, thanks.

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel