mbox series

[00/43] ep93xx device tree conversion

Message ID 20230424123522.18302-1-nikita.shubin@maquefel.me
Headers show
Series ep93xx device tree conversion | expand

Message

Nikita Shubin April 24, 2023, 12:34 p.m. UTC
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.

Alexander Sverdlin (4):
  ARM: dts: ep93xx: Add ADC node
  ARM: dts: ep93xx: Add I2S and AC97 nodes
  ARM: dts: ep93xx: Add EDB9302 DT
  ASoC: cirrus: edb93xx: Delete driver

Nikita Shubin (39):
  gpio: ep93xx: split device in multiple
  soc: Add SoC driver for Cirrus ep93xx
  dt-bindings: pinctrl: Add DT bindings ep93xx pinctrl
  pinctrl: add a Cirrus ep93xx SoC pin controller
  dt-bindings: timers: add DT bindings for Cirrus EP93xx
  clocksource: ep93xx: Add driver for Cirrus Logic EP93xx
  dt-bindings: rtc: add DT bindings for Cirrus EP93xx
  rtc: ep93xx: add DT support for Cirrus EP93xx
  dt-bindings: watchdog: add DT bindings for Cirrus EP93x
  watchdog: ep93xx: add DT support for Cirrus EP93xx
  dt-bindings: clock: add DT bindings for Cirrus EP93xx
  clk: ep93xx: add DT support for Cirrus EP93xx
  power: reset: Add a driver for the ep93xx reset
  dt-bindings: pwm: Add DT bindings ep93xx PWM
  pwm: ep93xx: add DT support for Cirrus EP93xx
  dt-bindings: spi: Add DT bindings ep93xx spi
  spi: ep93xx: add DT support for Cirrus EP93xx
  dt-bindings: net: Add DT bindings ep93xx eth
  net: cirrus: add DT support for Cirrus EP93xx
  dt-bindings: dma: Add DT bindings ep93xx dma
  dma: cirrus: add DT support for Cirrus EP93xx
  dt-bindings: mtd: add DT bindings for ts7250 nand
  mtd: ts72xx_nand: add platform helper
  dt-bindings: ata: Add DT bindings ep93xx pata
  pata: cirrus: add DT support for Cirrus EP93xx
  dt-bindings: input: Add DT bindings ep93xx keypad
  input: keypad: ep93xx: add DT support for Cirrus EP93xx
  dt-bindings: rtc: Add DT binding m48t86 rtc
  rtc: m48t86: add DT support for m48t86
  dt-bindings: wdt: Add DT binding ts72xx wdt
  wdt: ts72xx: add DT support for ts72xx
  dt-bindings: gpio: Add DT bindings ep93xx gpio
  gpio: ep93xx: add DT support for gpio-ep93xx
  ARM: dts: add device tree for ep93xx Soc
  ARM: ep93xx: DT for the Cirrus ep93xx SoC platforms
  pwm: ep93xx: drop legacy pinctrl
  input: keypad: ep93xx: drop legacy pinctrl
  ARM: ep93xx: soc: drop defines
  ARM: ep93xx: delete all boardfiles

 .../devicetree/bindings/arm/ep93xx.yaml       |   99 +
 .../bindings/ata/cirrus,ep93xx-pata.yaml      |   40 +
 .../bindings/dma/cirrus,ep93xx-dma-m2m.yaml   |   66 +
 .../bindings/dma/cirrus,ep93xx-dma-m2p.yaml   |  102 +
 .../devicetree/bindings/gpio/gpio-ep93xx.yaml |  161 ++
 .../bindings/input/cirrus,ep93xx-keypad.yaml  |  123 ++
 .../bindings/mtd/technologic,nand.yaml        |   56 +
 .../bindings/net/cirrus,ep93xx_eth.yaml       |   51 +
 .../pinctrl/cirrus,ep93xx-pinctrl.yaml        |   66 +
 .../bindings/pwm/cirrus,ep93xx-pwm.yaml       |   45 +
 .../bindings/rtc/cirrus,ep93xx-rtc.yaml       |   32 +
 .../bindings/rtc/dallas,rtc-m48t86.yaml       |   33 +
 .../devicetree/bindings/spi/spi-ep93xx.yaml   |   68 +
 .../bindings/timer/cirrus,ep93xx-timer.yaml   |   41 +
 .../bindings/watchdog/cirrus,ep93xx-wdt.yaml  |   38 +
 .../watchdog/technologic,ts72xx-wdt.yaml      |   39 +
 arch/arm/Makefile                             |    1 -
 arch/arm/boot/dts/Makefile                    |    1 +
 arch/arm/boot/dts/ep93xx-bk3.dts              |   96 +
 arch/arm/boot/dts/ep93xx-edb9302.dts          |  150 ++
 arch/arm/boot/dts/ep93xx-ts7250.dts           |  113 ++
 arch/arm/boot/dts/ep93xx.dtsi                 |  466 +++++
 arch/arm/mach-ep93xx/Kconfig                  |   20 +-
 arch/arm/mach-ep93xx/Makefile                 |   11 -
 arch/arm/mach-ep93xx/core.c                   | 1017 ----------
 arch/arm/mach-ep93xx/dma.c                    |  114 --
 arch/arm/mach-ep93xx/edb93xx.c                |  344 ----
 arch/arm/mach-ep93xx/ep93xx-regs.h            |   38 -
 arch/arm/mach-ep93xx/gpio-ep93xx.h            |  111 --
 arch/arm/mach-ep93xx/hardware.h               |   25 -
 arch/arm/mach-ep93xx/irqs.h                   |   76 -
 arch/arm/mach-ep93xx/platform.h               |   42 -
 arch/arm/mach-ep93xx/soc.h                    |  212 --
 arch/arm/mach-ep93xx/ts72xx.c                 |  422 ----
 arch/arm/mach-ep93xx/ts72xx.h                 |   94 -
 arch/arm/mach-ep93xx/vision_ep9307.c          |  311 ---
 drivers/ata/pata_ep93xx.c                     |    9 +
 drivers/clk/Kconfig                           |    8 +
 drivers/clk/Makefile                          |    1 +
 .../clock.c => drivers/clk/clk-ep93xx.c       |  491 +++--
 drivers/clocksource/Kconfig                   |   11 +
 drivers/clocksource/Makefile                  |    1 +
 .../clocksource}/timer-ep93xx.c               |  143 +-
 drivers/dma/ep93xx_dma.c                      |  119 +-
 drivers/gpio/gpio-ep93xx.c                    |  329 ++--
 drivers/input/keyboard/ep93xx_keypad.c        |   25 +-
 drivers/mtd/nand/raw/Kconfig                  |    8 +
 drivers/mtd/nand/raw/Makefile                 |    1 +
 drivers/mtd/nand/raw/ts72xx_nand.c            |   94 +
 drivers/net/ethernet/cirrus/ep93xx_eth.c      |   49 +-
 drivers/pinctrl/Kconfig                       |    7 +
 drivers/pinctrl/Makefile                      |    1 +
 drivers/pinctrl/pinctrl-ep93xx.c              | 1698 +++++++++++++++++
 drivers/power/reset/Kconfig                   |   10 +
 drivers/power/reset/Makefile                  |    1 +
 drivers/power/reset/ep93xx-restart.c          |   65 +
 drivers/pwm/pwm-ep93xx.c                      |   24 +-
 drivers/rtc/rtc-ep93xx.c                      |    8 +
 drivers/rtc/rtc-m48t86.c                      |   10 +
 drivers/soc/Kconfig                           |    1 +
 drivers/soc/Makefile                          |    1 +
 drivers/soc/cirrus/Kconfig                    |   11 +
 drivers/soc/cirrus/Makefile                   |    2 +
 drivers/soc/cirrus/soc-ep93xx.c               |  134 ++
 drivers/spi/spi-ep93xx.c                      |   31 +-
 drivers/watchdog/ep93xx_wdt.c                 |    8 +
 drivers/watchdog/ts72xx_wdt.c                 |    8 +
 .../dt-bindings/clock/cirrus,ep93xx-clock.h   |   53 +
 include/linux/platform_data/dma-ep93xx.h      |    3 +
 include/linux/soc/cirrus/ep93xx.h             |   28 +-
 sound/soc/cirrus/Kconfig                      |    9 -
 sound/soc/cirrus/Makefile                     |    4 -
 sound/soc/cirrus/edb93xx.c                    |  119 --
 73 files changed, 4796 insertions(+), 3453 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/ep93xx.yaml
 create mode 100644 Documentation/devicetree/bindings/ata/cirrus,ep93xx-pata.yaml
 create mode 100644 Documentation/devicetree/bindings/dma/cirrus,ep93xx-dma-m2m.yaml
 create mode 100644 Documentation/devicetree/bindings/dma/cirrus,ep93xx-dma-m2p.yaml
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-ep93xx.yaml
 create mode 100644 Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
 create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml
 create mode 100644 Documentation/devicetree/bindings/net/cirrus,ep93xx_eth.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/cirrus,ep93xx-pwm.yaml
 create mode 100644 Documentation/devicetree/bindings/rtc/cirrus,ep93xx-rtc.yaml
 create mode 100644 Documentation/devicetree/bindings/rtc/dallas,rtc-m48t86.yaml
 create mode 100644 Documentation/devicetree/bindings/spi/spi-ep93xx.yaml
 create mode 100644 Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml
 create mode 100644 Documentation/devicetree/bindings/watchdog/cirrus,ep93xx-wdt.yaml
 create mode 100644 Documentation/devicetree/bindings/watchdog/technologic,ts72xx-wdt.yaml
 create mode 100644 arch/arm/boot/dts/ep93xx-bk3.dts
 create mode 100644 arch/arm/boot/dts/ep93xx-edb9302.dts
 create mode 100644 arch/arm/boot/dts/ep93xx-ts7250.dts
 create mode 100644 arch/arm/boot/dts/ep93xx.dtsi
 delete mode 100644 arch/arm/mach-ep93xx/Makefile
 delete mode 100644 arch/arm/mach-ep93xx/core.c
 delete mode 100644 arch/arm/mach-ep93xx/dma.c
 delete mode 100644 arch/arm/mach-ep93xx/edb93xx.c
 delete mode 100644 arch/arm/mach-ep93xx/ep93xx-regs.h
 delete mode 100644 arch/arm/mach-ep93xx/gpio-ep93xx.h
 delete mode 100644 arch/arm/mach-ep93xx/hardware.h
 delete mode 100644 arch/arm/mach-ep93xx/irqs.h
 delete mode 100644 arch/arm/mach-ep93xx/platform.h
 delete mode 100644 arch/arm/mach-ep93xx/soc.h
 delete mode 100644 arch/arm/mach-ep93xx/ts72xx.c
 delete mode 100644 arch/arm/mach-ep93xx/ts72xx.h
 delete mode 100644 arch/arm/mach-ep93xx/vision_ep9307.c
 rename arch/arm/mach-ep93xx/clock.c => drivers/clk/clk-ep93xx.c (60%)
 rename {arch/arm/mach-ep93xx => drivers/clocksource}/timer-ep93xx.c (51%)
 create mode 100644 drivers/mtd/nand/raw/ts72xx_nand.c
 create mode 100644 drivers/pinctrl/pinctrl-ep93xx.c
 create mode 100644 drivers/power/reset/ep93xx-restart.c
 create mode 100644 drivers/soc/cirrus/Kconfig
 create mode 100644 drivers/soc/cirrus/Makefile
 create mode 100644 drivers/soc/cirrus/soc-ep93xx.c
 create mode 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h
 delete mode 100644 sound/soc/cirrus/edb93xx.c

