Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 12 Mar 2022 20:23:48 +0100
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Andreas Kemnade <andreas@kemnade.info>
Cc:     p.zabel@pengutronix.de, airlied@linux.ie, daniel@ffwll.ch,
        robh+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
        kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
        maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, alistair@alistair23.me,
        samuel@sholland.org, josua.mayer@jm0.eu,
        letux-kernel@openphoenux.org
Subject: Re: [RFC PATCH 1/6] dt-bindings: display: imx: Add EPDC
Message-ID: <YizzRKSDvHon83ks@latitude>
References: <20220206080016.796556-1-andreas@kemnade.info>
 <20220206080016.796556-2-andreas@kemnade.info>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="4dGDc1ulGkdHTd51"
Content-Disposition: inline
In-Reply-To: <20220206080016.796556-2-andreas@kemnade.info>
Precedence: bulk


--4dGDc1ulGkdHTd51
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello Andreas,

Sorry for the delay, I finally got around to having a look at the
patchset.

Some comments from skimming the patches below, and in my other replies.


On Sun, Feb 06, 2022 at 09:00:11AM +0100, Andreas Kemnade wrote:
> Add a binding for the Electrophoretic Display Controller found at least
> in the i.MX6.
> The timing subnode is directly here to avoid having display parameters
> spread all over the plate.
>=20
> Supplies are organized the same way as in the fbdev driver in the
> NXP/Freescale kernel forks. The regulators used for that purpose,
> like the TPS65185, the SY7636A and MAX17135 have typically a single bit to
> start a bunch of regulators of higher or negative voltage with a
> well-defined timing. VCOM can be handled separately, but can also be
> incorporated into that single bit.
>=20
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  .../bindings/display/imx/fsl,mxc-epdc.yaml    | 159 ++++++++++++++++++
>  1 file changed, 159 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,mxc=
-epdc.yaml
>=20
> diff --git a/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.y=
aml b/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
> new file mode 100644
> index 000000000000..7e0795cc3f70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/imx/fsl,mxc-epdc.yaml
> @@ -0,0 +1,159 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
[...]
> +  - vscan-holdoff
> +  - sdoed-width
> +  - sdoed-delay
> +  - sdoez-width
> +  - sdoez-delay
> +  - gdclk-hp-offs
> +  - gdsp-offs
> +  - gdoe-offs
> +  - gdclk-offs
> +  - num-ce

These parameters should perhaps have sane defaults in the driver, and be
optional in the DT.


> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx6sl-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    epdc: epdc@20f4000 {
[...]
> +
> +        timing {
> +                clock-frequency =3D <80000000>;
> +                hactive =3D <1448>;
> +                hback-porch =3D <16>;
> +                hfront-porch =3D <102>;
> +                hsync-len =3D <28>;
> +                vactive =3D <1072>;
> +                vback-porch =3D <4>;
> +                vfront-porch =3D <4>;
> +                vsync-len =3D <2>;
> +        };
> +    };

The way you did it here, the timing parameters are directly under the
EPDC node in the DT, but I wonder if it would be better to have a
separate node for the display panel, which can then provide the timing
parameters either in the DT or in the panel driver (selected by compatible
string of the panel).


Jonathan

--4dGDc1ulGkdHTd51
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmIs8yAACgkQCDBEmo7z
X9tJ5hAAxertxtsQerEUdypFRk6tF6GZ7tpwhND2/arfmp7+/DAZ8Uq52gl8tHNN
gBfg99ukZ4HftCbcoPMLaqNi5K1nB6+igACg6tIuXj9GoTyniynnZjrcDT/0Hzhu
zEQQxUDcDIZc1JuqVmaE5nMyE+Tgpz4bHxZNqfSnYcvcqxaE1UHczrPZxblz80X0
ZDFBPYmgFKXKHbmQYaaODTpgDVMQ74EEuTSuWcPLqUEL68kQsYkrOnULAr5A5AhZ
ppFSj2C5AfAGRN3Qe08P7FEEi7SyhGC+iARK/wnLHkSNamYXgz8SVpKsgMnoZeal
QUnOft+ZkAyPIKBo3aDVpRfodPJ48s0gj1NhCarYr8cEuNsyZXfysoeRCGOiRlzp
8+B5zLoyh1FDrJVBL5ptNDlYWyqew6MnXgVnmt6rcgMKKVrJvZ8hTbMBEkV3ZsY1
vs/Se9Jyhhasow734cG2+2zt0IPhrO4m7/Zoeci2zNPmvj4ZtPYc8aTdXQx7RUl8
CLfX59/st5hWgVV2O/CY8tQUrjdLCXp0jHYYWIxoKprFh7ekNYdw/D8BqwNI5c+S
RUO5yw8Tu6Y/wVAsrHtn+E8m6mTI6B9yAxM7ltwY2/9svVsLcPO0UCLUSnDtFvlg
X5MGbxTMdPI4Kbz2rixXEIp3en0OobFSJctn9SLZWqXhXTzyQjs=
=lMbE
-----END PGP SIGNATURE-----

--4dGDc1ulGkdHTd51--