Re: [PATCH v3 4/4] arm64: dts: freescale: Add support for phyBOARD-Pollux-i.MX8MP

[Teresa Remmet <t.remmet@phytec.de>]

Hello Alexander,

Am Freitag, den 11.12.2020, 15:55 +0100 schrieb Alexander Dahl:
> Hello Teresa,
> 
> I'm sorry if I was too brief in my review last time, see below.
> 
> Am Freitag, 11. Dezember 2020, 14:48:55 CET schrieb Teresa Remmet:
> > Add initial support for phyBOARD-Pollux-i.MX8MP.
> > Supported basic features:
> > 	* eMMC
> > 	* i2c EEPROM
> > 	* i2c RTC
> > 	* i2c LED
> > 	* PMIC
> > 	* debug UART
> > 	* SD card
> > 	* 1Gbit Ethernet (fec)
> > 	* watchdog
> > 
> > Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  arch/arm64/boot/dts/freescale/Makefile             |   1 +
> >  .../dts/freescale/imx8mp-phyboard-pollux-rdk.dts   | 163
> > ++++++++++++
> >  .../boot/dts/freescale/imx8mp-phycore-som.dtsi     | 296
> > +++++++++++++++++++++ 3 files changed, 460 insertions(+)
> >  create mode 100644
> > arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts create
> > mode
> > 100644 arch/arm64/boot/dts/freescale/imx8mp-phycore-som.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/Makefile
> > b/arch/arm64/boot/dts/freescale/Makefile index
> > acfb8af45912..a43b496678be
> > 100644
> > --- a/arch/arm64/boot/dts/freescale/Makefile
> > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > @@ -37,6 +37,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mn-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mn-ddr4-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mn-var-som-symphony.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk.dtb
> > +dtb-$(CONFIG_ARCH_MXC) += imx8mp-phyboard-pollux-rdk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-hummingboard-pulse.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-
> > rdk.dts
> > b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts new
> > file
> > mode 100644
> > index 000000000000..e92868c10526
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts
> > @@ -0,0 +1,163 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 PHYTEC Messtechnik GmbH
> > + * Author: Teresa Remmet <t.remmet@phytec.de>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include <dt-bindings/leds/leds-pca9532.h>
> > +#include <dt-bindings/pwm/pwm.h>
> > +#include "imx8mp-phycore-som.dtsi"
> > +
> > +/ {
> > +	model = "PHYTEC phyBOARD-Pollux i.MX8MP";
> > +	compatible = "phytec,imx8mp-phyboard-pollux-rdk",
> > +		     "phytec,imx8mp-phycore-som", "fsl,imx8mp";
> > +
> > +	chosen {
> > +		stdout-path = &uart2;
> > +	};
> > +
> > +	reg_usdhc2_vmmc: regulator-usdhc2 {
> > +		compatible = "regulator-fixed";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>;
> > +		regulator-name = "VSD_3V3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +		startup-delay-us = <100>;
> > +		off-on-delay-us = <12000>;
> > +	};
> > +};
> > +
> > +&i2c2 {
> > +	clock-frequency = <400000>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_i2c2>;
> > +	pinctrl-1 = <&pinctrl_i2c2_gpio>;
> > +	sda-gpios = <&gpio5 17 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +	scl-gpios = <&gpio5 16 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +	status = "okay";
> > +
> > +	eeprom@51 {
> > +		compatible = "atmel,24c02";
> > +		reg = <0x51>;
> > +		pagesize = <16>;
> > +		status = "okay";
> > +	};
> > +
> > +	leds@62 {
> > +		compatible = "nxp,pca9533";
> > +		reg = <0x62>;
> > +		status = "okay";
> > +
> > +		led1 {
> > +			type = <PCA9532_TYPE_LED>;
> > +		};
> > +
> > +		led2 {
> > +			type = <PCA9532_TYPE_LED>;
> > +		};
> > +
> > +		led3 {
> > +			type = <PCA9532_TYPE_LED>;
> > +		};
> > +	};
> 
> You just removed the "label" property. Now the label is generated 
> automatically (which is the preferred way), but you did neither add
> the 
> property "color" nor "function", so the label will be constructed
> from the 
> node name only.

I have tried to set the color property. But in sysfs only the node name
showed up. 

Looking at the code the leds-pca9532 calls devm_led_classdev_register()
and does not pass any init_data. So led_classdev_register_ext() always
sets the node name.

I did not want to set a property that does not have an effect. That's
why I just removed the deprecated one.

Teresa 

> 
> Well I just saw the binding for that LED controller is not converted
> to yaml 
> yet … Documentation/devicetree/bindings/leds/leds-pca9532.txt
> 
> Anyways, the modern approach would look like somehow like this:
> 
>                 led-0 {
>                         function = LED_FUNCTION_ALARM;
>                         color = <LED_COLOR_ID_RED>;
>                 };
> 
>                 led-1 {
>                         function = LED_FUNCTION_STATUS;
>                         color = <LED_COLOR_ID_GREEN>;
>                 };
> 
>                 led-2 {
>                         function = LED_FUNCTION_INDICATOR;
>                         color = <LED_COLOR_ID_RED>;
>                         function-enumerator = <1>;
>                 };
> 
>                 led-3 {
>                         function = LED_FUNCTION_INDICATOR;
>                         color = <LED_COLOR_ID_RED>;
>                         function-enumerator = <2>;
>                 };
> 
> Hope that helps, for more see Documentation/devicetree/bindings/leds
> and 
> especially the bindings already converted to yaml. The available
> macros are in 
> include/dt-bindings/leds/common.h 
> 
> Maybe just add the colors for now, if you're not sure what the
> function should 
> be. As far as I could see the driver for that LED controller does not
> yet 
> support multicolor, but I added linux-leds to Cc, maybe someone over
> there 
> knows more?
> 
> Sorry to nag about this, but I think it's better to not introduce new
> dts 
> files with deprecated properties. If that kind of feedback is not
> desired, 
> please let me know.
> 
> Greets
> Alex
> 
> >