On Mon, Apr 24, 2023 at 03:34:52PM +0300, Nikita Shubin wrote:
> Drop legacy gpio request/free since we are using
> pinctrl for this now.
>
> Signed-off-by: Nikita Shubin <[email protected]>
> ---
> drivers/pwm/pwm-ep93xx.c | 16 ----------------
> 1 file changed, 16 deletions(-)
>
> diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
> index 8bfe6cfbb3db..657adb011aeb 100644
> --- a/drivers/pwm/pwm-ep93xx.c
> +++ b/drivers/pwm/pwm-ep93xx.c
> @@ -45,20 +45,6 @@ static inline struct ep93xx_pwm *to_ep93xx_pwm(struct pwm_chip *chip)
> return container_of(chip, struct ep93xx_pwm, chip);
> }
>
> -static int ep93xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct platform_device *pdev = to_platform_device(chip->dev);
> -
> - return ep93xx_pwm_acquire_gpio(pdev);

I didn't get the whole series and didn't spot a relevant followup change
on lore.k.o, so: I assume ep93xx_pwm_acquire_gpio() and
ep93xx_pwm_release_gpio() will be unused in the end? Do you drop them?

I assume this series target to be taken via arm-soc (once the review
feedback is positive)?

I wonder if this change breaks non-dt machine support?

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (1.35 kB)
signature.asc (499.00 B)
Download all attachments

2023-05-16 03:58:31

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH 00/43] ep93xx device tree conversion

On 4/24/2023 5:34 AM, Nikita Shubin wrote:
> This series aims to convert ep93xx from platform to full device tree support.
>
> Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302.
>
> Thank you Linus and Arnd for your support, review and comments, sorry if i missed something -
> these series are quite big for me.
>
> Big thanks to Alexander Sverdlin for his testing, support, review, fixes and patches.

If anyone is interested I still have a TS-7300 board [1] that is fully
functional and could be sent out to a new home.

https://www.embeddedts.com/products/TS-7300
--
Florian

2023-05-16 07:53:39

by Nikita Shubin

[permalink] [raw]

Subject: Re: [PATCH 00/43] ep93xx device tree conversion

Hello Florian!

On Mon, 2023-05-15 at 20:47 -0700, Florian Fainelli wrote:
>
>
> On 4/24/2023 5:34 AM, Nikita Shubin wrote:
> > This series aims to convert ep93xx from platform to full device
> > tree support.
> >
> > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302.
> >
> > Thank you Linus and Arnd for your support, review and comments,
> > sorry if i missed something -
> > these series are quite big for me.
> >
> > Big thanks to Alexander Sverdlin for his testing, support, review,
> > fixes and patches.
>
> If anyone is interested I still have a TS-7300 board [1] that is
> fully
> functional and could be sent out to a new home.

Thank you kindly, i'll keep this in mind !

>
> https://www.embeddedts.com/products/TS-7300

2023-05-16 08:09:03

by Nikita Shubin

[permalink] [raw]

Subject: Re: [PATCH 36/43] pwm: ep93xx: drop legacy pinctrl

Hello Uwe!

On Mon, 2023-05-15 at 16:32 +0200, Uwe Kleine-König wrote:
> On Mon, Apr 24, 2023 at 03:34:52PM +0300, Nikita Shubin wrote:
> > Drop legacy gpio request/free since we are using
> > pinctrl for this now.
> >
> > Signed-off-by: Nikita Shubin <[email protected]>
> > ---
> > drivers/pwm/pwm-ep93xx.c | 16 ----------------
> > 1 file changed, 16 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
> > index 8bfe6cfbb3db..657adb011aeb 100644
> > --- a/drivers/pwm/pwm-ep93xx.c
> > +++ b/drivers/pwm/pwm-ep93xx.c
> > @@ -45,20 +45,6 @@ static inline struct ep93xx_pwm
> > *to_ep93xx_pwm(struct pwm_chip *chip)
> >         return container_of(chip, struct ep93xx_pwm, chip);
> > }
> >
> > -static int ep93xx_pwm_request(struct pwm_chip *chip, struct
> > pwm_device *pwm)
> > -{
> > -       struct platform_device *pdev = to_platform_device(chip-
> > >dev);
> > -
> > -       return ep93xx_pwm_acquire_gpio(pdev);
>
> I didn't get the whole series and didn't spot a relevant followup
> change

https://lore.kernel.org/all/[email protected]/

> on lore.k.o, so: I assume ep93xx_pwm_acquire_gpio() and
> ep93xx_pwm_release_gpio() will be unused in the end? Do you drop
> them?
>
> I assume this series target to be taken via arm-soc (once the review
> feedback is positive)?
>
> I wonder if this change breaks non-dt machine support?

The aim for the whole series is fully converting to dt, this means
platform files will be dropped.

The v1 series tries not to break anything until platform removal
commit, before this commit non-dt version should be compilable and
fully functional.

>
> Best regards
> Uwe
>

2023-05-17 06:24:21

2023-06-01 06:02:24

by Nikita Shubin

[permalink] [raw]

2023-06-01 06:58:35

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v1 06/43] dt-bindings: pinctrl: Add Cirrus EP93xx

On 01/06/2023 07:33, Nikita Shubin wrote:
> Add YAML bindings for ep93xx SoC pinctrl.
>
> Signed-off-by: Nikita Shubin <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> ---
>
> Notes:
> v0 -> v1:
>
> Krzysztof Kozlowski:
> - removed wildcards
> - use fallback compatible and list all possible compatibles
> - fix ident
> - dropped bindings in title
>
> .../pinctrl/cirrus,ep9301-pinctrl.yaml | 66 +++++++++++++++++++
> 1 file changed, 66 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml
> new file mode 100644
> index 000000000000..ff7b30a11bab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/cirrus,ep9301-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus ep93xx pins mux controller
> +
> +maintainers:
> + - Nikita Shubin <[email protected]>
> + - Alexander Sverdlin <[email protected]>
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-pinctrl
> + - items:
> + - enum:
> + - cirrus,ep9302-pinctrl
> + - cirrus,ep9307-pinctrl
> + - cirrus,ep9312-pinctrl
> + - cirrus,ep9315-pinctrl
> + - const: cirrus,ep9301-pinctrl
> +
> +patternProperties:
> + '^pins-':
> + type: object
> + description: pin node
> + $ref: pinmux-node.yaml#
> +
> + properties:
> + function:
> + enum: [ spi, ac97, i2s, pwm, keypad, pata, lcd, gpio ]

Blank line.

> + groups:
> + minItems: 1
> + maxItems: 2

How one pin can belong to two groups? What does it mean?

> + items:
> + enum: [ ssp, ac97, i2s_on_ssp, i2s_on_ac97, pwm1, gpio1agrp,
> + gpio2agrp, gpio3agrp, gpio4agrp, gpio6agrp, gpio7agrp,
> + rasteronsdram0grp, rasteronsdram3grp, keypadgrp, idegrp]
> +
> + required:
> + - function
> + - groups
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + syscon@80930000 {
> + compatible = "cirrus,ep9301-syscon",
> + "syscon", "simple-mfd";

Weird wrapping.

> + reg = <0x80930000 0x1000>;
> + #clock-cells = <1>;
> + #reset-cells = <1>;
> + pinctrl {
> + compatible = "cirrus,ep9312-pinctrl", "cirrus,ep9301-pinctrl";
> + spi_default_pins: pins-spi {
> + function = "spi";
> + groups = "ssp";
> + };
> + };
> + };

Best regards,
Krzysztof

2023-06-01 06:58:35

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v1 04/43] dt-bindings: clock: Add Cirrus EP93xx

On 01/06/2023 07:33, Nikita Shubin wrote:
> This adds device tree bindings for the Cirrus Logic EP93xx
> clock block used in these SoCs.
>
> Signed-off-by: Nikita Shubin <[email protected]>
> ---
>
> Notes:
> v0 -> v1:
>
> - it's now a clock controller
>
> .../bindings/clock/cirrus,ep9301.yaml | 64 +++++++++++++++++++
> 1 file changed, 64 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml
>
> diff --git a/Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml b/Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml
> new file mode 100644
> index 000000000000..4f9e0d483698
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/cirrus,ep9301.yaml

