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