| Message ID | 20201002230823.242147-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/bridge: lvds-codec: Add support for pixel data sampling edge select | expand |
Hi Marek. On Sat, Oct 03, 2020 at 01:08:23AM +0200, Marek Vasut wrote: > The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to > select input pixel data sampling edge. Add DT property "pixelclk-active", > same as the one used by display timings, and configure bus flags based on > this DT property. Why is it that this information cannot come from the panel driver? The property tell when data are sampled and the FIN3385 Parallel-to-LVDS is the one that transmit the data - not then one that samples the data. Correct? Sam > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Antonio Borneo <antonio.borneo@st.com> > Cc: Benjamin Gaignard <benjamin.gaignard@st.com> > Cc: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Philippe Cornu <philippe.cornu@st.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Vincent Abriou <vincent.abriou@st.com> > Cc: Yannick Fertre <yannick.fertre@st.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > To: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/bridge/lvds-codec.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index f52ccffc1bd1..bc941d4fb5b9 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -19,6 +19,7 @@ struct lvds_codec { > struct device *dev; > struct drm_bridge bridge; > struct drm_bridge *panel_bridge; > + struct drm_bridge_timings timings; > struct regulator *vcc; > struct gpio_desc *powerdown_gpio; > u32 connector_type; > @@ -80,6 +81,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > struct device_node *panel_node; > struct drm_panel *panel; > struct lvds_codec *lvds_codec; > + u32 val; > int ret; > > lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > @@ -124,6 +126,12 @@ static int lvds_codec_probe(struct platform_device *pdev) > if (IS_ERR(lvds_codec->panel_bridge)) > return PTR_ERR(lvds_codec->panel_bridge); > > + if (!of_property_read_u32(dev->of_node, "pixelclk-active", &val)) { > + lvds_codec->timings.input_bus_flags = val ? > + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE : > + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE; > + } > + > /* > * The panel_bridge bridge is attached to the panel's of_node, > * but we need a bridge attached to our of_node for our user > @@ -131,6 +139,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > */ > lvds_codec->bridge.of_node = dev->of_node; > lvds_codec->bridge.funcs = &funcs; > + lvds_codec->bridge.timings = &lvds_codec->timings; > drm_bridge_add(&lvds_codec->bridge); > > platform_set_drvdata(pdev, lvds_codec); > -- > 2.28.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 10/16/20 6:25 PM, Sam Ravnborg wrote: > Hi Marek. Hello Sam, > On Sat, Oct 03, 2020 at 01:08:23AM +0200, Marek Vasut wrote: >> The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to >> select input pixel data sampling edge. Add DT property "pixelclk-active", >> same as the one used by display timings, and configure bus flags based on >> this DT property. > > Why is it that this information cannot come from the panel driver? Because the sampling edge of the panel can be different (is there even such an edge for LVDS panel?). What we care about specifying here is the edge on which the FIN3385 samples the data on the Parallel bus, and that is a property of the FIN3385, because that is configured via a dedicated pin on the FIN3385. > The property tell when data are sampled and the FIN3385 Parallel-to-LVDS > is the one that transmit the data - not then one that samples the data. > Correct? Not correct I'm afraid, see above.
Hi Marek, Thank you for the patch. On Sat, Oct 03, 2020 at 01:08:23AM +0200, Marek Vasut wrote: > The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to > select input pixel data sampling edge. Add DT property "pixelclk-active", > same as the one used by display timings, and configure bus flags based on > this DT property. The feature looks good to me. I however wonder if we shouldn't use the standard pclk-sample endpoint property (documented in [1]) instead of a custom properly. The DT bindings for the lvds-codec should be updated accordingly. And the property should only be taken into account when operating in encoder mode, as for decoder mode there's no polarity for the sampling of LVDS signals, as you've explained in a reply to Sam. [1] Documentation/devicetree/bindings/media/video-interfaces.txt > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Antonio Borneo <antonio.borneo@st.com> > Cc: Benjamin Gaignard <benjamin.gaignard@st.com> > Cc: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Philippe Cornu <philippe.cornu@st.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Vincent Abriou <vincent.abriou@st.com> > Cc: Yannick Fertre <yannick.fertre@st.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > To: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/bridge/lvds-codec.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index f52ccffc1bd1..bc941d4fb5b9 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -19,6 +19,7 @@ struct lvds_codec { > struct device *dev; > struct drm_bridge bridge; > struct drm_bridge *panel_bridge; > + struct drm_bridge_timings timings; > struct regulator *vcc; > struct gpio_desc *powerdown_gpio; > u32 connector_type; > @@ -80,6 +81,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > struct device_node *panel_node; > struct drm_panel *panel; > struct lvds_codec *lvds_codec; > + u32 val; > int ret; > > lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > @@ -124,6 +126,12 @@ static int lvds_codec_probe(struct platform_device *pdev) > if (IS_ERR(lvds_codec->panel_bridge)) > return PTR_ERR(lvds_codec->panel_bridge); > > + if (!of_property_read_u32(dev->of_node, "pixelclk-active", &val)) { > + lvds_codec->timings.input_bus_flags = val ? > + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE : > + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE; > + } > + > /* > * The panel_bridge bridge is attached to the panel's of_node, > * but we need a bridge attached to our of_node for our user > @@ -131,6 +139,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > */ > lvds_codec->bridge.of_node = dev->of_node; > lvds_codec->bridge.funcs = &funcs; > + lvds_codec->bridge.timings = &lvds_codec->timings; > drm_bridge_add(&lvds_codec->bridge); > > platform_set_drvdata(pdev, lvds_codec);
On 10/19/20 2:54 AM, Laurent Pinchart wrote: > Hi Marek, Hi, > Thank you for the patch. > > On Sat, Oct 03, 2020 at 01:08:23AM +0200, Marek Vasut wrote: >> The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to >> select input pixel data sampling edge. Add DT property "pixelclk-active", >> same as the one used by display timings, and configure bus flags based on >> this DT property. > > The feature looks good to me. I however wonder if we shouldn't use the > standard pclk-sample endpoint property (documented in [1]) instead of a > custom properly. Either is fine by me, but I think pixelclk-active, which comes from panel-timings.yaml is closer to the video than multimedia bindings. (the multimedia and video bindings should be aligned, sigh) > The DT bindings for the lvds-codec should be updated accordingly. And > the property should only be taken into account when operating in encoder > mode, as for decoder mode there's no polarity for the sampling of LVDS > signals, as you've explained in a reply to Sam. So, we need to rework the whole compatible data and identify what is encoder/decoder in addition to connector there.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index f52ccffc1bd1..bc941d4fb5b9 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -19,6 +19,7 @@ struct lvds_codec { struct device *dev; struct drm_bridge bridge; struct drm_bridge *panel_bridge; + struct drm_bridge_timings timings; struct regulator *vcc; struct gpio_desc *powerdown_gpio; u32 connector_type; @@ -80,6 +81,7 @@ static int lvds_codec_probe(struct platform_device *pdev) struct device_node *panel_node; struct drm_panel *panel; struct lvds_codec *lvds_codec; + u32 val; int ret; lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); @@ -124,6 +126,12 @@ static int lvds_codec_probe(struct platform_device *pdev) if (IS_ERR(lvds_codec->panel_bridge)) return PTR_ERR(lvds_codec->panel_bridge); + if (!of_property_read_u32(dev->of_node, "pixelclk-active", &val)) { + lvds_codec->timings.input_bus_flags = val ? + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE : + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE; + } + /* * The panel_bridge bridge is attached to the panel's of_node, * but we need a bridge attached to our of_node for our user @@ -131,6 +139,7 @@ static int lvds_codec_probe(struct platform_device *pdev) */ lvds_codec->bridge.of_node = dev->of_node; lvds_codec->bridge.funcs = &funcs; + lvds_codec->bridge.timings = &lvds_codec->timings; drm_bridge_add(&lvds_codec->bridge); platform_set_drvdata(pdev, lvds_codec);
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to select input pixel data sampling edge. Add DT property "pixelclk-active", same as the one used by display timings, and configure bus flags based on this DT property. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexandre Torgue <alexandre.torgue@st.com> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Antonio Borneo <antonio.borneo@st.com> Cc: Benjamin Gaignard <benjamin.gaignard@st.com> Cc: Biju Das <biju.das.jz@bp.renesas.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Philippe Cornu <philippe.cornu@st.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Vincent Abriou <vincent.abriou@st.com> Cc: Yannick Fertre <yannick.fertre@st.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/bridge/lvds-codec.c | 9 +++++++++ 1 file changed, 9 insertions(+)