Re: [PATCH 2/2] arm64: dts: imx8mp: Add support for Data Modul i.MX8M Plus eDM SBC

[Shawn Guo <shawnguo@kernel.org>]

On Thu, Jan 05, 2023 at 10:30:17PM +0100, Marek Vasut wrote:
> On 1/1/23 05:00, Shawn Guo wrote:
> 
> [...]
> 
> > > +	panel: panel {
> > 
> > No compatible?
> 
> The compatible string is filled in by expansion module DT overlay, so no
> default compatible string in the panel node here. The panel interface is the
> same for all panels that can be atteched to this board, so the panel node is
> common for all DTOs and can be in the base DT.
> 
> > > +		backlight = <&backlight>;
> > > +		power-supply = <&reg_panel_vcc>;
> > > +		/* Disabled by default, unless display board plugged in. */
> > > +		status = "disabled";
> > > +	};
> > > +
> > > +	reg_panel_vcc: regulator-panel-vcc {
> > > +		compatible = "regulator-fixed";
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pinctrl_panel_vcc_reg>;
> > > +		regulator-name = "PANEL_VCC";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		gpio = <&gpio3 6 0>;
> > 
> > GPIO_ACTIVE_HIGH?
> 
> No, the 0 is correct and you're not the first one to wonder about this
> oddity.

I understand that the polarity is ignored by Linux Kernel.  But it
shouldn't prevent us from describing the polarity cell with defines
for better readability.

I'm always looking for the pattern below when reviewing the device tree.

	regulator-xxx {
		compatible = "regulator-fixed";
		...
		gpio = <&gpio3 6 GPIO_ACTIVE_HIGH>;
		enable-active-high;
	}

Or for low polarity:

	regulator-xxx {
		compatible = "regulator-fixed";
		...
		gpio = <&gpio3 6 GPIO_ACTIVE_LOW>;
	}

The polarity define is helpful for me to validate whether
`enable-active-high` property should present.

Shawn

> See drivers/gpio/gpiolib-of.c :
> 
>  203 /*
>  204  * The regulator GPIO handles are specified such that the
>  205  * presence or absence of "enable-active-high" solely controls
>  206  * the polarity of the GPIO line. Any phandle flags must
>  207  * be actively ignored.
>  208  */
>  209 #if IS_ENABLED(CONFIG_REGULATOR_FIXED_VOLTAGE)
>  210     { "regulator-fixed",   "gpios",    "enable-active-high" },
>  211     { "regulator-fixed",   "gpio",     "enable-active-high" },
> 
> [...]