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

[Conor Dooley]

On Fri, Jan 26, 2024 at 02:22:42PM +0000, Dharma.B@xxxxxxxxxxxxx wrote:
> On 25/01/24 1:57 pm, Conor Dooley - M52691 wrote:
> > 
> >>> If the lvds pll is an input to the hlcdc, you need to add it here.
> >>>   From your description earlier it does sound like it is an input to
> >>> the hlcdc, but now you are claiming that it is not.
> >>
> >> The LVDS PLL serves as an input to both the LCDC and LVDSC
> > 
> > Then it should be an input to both the LCDC and LVDSC in the devicetree.
> 
> For the LVDSC to operate, the presence of the LVDS PLL is crucial. However, in the case of the LCDC, LVDS PLL is not essential for its operation unless LVDS interface is used and when it is used lvds driver will take care of preparing and enabling the LVDS PLL.

Please fix your line wrapping, not sure what's going on here, but these
lines are super long.

> Consequently, it seems that there might not be any significant actions we can take within the LCD driver regarding the LVDS PLL.

You should be getting a reference to the clock and calling enable on it
etc, even if the LVDSC is also doing so. That will allow the clock
framework to correctly track users.

> If there are no intentions to utilize it within the driver, is it necessary to explicitly designate it as an input in the device tree?

The binding describes the hardware, so yes it should be there. What the
driver implementation does with the clock is not relevant. That said, I
think the driver should actually be using it, as I wrote above.

> 
> If yes, I will update the bindings with optional LVDS PLL clock.
> 
> clock-names:
>   items:
>     - const: periph_clk
>     - const: sys_clk
>     - const: slow_clk
>     - const: lvds_pll  # Optional clock

This looks correct, but the comment is not needed. Setting minItems: 3
does this for you.

> >> with the
> >> LVDS_PLL multiplied by 7 for the Pixel clock to the LVDS PHY, and
> > 
> > Are you sure? The diagram doesn't show a multiplier, the 7x comment
> > there seems to be showing relations?
> 
> Sorry, 
> LVDS PLL = (PCK * 7) goes to LVDSC PHY
> PCK = (LVDS PLL / 7) goes to LCDC

I'll take your word for it :)

> >> LVDS_PLL divided by 7 for the Pixel clock to the LCDC.
> > 
> >> I am inclined to believe that appropriately configuring and enabling it
> >> in the LVDS driver would be the appropriate course of action.
> > 
> > We're talking about bindings here, not drivers, but I would imagine that
> > if two peripherals are using the same clock then both of them should be
> > getting a reference to and enabling that clock so that the clock
> > framework can correctly track the users.
> > 
> >>> I don't know your hardware, so I have no idea which of the two is
> >>> correct, but it sounds like the former. Without digging into how this
> >>> works my assumption about the hardware here looks like is that the lvds
> >>> controller is a clock provider,
> >>
> >> It's a PLL clock from PMC.
> >>
> >>> and that the lvds controller's clock is
> >>> an optional input for the hlcdc.
> >>
> >> Again it's a PLL clock from PMC.
> >>
> >> Please refer Section 39.3
> >> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAM9X7-Series-Data-Sheet-DS60001813.pdf
> > 
> > It is not the same exact clock as you pointed out above though, so the
> > by 7 divider should be modelled.
> 
> Modelled in mfd binding? If possible, could you please provide an example for better clarity? Thank you.

Whatever node corresponds to the register range controlling this PLL
should be a "clock-controller" (like any other clock provider does).
Your PMC should have this property. I don't know if the correct location
is the mfd node or somewhere else, you'll have to check your docs.

Thanks,
Conor.

Attachment: signature.asc
Description: PGP signature