Comments

Arnd Bergmann April 24, 2023, 11:31 a.m. UTC | #1
On Mon, Apr 24, 2023, at 14:34, 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.

Thanks a lot for your continued work. I can't merge any of this at
the moment since the upstream merge window just opened, but I'm
happy to take this all through the soc tree for 6.5, provided we
get the sufficient Acks from the subsystem maintainers. Merging
it through each individual tree would take a lot longer, so I
hope we can avoid that.

      Arnd
Rob Herring April 24, 2023, 1:28 p.m. UTC | #2
On Mon, 24 Apr 2023 15:34:19 +0300, Nikita Shubin wrote:
> Add YAML bindings ep93xx SoC.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> 
> Notes:
>     rfc->v0:
>     - dropped separate bindings description, left only one with all groups,
>       functions and etc...
>     - added Alexander Sverdlin to maintainers
>     - added Linus Reviwed-by tags, through i shoudn't =) too many changes
>     - fixed warning and added seq_file header
> 
>  .../pinctrl/cirrus,ep93xx-pinctrl.yaml        | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.example.dtb: /example-0/syscon@80930000: failed to match any schema with compatible: ['cirrus,ep9301-syscon', 'syscon', 'simple-mfd']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230424123522.18302-4-nikita.shubin@maquefel.me

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski April 25, 2023, 9:20 a.m. UTC | #3
On 25/04/2023 00:29, Jakub Kicinski wrote:
> On Mon, 24 Apr 2023 13:31:25 +0200 Arnd Bergmann wrote:
>> Thanks a lot for your continued work. I can't merge any of this at
>> the moment since the upstream merge window just opened, but I'm
>> happy to take this all through the soc tree for 6.5, provided we
>> get the sufficient Acks from the subsystem maintainers. Merging
>> it through each individual tree would take a lot longer, so I
>> hope we can avoid that.
> 
> Is there a dependency between the patches?