Filename matching compatible.

> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/cirrus,ep9301.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic ep93xx SoC's clock controller
> +
> +maintainers:
> + - Nikita Shubin <[email protected]>
> + - Alexander Sverdlin <[email protected]>
> +
> +description: |
> + Cirrus Logic EP93XX SoC clocks driver bindings. The clock
> + controller node must be defined as a child node of the ep93xx
> + system controller node.
> +
> + See also:
> + - dt-bindings/clock/cirrus,ep93xx-clock.h
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-clk
> + - items:
> + - enum:
> + - cirrus,ep9302-clk
> + - cirrus,ep9307-clk
> + - cirrus,ep9312-clk
> + - cirrus,ep9315-clk
> + - const: cirrus,ep9301-clk
> +
> + "#clock-cells":
> + const: 1
> +
> + clocks:
> + description: List of clock specifiers which are external input
> + clocks to the given clock controller.

Drop description, it's obvious.

> + items:
> + - description: reference clock
> +
> +required:
> + - compatible
> + - "#clock-cells"
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + clocks {
> + xtali: oscillator {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <14745600>;
> + };
> + };

Drop this part of example, it's obvious.

> +
> + clock-controller {
> + #clock-cells = <1>;
> + compatible = "cirrus,ep9301-clk";

compatible is always first property.

> + clocks = <&xtali>;
> + };
> +...

Best regards,
Krzysztof

2023-06-01 06:58:38

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v1 02/43] dt-bindings: soc: Add Cirrus EP93xx

On 01/06/2023 07:33, Nikita Shubin wrote:
> This adds device tree bindings for the Cirrus Logic EP93xx.
>
> Signed-off-by: Nikita Shubin <[email protected]>

You already sent v1. This patchset is attached to the previous thread
making it more complicated for me to process. This buries it deep in the
mailbox and might interfere with applying entire sets.

Is this the next version, so v3? You already had at least two versions
before, so this cannot be v1.

> ---
>
> Notes:
> v0 -> v1:
>
> - fixed compatible - now it specifies three boards
> - ts7250
> - bk3
> - edb9302
> - fixed identation in example
> - dropped labels
>
> .../devicetree/bindings/arm/ep93xx.yaml | 107 ++++++++++++++++++
> .../dt-bindings/clock/cirrus,ep93xx-clock.h | 53 +++++++++
> 2 files changed, 160 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/ep93xx.yaml
> create mode 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h
>
> diff --git a/Documentation/devicetree/bindings/arm/ep93xx.yaml b/Documentation/devicetree/bindings/arm/ep93xx.yaml
> new file mode 100644
> index 000000000000..bcf9754d0763
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/ep93xx.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/ep93xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic EP93xx device tree bindings

No improvements.

> +
> +description: |+

no improvements. Do not need '|+' unless you need to preserve formatting.

> + The EP93xx SoC is a ARMv4T-based with 200 MHz ARM9 CPU.
> +
> +maintainers:
> + - Alexander Sverdlin <[email protected]>
> + - Nikita Shubin <[email protected]>
> +
> +properties:
> + $nodename:
> + const: '/'
> + compatible:
> + oneOf:
> + - description: The TS-7250 is a compact, full-featured Single Board Computer (SBC)
> + based upon the Cirrus EP9302 ARM9 CPU
> + items:
> + - const: technologic,ts7250
> + - const: cirrus,ep9301
> +
> + - description: The Liebherr BK3 is a derivate from ts7250 board
> + items:
> + - const: liebherr,bk3
> + - const: cirrus,ep9301
> +
> + - description: EDB302 is an evaluation board by Cirrus Logic,
> + based on a Cirrus Logic EP9302 CPU
> + items:
> + - const: cirrus,edb9302
> + - const: cirrus,ep9301
> +
> + soc:
> + type: object
> + patternProperties:
> + "^.*syscon@80930000$":
> + type: object
> + properties:
> + compatible:
> + items:
> + - const: cirrus,ep9301-syscon
> + - const: syscon
> + - const: simple-mfd
> + ep9301-reboot:
> + type: object
> + properties:
> + compatible:
> + const: cirrus,ep9301-reboot
> + required:
> + - compatible
> + - reg
> + - ep9301-reboot
> +
> + "^.*timer@80810000$":
> + type: object
> + properties:
> + compatible:
> + const: cirrus,ep9301-timer
> +
> + required:
> + - syscon@80930000
> + - timer@80810000

I don't understand what are you putting here. Why addresses are in
bindings (they should not be), why some nodes are documented in
top-level compatible. Drop all this.

Open existing files and look how it is done there.

> +
> +required:
> + - compatible
> + - soc> +
> +additionalProperties: true
> +
> +examples:
> + - |
> + / {
> + compatible = "technologic,ts7250", "cirrus,ep9301";
> + model = "TS-7250 SBC";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + soc {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + compatible = "simple-bus";
> +
> + syscon@80930000 {
> + compatible = "cirrus,ep9301-syscon",
> + "syscon", "simple-mfd";
> + reg = <0x80930000 0x1000>;
> +
> + ep9301-reboot {
> + compatible = "cirrus,ep9301-reboot";
> + };
> + };
> +
> + timer@80810000 {
> + compatible = "cirrus,ep9301-timer";
> + reg = <0x80810000 0x100>;
> + interrupt-parent = <&vic1>;
> + interrupts = <19>;
> + };
> + };
> + };

Drop all this. There is no existing binding like that.

> +
> +...
> diff --git a/include/dt-bindings/clock/cirrus,ep93xx-clock.h b/include/dt-bindings/clock/cirrus,ep93xx-clock.h

Not related to top level compatible.

> new file mode 100644
> index 000000000000..6a8cf33d811b
> --- /dev/null
> +++ b/include/dt-bindings/clock/cirrus,ep93xx-clock.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

Dual license.

> +#ifndef DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> +#define DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> +
> +#define EP93XX_CLK_XTALI 0
> +

Best regards,
Krzysztof

2023-06-01 06:58:45

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v1 08/43] dt-bindings: timers: Add Cirrus EP93xx

On 01/06/2023 07:33, Nikita Shubin wrote:
> This adds device tree bindings for the Cirrus Logic EP93xx
> timer block used in these SoCs.
>
> Signed-off-by: Nikita Shubin <[email protected]>
> ---
>
> Notes:
> v0 -> v1:
> - as Hartley (unfortunately) looks like not interested in dt rework,
> added myself instead
>
> Krzysztof Kozlowski:
> - removed wildcards
> - use fallback compatible and list all possible compatibles
> - fix ident
> - dropped bindings in title

I gave you already the tag. I also explained in lengthy message what to
do with it, yet you did not include it here.

I'll stop the review.

Best regards,
Krzysztof

2023-06-01 06:58:59

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v1 04/43] dt-bindings: clock: Add Cirrus EP93xx

2023-06-01 07:23:59

by Nikita Shubin

[permalink] [raw]

Subject: Re: [PATCH v1 02/43] dt-bindings: soc: Add Cirrus EP93xx

Hello Krzysztof!

On Thu, 1 Jun 2023 08:37:02 +0200
Krzysztof Kozlowski <[email protected]> wrote:

> On 01/06/2023 07:33, Nikita Shubin wrote:
> > This adds device tree bindings for the Cirrus Logic EP93xx.
> >
> > Signed-off-by: Nikita Shubin <[email protected]>
>
> You already sent v1. This patchset is attached to the previous thread
> making it more complicated for me to process. This buries it deep in
> the mailbox and might interfere with applying entire sets.

Sorry for that, i've already realized my mistake looking at my own
mailbox.

>
> Is this the next version, so v3? You already had at least two versions
> before, so this cannot be v1.

It's second on the mail lists, the first one was closed RFC.

The first was without any version, i.e. v0, this one is v1 (should be
v2).

