| Message ID | 620a740cec4186177ce346b092d4ba451e1420dc.1581682983.git-series.maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: Support clock and data polarities on LVDS output | expand |
Hi Maxime. On Fri, Feb 14, 2020 at 01:24:39PM +0100, Maxime Ripard wrote: > Some LVDS encoders can support multiple polarities on the data and > clock lanes, and similarly some panels require a given polarity on > their inputs. Add a property on the panel to configure the encoder > properly. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Not a fan of this binding... In display-timing.txt we have a specification/description of the panel-timing node. The panel-timing node already include information such as: - hsync-active: - vsync-active: - de-active: - pixelclk-active: - syncclk-active: But clock-active-low and data-active-low refer to the bus more than an individual timing. So maybe OK not to have it in a panel-timing node. But then it would IMO be better to include this in the display-timing node - so we make this available and standard for all users of the display-timing node. I will dig up my patchset to make proper bindings for panel-timing and display-timing this weeked and resend them. Then we can discuss if this goes on top or this is specific for the lvds binding. > --- > Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 ++++++++- > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml > index d0083301acbe..4a1111a1ab38 100644 > --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml > +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml > @@ -90,6 +90,16 @@ properties: > CTL2: Data Enable > CTL3: 0 > > + clock-active-low: > + type: boolean > + description: > Should this be "|" and not ">"? Did this pass dt_binding_check? > + If set, reverse the clock polarity on the clock lane. This text could be a bit more specific. If this is set then what? And it seems strange that a clock is active low. For a clock we often talk about raising or falling edge. > + > + data-active-low: > + type: boolean > + description: > Same comment with ">" > + If set, reverse the bit polarity on all data lanes. Same comment about a more explicit description. Sam > data-mirror: > type: boolean > description: > -- > git-series 0.9.1
On Fri, Feb 14, 2020 at 01:24:39PM +0100, Maxime Ripard wrote: > Some LVDS encoders can support multiple polarities on the data and > clock lanes, and similarly some panels require a given polarity on > their inputs. Add a property on the panel to configure the encoder > properly. If the panel requires a specific setting, then that can be implied by the panel's compatible. Does Boris' format constraint solving series help here? Rob
On Fri, Feb 14, 2020 at 05:11:56PM +0100, Sam Ravnborg wrote: > Hi Maxime. > > On Fri, Feb 14, 2020 at 01:24:39PM +0100, Maxime Ripard wrote: > > Some LVDS encoders can support multiple polarities on the data and > > clock lanes, and similarly some panels require a given polarity on > > their inputs. Add a property on the panel to configure the encoder > > properly. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Not a fan of this binding... > In display-timing.txt we have a specification/description of > the panel-timing node. > > The panel-timing node already include information such as: > - hsync-active: > - vsync-active: > - de-active: > - pixelclk-active: > - syncclk-active: > > But clock-active-low and data-active-low refer to the bus > more than an individual timing. > So maybe OK not to have it in a panel-timing node. > But then it would IMO be better to include > this in the display-timing node - so we make > this available and standard for all users of the > display-timing node. > > I will dig up my patchset to make proper bindings for panel-timing and > display-timing this weeked and resend them. > Then we can discuss if this goes on top or this is specific for the > lvds binding. That makes sense, I'll wait for them to be merged then :) > > > --- > > Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 ++++++++- > > 1 file changed, 10 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml > > index d0083301acbe..4a1111a1ab38 100644 > > --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml > > +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml > > @@ -90,6 +90,16 @@ properties: > > CTL2: Data Enable > > CTL3: 0 > > > > + clock-active-low: > > + type: boolean > > + description: > > > Should this be "|" and not ">"? > Did this pass dt_binding_check? Yes. > means that this is a multi-line string that will drop the \n between each line, while | will keep it For a string like this, I believe it makes more sense to let whatever is using to handle the wrapping, but I don't really have a strong opinion :) > > > + If set, reverse the clock polarity on the clock lane. > This text could be a bit more specific. If this is set then > what? > And it seems strange that a clock is active low. > For a clock we often talk about raising or falling edge. > > > + > > + data-active-low: > > + type: boolean > > + description: > > Same comment with ">" > > > + If set, reverse the bit polarity on all data lanes. > Same comment about a more explicit description. I'll try to come up with something better. Thanks! Maxime
diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml index d0083301acbe..4a1111a1ab38 100644 --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml @@ -90,6 +90,16 @@ properties: CTL2: Data Enable CTL3: 0 + clock-active-low: + type: boolean + description: > + If set, reverse the clock polarity on the clock lane. + + data-active-low: + type: boolean + description: > + If set, reverse the bit polarity on all data lanes. + data-mirror: type: boolean description:
Some LVDS encoders can support multiple polarities on the data and clock lanes, and similarly some panels require a given polarity on their inputs. Add a property on the panel to configure the encoder properly. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 ++++++++- 1 file changed, 10 insertions(+)