I didn't get entire patchset and cover letter does not mention
dependencies, but usually there shouldn't be such. Maybe for the next
versions this should be split per subsystem?

Best regards,
Krzysztof
Krzysztof Kozlowski April 25, 2023, 9:24 a.m. UTC | #4
On 24/04/2023 14:34, Nikita Shubin wrote:
> Add YAML bindings ep93xx SoC.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> 
> Notes:
>     rfc->v0:
>     - dropped separate bindings description, left only one with all groups,
>       functions and etc...
>     - added Alexander Sverdlin to maintainers
>     - added Linus Reviwed-by tags, through i shoudn't =) too many changes
>     - fixed warning and added seq_file header
> 
>  .../pinctrl/cirrus,ep93xx-pinctrl.yaml        | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml
> new file mode 100644
> index 000000000000..cba4be7c5994
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-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,ep93xx-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus ep93xx pins mux controller
> +
> +maintainers:
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - cirrus,ep9301-pinctrl
> +      - cirrus,ep9307-pinctrl
> +      - cirrus,ep9312-pinctrl

Blank line

> +  regmap:
> +    description: phandle to syscon

This should be specific - vendor prefix, property name, explanation what
do you need here in description. "phandle to syscon" is redundant.

You also miss type.

Anyway, your example contradicts this. You do not have regmap.

