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

[Francesco Dolcini]

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.

> > > +	/* CAN controller on the baseboard */
> > > +	canfd: can at 0 {
> > > +		compatible = "microchip,mcp2518fd";
> > > +		clocks = <&clk20m>;
> > You are using the node defined in the verdin.dtsi here, while I guess
> > this is a separate oscillator part of the carrier board.
> > 
> > Should you define a new clock? My concern is that we had discussion to
> > change the SoM to move from 20 to 40 MHz, and we would remove this entry
> > from the dtsi if we would do such a change.
> 
> Is the 20 MHz oscillator on your SoM or on the carrier board ?
In the SoM, not available on the SODIMM connector. Your clock if for
sure not this one.

> > > +&i2c4 {
> > > +	/* IMX8MM ERRATA e7805 -- I2C is limited to 384 kHz due to SoC bug */
> > > +	clock-frequency = <100000>;
> > > +	/delete-node/ bridge at 2c;
> > > +	/delete-node/ hwmon at 40;
> > > +	/delete-node/ hdmi at 48;
> > > +	/delete-node/ touch at 4a;
> > > +	/delete-node/ hwmontemp at 4f;
> > > +	/delete-node/ eeprom at 50;
> > > +	/delete-node/ eeprom at 57;
> > All of those are disabled in the dtsi, is it really worth deleting the
> > nodes?
> 
> They are not present on the hardware, why should they be described in the DT
> ? Hence the /delete-node/ .
Not 100% correct (see the SoM datasheet), anyway is this causing you any
real trouble having those nodes disabled instead of removed from the
dts? I assume nor the file size nor the parsing time are an issue there.

Of course, not a big deal for me you removing this nodes in the menlo
dts, but in general I'm happy to make your life easier when using our
som dtsi file, this is the reason for asking.

Francesco