I promise to be more careful next series.

All other comments acknowledged.

>
> > ---
> >
> > Notes:
> > v0 -> v1:
> >
> > - fixed compatible - now it specifies three boards
> > - ts7250
> > - bk3
> > - edb9302
> > - fixed identation in example
> > - dropped labels
> >
> > .../devicetree/bindings/arm/ep93xx.yaml | 107
> > ++++++++++++++++++ .../dt-bindings/clock/cirrus,ep93xx-clock.h |
> > 53 +++++++++ 2 files changed, 160 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/arm/ep93xx.yaml create mode
> > 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h
> >
> > diff --git a/Documentation/devicetree/bindings/arm/ep93xx.yaml
> > b/Documentation/devicetree/bindings/arm/ep93xx.yaml new file mode
> > 100644 index 000000000000..bcf9754d0763
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ep93xx.yaml
> > @@ -0,0 +1,107 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/ep93xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cirrus Logic EP93xx device tree bindings
>
> No improvements.
>
> > +
> > +description: |+
>
> no improvements. Do not need '|+' unless you need to preserve
> formatting.
>
>
> > + The EP93xx SoC is a ARMv4T-based with 200 MHz ARM9 CPU.
> > +
> > +maintainers:
> > + - Alexander Sverdlin <[email protected]>
> > + - Nikita Shubin <[email protected]>
> > +
> > +properties:
> > + $nodename:
> > + const: '/'
> > + compatible:
> > + oneOf:
> > + - description: The TS-7250 is a compact, full-featured
> > Single Board Computer (SBC)
> > + based upon the Cirrus EP9302 ARM9 CPU
> > + items:
> > + - const: technologic,ts7250
> > + - const: cirrus,ep9301
> > +
> > + - description: The Liebherr BK3 is a derivate from ts7250
> > board
> > + items:
> > + - const: liebherr,bk3
> > + - const: cirrus,ep9301
> > +
> > + - description: EDB302 is an evaluation board by Cirrus Logic,
> > + based on a Cirrus Logic EP9302 CPU
> > + items:
> > + - const: cirrus,edb9302
> > + - const: cirrus,ep9301
> > +
> > + soc:
> > + type: object
> > + patternProperties:
> > + "^.*syscon@80930000$":
> > + type: object
> > + properties:
> > + compatible:
> > + items:
> > + - const: cirrus,ep9301-syscon
> > + - const: syscon
> > + - const: simple-mfd
> > + ep9301-reboot:
> > + type: object
> > + properties:
> > + compatible:
> > + const: cirrus,ep9301-reboot
> > + required:
> > + - compatible
> > + - reg
> > + - ep9301-reboot
> > +
> > + "^.*timer@80810000$":
> > + type: object
> > + properties:
> > + compatible:
> > + const: cirrus,ep9301-timer
> > +
> > + required:
> > + - syscon@80930000
> > + - timer@80810000
>
> I don't understand what are you putting here. Why addresses are in
> bindings (they should not be), why some nodes are documented in
> top-level compatible. Drop all this.
>
> Open existing files and look how it is done there.
>
> > +
> > +required:
> > + - compatible
> > + - soc> +
> > +additionalProperties: true
> > +
> > +examples:
> > + - |
> > + / {
> > + compatible = "technologic,ts7250", "cirrus,ep9301";
> > + model = "TS-7250 SBC";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + soc {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > + compatible = "simple-bus";
> > +
> > + syscon@80930000 {
> > + compatible = "cirrus,ep9301-syscon",
> > + "syscon", "simple-mfd";
> > + reg = <0x80930000 0x1000>;
> > +
> > + ep9301-reboot {
> > + compatible = "cirrus,ep9301-reboot";
> > + };
> > + };
> > +
> > + timer@80810000 {
> > + compatible = "cirrus,ep9301-timer";
> > + reg = <0x80810000 0x100>;
> > + interrupt-parent = <&vic1>;
> > + interrupts = <19>;
> > + };
> > + };
> > + };
>
> Drop all this. There is no existing binding like that.
>
> > +
> > +...
> > diff --git a/include/dt-bindings/clock/cirrus,ep93xx-clock.h
> > b/include/dt-bindings/clock/cirrus,ep93xx-clock.h
>
> Not related to top level compatible.
>
> > new file mode 100644
> > index 000000000000..6a8cf33d811b
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/cirrus,ep93xx-clock.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
>
> Dual license.
>
> > +#ifndef DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> > +#define DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> > +
> > +#define EP93XX_CLK_XTALI 0
> > +
>
>
> Best regards,
> Krzysztof
>

2023-06-01 07:25:18

Hello Uwe!

On Thu, 1 Jun 2023 09:01:28 +0200
Uwe Kleine-König <[email protected]> wrote:

> Hello,
>
> On Thu, Jun 01, 2023 at 08:34:07AM +0300, Nikita Shubin wrote:
> > - find register range from the device tree
> > - provide clock access via of
> >
> > Signed-off-by: Nikita Shubin <[email protected]>
>
> Looks good to me,
>
> Acked-by: Uwe Kleine-König <[email protected]>

Thank you.

>
> There don't seem to be dependencies interdependencies to other patches
> in this series?! What is the merge plan here. Should this patch go in
> via the PWM tree, or will the whole series go in via armsoc?

We are hoping to get it merged as a whole, and not in pieces.

So we are trying to get Ack for all patches in series.

>
> Best regards
> Uwe
>

2023-06-01 11:51:25

On Thu, Jun 01, 2023 at 03:41:54PM +0300, Nikita Shubin wrote:
> Mark Brown <[email protected]> wrote:
> > On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote:

> > > + cirrus,ep9301-use-dma:
> > > + description: Flag indicating that the SPI should use dma
> > > + type: boolean

> > My previous feedback on this property still applies.

> > > + cirrus,ep9301-use-dma:

> The reason is that ep93xx DMA state is not quite device-tree ready at
> this moment, and clients use it with the help of:

> https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h

> I was hoping to slip by without changing much in ep93xx DMA driver, so

You're definign new ABI here, that's not a good thing to do for a
temporary workaround.

> I can move "use-dma" to module parameters, if this is acceptable.

That's less bad. I guess you could also define the bindings for the DMA
controller so that the properties are there then instead of properly
using the DMA API in the clients just check to see if the DMA properties
are present and then proceed accordingly?

Attachments:

(No filename) (1.10 kB)
signature.asc (499.00 B)
Download all attachments

2023-06-01 13:46:52

by Nikita Shubin

[permalink] [raw]

Subject: Re: [PATCH v1 17/43] dt-bindings: spi: Add Cirrus EP93xx

On Thu, 1 Jun 2023 13:55:03 +0100
Mark Brown <[email protected]> wrote:

> On Thu, Jun 01, 2023 at 03:41:54PM +0300, Nikita Shubin wrote:
> > Mark Brown <[email protected]> wrote:
> > > On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote:
>
> > > > + cirrus,ep9301-use-dma:
> > > > + description: Flag indicating that the SPI should use dma
> > > > + type: boolean
>
> > > My previous feedback on this property still applies.
>
> > > > + cirrus,ep9301-use-dma:
>
> > The reason is that ep93xx DMA state is not quite device-tree ready
> > at this moment, and clients use it with the help of:
>
> > https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h
> >
>
> > I was hoping to slip by without changing much in ep93xx DMA driver,
> > so
>
> You're definign new ABI here, that's not a good thing to do for a
> temporary workaround.
>
> > I can move "use-dma" to module parameters, if this is acceptable.
>
> That's less bad. I guess you could also define the bindings for the
> DMA controller so that the properties are there then instead of
> properly using the DMA API in the clients just check to see if the
> DMA properties are present and then proceed accordingly?

This sounds like a way to go. Thank you, Mark!

2023-06-01 14:25:22

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v1 40/43] ARM: ep93xx: soc: drop defines

Hi Nikita,