> +
> +patternProperties:
> +  '^pinctrl-':

'^pins-' instead, because pinctrl is the entire device.

> +    type: object
> +    description: pin node
> +    $ref: pinmux-node.yaml#
> +
> +    properties:
> +      function:
> +        enum: [ spi, ac97, i2s, pwm, keypad, pata, lcd, gpio1, gpio2, gpio3,
> +                gpio4, gpio6, gpio7 ]

Why gpio has different versions? gpio should be one function.

> +      groups:
> +        minItems: 1
> +        maxItems: 2
> +        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
> +  - regmap
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    syscon: syscon@80930000 {
> +        compatible = "cirrus,ep9301-syscon",
> +                     "syscon", "simple-mfd";

You created now dependency on this binding. Drop the parent node. If
this is supposed to be always part of syscon, then example could be in
parent's node... but I have doubts that is a part of syscon in the first
place.

> +        reg = <0x80930000 0x1000>;
> +        #clock-cells = <1>;
> +        #reset-cells = <1>;
> +        pinctrl: pinctrl {
> +                compatible = "cirrus,ep9312-pinctrl";

Mixed indentation. Actually before also looks odd...
Use 4 spaces for example indentation.

> +                regmap = <&syscon>;
> +                spi_default_pins: pinctrl-spi {
> +                        function = "spi";
> +                        groups = "ssp";
> +                };
> +        };
> +    };

Best regards,
Krzysztof
Arnd Bergmann April 25, 2023, 1:27 p.m. UTC | #5
On Tue, Apr 25, 2023, at 10:20, Krzysztof Kozlowski wrote:
> On 25/04/2023 00:29, Jakub Kicinski wrote:
>> On Mon, 24 Apr 2023 13:31:25 +0200 Arnd Bergmann wrote:
>>> Thanks a lot for your continued work. I can't merge any of this at
>>> the moment since the upstream merge window just opened, but I'm
>>> happy to take this all through the soc tree for 6.5, provided we
>>> get the sufficient Acks from the subsystem maintainers. Merging
>>> it through each individual tree would take a lot longer, so I
>>> hope we can avoid that.
>> 
>> Is there a dependency between the patches?
>
> I didn't get entire patchset and cover letter does not mention
> dependencies, but usually there shouldn't be such. Maybe for the next
> versions this should be split per subsystem?

