mbox series

[0/4] drm/sun4i: Support clock and data polarities on LVDS output

Message ID cover.b12a054012ce067fa2094894147f953ab816d8d0.1581682983.git-series.maxime@cerno.tech
Headers show
Series drm/sun4i: Support clock and data polarities on LVDS output | expand

Message

Maxime Ripard Feb. 14, 2020, 12:24 p.m. UTC 
Hi,

The Allwinner LVDS encoder allows to change the polarity of clocks and
data lanes, so let's add the needed bus flags to DRM, panel-lvds and
our encoder driver.

Let me know what you think,
Maxime

Maxime Ripard (4):
  drm/connector: Add data polarity flags
  dt-bindings: panel: lvds: Add properties for clock and data polarities
  drm/panel: lvds: Support data and clock polarity flags
  drm/sun4i: lvds: Support data and clock polarity flags

 Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 +++-
 drivers/gpu/drm/panel/panel-lvds.c                        | 25 +++++++-
 drivers/gpu/drm/sun4i/sun4i_tcon.c                        | 16 ++++-
 include/drm/drm_connector.h                               |  4 +-
 4 files changed, 49 insertions(+), 6 deletions(-)

base-commit: bb6d3fb354c5ee8d6bde2d576eb7220ea09862b9

Comments

Sam Ravnborg Feb. 14, 2020, 4:13 p.m. UTC | #1 
Hi Maxime.

On Fri, Feb 14, 2020 at 01:24:38PM +0100, Maxime Ripard wrote:
> Some LVDS encoders can change the polarity of the data signals being
> sent. Add a DRM bus flag to reflect this.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  include/drm/drm_connector.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 221910948b37..9a08fe6ab7c2 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -330,6 +330,8 @@ enum drm_panel_orientation {
>   *					edge of the pixel clock
>   * @DRM_BUS_FLAG_SHARP_SIGNALS:		Set if the Sharp-specific signals
>   *					(SPL, CLS, PS, REV) must be used
> + * @DRM_BUS_FLAG_DATA_LOW:		The Data signals are active low
> + * @DRM_BUS_FLAG_DATA_HIGH:		The Data signals are active high
Reading the description of these falgs always confuses me.
In this case if neither bit 9 nor bit 10 is set then the data signals
are netiher active low nor active high.
So what can I then expect?

We have the same logic loophole for DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE
and DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE.
So it is not new, but can we do better here?

	Sam


>   */
>  enum drm_bus_flags {
>  	DRM_BUS_FLAG_DE_LOW = BIT(0),
> @@ -349,6 +351,8 @@ enum drm_bus_flags {
>  	DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE = DRM_BUS_FLAG_SYNC_NEGEDGE,
>  	DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE = DRM_BUS_FLAG_SYNC_POSEDGE,
>  	DRM_BUS_FLAG_SHARP_SIGNALS = BIT(8),
> +	DRM_BUS_FLAG_DATA_LOW = BIT(9),
> +	DRM_BUS_FLAG_DATA_HIGH = BIT(10),
>  };
>  
>  /**
> -- 
> git-series 0.9.1
Maxime Ripard Feb. 20, 2020, 6 p.m. UTC | #2 
On Fri, Feb 14, 2020 at 05:13:59PM +0100, Sam Ravnborg wrote:
> Hi Maxime.
>
> On Fri, Feb 14, 2020 at 01:24:38PM +0100, Maxime Ripard wrote:
> > Some LVDS encoders can change the polarity of the data signals being
> > sent. Add a DRM bus flag to reflect this.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  include/drm/drm_connector.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 221910948b37..9a08fe6ab7c2 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -330,6 +330,8 @@ enum drm_panel_orientation {
> >   *					edge of the pixel clock
> >   * @DRM_BUS_FLAG_SHARP_SIGNALS:		Set if the Sharp-specific signals
> >   *					(SPL, CLS, PS, REV) must be used
> > + * @DRM_BUS_FLAG_DATA_LOW:		The Data signals are active low
> > + * @DRM_BUS_FLAG_DATA_HIGH:		The Data signals are active high
> Reading the description of these falgs always confuses me.
> In this case if neither bit 9 nor bit 10 is set then the data signals
> are netiher active low nor active high.
> So what can I then expect?
>
> We have the same logic loophole for DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE
> and DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE.
> So it is not new, but can we do better here?

Honestly, I don't really get it either. I *think* this is to handle
the sampling / output inversion properly which wouldn't be possible if
this was only a bit set or not.

Maxime