The LDB driver currently checks whether dual mode is used, otherwise it
assumes only channel 0 is in use. Add support for using only channel 1. In
device tree terms, this means linking port 2 only.
Doing this cleanly requires changing the logic of the probe functions from
this:
1. use of_graph_get_remote_node() on port 1 to find the panel
2. use drm_of_lvds_get_dual_link_pixel_order() to detect dual mode
to this:
1. use of_graph_get_remote_node() twice to find remote ports
2. reuse the result of the above to know whether each channel is enabled
and to find the panel
3. if (both channels as enabled)
use drm_of_lvds_get_dual_link_pixel_order() to detect dual mode
Also add a dev_dbg() to log the detected mode and log an error in case no
panel was found (no channel enabled).
Signed-off-by: Luca Ceresoli <[email protected]>
---
drivers/gpu/drm/bridge/fsl-ldb.c | 112 ++++++++++++++++++-------------
1 file changed, 67 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 6bac160b395b..d844fb946981 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -84,10 +84,16 @@ struct fsl_ldb {
struct drm_bridge *panel_bridge;
struct clk *clk;
struct regmap *regmap;
- bool lvds_dual_link;
const struct fsl_ldb_devdata *devdata;
+ bool ch0_enabled;
+ bool ch1_enabled;
};
+static bool fsl_ldb_is_dual(const struct fsl_ldb *fsl_ldb)
+{
+ return (fsl_ldb->ch0_enabled && fsl_ldb->ch1_enabled);
+}
+
static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
{
return container_of(bridge, struct fsl_ldb, bridge);
@@ -95,7 +101,7 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
{
- if (fsl_ldb->lvds_dual_link)
+ if (fsl_ldb_is_dual(fsl_ldb))
return clock * 3500;
else
return clock * 7000;
@@ -177,28 +183,25 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
clk_prepare_enable(fsl_ldb->clk);
/* Program LDB_CTRL */
- reg = LDB_CTRL_CH0_ENABLE;
-
- if (fsl_ldb->lvds_dual_link)
- reg |= LDB_CTRL_CH1_ENABLE | LDB_CTRL_SPLIT_MODE;
-
- if (lvds_format_24bpp) {
- reg |= LDB_CTRL_CH0_DATA_WIDTH;
- if (fsl_ldb->lvds_dual_link)
- reg |= LDB_CTRL_CH1_DATA_WIDTH;
- }
-
- if (lvds_format_jeida) {
- reg |= LDB_CTRL_CH0_BIT_MAPPING;
- if (fsl_ldb->lvds_dual_link)
- reg |= LDB_CTRL_CH1_BIT_MAPPING;
- }
-
- if (mode->flags & DRM_MODE_FLAG_PVSYNC) {
- reg |= LDB_CTRL_DI0_VSYNC_POLARITY;
- if (fsl_ldb->lvds_dual_link)
- reg |= LDB_CTRL_DI1_VSYNC_POLARITY;
- }
+ reg =
+ (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_ENABLE : 0) |
+ (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_ENABLE : 0) |
+ (fsl_ldb_is_dual(fsl_ldb) ? LDB_CTRL_SPLIT_MODE : 0);
+
+ if (lvds_format_24bpp)
+ reg |=
+ (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_DATA_WIDTH : 0) |
+ (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_DATA_WIDTH : 0);
+
+ if (lvds_format_jeida)
+ reg |=
+ (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_BIT_MAPPING : 0) |
+ (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_BIT_MAPPING : 0);
+
+ if (mode->flags & DRM_MODE_FLAG_PVSYNC)
+ reg |=
+ (fsl_ldb->ch0_enabled ? LDB_CTRL_DI0_VSYNC_POLARITY : 0) |
+ (fsl_ldb->ch1_enabled ? LDB_CTRL_DI1_VSYNC_POLARITY : 0);
regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, reg);
@@ -210,9 +213,9 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
/* Wait for VBG to stabilize. */
usleep_range(15, 20);
- reg |= LVDS_CTRL_CH0_EN;
- if (fsl_ldb->lvds_dual_link)
- reg |= LVDS_CTRL_CH1_EN;
+ reg |=
+ (fsl_ldb->ch0_enabled ? LVDS_CTRL_CH0_EN : 0) |
+ (fsl_ldb->ch1_enabled ? LVDS_CTRL_CH1_EN : 0);
regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, reg);
}
@@ -265,7 +268,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
{
struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
- if (mode->clock > (fsl_ldb->lvds_dual_link ? 160000 : 80000))
+ if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000))
return MODE_CLOCK_HIGH;
return MODE_OK;
@@ -286,7 +289,7 @@ static int fsl_ldb_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *panel_node;
- struct device_node *port1, *port2;
+ struct device_node *remote1, *remote2;
struct drm_panel *panel;
struct fsl_ldb *fsl_ldb;
int dual_link;
@@ -311,10 +314,23 @@ static int fsl_ldb_probe(struct platform_device *pdev)
if (IS_ERR(fsl_ldb->regmap))
return PTR_ERR(fsl_ldb->regmap);
- /* Locate the panel DT node. */
- panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
- if (!panel_node)
- return -ENXIO;
+ /* Locate the remote ports and the panel node */
+ remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
+ remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
+ fsl_ldb->ch0_enabled = (remote1 != NULL);
+ fsl_ldb->ch1_enabled = (remote2 != NULL);
+ panel_node = of_node_get(remote1 ? remote1 : remote2);
+ of_node_put(remote1);
+ of_node_put(remote2);
+
+ if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
+ of_node_put(panel_node);
+ return dev_err_probe(dev, -ENXIO, "No panel node found");
+ }
+
+ dev_dbg(dev, "Using %s\n",
+ fsl_ldb_is_dual(fsl_ldb) ? "dual mode" :
+ fsl_ldb->ch0_enabled ? "channel 0" : "channel 1");
panel = of_drm_find_panel(panel_node);
of_node_put(panel_node);
@@ -325,20 +341,26 @@ static int fsl_ldb_probe(struct platform_device *pdev)
if (IS_ERR(fsl_ldb->panel_bridge))
return PTR_ERR(fsl_ldb->panel_bridge);
- /* Determine whether this is dual-link configuration */
- port1 = of_graph_get_port_by_id(dev->of_node, 1);
- port2 = of_graph_get_port_by_id(dev->of_node, 2);
- dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
- of_node_put(port1);
- of_node_put(port2);
- if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
- dev_err(dev, "LVDS channel pixel swap not supported.\n");
- return -EINVAL;
- }
+ if (fsl_ldb_is_dual(fsl_ldb)) {
+ struct device_node *port1, *port2;
+
+ port1 = of_graph_get_port_by_id(dev->of_node, 1);
+ port2 = of_graph_get_port_by_id(dev->of_node, 2);
+ dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
+ of_node_put(port1);
+ of_node_put(port2);
- if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS)
- fsl_ldb->lvds_dual_link = true;
+ if (dual_link < 0)
+ return dev_err_probe(dev, dual_link,
+ "Error getting dual link configuration");
+
+ /* Only DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS is supported */
+ if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
+ dev_err(dev, "LVDS channel pixel swap not supported.\n");
+ return -EINVAL;
+ }
+ }
platform_set_drvdata(pdev, fsl_ldb);
--
2.34.1
On 4/4/23 09:37, Luca Ceresoli wrote:
[...]
> @@ -177,28 +183,25 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> clk_prepare_enable(fsl_ldb->clk);
>
> /* Program LDB_CTRL */
> - reg = LDB_CTRL_CH0_ENABLE;
> -
> - if (fsl_ldb->lvds_dual_link)
> - reg |= LDB_CTRL_CH1_ENABLE | LDB_CTRL_SPLIT_MODE;
> -
> - if (lvds_format_24bpp) {
> - reg |= LDB_CTRL_CH0_DATA_WIDTH;
> - if (fsl_ldb->lvds_dual_link)
> - reg |= LDB_CTRL_CH1_DATA_WIDTH;
> - }
> -
> - if (lvds_format_jeida) {
> - reg |= LDB_CTRL_CH0_BIT_MAPPING;
> - if (fsl_ldb->lvds_dual_link)
> - reg |= LDB_CTRL_CH1_BIT_MAPPING;
> - }
> -
> - if (mode->flags & DRM_MODE_FLAG_PVSYNC) {
> - reg |= LDB_CTRL_DI0_VSYNC_POLARITY;
> - if (fsl_ldb->lvds_dual_link)
> - reg |= LDB_CTRL_DI1_VSYNC_POLARITY;
> - }
> + reg =
Cosmetic nit, do we need the newline here , can't we just move the first
'(fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_ENABLE : 0) |' on the same line as
'reg =' ? It might need a bit of indent with spaces, but that should be OK.
> + (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_ENABLE : 0) |
> + (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_ENABLE : 0) |
> + (fsl_ldb_is_dual(fsl_ldb) ? LDB_CTRL_SPLIT_MODE : 0);
> +
> + if (lvds_format_24bpp)
> + reg |=
> + (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_DATA_WIDTH : 0) |
> + (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_DATA_WIDTH : 0);
> +
> + if (lvds_format_jeida)
> + reg |=
> + (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_BIT_MAPPING : 0) |
> + (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_BIT_MAPPING : 0);
> +
> + if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> + reg |=
> + (fsl_ldb->ch0_enabled ? LDB_CTRL_DI0_VSYNC_POLARITY : 0) |
> + (fsl_ldb->ch1_enabled ? LDB_CTRL_DI1_VSYNC_POLARITY : 0);
>
> regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, reg);
[...]
> @@ -311,10 +314,23 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> if (IS_ERR(fsl_ldb->regmap))
> return PTR_ERR(fsl_ldb->regmap);
>
> - /* Locate the panel DT node. */
> - panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> - if (!panel_node)
> - return -ENXIO;
> + /* Locate the remote ports and the panel node */
> + remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
> + remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
> + fsl_ldb->ch0_enabled = (remote1 != NULL);
> + fsl_ldb->ch1_enabled = (remote2 != NULL);
> + panel_node = of_node_get(remote1 ? remote1 : remote2);
You can even do this without the middle 'remote1' I think:
panel_node = of_node_get(remote1 ? : remote2);
> + of_node_put(remote1);
> + of_node_put(remote2);
> +
> + if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
> + of_node_put(panel_node);
> + return dev_err_probe(dev, -ENXIO, "No panel node found");
> + }
> +
> + dev_dbg(dev, "Using %s\n",
> + fsl_ldb_is_dual(fsl_ldb) ? "dual mode" :
I think this is called "dual-link mode" , maybe update the string .
> + fsl_ldb->ch0_enabled ? "channel 0" : "channel 1");
>
> panel = of_drm_find_panel(panel_node);
> of_node_put(panel_node);
> @@ -325,20 +341,26 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> if (IS_ERR(fsl_ldb->panel_bridge))
> return PTR_ERR(fsl_ldb->panel_bridge);
>
> - /* Determine whether this is dual-link configuration */
> - port1 = of_graph_get_port_by_id(dev->of_node, 1);
> - port2 = of_graph_get_port_by_id(dev->of_node, 2);
> - dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> - of_node_put(port1);
> - of_node_put(port2);
>
> - if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> - dev_err(dev, "LVDS channel pixel swap not supported.\n");
> - return -EINVAL;
> - }
> + if (fsl_ldb_is_dual(fsl_ldb)) {
> + struct device_node *port1, *port2;
> +
> + port1 = of_graph_get_port_by_id(dev->of_node, 1);
> + port2 = of_graph_get_port_by_id(dev->of_node, 2);
> + dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> + of_node_put(port1);
> + of_node_put(port2);
>
> - if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS)
> - fsl_ldb->lvds_dual_link = true;
> + if (dual_link < 0)
> + return dev_err_probe(dev, dual_link,
> + "Error getting dual link configuration");
Does this need a trailing '\n' in the formatting string or not ? I think
yes.
The rest looks good, with the few details fixed:
Reviewed-by: Marek Vasut <[email protected]>
Hi Marek,
thanks for the quick and detailed review!
On Wed, 5 Apr 2023 05:28:16 +0200
Marek Vasut <[email protected]> wrote:
> On 4/4/23 09:37, Luca Ceresoli wrote:
>
> [...]
>
> > @@ -177,28 +183,25 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> > clk_prepare_enable(fsl_ldb->clk);
> >
> > /* Program LDB_CTRL */
> > - reg = LDB_CTRL_CH0_ENABLE;
> > -
> > - if (fsl_ldb->lvds_dual_link)
> > - reg |= LDB_CTRL_CH1_ENABLE | LDB_CTRL_SPLIT_MODE;
> > -
> > - if (lvds_format_24bpp) {
> > - reg |= LDB_CTRL_CH0_DATA_WIDTH;
> > - if (fsl_ldb->lvds_dual_link)
> > - reg |= LDB_CTRL_CH1_DATA_WIDTH;
> > - }
> > -
> > - if (lvds_format_jeida) {
> > - reg |= LDB_CTRL_CH0_BIT_MAPPING;
> > - if (fsl_ldb->lvds_dual_link)
> > - reg |= LDB_CTRL_CH1_BIT_MAPPING;
> > - }
> > -
> > - if (mode->flags & DRM_MODE_FLAG_PVSYNC) {
> > - reg |= LDB_CTRL_DI0_VSYNC_POLARITY;
> > - if (fsl_ldb->lvds_dual_link)
> > - reg |= LDB_CTRL_DI1_VSYNC_POLARITY;
> > - }
> > + reg =
>
> Cosmetic nit, do we need the newline here , can't we just move the first
> '(fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_ENABLE : 0) |' on the same line as
> 'reg =' ? It might need a bit of indent with spaces, but that should be OK.
Sure. Maybe 'reg =<tab>(fsl...' would be even better, it checkpatch
allows.
> > @@ -311,10 +314,23 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> > if (IS_ERR(fsl_ldb->regmap))
> > return PTR_ERR(fsl_ldb->regmap);
> >
> > - /* Locate the panel DT node. */
> > - panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > - if (!panel_node)
> > - return -ENXIO;
> > + /* Locate the remote ports and the panel node */
> > + remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
> > + remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
> > + fsl_ldb->ch0_enabled = (remote1 != NULL);
> > + fsl_ldb->ch1_enabled = (remote2 != NULL);
> > + panel_node = of_node_get(remote1 ? remote1 : remote2);
>
> You can even do this without the middle 'remote1' I think:
>
> panel_node = of_node_get(remote1 ? : remote2);
Apparently, but honestly with such short expressions clearly having no
side effects I think it's not helping readability.
> > + of_node_put(remote1);
> > + of_node_put(remote2);
> > +
> > + if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
> > + of_node_put(panel_node);
> > + return dev_err_probe(dev, -ENXIO, "No panel node found");
> > + }
> > +
> > + dev_dbg(dev, "Using %s\n",
> > + fsl_ldb_is_dual(fsl_ldb) ? "dual mode" :
>
> I think this is called "dual-link mode" , maybe update the string .
I was using the terms from the NXP docs, but indeed in the kernel it
seems that "dual-link" is the common name. Updating that.
> > @@ -325,20 +341,26 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> > if (IS_ERR(fsl_ldb->panel_bridge))
> > return PTR_ERR(fsl_ldb->panel_bridge);
> >
> > - /* Determine whether this is dual-link configuration */
> > - port1 = of_graph_get_port_by_id(dev->of_node, 1);
> > - port2 = of_graph_get_port_by_id(dev->of_node, 2);
> > - dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> > - of_node_put(port1);
> > - of_node_put(port2);
> >
> > - if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> > - dev_err(dev, "LVDS channel pixel swap not supported.\n");
> > - return -EINVAL;
> > - }
> > + if (fsl_ldb_is_dual(fsl_ldb)) {
> > + struct device_node *port1, *port2;
> > +
> > + port1 = of_graph_get_port_by_id(dev->of_node, 1);
> > + port2 = of_graph_get_port_by_id(dev->of_node, 2);
> > + dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> > + of_node_put(port1);
> > + of_node_put(port2);
> >
> > - if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS)
> > - fsl_ldb->lvds_dual_link = true;
> > + if (dual_link < 0)
> > + return dev_err_probe(dev, dual_link,
> > + "Error getting dual link configuration");
>
> Does this need a trailing '\n' in the formatting string or not ? I think
> yes.
Oops, good catch.
> The rest looks good, with the few details fixed:
>
> Reviewed-by: Marek Vasut <[email protected]>
Thanks!
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 4/5/23 09:30, Luca Ceresoli wrote:
[...]
>>> @@ -311,10 +314,23 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>>> if (IS_ERR(fsl_ldb->regmap))
>>> return PTR_ERR(fsl_ldb->regmap);
>>>
>>> - /* Locate the panel DT node. */
>>> - panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
>>> - if (!panel_node)
>>> - return -ENXIO;
>>> + /* Locate the remote ports and the panel node */
>>> + remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
>>> + remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
>>> + fsl_ldb->ch0_enabled = (remote1 != NULL);
>>> + fsl_ldb->ch1_enabled = (remote2 != NULL);
>>> + panel_node = of_node_get(remote1 ? remote1 : remote2);
>>
>> You can even do this without the middle 'remote1' I think:
>>
>> panel_node = of_node_get(remote1 ? : remote2);
>
> Apparently, but honestly with such short expressions clearly having no
> side effects I think it's not helping readability.
I think even the ternary operator itself isn't helpful much, but that's
a matter of taste, and I don't have a better suggestion which would
improve the readability either (I tried to expand it into if()... but
that looks bad too).
No need to change anything.
[...]
Thanks for the patch.