Re: [PATCH v3 3/3] dt-bindings: mfd: atmel,hlcdc: Convert to DT schema format

[Conor Dooley]

On Fri, Jan 19, 2024 at 03:32:49AM +0000, Dharma.B@xxxxxxxxxxxxx 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 expects
> >> 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 for
> >> systems using the HLCDC IP.
> >>
> >> Signed-off-by: Dharma Balasubiramani<dharma.b@xxxxxxxxxxxxx>
> >> ---
> >> 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 driver.
> >> - 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.yaml
> >>   delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt
> >>
> >> 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@xxxxxxxxxxxxx>
> >> +  - Alexandre Belloni<alexandre.belloni@xxxxxxxxxxx>
> >> +  - Claudiu Beznea<claudiu.beznea@xxxxxxxxx>
> >> +
> >> +description:
> >> +  The Atmel HLCDC (HLCD Controller) IP available on Atmel SoCs exposes 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?
> 
>    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.

Attachment: signature.asc
Description: PGP signature