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@xxxxxxxxx>
> > Reviewed-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
> > + */
> > +
> > +/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
>
> >