Clearly the last patch that removes the board files depends on
all the previous patches, but I assume that the other ones
are all independent.

We don't do complete conversions from boardfiles to DT that often
any more, but in the past we tended to do this through a cross-
subsystem branch in the soc tree, which helps do it more quickly
and is less work for Nikita. In this case, I would make it a
separate top-level branch in the soc tree.

If anyone strongly feels that the patches should go through
the subsystem trees here, we'll take the longer path and
do the changes separately, with the boardfile removal
coming a release later.

     Arnd
Linus Walleij April 26, 2023, 8:56 p.m. UTC | #6
On Mon, Apr 24, 2023 at 11:35 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:

> This series aims to convert ep93xx from platform to full device tree support.
>
> Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302.

Neat, I'd say let's merge this for 6.5 once the final rough edges are
off. The DT bindings should be easy to fix.

This is a big patch set and the improvement to the ARM kernel it
brings is great, so I am a bit worried about over-review stalling the
merged. If there start to be nitpicky comments I would prefer that
we merge it and let minor comments and "nice-to-haves" be
addressed in-tree during the development cycle.

I encourage you to use b4 to manage the patch series if you
have time to learn it, it could help you:
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1

Yours,
Linus Walleij
Linus Walleij April 26, 2023, 9:06 p.m. UTC | #7
On Wed, Apr 26, 2023 at 11:02 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Apr 26, 2023 at 10:56:53PM +0200, Linus Walleij wrote:
>
> > This is a big patch set and the improvement to the ARM kernel it
> > brings is great, so I am a bit worried about over-review stalling the
> > merged. If there start to be nitpicky comments I would prefer that
> > we merge it and let minor comments and "nice-to-haves" be
> > addressed in-tree during the development cycle.
>
> I'm really not enthusiastic about the SPI bindings being merged as-is.

Agree, the bindings are more important than the code IMO,
they tend to get written in stone.

Yours,
Linus Walleij
Florian Fainelli May 16, 2023, 3:47 a.m. UTC | #8
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
Nikita Shubin May 16, 2023, 10:37 a.m. UTC | #9
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
Krzysztof Kozlowski June 1, 2023, 6:42 a.m. UTC | #10
On 01/06/2023 07:33, Nikita Shubin wrote:
> Add YAML bindings for ep93xx SoC pinctrl.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> 
> 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 <nikita.shubin@maquefel.me>
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +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
Andy Shevchenko June 2, 2023, 1:50 a.m. UTC | #11
Thu, Jun 01, 2023 at 08:33:52AM +0300, Nikita Shubin kirjoitti:
> This prepares ep93xx SOC gpio to convert into device tree driver:
> - dropped banks and legacy defines
> - split AB IRQ and make it shared
> 
> We are relying on IRQ number information A, B ports have single shared
> IRQ, while F port have dedicated IRQ for each line.
> 
> Also we had to split single ep93xx platform_device into multiple, one
> for each port, without this we can't do a full working transition from
> legacy platform code into device tree capable. All GPIO_LOOKUP were
> change to match new chip namings.

First of all, check if you added In-Reply-to email header to the previous
thread, at least `b4` downloaded 188 messages in this one so far. Second,
the previous was kinda v0, while we usually assume that non-versioned series
is v1. This is a bit ambiguous.

...

> +		GPIO_LOOKUP_IDX("gpio-ep93xx.4", 1,	NULL, 1, GPIO_ACTIVE_HIGH),

TAB used instead of space.

...

>  struct device __init *ep93xx_init_devices(void)
>  {
>  	struct device *parent;
> +	int i;

It's unsigned, right?

> +	for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_device); i++)
> +		platform_device_register(ep93xx_gpio_device[i]);

...

>  	writeb(eic->int_debounce,
> -	       epg->base + eic->irq_offset + EP93XX_INT_DEBOUNCE_OFFSET);
> +	       eic->base + EP93XX_INT_DEBOUNCE_OFFSET);

Now this can be a single line. Also some other cases may be optimized.

...

> +	void __iomem *intr = devm_platform_ioremap_resource_byname(pdev, "intr");

It's less error prone if the assignment is split from definition and moved
closer to its (first) user...

