| Message ID | 20211017001204.299940-2-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/2] dt-bindings: display: bridge: lvds-codec: Document pixel data sampling edge select | expand |
Hi Marek, On Sun, Oct 17, 2021 at 02:12:04AM +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 "pclk-sample", not > the same as the one used by display timings but rather the same as used by > media, and configure bus flags based on this DT property. Late to the party here - sorry. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: devicetree@vger.kernel.org > To: dri-devel@lists.freedesktop.org > --- > V2: - Limit the pixelclk-active to encoders only > - Update DT binding document > V3: - Determine whether this is encoder from connector, i.e. > lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS > V4: - Switch to pclk-sample. Note that the value of this is inverted, > so all the existing users of pixelclk-active using previous > previous version of this patch must be reworked > V5: Rebase on recent linux-next > --- > drivers/gpu/drm/bridge/lvds-codec.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index f991842a161f..702ea803a743 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -21,6 +21,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; > @@ -119,6 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > struct device_node *bus_node; > struct drm_panel *panel; > struct lvds_codec *lvds_codec; > + u32 val; > int ret; > > lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > @@ -187,12 +189,25 @@ static int lvds_codec_probe(struct platform_device *pdev) > } > } > > + /* > + * Encoder might sample data on different clock edge than the display, > + * for example OnSemi FIN3385 has a dedicated strapping pin to select > + * the sampling edge. > + */ > + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && > + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { > + lvds_codec->timings.input_bus_flags = val ? > + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : > + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; > + } > + > /* > * 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 > * to look up. > */ > lvds_codec->bridge.of_node = dev->of_node; > + lvds_codec->bridge.timings = &lvds_codec->timings; I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info. Maybe I just misses something obvious? Sam > drm_bridge_add(&lvds_codec->bridge); > > platform_set_drvdata(pdev, lvds_codec); > -- > 2.33.0
On 10/17/21 6:49 PM, Sam Ravnborg wrote: [...] >> + /* >> + * Encoder might sample data on different clock edge than the display, >> + * for example OnSemi FIN3385 has a dedicated strapping pin to select >> + * the sampling edge. >> + */ >> + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && >> + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { >> + lvds_codec->timings.input_bus_flags = val ? >> + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : >> + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; >> + } >> + >> /* >> * 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 >> * to look up. >> */ >> lvds_codec->bridge.of_node = dev->of_node; >> + lvds_codec->bridge.timings = &lvds_codec->timings; > I do not understand how this will work. The only field that is set is timings.input_bus_flags > but any user will see bridge.timings is set and will think this is all > timing info. > > Maybe I just misses something obvious? Is there anything else in those timings that should be set ? See include/drm/drm_bridge.h around line 640 setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it is 0 or false anyway, i.e. no change.
Hi Marek, On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote: > On 10/17/21 6:49 PM, Sam Ravnborg wrote: > > [...] > > > > + /* > > > + * Encoder might sample data on different clock edge than the display, > > > + * for example OnSemi FIN3385 has a dedicated strapping pin to select > > > + * the sampling edge. > > > + */ > > > + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && > > > + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { > > > + lvds_codec->timings.input_bus_flags = val ? > > > + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : > > > + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; > > > + } > > > + > > > /* > > > * 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 > > > * to look up. > > > */ > > > lvds_codec->bridge.of_node = dev->of_node; > > > + lvds_codec->bridge.timings = &lvds_codec->timings; > > I do not understand how this will work. The only field that is set is timings.input_bus_flags > > but any user will see bridge.timings is set and will think this is all > > timing info. > > > > Maybe I just misses something obvious? > > Is there anything else in those timings that should be set ? See > include/drm/drm_bridge.h around line 640 > > setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it > is 0 or false anyway, i.e. no change. Just me being confused with display_timings. Patch looks good. Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Ping me in a few days to apply it if there is no more feedback. Sam
On 10/17/21 7:40 PM, Sam Ravnborg wrote: > Hi Marek, > > On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote: >> On 10/17/21 6:49 PM, Sam Ravnborg wrote: >> >> [...] >> >>>> + /* >>>> + * Encoder might sample data on different clock edge than the display, >>>> + * for example OnSemi FIN3385 has a dedicated strapping pin to select >>>> + * the sampling edge. >>>> + */ >>>> + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && >>>> + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { >>>> + lvds_codec->timings.input_bus_flags = val ? >>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : >>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; >>>> + } >>>> + >>>> /* >>>> * 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 >>>> * to look up. >>>> */ >>>> lvds_codec->bridge.of_node = dev->of_node; >>>> + lvds_codec->bridge.timings = &lvds_codec->timings; >>> I do not understand how this will work. The only field that is set is timings.input_bus_flags >>> but any user will see bridge.timings is set and will think this is all >>> timing info. >>> >>> Maybe I just misses something obvious? >> >> Is there anything else in those timings that should be set ? See >> include/drm/drm_bridge.h around line 640 >> >> setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it >> is 0 or false anyway, i.e. no change. > > Just me being confused with display_timings. Patch looks good. > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > Ping me in a few days to apply it if there is no more feedback. ACK
On 10/17/21 7:40 PM, Sam Ravnborg wrote: > Hi Marek, Hi, > On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote: >> On 10/17/21 6:49 PM, Sam Ravnborg wrote: >> >> [...] >> >>>> + /* >>>> + * Encoder might sample data on different clock edge than the display, >>>> + * for example OnSemi FIN3385 has a dedicated strapping pin to select >>>> + * the sampling edge. >>>> + */ >>>> + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && >>>> + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { >>>> + lvds_codec->timings.input_bus_flags = val ? >>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : >>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; >>>> + } >>>> + >>>> /* >>>> * 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 >>>> * to look up. >>>> */ >>>> lvds_codec->bridge.of_node = dev->of_node; >>>> + lvds_codec->bridge.timings = &lvds_codec->timings; >>> I do not understand how this will work. The only field that is set is timings.input_bus_flags >>> but any user will see bridge.timings is set and will think this is all >>> timing info. >>> >>> Maybe I just misses something obvious? >> >> Is there anything else in those timings that should be set ? See >> include/drm/drm_bridge.h around line 640 >> >> setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it >> is 0 or false anyway, i.e. no change. > > Just me being confused with display_timings. Patch looks good. > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > Ping me in a few days to apply it if there is no more feedback. Ping I guess ... Laurent ?
On 10/24/21 1:04 AM, Marek Vasut wrote: > On 10/17/21 7:40 PM, Sam Ravnborg wrote: >> Hi Marek, > > Hi, > >> On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote: >>> On 10/17/21 6:49 PM, Sam Ravnborg wrote: >>> >>> [...] >>> >>>>> + /* >>>>> + * Encoder might sample data on different clock edge than the >>>>> display, >>>>> + * for example OnSemi FIN3385 has a dedicated strapping pin to >>>>> select >>>>> + * the sampling edge. >>>>> + */ >>>>> + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && >>>>> + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { >>>>> + lvds_codec->timings.input_bus_flags = val ? >>>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : >>>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; >>>>> + } >>>>> + >>>>> /* >>>>> * 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 >>>>> * to look up. >>>>> */ >>>>> lvds_codec->bridge.of_node = dev->of_node; >>>>> + lvds_codec->bridge.timings = &lvds_codec->timings; >>>> I do not understand how this will work. The only field that is set >>>> is timings.input_bus_flags >>>> but any user will see bridge.timings is set and will think this is all >>>> timing info. >>>> >>>> Maybe I just misses something obvious? >>> >>> Is there anything else in those timings that should be set ? See >>> include/drm/drm_bridge.h around line 640 >>> >>> setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, >>> so it >>> is 0 or false anyway, i.e. no change. >> >> Just me being confused with display_timings. Patch looks good. >> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >> >> Ping me in a few days to apply it if there is no more feedback. > > Ping I guess ... Laurent ? Ping one more time ?
On 11/24/21 04:02, Marek Vasut wrote: > On 10/24/21 1:04 AM, Marek Vasut wrote: >> On 10/17/21 7:40 PM, Sam Ravnborg wrote: >>> Hi Marek, >> >> Hi, >> >>> On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote: >>>> On 10/17/21 6:49 PM, Sam Ravnborg wrote: >>>> >>>> [...] >>>> >>>>>> + /* >>>>>> + * Encoder might sample data on different clock edge than the >>>>>> display, >>>>>> + * for example OnSemi FIN3385 has a dedicated strapping pin >>>>>> to select >>>>>> + * the sampling edge. >>>>>> + */ >>>>>> + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && >>>>>> + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { >>>>>> + lvds_codec->timings.input_bus_flags = val ? >>>>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : >>>>>> + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; >>>>>> + } >>>>>> + >>>>>> /* >>>>>> * 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 >>>>>> * to look up. >>>>>> */ >>>>>> lvds_codec->bridge.of_node = dev->of_node; >>>>>> + lvds_codec->bridge.timings = &lvds_codec->timings; >>>>> I do not understand how this will work. The only field that is set >>>>> is timings.input_bus_flags >>>>> but any user will see bridge.timings is set and will think this is all >>>>> timing info. >>>>> >>>>> Maybe I just misses something obvious? >>>> >>>> Is there anything else in those timings that should be set ? See >>>> include/drm/drm_bridge.h around line 640 >>>> >>>> setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, >>>> so it >>>> is 0 or false anyway, i.e. no change. >>> >>> Just me being confused with display_timings. Patch looks good. >>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>> >>> Ping me in a few days to apply it if there is no more feedback. >> >> Ping I guess ... Laurent ? > > Ping one more time ? Ping yet again ?
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index f991842a161f..702ea803a743 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -21,6 +21,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; @@ -119,6 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) struct device_node *bus_node; struct drm_panel *panel; struct lvds_codec *lvds_codec; + u32 val; int ret; lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); @@ -187,12 +189,25 @@ static int lvds_codec_probe(struct platform_device *pdev) } } + /* + * Encoder might sample data on different clock edge than the display, + * for example OnSemi FIN3385 has a dedicated strapping pin to select + * the sampling edge. + */ + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { + lvds_codec->timings.input_bus_flags = val ? + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; + } + /* * 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 * to look up. */ lvds_codec->bridge.of_node = dev->of_node; + 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 "pclk-sample", not the same as the one used by display timings but rather the same as used by media, and configure bus flags based on this DT property. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: devicetree@vger.kernel.org To: dri-devel@lists.freedesktop.org --- V2: - Limit the pixelclk-active to encoders only - Update DT binding document V3: - Determine whether this is encoder from connector, i.e. lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS V4: - Switch to pclk-sample. Note that the value of this is inverted, so all the existing users of pixelclk-active using previous previous version of this patch must be reworked V5: Rebase on recent linux-next --- drivers/gpu/drm/bridge/lvds-codec.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)