Hi,

On 2/12/21 1:02 AM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kis...@siemens.com>
> 
> Add support for two Siemens SIMATIC IOT2050 variants, Basic and
> Advanced. They are based on the TI AM6528 GP and AM6548 SOCs HS, thus
> differ in their number of cores and availability of security features.
> Furthermore the Advanced version comes with more RAM, an eMMC and a few
> internal differences.
> 
> Based on original version by Le Jin.
> 
> Link: 
> https://new.siemens.com/global/en/products/automation/pc-based/iot-gateways/simatic-iot2050.html
> Link: https://github.com/siemens/meta-iot2050
> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>

Reviewed-by: Vignesh Raghavendra <vigne...@ti.com>

Few minor comments below:

[...]

> +
> +&mcu_i2c0 {
> +     pinctrl-names = "default";
> +     pinctrl-0 = <&mcu_i2c0_pins_default>;
> +     clock-frequency = <400000>;
> +
> +     psu: tps62363@60 {

Please use generic node names:

        psu: regulator@60 { ... };

> +             compatible = "ti,tps62363";
> +             reg =  <0x60>;
> +             regulator-name = "tps62363-vout";
> +             regulator-min-microvolt = <500000>;
> +             regulator-max-microvolt = <1500000>;
> +             regulator-boot-on;
> +             ti,vsel0-state-high;
> +             ti,vsel1-state-high;
> +             ti,enable-vout-discharge;
> +     };
> +
> +     /* D4200 */
> +     pcal9535_1: gpio@20 {
> +             compatible = "nxp,pcal9535";
> +             reg = <0x20>;
> +             #gpio-cells = <2>;
> +             gpio-controller;
> +             gpio-line-names =
> +                     "A0-pull", "A1-pull", "A2-pull", "A3-pull", "A4-pull",
> +                     "A5-pull", "", "",
> +                     "IO14-enable", "IO15-enable", "IO16-enable",
> +                     "IO17-enable", "IO18-enable", "IO19-enable";
> +     };
> +
> +     /* D4201 */
> +     pcal9535_2: gpio@21 {
> +             compatible = "nxp,pcal9535";
> +             reg = <0x21>;
> +             #gpio-cells = <2>;
> +             gpio-controller;
> +             gpio-line-names =
> +                     "IO0-direction", "IO1-direction", "IO2-direction",
> +                     "IO3-direction", "IO4-direction", "IO5-direction",
> +                     "IO6-direction", "IO7-direction",
> +                     "IO8-direction", "IO9-direction", "IO10-direction",
> +                     "IO11-direction", "IO12-direction", "IO13-direction",
> +                     "IO19-direction";
> +     };
> +
> +     /* D4202 */
> +     pcal9535_3: gpio@25 {
> +             compatible = "nxp,pcal9535";
> +             reg = <0x25>;
> +             #gpio-cells = <2>;
> +             gpio-controller;
> +             gpio-line-names =
> +                     "IO0-pull", "IO1-pull", "IO2-pull", "IO3-pull",
> +                     "IO4-pull", "IO5-pull", "IO6-pull", "IO7-pull",
> +                     "IO8-pull", "IO9-pull", "IO10-pull", "IO11-pull",
> +                     "IO12-pull", "IO13-pull";
> +     };
> +};

[...]

> +&dwc3_0 {
> +     status = "okay";
> +};
> +
> +&usb0_phy {
> +     status = "okay";
> +};
> +

These nodes are enabled by default right? Above is redundant.

> +&usb0 {
> +     pinctrl-names = "default";
> +     pinctrl-0 = <&usb0_pins_default>;
> +     dr_mode = "host";
> +};
> +
> +&dwc3_1 {
> +     status = "okay";
> +};
> +
> +&usb1_phy {
> +     status = "okay";
> +};
> +

Same here...

> +&usb1 {
> +     pinctrl-names = "default";
> +     pinctrl-0 = <&usb1_pins_default>;
> +     dr_mode = "host";
> +};
> +

[...]

> +&mcu_spi0 {
> +     pinctrl-names = "default";
> +     pinctrl-0 = <&mcu_spi0_pins_default>;
> +
> +     #address-cells = <1>;
> +     #size-cells= <0>;
> +     ti,pindir-d0-out-d1-in = <1>;
> +
> +     spidev@0 {
> +             compatible = "rohm,dh2228fv";
> +             spi-max-frequency = <20000000>;
> +             reg = <0>;
> +     };

Is the device really dh2228fv?

> +};
> +
> +&tscadc0 {
> +     status = "disabled";
> +};
> +
> +&tscadc1 {
> +     adc {
> +             ti,adc-channels = <0 1 2 3 4 5>;
> +     };
> +};
> +
> +&ospi0 {
> +     pinctrl-names = "default";
> +     pinctrl-0 = <&mcu_fss0_ospi0_pins_default>;
> +
> +     flash@0 {
> +             compatible = "jedec,spi-nor";
> +             reg = <0x0>;
> +             spi-tx-bus-width = <1>;
> +             spi-rx-bus-width = <1>;
> +             spi-max-frequency = <50000000>;
> +             cdns,tshsl-ns = <60>;
> +             cdns,tsd2d-ns = <60>;
> +             cdns,tchsh-ns = <60>;
> +             cdns,tslch-ns = <60>;
> +             cdns,read-delay = <2>;
> +             #address-cells = <1>;
> +             #size-cells = <1>;
> +     };
> +};
> +
> +&dss {
> +     status = "okay";
> +

Node is enabled by default. Please drop above line for consistency.

> +     pinctrl-names = "default";
> +     pinctrl-0 = <&dss_vout1_pins_default>;
> +
> +     assigned-clocks = <&k3_clks 67 2>;
> +     assigned-clock-parents = <&k3_clks 67 5>;
> +};
> +
> +&dss_ports {
> +     #address-cells = <1>;
> +     #size-cells = <0>;
> +     port@1 {
> +             reg = <1>;
> +
> +             dpi_out: endpoint {
> +                     remote-endpoint = <&bridge_in>;
> +             };
> +     };
> +};
> +

[...]

Regards
Vignesh

Reply via email to