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

[Marek Vasut]

On 1/16/23 11:51, Frieder Schrempf wrote:
> On 16.01.23 11:39, Marek Vasut wrote:
>> On 1/11/23 08:38, Ahmad Fatoum wrote:
>>> Hello Marek,
>>
>> Hi,
>>
>> [...]
>>
>>> Could you share your get_maintainers.pl invocation? I'd like to
>>> adjust the reviewer entry in MAINTAINERS, so such patches get
>>> into our kernel at pengutronix.de inbox as well.
>>
>> Plain get_maintainer -f path/to/dts
>>
>>> Some more comments below.
>>>
>>>> +    pmic: pmic at 25 {
>>>> +        compatible = "nxp,pca9450c";
>>>> +        reg = <0x25>;
>>>> +        pinctrl-names = "default";
>>>> +        pinctrl-0 = <&pinctrl_pmic>;
>>>> +        interrupt-parent = <&gpio1>;
>>>> +        interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>>>> +        sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
>>>
>>> Here you assume GPIO1_IO04 is muxed as GPIO.
>>>
>>>> +    pinctrl_usdhc2: usdhc2-grp {
>>>> +        fsl,pins = <
>>>> +            MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT        0xc1
>>>
>>> Here you mux it for USDHC2_VSELECT though. Is this intended?
>>
>> That's a good question, other DTs do the same thing, I suspect the
>> sd-vsel-gpios could be dropped,
> 
> The sd-vsel-gpios is only needed when the hardware has an arbitrary GPIO
> connected to the SD_VSEL signal of the PCA9450. IMHO, if SD_VSEL is
> connected to the VSELECT signal it would be better to drop sd-vsel-gpios.

Current practice does not match this statement however, notice how they 
are all gpio 1 4 :

$ git grep sd-vsel-gpios
Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml: 
sd-vsel-gpios:
arch/arm64/boot/dts/freescale/imx8mm-verdin.dtsi: 
sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
arch/arm64/boot/dts/freescale/imx8mp-data-modul-edm-sbc.dts: 
sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
arch/arm64/boot/dts/freescale/imx8mp-dhcom-som.dtsi: 
sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi: 
sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;

>> but as long as it is not outright
>> harmful, it can be used for backward compatibility to support less
>> complete OSes which may not handle the eSDHC VSELECT bit , so I figured
>> it is good to keep both options.
> 
> Would be interesting what OSes you have in mind? The eSDHC
> driver/hardware should always handle the VSELECT signal, no?

Not necessarily, but the ones I am aware of (U-Boot, Linux) do handle 
VSELECT.