| Message ID | 20211009224152.427219-2-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select | expand |
Hi Marek, On Sun, Oct 10, 2021 at 12:41:52AM +0200, Marek Vasut wrote: > Decoder input LVDS format is a property of the decoder chip or even > its strapping. Handle data-mapping the same way lvds-panel does. In > case data-mapping is not present, do nothing, since there are still > legacy bindings which do not specify this property. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > To: dri-devel@lists.freedesktop.org I am late to the game here - sorry. A few details in the following. With these considered: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Sam > --- > V2: - Move the data-mapping to endpoint > V3: - Rebase on V2 submitted a while ago, reinstate changelog > - Use .atomic_get_input_bus_fmts for the decoder, separate funcs for encoder > V4: - No change > V5: - Move the data-mapping property to port@0 , decoder LVDS input > - Add RB from Laurent > --- > drivers/gpu/drm/bridge/lvds-codec.c | 76 ++++++++++++++++++++++++++++- > 1 file changed, 75 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index dcf579a4cf83..08091bab4857 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -12,6 +12,7 @@ > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > > +#include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > #include <drm/drm_panel.h> > > @@ -22,6 +23,7 @@ struct lvds_codec { > struct regulator *vcc; > struct gpio_desc *powerdown_gpio; > u32 connector_type; > + unsigned int bus_format; > }; > > static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge) > @@ -74,12 +76,50 @@ static const struct drm_bridge_funcs funcs = { > .disable = lvds_codec_disable, > }; > > +#define MAX_INPUT_SEL_FORMATS 1 > +static u32 * > +lvds_codec_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + struct lvds_codec *lvds_codec = to_lvds_codec(bridge); > + u32 *input_fmts; > + > + *num_input_fmts = 0; > + > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > + GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + input_fmts[0] = lvds_codec->bus_format; > + *num_input_fmts = MAX_INPUT_SEL_FORMATS; > + > + return input_fmts; > +} > + > +static const struct drm_bridge_funcs funcs_decoder = { > + .attach = lvds_codec_attach, > + .enable = lvds_codec_enable, > + .disable = lvds_codec_disable, > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_reset = drm_atomic_helper_bridge_reset, > + .atomic_get_input_bus_fmts = lvds_codec_atomic_get_input_bus_fmts, > +}; > + > static int lvds_codec_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct device_node *panel_node; > + struct device_node *bus_node; > struct drm_panel *panel; > struct lvds_codec *lvds_codec; > + const char *mapping; > + int ret; > > lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > if (!lvds_codec) > @@ -119,13 +159,47 @@ static int lvds_codec_probe(struct platform_device *pdev) > if (IS_ERR(lvds_codec->panel_bridge)) > return PTR_ERR(lvds_codec->panel_bridge); > > + lvds_codec->bridge.funcs = &funcs; > + > + /* > + * Decoder input LVDS format is a property of the decoder chip or even > + * its strapping. Handle data-mapping the same way lvds-panel does. In > + * case data-mapping is not present, do nothing, since there are still > + * legacy bindings which do not specify this property. The missing data-mapping property is reported as an error, but this comments says it is OK. Info? > + */ > + if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) { > + bus_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); Are there any reg specified in the binding? If not then the second parameter should be -1 here. > + if (!bus_node) { > + dev_dbg(dev, "bus DT node not found\n"); > + return -ENXIO; > + } > + > + ret = of_property_read_string(bus_node, "data-mapping", > + &mapping); > + of_node_put(bus_node); > + if (ret < 0) { > + dev_err(dev, "missing 'data-mapping' DT property\n"); > + } else { It would be nice with a helper for the below code if we need this a third time. > + if (!strcmp(mapping, "jeida-18")) { > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG; > + } else if (!strcmp(mapping, "jeida-24")) { > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA; > + } else if (!strcmp(mapping, "vesa-24")) { > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG; > + } else { > + dev_err(dev, "invalid 'data-mapping' DT property\n"); > + return -EINVAL; > + } Add empty line here. > + lvds_codec->bridge.funcs = &funcs_decoder; > + } > + } > + > /* > * 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.funcs = &funcs; > drm_bridge_add(&lvds_codec->bridge); > > platform_set_drvdata(pdev, lvds_codec); > -- > 2.33.0
Hi Sam, On Sun, Oct 10, 2021 at 10:16:22AM +0200, Sam Ravnborg wrote: > On Sun, Oct 10, 2021 at 12:41:52AM +0200, Marek Vasut wrote: > > Decoder input LVDS format is a property of the decoder chip or even > > its strapping. Handle data-mapping the same way lvds-panel does. In > > case data-mapping is not present, do nothing, since there are still > > legacy bindings which do not specify this property. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > To: dri-devel@lists.freedesktop.org > > I am late to the game here - sorry. A few details in the following. > With these considered: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > > --- > > V2: - Move the data-mapping to endpoint > > V3: - Rebase on V2 submitted a while ago, reinstate changelog > > - Use .atomic_get_input_bus_fmts for the decoder, separate funcs for encoder > > V4: - No change > > V5: - Move the data-mapping property to port@0 , decoder LVDS input > > - Add RB from Laurent > > --- > > drivers/gpu/drm/bridge/lvds-codec.c | 76 ++++++++++++++++++++++++++++- > > 1 file changed, 75 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > index dcf579a4cf83..08091bab4857 100644 > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > @@ -12,6 +12,7 @@ > > #include <linux/platform_device.h> > > #include <linux/regulator/consumer.h> > > > > +#include <drm/drm_atomic_helper.h> > > #include <drm/drm_bridge.h> > > #include <drm/drm_panel.h> > > > > @@ -22,6 +23,7 @@ struct lvds_codec { > > struct regulator *vcc; > > struct gpio_desc *powerdown_gpio; > > u32 connector_type; > > + unsigned int bus_format; > > }; > > > > static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge) > > @@ -74,12 +76,50 @@ static const struct drm_bridge_funcs funcs = { > > .disable = lvds_codec_disable, > > }; > > > > +#define MAX_INPUT_SEL_FORMATS 1 > > +static u32 * > > +lvds_codec_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > > + struct drm_bridge_state *bridge_state, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state, > > + u32 output_fmt, > > + unsigned int *num_input_fmts) > > +{ > > + struct lvds_codec *lvds_codec = to_lvds_codec(bridge); > > + u32 *input_fmts; > > + > > + *num_input_fmts = 0; > > + > > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > > + GFP_KERNEL); > > + if (!input_fmts) > > + return NULL; > > + > > + input_fmts[0] = lvds_codec->bus_format; > > + *num_input_fmts = MAX_INPUT_SEL_FORMATS; > > + > > + return input_fmts; > > +} > > + > > +static const struct drm_bridge_funcs funcs_decoder = { > > + .attach = lvds_codec_attach, > > + .enable = lvds_codec_enable, > > + .disable = lvds_codec_disable, > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > + .atomic_reset = drm_atomic_helper_bridge_reset, > > + .atomic_get_input_bus_fmts = lvds_codec_atomic_get_input_bus_fmts, > > +}; > > + > > static int lvds_codec_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct device_node *panel_node; > > + struct device_node *bus_node; > > struct drm_panel *panel; > > struct lvds_codec *lvds_codec; > > + const char *mapping; > > + int ret; > > > > lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > > if (!lvds_codec) > > @@ -119,13 +159,47 @@ static int lvds_codec_probe(struct platform_device *pdev) > > if (IS_ERR(lvds_codec->panel_bridge)) > > return PTR_ERR(lvds_codec->panel_bridge); > > > > + lvds_codec->bridge.funcs = &funcs; > > + > > + /* > > + * Decoder input LVDS format is a property of the decoder chip or even > > + * its strapping. Handle data-mapping the same way lvds-panel does. In > > + * case data-mapping is not present, do nothing, since there are still > > + * legacy bindings which do not specify this property. > > The missing data-mapping property is reported as an error, but this > comments says it is OK. Info? It's not a fatal error, probe still continues in backward-compatibility mode. The message is there to tell that the DT should be updated. Would you downgrade that to a warning ? > > + */ > > + if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) { > > + bus_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); > > Are there any reg specified in the binding? If not then the second > parameter should be -1 here. Yes, the DT node has two ports, with port 0 being the input. For LVDS decoders, that's where the LVDS bus is. > > + if (!bus_node) { > > + dev_dbg(dev, "bus DT node not found\n"); > > + return -ENXIO; > > + } > > + > > + ret = of_property_read_string(bus_node, "data-mapping", > > + &mapping); > > + of_node_put(bus_node); > > + if (ret < 0) { > > + dev_err(dev, "missing 'data-mapping' DT property\n"); > > + } else { > > It would be nice with a helper for the below code if we need this a third > time. Where would you store it ? > > + if (!strcmp(mapping, "jeida-18")) { > > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG; > > + } else if (!strcmp(mapping, "jeida-24")) { > > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA; > > + } else if (!strcmp(mapping, "vesa-24")) { > > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG; > > + } else { > > + dev_err(dev, "invalid 'data-mapping' DT property\n"); > > + return -EINVAL; > > + } > > Add empty line here. > > > + lvds_codec->bridge.funcs = &funcs_decoder; > > + } > > + } > > + > > /* > > * 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.funcs = &funcs; > > drm_bridge_add(&lvds_codec->bridge); > > > > platform_set_drvdata(pdev, lvds_codec);
Hi Laurent, > > > + > > > + /* > > > + * Decoder input LVDS format is a property of the decoder chip or even > > > + * its strapping. Handle data-mapping the same way lvds-panel does. In > > > + * case data-mapping is not present, do nothing, since there are still > > > + * legacy bindings which do not specify this property. > > > > The missing data-mapping property is reported as an error, but this > > comments says it is OK. Info? > > It's not a fatal error, probe still continues in backward-compatibility > mode. The message is there to tell that the DT should be updated. Would > you downgrade that to a warning ? Warning would IMO be better as this is not an error that stops it from working. > > > > + */ > > > + if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) { > > > + bus_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); > > > > Are there any reg specified in the binding? If not then the second > > parameter should be -1 here. > > Yes, the DT node has two ports, with port 0 being the input. For LVDS > decoders, that's where the LVDS bus is. OK. > > > > + if (!bus_node) { > > > + dev_dbg(dev, "bus DT node not found\n"); > > > + return -ENXIO; > > > + } > > > + > > > + ret = of_property_read_string(bus_node, "data-mapping", > > > + &mapping); > > > + of_node_put(bus_node); > > > + if (ret < 0) { > > > + dev_err(dev, "missing 'data-mapping' DT property\n"); > > > + } else { > > > > It would be nice with a helper for the below code if we need this a third > > time. > > Where would you store it ? drm_connector.c seems to be a good place. Or maybe a static inline in drm_connector.h. > > > > + if (!strcmp(mapping, "jeida-18")) { > > > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG; > > > + } else if (!strcmp(mapping, "jeida-24")) { > > > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA; > > > + } else if (!strcmp(mapping, "vesa-24")) { > > > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG; > > > + } else { > > > + dev_err(dev, "invalid 'data-mapping' DT property\n"); > > > + return -EINVAL; > > > + } Sam
On 10/10/21 2:27 PM, Sam Ravnborg wrote: Hi, [...] >>>> + if (!bus_node) { >>>> + dev_dbg(dev, "bus DT node not found\n"); >>>> + return -ENXIO; >>>> + } >>>> + >>>> + ret = of_property_read_string(bus_node, "data-mapping", >>>> + &mapping); >>>> + of_node_put(bus_node); >>>> + if (ret < 0) { >>>> + dev_err(dev, "missing 'data-mapping' DT property\n"); >>>> + } else { >>> >>> It would be nice with a helper for the below code if we need this a third >>> time. >> >> Where would you store it ? > drm_connector.c seems to be a good place. > Or maybe a static inline in drm_connector.h. Sorry, this patch has been dragging on for over half a year, another patch which depends on this has been dragging on for over a year, so I don't feel particularly motivated to prolong that further by starting a subsystem patch discussion here now. If someone wants to make this code more generic, that can surely be a subsequent patch?
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index dcf579a4cf83..08091bab4857 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -12,6 +12,7 @@ #include <linux/platform_device.h> #include <linux/regulator/consumer.h> +#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_panel.h> @@ -22,6 +23,7 @@ struct lvds_codec { struct regulator *vcc; struct gpio_desc *powerdown_gpio; u32 connector_type; + unsigned int bus_format; }; static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge) @@ -74,12 +76,50 @@ static const struct drm_bridge_funcs funcs = { .disable = lvds_codec_disable, }; +#define MAX_INPUT_SEL_FORMATS 1 +static u32 * +lvds_codec_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + struct lvds_codec *lvds_codec = to_lvds_codec(bridge); + u32 *input_fmts; + + *num_input_fmts = 0; + + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), + GFP_KERNEL); + if (!input_fmts) + return NULL; + + input_fmts[0] = lvds_codec->bus_format; + *num_input_fmts = MAX_INPUT_SEL_FORMATS; + + return input_fmts; +} + +static const struct drm_bridge_funcs funcs_decoder = { + .attach = lvds_codec_attach, + .enable = lvds_codec_enable, + .disable = lvds_codec_disable, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_get_input_bus_fmts = lvds_codec_atomic_get_input_bus_fmts, +}; + static int lvds_codec_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct device_node *panel_node; + struct device_node *bus_node; struct drm_panel *panel; struct lvds_codec *lvds_codec; + const char *mapping; + int ret; lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); if (!lvds_codec) @@ -119,13 +159,47 @@ static int lvds_codec_probe(struct platform_device *pdev) if (IS_ERR(lvds_codec->panel_bridge)) return PTR_ERR(lvds_codec->panel_bridge); + lvds_codec->bridge.funcs = &funcs; + + /* + * Decoder input LVDS format is a property of the decoder chip or even + * its strapping. Handle data-mapping the same way lvds-panel does. In + * case data-mapping is not present, do nothing, since there are still + * legacy bindings which do not specify this property. + */ + if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) { + bus_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); + if (!bus_node) { + dev_dbg(dev, "bus DT node not found\n"); + return -ENXIO; + } + + ret = of_property_read_string(bus_node, "data-mapping", + &mapping); + of_node_put(bus_node); + if (ret < 0) { + dev_err(dev, "missing 'data-mapping' DT property\n"); + } else { + if (!strcmp(mapping, "jeida-18")) { + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG; + } else if (!strcmp(mapping, "jeida-24")) { + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA; + } else if (!strcmp(mapping, "vesa-24")) { + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG; + } else { + dev_err(dev, "invalid 'data-mapping' DT property\n"); + return -EINVAL; + } + lvds_codec->bridge.funcs = &funcs_decoder; + } + } + /* * 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.funcs = &funcs; drm_bridge_add(&lvds_codec->bridge); platform_set_drvdata(pdev, lvds_codec);