On Wed, Nov 22, 2023 at 11:59:44AM +0300, Nikita Shubin wrote:
> Add device tree bindings for the Cirrus Logic EP93xx SoC.
>
> Signed-off-by: Nikita Shubin <[email protected]>
> ---
> .../bindings/arm/cirrus/cirrus,ep9301.yaml | 38 ++++++++++

> .../bindings/soc/cirrus/cirrus,ep9301-syscon.yaml | 83 ++++++++++++++++++++++
> include/dt-bindings/soc/cirrus,ep9301-syscon.h | 46 ++++++++++++

This looks like it is mostly clocks. I'd put it in bindings/clock/ and
include/dt-bindings/clock/ instead.

> 3 files changed, 167 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/cirrus/cirrus,ep9301.yaml b/Documentation/devicetree/bindings/arm/cirrus/cirrus,ep9301.yaml
> new file mode 100644
> index 000000000000..97dd8b6aefa9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/cirrus/cirrus,ep9301.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/cirrus/cirrus,ep9301.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic EP93xx platforms
> +
> +description:
> + 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

Wrap lines at 80 unless there's some advantage to going to 100. Here
there is not.

> + 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
> +
> +additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/soc/cirrus/cirrus,ep9301-syscon.yaml b/Documentation/devicetree/bindings/soc/cirrus/cirrus,ep9301-syscon.yaml
> new file mode 100644
> index 000000000000..283cf2386d95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/cirrus/cirrus,ep9301-syscon.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/cirrus/cirrus,ep9301-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic EP93xx Platforms System Controller
> +
> +maintainers:
> + - Alexander Sverdlin <[email protected]>
> + - Nikita Shubin <[email protected]>
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - enum:
> + - cirrus,ep9302-syscon
> + - cirrus,ep9307-syscon
> + - cirrus,ep9312-syscon
> + - cirrus,ep9315-syscon

The registers of these are all the same as (or a superset of) the
EP9301? That's what you are claiming. Perhaps already discussed, but
there's no blurb here to explain the relationship.

'syscon' is a Linux term. Is this block really called this?

> + - const: cirrus,ep9301-syscon
> + - const: syscon
> + - const: simple-mfd
> + - items:
> + - const: cirrus,ep9301-syscon
> + - const: syscon
> + - const: simple-mfd

Doesn't look like an simple-mfd to me. You don't have independent child
blocks which don't depend on the parent node.

> +
> + reg:
> + maxItems: 1
> +
> + "#clock-cells":
> + const: 1
> +
> + clocks:
> + items:
> + - description: reference clock
> +
> +patternProperties:
> + '^pins-':
> + type: object
> + description: pin node
> + $ref: /schemas/pinctrl/pinmux-node.yaml
> +
> + properties:
> + function:
> + enum: [ spi, ac97, i2s, pwm, keypad, pata, lcd, gpio ]
> +
> + groups:
> + enum: [ ssp, ac97, i2s_on_ssp, i2s_on_ac97, pwm1, gpio1agrp,
> + gpio2agrp, gpio3agrp, gpio4agrp, gpio6agrp, gpio7agrp,
> + rasteronsdram0grp, rasteronsdram3grp, keypadgrp, idegrp ]
> +
> + required:
> + - function
> + - groups
> +
> + unevaluatedProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - "#clock-cells"
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + syscon@80930000 {
> + compatible = "cirrus,ep9301-syscon",
> + "syscon", "simple-mfd";
> + reg = <0x80930000 0x1000>;
> +
> + #clock-cells = <1>;
> + clocks = <&xtali>;
> +
> + spi_default_pins: pins-spi {
> + function = "spi";
> + groups = "ssp";
> + };
> + };
> diff --git a/include/dt-bindings/soc/cirrus,ep9301-syscon.h b/include/dt-bindings/soc/cirrus,ep9301-syscon.h
> new file mode 100644
> index 000000000000..6bb8f532e7d0
> --- /dev/null
> +++ b/include/dt-bindings/soc/cirrus,ep9301-syscon.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +#ifndef DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> +#define DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H
> +
> +#define EP93XX_CLK_PLL1 0
> +#define EP93XX_CLK_PLL2 1
> +
> +#define EP93XX_CLK_FCLK 2
> +#define EP93XX_CLK_HCLK 3
> +#define EP93XX_CLK_PCLK 4
> +
> +#define EP93XX_CLK_UART 5
> +#define EP93XX_CLK_SPI 6
> +#define EP93XX_CLK_PWM 7
> +#define EP93XX_CLK_USB 8
> +
> +#define EP93XX_CLK_M2M0 9
> +#define EP93XX_CLK_M2M1 10
> +
> +#define EP93XX_CLK_M2P0 11
> +#define EP93XX_CLK_M2P1 12
> +#define EP93XX_CLK_M2P2 13
> +#define EP93XX_CLK_M2P3 14
> +#define EP93XX_CLK_M2P4 15
> +#define EP93XX_CLK_M2P5 16
> +#define EP93XX_CLK_M2P6 17
> +#define EP93XX_CLK_M2P7 18
> +#define EP93XX_CLK_M2P8 19
> +#define EP93XX_CLK_M2P9 20
> +
> +#define EP93XX_CLK_UART1 21
> +#define EP93XX_CLK_UART2 22
> +#define EP93XX_CLK_UART3 23
> +
> +#define EP93XX_CLK_ADC 24
> +#define EP93XX_CLK_ADC_EN 25
> +
> +#define EP93XX_CLK_KEYPAD 26
> +
> +#define EP93XX_CLK_VIDEO 27
> +
> +#define EP93XX_CLK_I2S_MCLK 28
> +#define EP93XX_CLK_I2S_SCLK 29
> +#define EP93XX_CLK_I2S_LRCLK 30
> +
> +#endif /* DT_BINDINGS_CIRRUS_EP93XX_CLOCK_H */
>
> --
> 2.41.0
>

2023-12-01 20:18:57

Hello Andy!

On Wed, 2023-11-22 at 14:31 +0200, Andy Shevchenko wrote:
> On Wed, Nov 22, 2023 at 12:00:17PM +0300, Nikita Shubin wrote:
> > Remove DMA platform header, from now on we use device tree for dma
>
> DMA
>
> > clients.
>
> ...
>
> > +       if (device_is_compatible(chan->device->dev, "cirrus,ep9301-
> > dma-m2p"))
> > +               return true;
> > +
> > +       return !strcmp(dev_name(chan->device->dev), "ep93xx-dma-
> > m2p");
>
> Haven't you introduced an inliner with the similar flow? Why not
> reuse it?

It's the same, i moved it from platform header file into driver itself,
as i am removing "include/linux/platform_data/dma-ep93xx.h" completely.

>
> ...
>
> > +/**
> > + * ep93xx_dma_chan_direction - returns direction the channel can
> > be used
> > + * @chan: channel
> > + *
> > + * This function can be used in filter functions to find out
> > whether the
> > + * channel supports given DMA direction. Only M2P channels have
> > such
> > + * limitation, for M2M channels the direction is configurable.
>
> I believe
>
>         scripts/kernel-doc -v -none -Wall ...
>
> against this file (and maybe others!) will complain ("no return
> section"
> or alike).
>
> > + */
>

Agree, dropped doc style comment and converted it to simple comment, it
is internal to driver now, so no point putting it into docs.