Re: [RFC PATCH 0/7] add at91sam9 LCDC DRM driver

[Sam Ravnborg <sam@ravnborg.org>]

Hi Rob.

Thanks for the feedback, as I am rather new to this DT stuff
a few iterations are no suprise.

> > If I understand the proposal from you correct an example binding would
> > look like this:
> > 
> >        lcdc0: lcdc@700000 {
> >                 compatible = "atmel,at91sam9263-lcdc";
> >                 reg = <0x700000 0x1000>;
> >                 interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>;
> >                 clocks = <&lcd_clk>, <&lcd_clk>;
> >                 clock-names = "lcdc_clk", "hclk";
> > 
> >                 lcd-supply = <&lcdc_reg>;
> > 
> >                 port@0 {
> >                 };
> > 
> >                 #pwm-cells = <3>;
> >         };
> > 
> >         backlight: backlight {
> >                 compatible = "pwm-backlight";
> >                 pwms = <&lcd0 0 50000 0>;
> >         };
> > 
> > 
> > This is doable, but IMO it is less obvious that the LCDC IP core implements
> > two different features - a PWM and a LCD controller.
> 
> Features don't equate to nodes. If the sub-blocks can be separately 
> instantiated or have their own resources then sub-nodes make sense. 
> Otherwise, it's a single device (node) with multiple providers.
> 
> > Right now the preference is to stay with the v1 approach:
> > - It is a mirror of what we do today for hlcdc, so no suprises
> 
> Which BTW doesn't appear to have actually been reviewed.
> 
> Do these IP blocks actually share anything? 
They have registers in the same memory area - and I think that it.

 
> Consistency is nice, but keeping compatibility with the existing binding 
> by extending things in a backwards compatible way is more important. 
> Implementing a new driver is not license to change the binding. Shall I 
> let u-boot or *BSD developers change the binding too for their driver?

So in other words - the existing binding in atmel,lcdc.txt needs to be extended in
a backward compatible way.
I will take a look at that and post a proposal in v2.


> > - It shows in a nice way that the LCDC IP core implments both an LCD controller and a PWM
> > - The pwm functionality is not hiddin inside the lcdc stuff, and it is thus
> >   simpler to add good pin-ctrl handles with nice names that matches the usage.
> >   (I could always add more pin-ctrl, but it is common to refer to a single pin-ctrl.
> 
> Does anyone actually use this for a non-backlight PWM? I'm not sure the 
> abstraction is worth it. The driver could just register itself as a 
> backlight provider rather than a PWM provider.
Makes sense.

> 
> > One DT related Q:
> > The LCD Controller supports BGR565, but as this is less common some HW implmentations
> > exchange R and B, expessed in the old binding as wiring-mode like this:
> > 
> > 	atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG"
> > 
> > How can we express this wiring-mode in a generic way, both in DT and in code?
> > Is it something that in DRM belongs to the panel, the encoder, the connector, or?
> > And can any of the exisitng flags be used?
> 
> I thought we had come up with a common definition, but I guess it didn't 
> make it upstream. It's definitely needed and I've been rejecting 
> anything new that's vendor specific.
I found this: https://patchwork.ozlabs.org/patch/659570/
The suggestion with a boolean seems simple and I will try that.

	Sam