> +

...here.

> +	if (IS_ERR(intr))
> +		return PTR_ERR(intr);

...

> +	egc->eic = devm_kcalloc(dev, 1,
> +				sizeof(*egc->eic),
> +				GFP_KERNEL);

Why kcalloc(1), is this a part that will be (slightly) modified in the next
patches in the series?

> +	if (!egc->eic)
> +		return -ENOMEM;
>  
...

> +		irq = platform_get_irq(pdev, 0);

No return value check?

> +		ret = devm_request_irq(dev, irq,
> +				ep93xx_ab_irq_handler,
> +				IRQF_SHARED, gc->label, gc);
> +		if (ret) {
> +			dev_err(dev, "error requesting IRQ : %d\n", irq);
> +			return ret;

If it's soslely part of the ->probe() flow, you may use dev_err_probe().

> +		}
>  
> +		girq->parents[0] = irq;

...

>  		for (i = 0; i < girq->num_parents; i++) {
> +			irq = platform_get_irq(pdev, i);
> +			if (irq <= 0)

== 0 is never happen case. Why?

> +				continue;
> +
> +			girq->parents[i] = irq;
>  		}

> +	ret = bgpio_init(gc, &pdev->dev, 1, data, NULL, NULL, dir, NULL, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to init generic GPIO\n");
> +		return ret;

		return dev_err_probe(...);

>  	}

...

> +	if (platform_irq_count(pdev) > 0) {

Do you need this check?

> +		dev_dbg(&pdev->dev, "setting up irqs for %s\n", dev_name(&pdev->dev));
> +		ret = ep93xx_setup_irqs(pdev, egc);
> +		if (ret)
> +			dev_err(&pdev->dev, "setup irqs failed for %s\n", dev_name(&pdev->dev));

If it's an error, why continuing?

> +	}
Linus Walleij June 2, 2023, 7:40 a.m. UTC | #12
On Thu, Jun 1, 2023 at 10:20 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:

> > +title: EP93xx GPIO controller
> > +
> > +maintainers:
> > +  - Linus Walleij <linus.walleij@linaro.org>
> > +  - Bartosz Golaszewski <brgl@bgdev.pl>
>
> Did you choose correct maintainers? Bartosz, Linus, do you take care
> about EP93xx platform?

I'm fine with it (I have a platform).

Yours,
Linus Walleij
Bartosz Golaszewski June 13, 2023, 2:55 p.m. UTC | #13
On Fri, Jun 2, 2023 at 9:41 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Jun 1, 2023 at 10:20 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>
> > > +title: EP93xx GPIO controller
> > > +
> > > +maintainers:
> > > +  - Linus Walleij <linus.walleij@linaro.org>
> > > +  - Bartosz Golaszewski <brgl@bgdev.pl>
> >
> > Did you choose correct maintainers? Bartosz, Linus, do you take care
> > about EP93xx platform?
>
> I'm fine with it (I have a platform).

I don't but I'm actually not sure how DT bindings maintainership works
- do GPIO bindings all fall under the GPIO jurisdiction automatically?

Bart
Linus Walleij June 13, 2023, 6:09 p.m. UTC | #14
On Tue, Jun 13, 2023 at 4:55 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Fri, Jun 2, 2023 at 9:41 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, Jun 1, 2023 at 10:20 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:

> > > Did you choose correct maintainers? Bartosz, Linus, do you take care
> > > about EP93xx platform?
> >
> > I'm fine with it (I have a platform).
>
> I don't but I'm actually not sure how DT bindings maintainership works
> - do GPIO bindings all fall under the GPIO jurisdiction automatically?

Not really, more on the people selected by the person writing the
bindings, confirmed by them being merged.

Traditionally, Linux as the biggest software project with the most
active subsystem maintainers do the reviewing and take the
responsibility for them.

Technically, e.g. BSD people could appear
on the devicetree mailinglist and review patches and suggest
maintainership from their side, but I haven't seen them much
around. Neither Apple Computer or anyone else who ought
to be there but isn't.

Yours,
Linus Walleij
Nikita Shubin June 15, 2023, 4:56 p.m. UTC | #15
Hello Andy !