kernel test robot noticed the following build warnings:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.4-rc4]
[cannot apply to soc/for-next robh/for-next next-20230601]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Nikita-Shubin/dt-bindings-soc-Add-Cirrus-EP93xx/20230601-143415
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link: https://lore.kernel.org/r/20230601054549.10843-22-nikita.shubin%40maquefel.me
patch subject: [PATCH v1 40/43] ARM: ep93xx: soc: drop defines
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230601/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/372fa9eb75b475c6b8f26079e8716c95622ae9b7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nikita-Shubin/dt-bindings-soc-Add-Cirrus-EP93xx/20230601-143415
git checkout 372fa9eb75b475c6b8f26079e8716c95622ae9b7
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/input/keyboard/ sound/soc/cirrus/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from drivers/input/keyboard/ep93xx_keypad.c:30:
include/linux/soc/cirrus/ep93xx.h: In function 'ep93xx_devcfg_set_clear':
include/linux/soc/cirrus/ep93xx.h:22:16: error: 'return' with a value, in function returning void [-Werror=return-type]
22 | return 0;
| ^
include/linux/soc/cirrus/ep93xx.h:19:20: note: declared here
19 | static inline void ep93xx_devcfg_set_clear(unsigned int set_bits,
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: At top level:
>> include/linux/soc/cirrus/ep93xx.h:24:6: warning: no previous prototype for 'ep93xx_syscon_swlocked_write' [-Wmissing-prototypes]
24 | void ep93xx_syscon_swlocked_write(unsigned int val, unsigned int reg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: In function 'ep93xx_syscon_swlocked_write':
include/linux/soc/cirrus/ep93xx.h:26:16: error: 'return' with a value, in function returning void [-Werror=return-type]
26 | return 0;
| ^
include/linux/soc/cirrus/ep93xx.h:24:6: note: declared here
24 | void ep93xx_syscon_swlocked_write(unsigned int val, unsigned int reg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: At top level:
>> include/linux/soc/cirrus/ep93xx.h:28:6: warning: no previous prototype for 'ep93xx_swlocked_update_bits' [-Wmissing-prototypes]
28 | void ep93xx_swlocked_update_bits(unsigned int reg,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: In function 'ep93xx_swlocked_update_bits':
include/linux/soc/cirrus/ep93xx.h:31:16: error: 'return' with a value, in function returning void [-Werror=return-type]
31 | return 0;
| ^
include/linux/soc/cirrus/ep93xx.h:28:6: note: declared here
28 | void ep93xx_swlocked_update_bits(unsigned int reg,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/input/keyboard/ep93xx_keypad.c: In function 'ep93xx_keypad_probe':
drivers/input/keyboard/ep93xx_keypad.c:262:9: error: implicit declaration of function 'of_property_read_u32' [-Werror=implicit-function-declaration]
262 | of_property_read_u32(np, "cirrus,debounce-delay-ms", &keypad->debounce);
| ^~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from sound/soc/cirrus/ep93xx-i2s.c:28:
include/linux/soc/cirrus/ep93xx.h: In function 'ep93xx_devcfg_set_clear':
include/linux/soc/cirrus/ep93xx.h:22:16: error: 'return' with a value, in function returning void [-Werror=return-type]
22 | return 0;
| ^
include/linux/soc/cirrus/ep93xx.h:19:20: note: declared here
19 | static inline void ep93xx_devcfg_set_clear(unsigned int set_bits,
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: At top level:
>> include/linux/soc/cirrus/ep93xx.h:24:6: warning: no previous prototype for 'ep93xx_syscon_swlocked_write' [-Wmissing-prototypes]
24 | void ep93xx_syscon_swlocked_write(unsigned int val, unsigned int reg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: In function 'ep93xx_syscon_swlocked_write':
include/linux/soc/cirrus/ep93xx.h:26:16: error: 'return' with a value, in function returning void [-Werror=return-type]
26 | return 0;
| ^
include/linux/soc/cirrus/ep93xx.h:24:6: note: declared here
24 | void ep93xx_syscon_swlocked_write(unsigned int val, unsigned int reg)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: At top level:
>> include/linux/soc/cirrus/ep93xx.h:28:6: warning: no previous prototype for 'ep93xx_swlocked_update_bits' [-Wmissing-prototypes]
28 | void ep93xx_swlocked_update_bits(unsigned int reg,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/soc/cirrus/ep93xx.h: In function 'ep93xx_swlocked_update_bits':
include/linux/soc/cirrus/ep93xx.h:31:16: error: 'return' with a value, in function returning void [-Werror=return-type]
31 | return 0;
| ^
include/linux/soc/cirrus/ep93xx.h:28:6: note: declared here
28 | void ep93xx_swlocked_update_bits(unsigned int reg,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +/ep93xx_syscon_swlocked_write +24 include/linux/soc/cirrus/ep93xx.h

10
11 #if defined(CONFIG_ARCH_EP93XX)
12 unsigned int ep93xx_chip_revision(void);
13 void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits);
14 void ep93xx_syscon_swlocked_write(unsigned int val, unsigned int reg);
15 void ep93xx_swlocked_update_bits(unsigned int reg,
16 unsigned int mask, unsigned int val);
17 #else
18 static inline unsigned int ep93xx_chip_revision(void) { return 0; }
19 static inline void ep93xx_devcfg_set_clear(unsigned int set_bits,
20 unsigned int clear_bits)
21 {
> 22 return 0;
23 }
> 24 void ep93xx_syscon_swlocked_write(unsigned int val, unsigned int reg)
25 {
> 26 return 0;
27 }
> 28 void ep93xx_swlocked_update_bits(unsigned int reg,
29 unsigned int mask, unsigned int val)
30 {
31 return 0;
32 }
33 #endif
34

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-06-01 15:48:53

by kernel test robot

[permalink] [raw]

On Thu, Jun 01, 2023 at 08:45:33AM +0300, Nikita Shubin wrote:
> - get keymap from the device tree
> - find register range from the device tree
> - get interrupts from device tree
>
> Signed-off-by: Nikita Shubin <[email protected]>
> ---
>
> Notes:
> v0 -> v1:
>
> - fixed header
> - dropped coma in id table
> - take debounce, prescale from dt
> - remove ep93xx_keypad_platform_data
> - move flags to module params
> - drop setting clock rate, it's useless, as was never used,
> it seems we are okay with default clk rate used
> - move usefull defines from platform file here
> - drop platform header
>
> drivers/input/keyboard/ep93xx_keypad.c | 78 +++++++++++++-------------
> 1 file changed, 40 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c
> index 55075addcac2..8b0e73f56216 100644
> --- a/drivers/input/keyboard/ep93xx_keypad.c
> +++ b/drivers/input/keyboard/ep93xx_keypad.c
> @@ -20,6 +20,7 @@
> #include <linux/bits.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/interrupt.h>
> #include <linux/clk.h>
> #include <linux/io.h>
> @@ -27,7 +28,6 @@
> #include <linux/input/matrix_keypad.h>
> #include <linux/slab.h>
> #include <linux/soc/cirrus/ep93xx.h>
> -#include <linux/platform_data/keypad-ep93xx.h>
> #include <linux/pm_wakeirq.h>
>
> /*
> @@ -61,12 +61,18 @@
> #define KEY_REG_KEY1_MASK GENMASK(5, 0)
> #define KEY_REG_KEY1_SHIFT 0
>
> +#define EP93XX_MATRIX_ROWS (8)
> +#define EP93XX_MATRIX_COLS (8)
> +
> #define EP93XX_MATRIX_SIZE (EP93XX_MATRIX_ROWS * EP93XX_MATRIX_COLS)
>
> struct ep93xx_keypad {
> - struct ep93xx_keypad_platform_data *pdata;
> struct input_dev *input_dev;
> struct clk *clk;
> + unsigned int debounce;
> + unsigned int prescale;
> + unsigned int flags;
> + unsigned int clk_rate;
>
> void __iomem *mmio_base;
>
> @@ -80,6 +86,17 @@ struct ep93xx_keypad {
> bool enabled;
> };
>
> +/* flags for the ep93xx_keypad driver */
> +#define EP93XX_KEYPAD_DISABLE_3_KEY (1<<0) /* disable 3-key reset */
> +#define EP93XX_KEYPAD_DIAG_MODE (1<<1) /* diagnostic mode */
> +#define EP93XX_KEYPAD_BACK_DRIVE (1<<2) /* back driving mode */
> +#define EP93XX_KEYPAD_TEST_MODE (1<<3) /* scan only column 0 */
> +#define EP93XX_KEYPAD_AUTOREPEAT (1<<4) /* enable key autorepeat */
> +
> +static int ep93xx_keypad_flags;
> +module_param(ep93xx_keypad_flags, int, 0);
> +MODULE_PARM_DESC(ep93xx_keypad_flags, "EP93XX keypad flags.");
> +
> static irqreturn_t ep93xx_keypad_irq_handler(int irq, void *dev_id)
> {
> struct ep93xx_keypad *keypad = dev_id;
> @@ -133,23 +150,20 @@ static irqreturn_t ep93xx_keypad_irq_handler(int irq, void *dev_id)
>
> static void ep93xx_keypad_config(struct ep93xx_keypad *keypad)
> {
> - struct ep93xx_keypad_platform_data *pdata = keypad->pdata;
> unsigned int val = 0;
>
> - clk_set_rate(keypad->clk, pdata->clk_rate);
> -
> - if (pdata->flags & EP93XX_KEYPAD_DISABLE_3_KEY)
> + if (keypad->flags & EP93XX_KEYPAD_DISABLE_3_KEY)
> val |= KEY_INIT_DIS3KY;
> - if (pdata->flags & EP93XX_KEYPAD_DIAG_MODE)
> + if (keypad->flags & EP93XX_KEYPAD_DIAG_MODE)
> val |= KEY_INIT_DIAG;
> - if (pdata->flags & EP93XX_KEYPAD_BACK_DRIVE)
> + if (keypad->flags & EP93XX_KEYPAD_BACK_DRIVE)
> val |= KEY_INIT_BACK;
> - if (pdata->flags & EP93XX_KEYPAD_TEST_MODE)
> + if (keypad->flags & EP93XX_KEYPAD_TEST_MODE)
> val |= KEY_INIT_T2;
>
> - val |= ((pdata->debounce << KEY_INIT_DBNC_SHIFT) & KEY_INIT_DBNC_MASK);
> + val |= ((keypad->debounce << KEY_INIT_DBNC_SHIFT) & KEY_INIT_DBNC_MASK);
>
> - val |= ((pdata->prescale << KEY_INIT_PRSCL_SHIFT) & KEY_INIT_PRSCL_MASK);
> + val |= ((keypad->prescale << KEY_INIT_PRSCL_SHIFT) & KEY_INIT_PRSCL_MASK);
>
> __raw_writel(val, keypad->mmio_base + KEY_INIT);
> }
> @@ -220,17 +234,10 @@ static int ep93xx_keypad_resume(struct device *dev)
> static DEFINE_SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops,
> ep93xx_keypad_suspend, ep93xx_keypad_resume);
>
> -static void ep93xx_keypad_release_gpio_action(void *_pdev)
> -{
> - struct platform_device *pdev = _pdev;
> -
> - ep93xx_keypad_release_gpio(pdev);
> -}
> -
> static int ep93xx_keypad_probe(struct platform_device *pdev)
> {
> + struct device_node *np = pdev->dev.of_node;
> struct ep93xx_keypad *keypad;
> - const struct matrix_keymap_data *keymap_data;
> struct input_dev *input_dev;
> int err;
>
> @@ -238,14 +245,6 @@ static int ep93xx_keypad_probe(struct platform_device *pdev)
> if (!keypad)
> return -ENOMEM;
>
> - keypad->pdata = dev_get_platdata(&pdev->dev);
> - if (!keypad->pdata)
> - return -EINVAL;
> -
> - keymap_data = keypad->pdata->keymap_data;
> - if (!keymap_data)
> - return -EINVAL;
> -
> keypad->irq = platform_get_irq(pdev, 0);
> if (keypad->irq < 0)
> return keypad->irq;
> @@ -254,19 +253,15 @@ static int ep93xx_keypad_probe(struct platform_device *pdev)
> if (IS_ERR(keypad->mmio_base))
> return PTR_ERR(keypad->mmio_base);
>
> - err = ep93xx_keypad_acquire_gpio(pdev);
> - if (err)
> - return err;
> -
> - err = devm_add_action_or_reset(&pdev->dev,
> - ep93xx_keypad_release_gpio_action, pdev);
> - if (err)
> - return err;
> -
> keypad->clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(keypad->clk))
> return PTR_ERR(keypad->clk);
>
> + keypad->flags = ep93xx_keypad_flags;
> +
> + of_property_read_u32(np, "cirrus,debounce-delay-ms", &keypad->debounce);
> + of_property_read_u32(np, "cirrus,prescale", &keypad->prescale);

Please use device_property_read_*() API for this.

> +
> input_dev = devm_input_allocate_device(&pdev->dev);
> if (!input_dev)
> return -ENOMEM;
> @@ -278,13 +273,13 @@ static int ep93xx_keypad_probe(struct platform_device *pdev)
> input_dev->open = ep93xx_keypad_open;
> input_dev->close = ep93xx_keypad_close;
>
> - err = matrix_keypad_build_keymap(keymap_data, NULL,
> + err = matrix_keypad_build_keymap(NULL, NULL,
> EP93XX_MATRIX_ROWS, EP93XX_MATRIX_COLS,
> keypad->keycodes, input_dev);
> if (err)
> return err;
>
> - if (keypad->pdata->flags & EP93XX_KEYPAD_AUTOREPEAT)
> + if (keypad->flags & EP93XX_KEYPAD_AUTOREPEAT)
> __set_bit(EV_REP, input_dev->evbit);

I think this should be controlled by "autorepeat" device property.

> input_set_drvdata(input_dev, keypad);
>
> @@ -315,10 +310,17 @@ static int ep93xx_keypad_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct of_device_id ep93xx_keypad_of_ids[] = {
> + { .compatible = "cirrus,ep9307-keypad" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ep93xx_keypad_of_ids);
> +
> static struct platform_driver ep93xx_keypad_driver = {
> .driver = {
> .name = "ep93xx-keypad",
> .pm = pm_sleep_ptr(&ep93xx_keypad_pm_ops),
> + .of_match_table = ep93xx_keypad_of_ids,
> },
> .probe = ep93xx_keypad_probe,
> .remove = ep93xx_keypad_remove,
> --
> 2.37.4
>

Thanks.

--
Dmitry

2023-06-07 08:06:26

Hello Krzysztof!

On Thu, 2023-06-01 at 10:30 +0200, Krzysztof Kozlowski wrote:
> On 01/06/2023 07:45, Nikita Shubin wrote:
> > This adds a divice for Cirrus ep93xx SoC amd ts7250 board that has
> > been
>
> device
>
> > my testing target for ep93xx device support.
> >
> > Also inluded device tree for Liebherr BK3.1 board through it's not
> > a
>
> included
>
> > complete support.
>
> Thank you for your patch. There is something to discuss/improve.
>
>
> >
> > Signed-off-by: Nikita Shubin <[email protected]>
> > ---
> >
> > Notes:
> >     v0 -> v1:
> >
> >     - add empty chosen node
> >     - s/dallas,rtc-m48t86/st,m48t86/
> >     - changed phy_id to phy-handle
> >     - dropped gpio chip-label's
> >     - s/eth@80010000/ethernet@80010000
> >     - s/use_dma/ep9301,use-dma
> >     - added i2s to bk3
> >
> > arch/arm/boot/dts/Makefile          |   1 +
> > arch/arm/boot/dts/ep93xx-bk3.dts    | 119 +++++++
> > arch/arm/boot/dts/ep93xx-ts7250.dts | 132 ++++++++
> > arch/arm/boot/dts/ep93xx.dtsi       | 466
> > ++++++++++++++++++++++++++++
>
> Split adding DTSI from adding boards.
>
> > 4 files changed, 718 insertions(+)
> > create mode 100644 arch/arm/boot/dts/ep93xx-bk3.dts
> > create mode 100644 arch/arm/boot/dts/ep93xx-ts7250.dts
> > create mode 100644 arch/arm/boot/dts/ep93xx.dtsi
> >
> > diff --git a/arch/arm/boot/dts/Makefile
> > b/arch/arm/boot/dts/Makefile
> > index 59829fc90315..a68f868fffe7 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1670,3 +1670,4 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
> >         aspeed-bmc-vegman-n110.dtb \
> >         aspeed-bmc-vegman-rx20.dtb \
> >         aspeed-bmc-vegman-sx20.dtb
> > +dtb-$(CONFIG_ARCH_EP93XX) += ep93xx-ts7250.dtb
> > diff --git a/arch/arm/boot/dts/ep93xx-bk3.dts
> > b/arch/arm/boot/dts/ep93xx-bk3.dts
> > new file mode 100644
> > index 000000000000..215587c498e6
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/ep93xx-bk3.dts
> > @@ -0,0 +1,119 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree file for Liebherr controller BK3.1 based on Cirrus
> > EP9302 SoC
> > + */
> > +/dts-v1/;
> > +#include "ep93xx.dtsi"
> > +
> > +/ {
> > +       model = "Liebherr controller BK3.1";
> > +       compatible = "liebherr,bk3", "cirrus,ep9301";
> > +
> > +       chosen {
> > +       };
> > +
> > +       memory {
> > +               device_type = "memory";
> > +       };
> > +
> > +       soc {
> > +               nand-controller@60000000 {
>
> Override/extend by label/phandle.
>
> > +                       compatible = "technologic,ts7200-nand";
> > +                       reg = <0x60000000 0x8000000>;
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +
> > +                       partitions {
> > +                               compatible = "fixed-partitions";
> > +                               #address-cells = <1>;
> > +                               #size-cells = <1>;
> > +
> > +                               partition@0 {
> > +                                       label = "System";
> > +                                       reg = <0x00000000
> > 0x01e00000>;
> > +                                       read-only;
> > +                               };
> > +
> > +                               partition@1e00000 {
> > +                                       label = "Data";
> > +                                       reg = <0x01e00000
> > 0x05f20000>;
> > +                               };
> > +
> > +                               partition@7d20000 {
> > +                                       label = "RedBoot";
> > +                                       reg = <0x07d20000
> > 0x002e0000>;
> > +                                       read-only;
> > +                               };
> > +                       };
> > +               };
> > +
> > +               syscon: syscon@80930000 {
>
> Override/extend by label/phandle.
>
> > +                       pinctrl: pinctrl {
> > +                               compatible = "cirrus,ep9301-
> > pinctrl";
>
> Why this is board specific?

You are right - it's SoC specific, but currently we only have boards
with ep9302 with according device trees:
- ts7250
- bk3
- edb9302

We were hoping someone with other SoC's will join - like Hartley
Sweeten with Vision Engraving Systems EP9307 SoM, but no one showed up.

I've changed added compatible "cirrus,ep9301-pinctrl" in ep93xx.dtsi as
default and removed from all other board .dts.

>
> > +                       };
> > +               };
> > +
> > +               gpio1: gpio@80840004 {
>
> Override/extend by label/phandle.
>
> > +                       /* PWM */
> > +                       gpio-ranges = <&pinctrl 6 163 1>;
> > +               };
> > +       };
> > +};
> > +
> > +&gpio1 {
> > +       /* PWM */
> > +       gpio-ranges = <&pinctrl 6 163 1>;
> > +};
> > +
> > +&gpio4 {
> > +       gpio-ranges = <&pinctrl 0 97 2>;
> > +       status = "okay";
> > +};
> > +
> > +&gpio6 {
> > +       gpio-ranges = <&pinctrl 0 87 2>;
> > +       status = "okay";
> > +};
> > +
> > +&gpio7 {
> > +       gpio-ranges = <&pinctrl 2 199 4>;
> > +       status = "okay";
> > +};
> > +
> > +&i2c {
> > +       status = "okay";
> > +};
> > +
> > +&spi0: spi@808a0000 {
> > +       cs-gpios = <&gpio5 3 0>;
>
> Use proper defines for flags.
>
> > +       status = "okay";
>
> What's here? Empty enabled bus?
>
>
> > +};
> > +
> > +&eth0 {
> > +       phy-handle = <&phy0>;
> > +};
> > +
> > +&mdio0 {
> > +       phy0: ethernet-phy@1 {
> > +               reg = <1>;
> > +               device_type = "ethernet-phy";
> > +       };
> > +};
> > +
> > +&uart0 {
> > +       status = "okay";
> > +};
> > +
> > +&uart1 {
> > +       status = "okay";
> > +};
> > +
> > +&usb {
> > +       status = "okay";
> > +};
> > +
> > +&i2s {
>
> Up to you, but I seriously recommend keeping all labels ordered by
> name.
> Avoids conflicts.
>
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2s_on_ac97_pins>;
> > +       /delete-property/ status;
>
> ??? I don't understand. Why would you do this?
>
> ...
>
> > diff --git a/arch/arm/boot/dts/ep93xx.dtsi
> > b/arch/arm/boot/dts/ep93xx.dtsi
> > new file mode 100644
> > index 000000000000..6da556ceaf04
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/ep93xx.dtsi
> > @@ -0,0 +1,466 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree file for Cirrus Logic systems EP93XX SoC
> > + */
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/leds/common.h>
> > +#include <dt-bindings/input/input.h>
> > +#include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> > +/ {
> > +       soc {
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               ranges;
> > +               compatible = "simple-bus";
> > +
> > +               syscon: syscon@80930000 {
> > +                       compatible = "cirrus,ep9301-syscon",
> > +                                               "syscon", "simple-
> > mfd";
>
> Broken wrapping. Align these with previous ".
>
> > +                       reg = <0x80930000 0x1000>;
> > +
> > +                       ep9301-reboot {
>
> Just "reboot" (and fix bindings)... but why would you need it in the
> first place? I think something is seriously missing in your bindings.
>
> > +                               compatible = "cirrus,ep9301-
> > reboot";
> > +                       };
> > +
> > +                       eclk: clock-controller {
> > +                               #clock-cells = <1>;
> > +                               compatible = "cirrus,ep9301-clk";
> > +                               status = "okay";
> > +                               clocks = <&xtali>;
> > +                       };
> > +
> > +                       pinctrl: pinctrl {
>
> Missing compatible.
>
> > +                               spi_default_pins: pins-spi {
> > +                                       function = "spi";
> > +                                       groups = "ssp";
> > +                               };
> > +
> > +                               ac97_default_pins: pins-ac97 {
> > +                                       function = "ac97";
> > +                                       groups = "ac97";
> > +                               };
> > +
> > +                               i2s_on_ssp_pins: pins-i2sonssp {
> > +                                       function = "i2s";
> > +                                       groups = "i2s_on_ssp";
> > +                               };
> > +
> > +                               i2s_on_ac97_pins: pins-i2sonac97 {
> > +                                       function = "i2s";
> > +                                       groups = "i2s_on_ac97";
> > +                               };
> > +
> > +                               gpio1_default_pins: pins-gpio1 {
> > +                                       function = "gpio";
> > +                                       groups = "gpio1agrp";
> > +                               };
> > +
> > +                               pwm1_default_pins: pins-pwm1 {
> > +                                       function = "pwm";
> > +                                       groups = "pwm1";
> > +                               };
> > +
> > +                               gpio2_default_pins: pins-gpio2 {
> > +                                       function = "gpio";
> > +                                       groups = "gpio2agrp";
> > +                               };
> > +
> > +                               gpio3_default_pins: pins-gpio3 {
> > +                                       function = "gpio";
> > +                                       groups = "gpio3agrp";
> > +                               };
> > +
> > +                               keypad_default_pins: pins-keypad {
> > +                                       function = "keypad";
> > +                                       groups = "keypadgrp";
> > +                               };
> > +
> > +                               gpio4_default_pins: pins-gpio4 {
> > +                                       function = "gpio";
> > +                                       groups = "gpio4agrp";
> > +                               };
> > +
> > +                               gpio6_default_pins: pins-gpio6 {
> > +                                       function = "gpio";
> > +                                       groups = "gpio6agrp";
> > +                               };
> > +
> > +                               gpio7_default_pins: pins-gpio7 {
> > +                                       function = "gpio";
> > +                                       groups = "gpio7agrp";
> > +                               };
> > +
> > +                               ide_default_pins: pins-ide {
> > +                                       function = "pata";
> > +                                       groups = "idegrp";
> > +                               };
> > +
> > +                               lcd_on_dram0_pins: pins-
> > rasteronsdram0 {
> > +                                       function = "lcd";
> > +                                       groups =
> > "rasteronsdram0grp";
> > +                               };
> > +
> > +                               lcd_on_dram3_pins: pins-
> > rasteronsdram3 {
> > +                                       function = "lcd";
> > +                                       groups =
> > "rasteronsdram3grp";
>
> I would expect somewhere two groups since you explicitly allow it.
>
> > +                               };
> > +                       };
> > +               };
> > +
> > +               vic0: interrupt-controller@800b0000 {
> > +                       compatible = "arm,pl192-vic";
> > +                       interrupt-controller;
> > +                       reg = <0x800b0000 0x1000>;
>
> compatible first, reg is second. ranges if present - third. Fix it
> everywhere.
>
> > +                       #interrupt-cells = <1>;
> > +                       valid-mask = <0x7ffffffc>;
> > +                       valid-wakeup-mask = <0x0>;
> > +               };
> > +
> > +               vic1: interrupt-controller@800c0000 {
> > +                       compatible = "arm,pl192-vic";
> > +                       interrupt-controller;
> > +                       reg = <0x800c0000 0x1000>;
> > +                       #interrupt-cells = <1>;
> > +                       valid-mask = <0x1fffffff>;
> > +                       valid-wakeup-mask = <0x0>;
> > +               };
> > +
> > +               timer: timer@80810000 {
> > +                       compatible = "cirrus,ep9301-timer";
> > +                       reg = <0x80810000 0x100>;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <19>;
> > +               };
> > +
> > +               dma0: dma-controller@80000000 {
> > +                       compatible = "cirrus,ep9301-dma-m2p";
> > +                       reg = <0x80000000 0x0040>,
> > +                               <0x80000040 0x0040>,
> > +                               <0x80000080 0x0040>,
> > +                               <0x800000c0 0x0040>,
> > +                               <0x80000240 0x0040>,
> > +                               <0x80000200 0x0040>,
> > +                               <0x800002c0 0x0040>,
> > +                               <0x80000280 0x0040>,
> > +                               <0x80000340 0x0040>,
> > +                               <0x80000300 0x0040>;
> > +                       clocks = <&eclk EP93XX_CLK_M2P0>,
> > +                               <&eclk EP93XX_CLK_M2P1>,
> > +                               <&eclk EP93XX_CLK_M2P2>,
> > +                               <&eclk EP93XX_CLK_M2P3>,
> > +                               <&eclk EP93XX_CLK_M2P4>,
> > +                               <&eclk EP93XX_CLK_M2P5>,
> > +                               <&eclk EP93XX_CLK_M2P6>,
> > +                               <&eclk EP93XX_CLK_M2P7>,
> > +                               <&eclk EP93XX_CLK_M2P8>,
> > +                               <&eclk EP93XX_CLK_M2P9>;
> > +                       clock-names = "m2p0", "m2p1",
> > +                               "m2p2", "m2p3",
> > +                               "m2p4", "m2p5",
> > +                               "m2p6", "m2p7",
> > +                               "m2p8", "m2p9";
> > +                       interrupt-parent = <&vic0>;
> > +                       interrupts = <7>, <8>, <9>, <10>, <11>,
> > +                               <12>, <13>, <14>, <15>, <16>;
> > +                       #dma-cells = <1>;
> > +               };
> > +
> > +               dma1: dma-controller@80000100 {
> > +                       compatible = "cirrus,ep9301-dma-m2m";
> > +                       reg = <0x80000100 0x0040>,
> > +                               <0x80000140 0x0040>;
> > +                       clocks = <&eclk EP93XX_CLK_M2M0>,
> > +                               <&eclk EP93XX_CLK_M2M1>;
> > +                       clock-names = "m2m0", "m2m1";
> > +                       interrupt-parent = <&vic0>;
> > +                       interrupts = <17>, <18>;
> > +                       #dma-cells = <1>;
> > +               };
> > +
> > +               i2s: i2s@80820000 {
> > +                       compatible = "cirrus,ep9301-i2s";
> > +                       #sound-dai-cells = <0>;
> > +                       reg = <0x80820000 0x100>;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <28>;
> > +                       clocks = <&eclk EP93XX_CLK_I2S_MCLK
> > +                                 &eclk EP93XX_CLK_I2S_SCLK
> > +                                 &eclk EP93XX_CLK_I2S_LRCLK>;
> > +                       clock-names = "mclk", "sclk", "lrclk";
> > +                       status = "disabled";
> > +               };
> > +
> > +               gpio0: gpio@80840000 {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x80840000 0x04>,
> > +                       <0x80840010 0x04>,
> > +                       <0x80840090 0x1c>;
>
> Messed wrapping.
>
> > +                       reg-names = "data", "dir", "intr";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       interrupt-controller;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <27>;
> > +               };
> > +
> > +               gpio1: gpio@80840004 {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x80840004 0x04>,
> > +                       <0x80840014 0x04>,
> > +                       <0x808400ac 0x1c>;
>
> Ditto, in other places as well.
>
> > +                       reg-names = "data", "dir", "intr";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       interrupt-controller;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <27>;
> > +               };
> > +
> > +               gpio2: gpio@80840008 {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x80840008 0x04>,
> > +                       <0x80840018 0x04>;
> > +                       reg-names = "data", "dir";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       status = "disabled";
>
> Status is usually last.
>
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&gpio2_default_pins>;
> > +               };
> > +
> > +               gpio3: gpio@8084000c {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x8084000c 0x04>,
> > +                       <0x8084001c 0x04>;
> > +                       reg-names = "data", "dir";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       status = "disabled";
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&gpio3_default_pins>;
> > +               };
> > +
> > +               gpio4: gpio@80840020 {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x80840020 0x04>,
> > +                       <0x80840024 0x04>;
> > +                       reg-names = "data", "dir";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       status = "disabled";
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&gpio4_default_pins>;
> > +               };
> > +
> > +               gpio5: gpio@80840030 {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x80840030 0x04>,
> > +                       <0x80840034 0x04>,
> > +                       <0x8084004c 0x1c>;
> > +                       reg-names = "data", "dir", "intr";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       interrupt-controller;
> > +                       interrupts-extended = <&vic0 19>, <&vic0
> > 20>,
> > +                               <&vic0 21>, <&vic0 22>,
> > +                               <&vic1 15>, <&vic1 16>,
> > +                               <&vic1 17>, <&vic1 18>;
> > +               };
> > +
> > +               gpio6: gpio@80840038 {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x80840038 0x04>,
> > +                       <0x8084003c 0x04>;
> > +                       reg-names = "data", "dir";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       status = "disabled";
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&gpio6_default_pins>;
> > +               };
> > +
> > +               gpio7: gpio@80840040 {
> > +                       compatible = "cirrus,ep9301-gpio";
> > +                       reg = <0x80840040 0x04>,
> > +                       <0x80840044 0x04>;
> > +                       reg-names = "data", "dir";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       status = "disabled";
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&gpio7_default_pins>;
> > +               };
> > +
> > +               ide: ide@800a0000 {
> > +                       compatible = "cirrus,ep9312-pata";
> > +                       reg = <0x800a0000 0x38>;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <8>;
> > +                       status = "disabled";
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&ide_default_pins>;
> > +               };
> > +
> > +               uart0: uart@808c0000 {
>
> This should scream with dtbs_check. serial.
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for
> instructions).

make dtbs_check is totally clean now:

```
make -j34 -C build-linux ARCH=arm CROSS_COMPILE=armv4t-softfloat-linux-
gnueabi- dtbs_check
make[1]: Entering directory '/home/maquefel/workshop/ts7250-boot-
build/build-linux'
UPD include/config/kernel.release
DTC_CHK arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb
arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb:0:0: /soc/ac97@80880000:
failed to match any schema with compatible: ['cirrus,ep9301-ac97']
arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb:0:0:
/soc/spi@808a0000/codec@0: failed to match any schema with compatible:
['cirrus,cs4271']
/home/maquefel/workshop/ts7250-boot-build/build-
linux/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dtb: sound: 'simple-
audio-card,convert-sample-format' does not match any of the regexes:
'^simple-audio-card,codec(@[0-9a-f]+)?$', '^simple-audio-card,cpu(@[0-
9a-f]+)?$', '^simple-audio-card,dai-link(@[0-9a-f]+)?$', '^simple-
audio-card,plat(@[0-9a-f]+)?$', 'pinctrl-[0-9]+'
From schema: /home/maquefel/workshop/ts7250-boot-
build/linux/Documentation/devicetree/bindings/sound/simple-card.yaml
make[1]: Leaving directory '/home/maquefel/workshop/ts7250-boot-
build/build-linux'
```

>
> > +                       compatible = "arm,primecell";
> > +                       reg = <0x808c0000 0x1000>;
> > +                       arm,primecell-periphid = <0x00041010>;
> > +                       clocks = <&eclk EP93XX_CLK_UART1>, <&eclk
> > EP93XX_CLK_UART>;
> > +                       clock-names = "apb:uart1", "apb_pclk";
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <20>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               uart1: uart@808d0000 {
> > +                       compatible = "arm,primecell";
> > +                       reg = <0x808d0000 0x1000>;
> > +                       arm,primecell-periphid = <0x00041010>;
> > +                       clocks = <&eclk EP93XX_CLK_UART2>, <&eclk
> > EP93XX_CLK_UART>;
> > +                       clock-names = "apb:uart2", "apb_pclk";
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <22>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               uart2: uart@808b0000 {
> > +                       compatible = "arm,primecell";
> > +                       reg = <0x808b0000 0x1000>;
> > +                       arm,primecell-periphid = <0x00041010>;
> > +                       clocks = <&eclk EP93XX_CLK_UART3>, <&eclk
> > EP93XX_CLK_UART>;
> > +                       clock-names = "apb:uart3", "apb_pclk";
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <23>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               usb0: usb@80020000 {
> > +                       compatible = "generic-ohci";
> > +                       reg = <0x80020000 0x10000>;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <24>;
> > +                       clocks = <&eclk EP93XX_CLK_USB>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               eth0: ethernet@80010000 {
> > +                       compatible = "cirrus,ep9301-eth";
> > +                       reg = <0x80010000 0x10000>;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <7>;
> > +                       mdio0: mdio {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
>
> Your SoC comes with mdio? If so, why is this empty?

It has a builtin MDIO and only reason for this is to provide phy-id via
device tree in a correct way:

```
&eth0 {
phy-handle = <&phy0>;
};

&mdio0 {
phy0: ethernet-phy@1 {
reg = <1>;
device_type = "ethernet-phy";
};
};
```

There is no other purpose for this node.

Thank you Krzysztof!

All other issues fixed.

>
> > +                       };
> > +               };
> > +
> > +               rtc0: rtc@80920000 {
> > +                       compatible = "cirrus,ep9301-rtc";
> > +                       reg = <0x80920000 0x100>;
> > +               };
> > +
> > +               spi0: spi@808a0000 {
> > +                       compatible = "cirrus,ep9301-spi";
> > +                       #address-cells = <1>;
> > +                       #size-cells = <0>;
> > +                       reg = <0x808a0000 0x18>;
> > +                       interrupt-parent = <&vic1>;
> > +                       interrupts = <21>;
> > +                       clocks = <&eclk EP93XX_CLK_SPI>;
> > +                       cs-gpios = <&gpio5 2 0>;
>
> defines... but why is it here in the first place? Rarely CS gpios are
> part of the SoC. I have several of such questions further as well, so
> this looks like you are mixing SoC and boards in one file.
>
> > +                       cirrus,ep9301-use-dma;
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&spi_default_pins>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               adc: adc@80900000 {
> > +                       compatible = "cirrus,ep9301-adc";
> > +                       reg = <0x80900000 0x28>;
> > +                       clocks = <&eclk EP93XX_CLK_ADC>;
> > +                       interrupt-parent = <&vic0>;
> > +                       interrupts = <30>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               watchdog0: watchdog@80940000 {
> > +                       compatible = "cirrus,ep9301-wdt";
> > +                       reg = <0x80940000 0x08>;
> > +               };
> > +
> > +               pwm0: pwm@80910000 {
> > +                       compatible = "cirrus,ep9301-pwm";
> > +                       reg = <0x80910000 0x10>;
> > +                       clocks = <&eclk EP93XX_CLK_PWM>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               pwm1: pwm@80910020 {
> > +                       compatible = "cirrus,ep9301-pwm";
> > +                       reg = <0x80910020 0x10>;
> > +                       clocks = <&eclk EP93XX_CLK_PWM>;
> > +                       status = "disabled";
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&pwm1_default_pins>;
> > +               };
> > +
> > +               keypad: keypad@800f0000 {
> > +                       compatible = "cirrus,ep9307-keypad";
> > +                       reg = <0x800f0000 0x0c>;
> > +                       interrupt-parent = <&vic0>;
> > +                       interrupts = <29>;
> > +                       clocks = <&eclk EP93XX_CLK_KEYPAD>;
> > +                       pinctrl-names = "default";
> > +                       pinctrl-0 = <&keypad_default_pins>;
> > +                       linux,keymap =
> > +                                               <KEY_UP>,
> > +                                               <KEY_DOWN>,
> > +                                               <KEY_VOLUMEDOWN>,
> > +                                               <KEY_HOME>,
> > +                                               <KEY_RIGHT>,
> > +                                               <KEY_LEFT>,
> > +                                               <KEY_ENTER>,
> > +                                               <KEY_VOLUMEUP>,
> > +                                               <KEY_F6>,
> > +                                               <KEY_F8>,
> > +                                               <KEY_F9>,
> > +                                               <KEY_F10>,
> > +                                               <KEY_F1>,
> > +                                               <KEY_F2>,
> > +                                               <KEY_F3>,
> > +                                               <KEY_POWER>;
> > +               };
> > +       };
> > +
> > +       xtali: oscillator {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <14745600>;
> > +               clock-output-names = "xtali";
> > +       };
> > +
> > +       i2c0: i2c0 {
>
> i2c or i2c-0
>
> > +               compatible = "i2c-gpio";
> > +               sda-gpios = <&gpio6 1
> > (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +               scl-gpios = <&gpio6 0
> > (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +               status = "disabled";
>
> Wait, what? Why this is disabled? If this is part of the SoC,
> although
> hardly looks like, then it should be complete. What is missing? How
> one
> could design SoC with incomplete GPIO I2C controller?
>
> > +       };
> > +
> > +       leds {
> > +               compatible = "gpio-leds";
>
> I really doubt this is property of the SoC. Please double check as it
> really looks wrong.
>
>
> > +               led0 {
>
> led-0
>
> > +                       label = "grled";
> > +                       gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> > +                       linux,default-trigger = "heartbeat";
> > +                       function = LED_FUNCTION_HEARTBEAT;
> > +               };
> > +
> > +               led1 {
> led-1
>
> > +                       label = "rdled";
> > +                       gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
> > +                       function = LED_FUNCTION_FAULT;
> > +               };
> > +       };
> > +};
>
> Best regards,
> Krzysztof
>