by Heiko Stübner

[permalink] [raw]

Subject: Re: [PATCH v2 01/16] dt-bindings: display: convert ilitek,ili9341.txt to dt-schema

Am Dienstag, 28. Juni 2022, 17:00:56 CEST schrieb Heiko St?bner:
> Hi Conor,
>
> Am Montag, 27. Juni 2022, 21:39:49 CEST schrieb Conor Dooley:
> > From: Conor Dooley <[email protected]>
> >
> > A dt-schema binding for the Ilitek ili9341 was created as
> > panel/ilitek,ili9341.yaml but the txt binding was ignored in the
> > process. Move the remaining items in the txt binding to the yaml one &
> > delete it.
> >
> > The example in the txt binding has a spi-max-frequency which disagrees
> > with the yaml replacement (and its own documentation) so change that to
> > conform with the binding. There are no users in tree of the Adafruit
> > yx240qv29 to check against.
> >
> > Link: https://cdn-learn.adafruit.com/assets/assets/000/046/879/original/SPEC-YX240QV29-T_Rev.A__1_.pdf
> > Signed-off-by: Conor Dooley <[email protected]>
>
> in your v1 you already got a [0]
> Reviewed-by: Rob Herring <[email protected]>
> for this patch
>
> Looking at the diffstat, which didn't change, you should
> include such tags in followup revisions, to prevent
> reviewers from double efforts.

and now I see that the review actually happened _after_ v2 was send ;-)

> [0] https://lore.kernel.org/r/[email protected]
>
> > ---
> > .../bindings/display/ilitek,ili9341.txt | 27 -----------
> > .../display/panel/ilitek,ili9341.yaml | 48 +++++++++++++------
> > 2 files changed, 34 insertions(+), 41 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> > deleted file mode 100644
> > index 169b32e4ee4e..000000000000
> > --- a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
> > +++ /dev/null
> > @@ -1,27 +0,0 @@
> > -Ilitek ILI9341 display panels
> > -
> > -This binding is for display panels using an Ilitek ILI9341 controller in SPI
> > -mode.
> > -
> > -Required properties:
> > -- compatible: "adafruit,yx240qv29", "ilitek,ili9341"
> > -- dc-gpios: D/C pin
> > -- reset-gpios: Reset pin
> > -
> > -The node for this driver must be a child node of a SPI controller, hence
> > -all mandatory properties described in ../spi/spi-bus.txt must be specified.
> > -
> > -Optional properties:
> > -- rotation: panel rotation in degrees counter clockwise (0,90,180,270)
> > -- backlight: phandle of the backlight device attached to the panel
> > -
> > -Example:
> > - display@0{
> > - compatible = "adafruit,yx240qv29", "ilitek,ili9341";
> > - reg = <0>;
> > - spi-max-frequency = <32000000>;
> > - dc-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
> > - reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
> > - rotation = <270>;
> > - backlight = <&backlight>;
> > - };
> > diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
> > index 6058948a9764..94ca92878434 100644
> > --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
> > @@ -23,6 +23,7 @@ properties:
> > - enum:
> > # ili9341 240*320 Color on stm32f429-disco board
> > - st,sf-tc240t-9370-t
> > + - adafruit,yx240qv29
> > - const: ilitek,ili9341
> >
> > reg: true
> > @@ -47,31 +48,50 @@ properties:
> > vddi-led-supply:
> > description: Voltage supply for the LED driver (1.65 .. 3.3 V)
> >
> > -additionalProperties: false
> > +unevaluatedProperties: false
> >
> > required:
> > - compatible
> > - reg
> > - dc-gpios
> > - - port
> > +
> > +if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - st,sf-tc240t-9370-t
> > +then:
> > + required:
> > + - port
> >
> > examples:
> > - |+
> > + #include <dt-bindings/gpio/gpio.h>
> > spi {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > panel: display@0 {
> > - compatible = "st,sf-tc240t-9370-t",
> > - "ilitek,ili9341";
> > - reg = <0>;
> > - spi-3wire;
> > - spi-max-frequency = <10000000>;
> > - dc-gpios = <&gpiod 13 0>;
> > - port {
> > - panel_in: endpoint {
> > - remote-endpoint = <&display_out>;
> > - };
> > - };
> > - };
> > + compatible = "st,sf-tc240t-9370-t",
> > + "ilitek,ili9341";
> > + reg = <0>;
> > + spi-3wire;
> > + spi-max-frequency = <10000000>;
> > + dc-gpios = <&gpiod 13 0>;
> > + port {
> > + panel_in: endpoint {
> > + remote-endpoint = <&display_out>;
> > + };
> > + };
> > + };
> > + display@1{
> > + compatible = "adafruit,yx240qv29", "ilitek,ili9341";
> > + reg = <1>;
> > + spi-max-frequency = <10000000>;
> > + dc-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
> > + reset-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
> > + rotation = <270>;
> > + backlight = <&backlight>;
> > };
> > + };
> > ...
> >
>
>

2022-06-28 16:01:22

by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v2 04/16] spi: dt-bindings: dw-apb-ssi: update spi-{r,t}x-bus-width

On Mon, Jun 27, 2022 at 11:21:49PM +0300, Serge Semin wrote:
> On Mon, Jun 27, 2022 at 08:39:52PM +0100, Conor Dooley wrote:
> > From: Conor Dooley <[email protected]>
> >
> > Most users of dw-apb-ssi use spi-{r,t}x-bus-width of 1, however the
> > Canaan k210 is wired up for a width of 4.
> > Quoting Serge:
> > The modern DW APB SSI controllers of v.4.* and newer also support the
> > enhanced SPI Modes too (Dual, Quad and Octal). Since the IP-core
> > version is auto-detected at run-time there is no way to create a
> > DT-schema correctly constraining the Rx/Tx SPI bus widths.
> > /endquote
> >
> > As such, drop the restriction on only supporting a bus width of 1.
> >
> > Link: https://lore.kernel.org/all/20220620205654.g7fyipwytbww5757@mobilestation/
> > Signed-off-by: Conor Dooley <[email protected]>
> > ---
> > Serge, I dropped your R-b when I swapped to the default
> > property since it changed the enum.
> > ---
> > Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 6 ------
> > 1 file changed, 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > index e25d44c218f2..0a43d6e0ef91 100644
> > --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> > @@ -143,12 +143,6 @@ patternProperties:
> > minimum: 0
> > maximum: 3
> >
>
> > - spi-rx-bus-width:
> > - const: 1
> > -
> > - spi-tx-bus-width:
> > - const: 1
> > -
>
> My comment was:
> > > > You can just use a more relaxed constraint "enum: [1 2 4 8]" here
> > >
> > > 8 too? sure.
> Then Rob said:
> > Then no constraints needed because the common definition already has
> > this presumably.
>
> IMO preserving the device-specific constraints even if they match the
> generic ones has some maintainability benefits. What if you get to
> discover a new HW which supports Hexal mode?

x16? Wouldn't we be back to parallel NOR and the problems with parallel
buses?

> Then you would have
> needed to update the common schema constraints. But that would have
> caused permitting the unsupported bus-mode for all the schemas, which
> isn't correct. So as I see it the explicit bus-width enumeration would
> be ok to have here. But I'll leave it for Rob to make a final
> decision.

Assuming a new width does appear, it's just a matter of time before the
DW block has a new rev supporting it too, so there's 2 places to update.
Also, a given platform may pinout less than the block supports, so you
can't ever be 100% sure an out of range value is in a DT.

But either way is okay with me. If you do keep constraints, you only
need 'maximum: 8'.

Acked-by: Rob Herring <[email protected]>

Rob