Received-SPF: pass (google.com: domain of linux-kernel+bounces-31124-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 19 Jan 2024 12:03:43 +0000
From: Conor Dooley <conor.dooley@microchip.com>
To: <Dharma.B@microchip.com>
CC: <conor@kernel.org>, <sam@ravnborg.org>, <bbrezillon@kernel.org>,
	<maarten.lankhorst@linux.intel.com>, <mripard@kernel.org>,
	<tzimmermann@suse.de>, <airlied@gmail.com>, <daniel@ffwll.ch>,
	<robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
	<conor+dt@kernel.org>, <Nicolas.Ferre@microchip.com>,
	<alexandre.belloni@bootlin.com>, <claudiu.beznea@tuxon.dev>,
	<dri-devel@lists.freedesktop.org>, <devicetree@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<lee@kernel.org>, <thierry.reding@gmail.com>,
	<u.kleine-koenig@pengutronix.de>, <linux-pwm@vger.kernel.org>,
	<Linux4Microchip@microchip.com>
Subject: Re: [PATCH v3 3/3] dt-bindings: mfd: atmel,hlcdc: Convert to DT
 schema format
Message-ID: <20240119-character-mardi-43571d7fe7d5@wendy>
References: <20240118092612.117491-1-dharma.b@microchip.com>
 <20240118092612.117491-4-dharma.b@microchip.com>
 <20240118-recent-glorified-fd35d72e006e@spud>
 <c33868c8-dc42-4800-885c-5e5f24c2044e@microchip.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="a0WqJfirCmr9EVhF"
Content-Disposition: inline
In-Reply-To: <c33868c8-dc42-4800-885c-5e5f24c2044e@microchip.com>

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

On Fri, Jan 19, 2024 at 03:32:49AM +0000, Dharma.B@microchip.com wrote:
> On 18/01/24 9:10 pm, Conor Dooley wrote:
> > On Thu, Jan 18, 2024 at 02:56:12PM +0530, Dharma Balasubiramani wrote:
> >> Convert the atmel,hlcdc binding to DT schema format.
> >>
> >> Adjust the clock-names property to clarify that the LCD controller exp=
ects
> >> one of these clocks (either sys_clk or lvds_pll_clk to be present but =
not
> >> both) along with the slow_clk and periph_clk. This alignment with the =
actual
> >> hardware requirements will enable accurate device tree configuration f=
or
> >> systems using the HLCDC IP.
> >>
> >> Signed-off-by: Dharma Balasubiramani<dharma.b@microchip.com>
> >> ---
> >> changelog
> >> v2 -> v3
> >> - Rename hlcdc-display-controller and hlcdc-pwm to generic names.
> >> - Modify the description by removing the unwanted comments and '|'.
> >> - Modify clock-names simpler.
> >> v1 -> v2
> >> - Remove the explicit copyrights.
> >> - Modify title (not include words like binding/driver).
> >> - Modify description actually describing the hardware and not the driv=
er.
> >> - Add details of lvds_pll addition in commit message.
> >> - Ref endpoint and not endpoint-base.
> >> - Fix coding style.
> >> ...
> >>   .../devicetree/bindings/mfd/atmel,hlcdc.yaml  | 97 +++++++++++++++++=
++
> >>   .../devicetree/bindings/mfd/atmel-hlcdc.txt   | 56 -----------
> >>   2 files changed, 97 insertions(+), 56 deletions(-)
> >>   create mode 100644 Documentation/devicetree/bindings/mfd/atmel,hlcdc=
=2Eyaml
> >>   delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-hlcdc=
=2Etxt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml b/=
Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml
> >> new file mode 100644
> >> index 000000000000..eccc998ac42c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml
> >> @@ -0,0 +1,97 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id:http://devicetree.org/schemas/mfd/atmel,hlcdc.yaml#
> >> +$schema:http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Atmel's HLCD Controller
> >> +
> >> +maintainers:
> >> +  - Nicolas Ferre<nicolas.ferre@microchip.com>
> >> +  - Alexandre Belloni<alexandre.belloni@bootlin.com>
> >> +  - Claudiu Beznea<claudiu.beznea@tuxon.dev>
> >> +
> >> +description:
> >> +  The Atmel HLCDC (HLCD Controller) IP available on Atmel SoCs expose=
s two
> >> +  subdevices, a PWM chip and a Display Controller.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - atmel,at91sam9n12-hlcdc
> >> +      - atmel,at91sam9x5-hlcdc
> >> +      - atmel,sama5d2-hlcdc
> >> +      - atmel,sama5d3-hlcdc
> >> +      - atmel,sama5d4-hlcdc
> >> +      - microchip,sam9x60-hlcdc
> >> +      - microchip,sam9x75-xlcdc
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> >> +  clocks:
> >> +    maxItems: 3
> > Hmm, one thing I probably should have said on the previous version, but
> > I missed somehow: It would be good to add an items list to the clocks
> > property here to explain what the 3 clocks are/are used for - especially
> > since there is additional complexity being added here to use either the
> > sys or lvds clocks.
> May I inquire if this approach is likely to be effective?
>=20
>    clocks:
>      items:
>        - description: peripheral clock
>        - description: generic clock or lvds pll clock
>            Once the LVDS PLL is enabled, the pixel clock is used as the
>            clock for LCDC, so its GCLK is no longer needed.
>        - description: slow clock
>      maxItems: 3

Hmm that sounds very suspect to me. "Once the lvdspll is enabled the
generic clock is no longer needed" sounds like both clocks can be provided
to the IP on different pins and their provision is not mutually
exclusive, just that the IP will only actually use one at a time. If
that is the case, then this patch is nott correct and the binding should
allow for 4 clocks, with both the generic clock and the lvds pll being
present in the DT at the same time.

I vaguely recall internal discussion about this problem some time back
but the details all escape me.

Thanks,
Conor.

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

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

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZaplHwAKCRB4tDGHoIJi
0lj2AP9K84iq37Rz1ulvFhMipsNqhqMxklWv5GvyAq1ZD/lqSwEAq0vY9saiWwW2
ah0UOybo+9cGrmPqOmpiVe3FFc6voAU=
=dla+
-----END PGP SIGNATURE-----

--a0WqJfirCmr9EVhF--