On Fri, 2023-06-02 at 04:50 +0300, andy.shevchenko@gmail.com wrote:
> Thu, Jun 01, 2023 at 08:33:52AM +0300, Nikita Shubin kirjoitti:
> > This prepares ep93xx SOC gpio to convert into device tree driver:
> > - dropped banks and legacy defines
> > - split AB IRQ and make it shared
> > 
> > We are relying on IRQ number information A, B ports have single
> > shared
> > IRQ, while F port have dedicated IRQ for each line.
> > 
> > Also we had to split single ep93xx platform_device into multiple,
> > one
> > for each port, without this we can't do a full working transition
> > from
> > legacy platform code into device tree capable. All GPIO_LOOKUP were
> > change to match new chip namings.
> 
> First of all, check if you added In-Reply-to email header to the
> previous
> thread, at least `b4` downloaded 188 messages in this one so far.
> Second,
> the previous was kinda v0, while we usually assume that non-versioned
> series
> is v1. This is a bit ambiguous.
> 
> ...
> 
> > +               GPIO_LOOKUP_IDX("gpio-ep93xx.4", 1,     NULL, 1,
> > GPIO_ACTIVE_HIGH),
> 
> TAB used instead of space.
> 
> ...
> 
> >  struct device __init *ep93xx_init_devices(void)
> >  {
> >         struct device *parent;
> > +       int i;
> 
> It's unsigned, right?
> 
> > +       for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_device); i++)
> > +               platform_device_register(ep93xx_gpio_device[i]);
> 
> ...
> 
> >         writeb(eic->int_debounce,
> > -              epg->base + eic->irq_offset +
> > EP93XX_INT_DEBOUNCE_OFFSET);
> > +              eic->base + EP93XX_INT_DEBOUNCE_OFFSET);
> 
> Now this can be a single line. Also some other cases may be
> optimized.
> 
> ...
> 
> > +       void __iomem *intr =
> > devm_platform_ioremap_resource_byname(pdev, "intr");
> 
> It's less error prone if the assignment is split from definition and
> moved
> closer to its (first) user...
> 
> > +
> 
> ...here.
> 
> > +       if (IS_ERR(intr))
> > +               return PTR_ERR(intr);
> 
> ...
> 
> > +       egc->eic = devm_kcalloc(dev, 1,
> > +                               sizeof(*egc->eic),
> > +                               GFP_KERNEL);
> 
> Why kcalloc(1), is this a part that will be (slightly) modified in
> the next
> patches in the series?
> 
> > +       if (!egc->eic)
> > +               return -ENOMEM;
> >  
> ...
> 
> > +               irq = platform_get_irq(pdev, 0);
> 
> No return value check?
> 
> > +               ret = devm_request_irq(dev, irq,
> > +                               ep93xx_ab_irq_handler,
> > +                               IRQF_SHARED, gc->label, gc);
> > +               if (ret) {
> > +                       dev_err(dev, "error requesting IRQ : %d\n",
> > irq);
> > +                       return ret;
> 
> If it's soslely part of the ->probe() flow, you may use
> dev_err_probe().
> 
> > +               }
> >  
> > +               girq->parents[0] = irq;
> 
> ...
> 
> >                 for (i = 0; i < girq->num_parents; i++) {
> > +                       irq = platform_get_irq(pdev, i);
> > +                       if (irq <= 0)
> 
> == 0 is never happen case. Why?
> 
> > +                               continue;
> > +
> > +                       girq->parents[i] = irq;
> >                 }
> 
> > +       ret = bgpio_init(gc, &pdev->dev, 1, data, NULL, NULL, dir,
> > NULL, 0);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "unable to init generic
> > GPIO\n");
> > +               return ret;
> 
>                 return dev_err_probe(...);
> 
> >         }
> 
> ...
> 
> > +       if (platform_irq_count(pdev) > 0) {
> 
> Do you need this check?

Only A/B/F ports have irq's so we don't bother setting up for other
ports. 

> 
> > +               dev_dbg(&pdev->dev, "setting up irqs for %s\n",
> > dev_name(&pdev->dev));
> > +               ret = ep93xx_setup_irqs(pdev, egc);
> > +               if (ret)
> > +                       dev_err(&pdev->dev, "setup irqs failed for
> > %s\n", dev_name(&pdev->dev));
> 
> If it's an error, why continuing?

Well - it's not fatal, we can still use gpios even without irq's but we
should warn.

All other comments acknowledged and fixed, thank you.

> 
> > +       }
>