LinuxLists.cc - [PATCH v2 0/9] drm: Switch from dev_err to dev_err

[permalink] [raw]

Subject: [PATCH v2 2/9] drm/bridge: icn6211: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Fixes: 8dde6f7452a1 ("drm: bridge: icn6211: Add I2C configuration support")
Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 82d23e4df09e..ff3284b6b1a3 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -563,10 +563,8 @@ static int chipone_dsi_host_attach(struct chipone *icn)

host = of_find_mipi_dsi_host_by_node(host_node);
of_node_put(host_node);
- if (!host) {
- dev_err(dev, "failed to find dsi host\n");
- return -EPROBE_DEFER;
- }
+ if (!host)
+ return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");

dsi = mipi_dsi_device_register_full(host, &info);
if (IS_ERR(dsi)) {

--
2.44.0

2024-03-01 00:13:11

[permalink] [raw]

Subject: [PATCH v2 3/9] drm/bridge: lt8912b: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Fixes: 30e2ae943c26 ("drm/bridge: Introduce LT8912B DSI to HDMI bridge")
Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/bridge/lontium-lt8912b.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c
index 273157428c82..15aa890c3e6d 100644
--- a/drivers/gpu/drm/bridge/lontium-lt8912b.c
+++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c
@@ -496,10 +496,8 @@ static int lt8912_attach_dsi(struct lt8912 *lt)
};

host = of_find_mipi_dsi_host_by_node(lt->host_node);
- if (!host) {
- dev_err(dev, "failed to find dsi host\n");
- return -EPROBE_DEFER;
- }
+ if (!host)
+ return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");

dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
if (IS_ERR(dsi)) {

--
2.44.0

2024-03-01 00:13:24

[permalink] [raw]

Subject: [PATCH v2 4/9] drm/bridge: lt9611: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Fixes: 23278bf54afe ("drm/bridge: Introduce LT9611 DSI to HDMI bridge")
Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/bridge/lontium-lt9611.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 9663601ce098..89bdd938757e 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -760,10 +760,8 @@ static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611,
int ret;

host = of_find_mipi_dsi_host_by_node(dsi_node);
- if (!host) {
- dev_err(lt9611->dev, "failed to find dsi host\n");
- return ERR_PTR(-EPROBE_DEFER);
- }
+ if (!host)
+ return ERR_PTR(dev_err_probe(lt9611->dev, -EPROBE_DEFER, "failed to find dsi host\n"));

dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
if (IS_ERR(dsi)) {

--
2.44.0

2024-03-01 00:13:37

[permalink] [raw]

Subject: [PATCH v2 5/9] drm/bridge: lt9611uxc: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge")
Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index e971b75e90ad..b803899126d5 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
@@ -265,10 +265,8 @@ static struct mipi_dsi_device *lt9611uxc_attach_dsi(struct lt9611uxc *lt9611uxc,
int ret;

host = of_find_mipi_dsi_host_by_node(dsi_node);
- if (!host) {
- dev_err(dev, "failed to find dsi host\n");
- return ERR_PTR(-EPROBE_DEFER);
- }
+ if (!host)
+ return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n"));

dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
if (IS_ERR(dsi)) {

--
2.44.0

2024-03-01 00:13:50

[permalink] [raw]

Subject: [PATCH v2 6/9] drm/bridge: tc358775: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Fixes: b26975593b17 ("display/drm/bridge: TC358775 DSI/LVDS driver")
Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/bridge/tc358775.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
index 90a89d70d832..fea4f00a20f8 100644
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -610,10 +610,8 @@ static int tc_attach_host(struct tc_data *tc)
};

host = of_find_mipi_dsi_host_by_node(tc->host_node);
- if (!host) {
- dev_err(dev, "failed to find dsi host\n");
- return -EPROBE_DEFER;
- }
+ if (!host)
+ return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");

dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
if (IS_ERR(dsi)) {

--
2.44.0

2024-03-01 00:14:16

[permalink] [raw]

Subject: [PATCH v2 8/9] drm/panel: novatek-nt35950: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC panels")
Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/panel/panel-novatek-nt35950.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
index 648ce9201426..028fdac293f7 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
@@ -556,10 +556,8 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
}
dsi_r_host = of_find_mipi_dsi_host_by_node(dsi_r);
of_node_put(dsi_r);
- if (!dsi_r_host) {
- dev_err(dev, "Cannot get secondary DSI host\n");
- return -EPROBE_DEFER;
- }
+ if (!dsi_r_host)
+ return dev_err_probe(dev, -EPROBE_DEFER, "Cannot get secondary DSI host\n");

nt->dsi[1] = mipi_dsi_device_register_full(dsi_r_host, info);
if (!nt->dsi[1]) {

--
2.44.0

2024-03-01 00:14:28

[permalink] [raw]

Subject: [PATCH v2 7/9] drm/bridge: dpc3433: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Also move the "failed to attach" error message so that it's only printed
when the devm_mipi_dsi_attach() call fails.

Fixes: 6352cd451ddb ("drm: bridge: Add TI DLPC3433 DSI to DMD bridge")
Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/bridge/ti-dlpc3433.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-dlpc3433.c b/drivers/gpu/drm/bridge/ti-dlpc3433.c
index ca3348109bcd..6b559e071301 100644
--- a/drivers/gpu/drm/bridge/ti-dlpc3433.c
+++ b/drivers/gpu/drm/bridge/ti-dlpc3433.c
@@ -319,12 +319,11 @@ static int dlpc_host_attach(struct dlpc *dlpc)
.channel = 0,
.node = NULL,
};
+ int ret;

host = of_find_mipi_dsi_host_by_node(dlpc->host_node);
- if (!host) {
- DRM_DEV_ERROR(dev, "failed to find dsi host\n");
- return -EPROBE_DEFER;
- }
+ if (!host)
+ return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");

dlpc->dsi = mipi_dsi_device_register_full(host, &info);
if (IS_ERR(dlpc->dsi)) {
@@ -336,7 +335,11 @@ static int dlpc_host_attach(struct dlpc *dlpc)
dlpc->dsi->format = MIPI_DSI_FMT_RGB565;
dlpc->dsi->lanes = dlpc->dsi_lanes;

- return devm_mipi_dsi_attach(dev, dlpc->dsi);
+ ret = devm_mipi_dsi_attach(dev, dlpc->dsi);
+ if (ret)
+ DRM_DEV_ERROR(dev, "failed to attach dsi host\n");
+
+ return ret;
}

static int dlpc3433_probe(struct i2c_client *client)
@@ -367,10 +370,8 @@ static int dlpc3433_probe(struct i2c_client *client)
drm_bridge_add(&dlpc->bridge);

ret = dlpc_host_attach(dlpc);
- if (ret) {
- DRM_DEV_ERROR(dev, "failed to attach dsi host\n");
+ if (ret)
goto err_remove_bridge;
- }

return 0;

--
2.44.0

2024-03-01 00:15:03

[permalink] [raw]

Subject: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found

Given that failing to find a DSI host causes the driver to defer probe,
make use of dev_err_probe() to log the reason. This makes the defer
probe reason available and avoids alerting userspace about something
that is not necessarily an error.

Suggested-by: AngeloGioacchino Del Regno <[email protected]>
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
index b73448cf349d..d447db912a61 100644
--- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
+++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
@@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device *dsi)

dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
of_node_put(dsi1);
- if (!dsi1_host) {
- dev_err(dev, "failed to find dsi host\n");
- return -EPROBE_DEFER;
- }
+ if (!dsi1_host)
+ return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");

/* register the second DSI device */
dsi1_device = mipi_dsi_device_register_full(dsi1_host, &info);

--
2.44.0

2024-03-01 00:31:46

by Abhinav Kumar

[permalink] [raw]

Subject: Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found

On 2/29/2024 4:12 PM, Nícolas F. R. A. Prado wrote:
> Given that failing to find a DSI host causes the driver to defer probe,
> make use of dev_err_probe() to log the reason. This makes the defer
> probe reason available and avoids alerting userspace about something
> that is not necessarily an error.
>
> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>

Reviewed-by: Abhinav Kumar <[email protected]>

2024-03-01 06:29:36

[permalink] [raw]

Subject: Re: [PATCH v2 8/9] drm/panel: novatek-nt35950: Don't log an error when DSI host can't be found

Hi Nicolas,

Thank you for the patch.

On Thu, Feb 29, 2024 at 07:12:14PM -0500, Nícolas F. R. A. Prado wrote:
> Given that failing to find a DSI host causes the driver to defer probe,
> make use of dev_err_probe() to log the reason. This makes the defer
> probe reason available and avoids alerting userspace about something
> that is not necessarily an error.
>
> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC panels")
> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> index 648ce9201426..028fdac293f7 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> @@ -556,10 +556,8 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
> }
> dsi_r_host = of_find_mipi_dsi_host_by_node(dsi_r);
> of_node_put(dsi_r);
> - if (!dsi_r_host) {
> - dev_err(dev, "Cannot get secondary DSI host\n");
> - return -EPROBE_DEFER;
> - }
> + if (!dsi_r_host)
> + return dev_err_probe(dev, -EPROBE_DEFER, "Cannot get secondary DSI host\n");

return dev_err_probe(dev, -EPROBE_DEFER,
"Cannot get secondary DSI host\n");

With this,

Reviewed-by: Laurent Pinchart <[email protected]>

>
> nt->dsi[1] = mipi_dsi_device_register_full(dsi_r_host, info);
> if (!nt->dsi[1]) {
>

--
Regards,

Laurent Pinchart

2024-03-01 06:30:38

[permalink] [raw]

Subject: Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found

Hi Nícolas,

Thank you for the patch.

On Thu, Feb 29, 2024 at 07:12:15PM -0500, Nícolas F. R. A. Prado wrote:
> Given that failing to find a DSI host causes the driver to defer probe,
> make use of dev_err_probe() to log the reason. This makes the defer
> probe reason available and avoids alerting userspace about something
> that is not necessarily an error.
>
> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> index b73448cf349d..d447db912a61 100644
> --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> @@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device *dsi)
>
> dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> of_node_put(dsi1);
> - if (!dsi1_host) {
> - dev_err(dev, "failed to find dsi host\n");
> - return -EPROBE_DEFER;
> - }
> + if (!dsi1_host)
> + return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");

return dev_err_probe(dev, -EPROBE_DEFER,
"failed to find dsi host\n");

With this addressed,

Reviewed-by: Laurent Pinchart <[email protected]>

>
> /* register the second DSI device */
> dsi1_device = mipi_dsi_device_register_full(dsi1_host, &info);
>

--
Regards,

Laurent Pinchart

2024-03-01 06:34:39

[permalink] [raw]

Subject: Re: [PATCH v2 0/9] drm: Switch from dev_err to dev_err_probe for missing DSI host error path

Hi Nícolas,

On Thu, Feb 29, 2024 at 07:12:06PM -0500, Nícolas F. R. A. Prado wrote:
> This series changes every occurence of the following pattern:
>
> dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> if (!dsi_host) {
> dev_err(dev, "failed to find dsi host\n");
> return -EPROBE_DEFER;
> }
>
> into
>
> dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> if (!dsi_host)
> return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
>
> This registers the defer probe reason (so it can later be printed by the
> driver core or checked on demand through the devices_deferred file in
> debugfs) and prevents errors to be spammed in the kernel log every time
> the driver retries to probe, unnecessarily alerting userspace about
> something that is a normal part of the boot process.

The idea is good, but I have a small issue with patches 1/9 to 7/9. They
all patch a function that is called by the probe function. Calling
dev_err_probe() in such functions is error-prone. I had to manually
check when reviewing the patches that those functions were indeed called
at probe time, and not through other code paths, and I also had to check
that no callers were using dev_err_probe() in the error handling path,
as that would have overridden the error message.

Would there be a way to move the dev_err_probe() to the top-level ? I
understand it's not always possible or convenient, but if it was doable
in at least some of the drivers, I think it would be better. I'll let
you be the judge.

> I have omitted a Fixes: tag in the last patch, for the truly-nt35597
> panel, because it predates the dev_err_probe() helper.
>
> Changes in v2:
> - Added patches 2 onwards to fix all occurences of this pattern instead
> of just for the anx7625 driver
> - Link to v1: https://lore.kernel.org/r/20240226-anx7625-defer-log-no-dsi-host-v1-1-242b1af31884@collabora.com
>
> ---
> Nícolas F. R. A. Prado (9):
> drm/bridge: anx7625: Don't log an error when DSI host can't be found
> drm/bridge: icn6211: Don't log an error when DSI host can't be found
> drm/bridge: lt8912b: Don't log an error when DSI host can't be found
> drm/bridge: lt9611: Don't log an error when DSI host can't be found
> drm/bridge: lt9611uxc: Don't log an error when DSI host can't be found
> drm/bridge: tc358775: Don't log an error when DSI host can't be found
> drm/bridge: dpc3433: Don't log an error when DSI host can't be found
> drm/panel: novatek-nt35950: Don't log an error when DSI host can't be found
> drm/panel: truly-nt35597: Don't log an error when DSI host can't be found
>
> drivers/gpu/drm/bridge/analogix/anx7625.c | 6 ++----
> drivers/gpu/drm/bridge/chipone-icn6211.c | 6 ++----
> drivers/gpu/drm/bridge/lontium-lt8912b.c | 6 ++----
> drivers/gpu/drm/bridge/lontium-lt9611.c | 6 ++----
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 6 ++----
> drivers/gpu/drm/bridge/tc358775.c | 6 ++----
> drivers/gpu/drm/bridge/ti-dlpc3433.c | 17 +++++++++--------
> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 6 ++----
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
> 9 files changed, 25 insertions(+), 40 deletions(-)
> ---
> base-commit: 2ae0a045e6814c8c1d676d6153c605a65746aa29
> change-id: 20240226-anx7625-defer-log-no-dsi-host-c3f9ccbcb287

--
Regards,

Laurent Pinchart

2024-03-01 08:44:51

[permalink] [raw]

Subject: Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found

Il 01/03/24 07:30, Laurent Pinchart ha scritto:
> Hi Nícolas,
>
> Thank you for the patch.
>
> On Thu, Feb 29, 2024 at 07:12:15PM -0500, Nícolas F. R. A. Prado wrote:
>> Given that failing to find a DSI host causes the driver to defer probe,
>> make use of dev_err_probe() to log the reason. This makes the defer
>> probe reason available and avoids alerting userspace about something
>> that is not necessarily an error.
>>
>> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
>> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
>> ---
>> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>> index b73448cf349d..d447db912a61 100644
>> --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
>> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>> @@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device *dsi)
>>
>> dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
>> of_node_put(dsi1);
>> - if (!dsi1_host) {
>> - dev_err(dev, "failed to find dsi host\n");
>> - return -EPROBE_DEFER;
>> - }
>> + if (!dsi1_host)
>> + return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
>
> return dev_err_probe(dev, -EPROBE_DEFER,
> "failed to find dsi host\n");
>
> With this addressed,

I disagree. That's 87 columns, and the 80-col rule is long gone.

Reviewed-by: AngeloGioacchino Del Regno <[email protected]>

>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
>>
>> /* register the second DSI device */
>> dsi1_device = mipi_dsi_device_register_full(dsi1_host, &info);
>>
>

2024-03-01 08:45:19

[permalink] [raw]

Subject: Re: [PATCH v2 8/9] drm/panel: novatek-nt35950: Don't log an error when DSI host can't be found

Il 01/03/24 01:12, Nícolas F. R. A. Prado ha scritto:
> Given that failing to find a DSI host causes the driver to defer probe,
> make use of dev_err_probe() to log the reason. This makes the defer
> probe reason available and avoids alerting userspace about something
> that is not necessarily an error.
>
> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC panels")
> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> index 648ce9201426..028fdac293f7 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> @@ -556,10 +556,8 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
> }
> dsi_r_host = of_find_mipi_dsi_host_by_node(dsi_r);
> of_node_put(dsi_r);
> - if (!dsi_r_host) {
> - dev_err(dev, "Cannot get secondary DSI host\n");
> - return -EPROBE_DEFER;
> - }
> + if (!dsi_r_host)
> + return dev_err_probe(dev, -EPROBE_DEFER, "Cannot get secondary DSI host\n");

Nicolas, this is going over 100 columns, which is not ok.
Please fix.

>
> nt->dsi[1] = mipi_dsi_device_register_full(dsi_r_host, info);
> if (!nt->dsi[1]) {
>

2024-03-01 08:56:34

[permalink] [raw]

Subject: Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found

On Fri, Mar 01, 2024 at 09:44:36AM +0100, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 07:30, Laurent Pinchart ha scritto:
> > On Thu, Feb 29, 2024 at 07:12:15PM -0500, Nícolas F. R. A. Prado wrote:
> >> Given that failing to find a DSI host causes the driver to defer probe,
> >> make use of dev_err_probe() to log the reason. This makes the defer
> >> probe reason available and avoids alerting userspace about something
> >> that is not necessarily an error.
> >>
> >> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
> >> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
> >> ---
> >> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
> >> 1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> >> index b73448cf349d..d447db912a61 100644
> >> --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
> >> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> >> @@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device *dsi)
> >>
> >> dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> >> of_node_put(dsi1);
> >> - if (!dsi1_host) {
> >> - dev_err(dev, "failed to find dsi host\n");
> >> - return -EPROBE_DEFER;
> >> - }
> >> + if (!dsi1_host)
> >> + return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
> >
> > return dev_err_probe(dev, -EPROBE_DEFER,
> > "failed to find dsi host\n");
> >
> > With this addressed,
>
> I disagree. That's 87 columns, and the 80-col rule is long gone.

It's still a maintainer's preference. I soft-enforce it in drivers I
maintain. In this case I'll let Neil decide, as he's listed as the
maintainer for drivers/gpu/drm/panel/.

> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>
> > Reviewed-by: Laurent Pinchart <[email protected]>
> >
> >>
> >> /* register the second DSI device */
> >> dsi1_device = mipi_dsi_device_register_full(dsi1_host, &info);

--
Regards,

Laurent Pinchart

2024-03-01 09:37:47

[permalink] [raw]

Subject: Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found

Il 01/03/24 09:56, Laurent Pinchart ha scritto:
> On Fri, Mar 01, 2024 at 09:44:36AM +0100, AngeloGioacchino Del Regno wrote:
>> Il 01/03/24 07:30, Laurent Pinchart ha scritto:
>>> On Thu, Feb 29, 2024 at 07:12:15PM -0500, Nícolas F. R. A. Prado wrote:
>>>> Given that failing to find a DSI host causes the driver to defer probe,
>>>> make use of dev_err_probe() to log the reason. This makes the defer
>>>> probe reason available and avoids alerting userspace about something
>>>> that is not necessarily an error.
>>>>
>>>> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
>>>> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
>>>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>>>> index b73448cf349d..d447db912a61 100644
>>>> --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
>>>> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>>>> @@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device *dsi)
>>>>
>>>> dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
>>>> of_node_put(dsi1);
>>>> - if (!dsi1_host) {
>>>> - dev_err(dev, "failed to find dsi host\n");
>>>> - return -EPROBE_DEFER;
>>>> - }
>>>> + if (!dsi1_host)
>>>> + return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
>>>
>>> return dev_err_probe(dev, -EPROBE_DEFER,
>>> "failed to find dsi host\n");
>>>
>>> With this addressed,
>>
>> I disagree. That's 87 columns, and the 80-col rule is long gone.
>
> It's still a maintainer's preference. I soft-enforce it in drivers I
> maintain. In this case I'll let Neil decide, as he's listed as the
> maintainer for drivers/gpu/drm/panel/.
>

Yes, that's right. Comes down to personal opinion.

Cheers

>> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>>
>>> Reviewed-by: Laurent Pinchart <[email protected]>
>>>
>>>>
>>>> /* register the second DSI device */
>>>> dsi1_device = mipi_dsi_device_register_full(dsi1_host, &info);
>

2024-03-01 10:54:56

by Neil Armstrong

[permalink] [raw]

Subject: Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found

On 01/03/2024 10:37, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 09:56, Laurent Pinchart ha scritto:
>> On Fri, Mar 01, 2024 at 09:44:36AM +0100, AngeloGioacchino Del Regno wrote:
>>> Il 01/03/24 07:30, Laurent Pinchart ha scritto:
>>>> On Thu, Feb 29, 2024 at 07:12:15PM -0500, Nícolas F. R. A. Prado wrote:
>>>>> Given that failing to find a DSI host causes the driver to defer probe,
>>>>> make use of dev_err_probe() to log the reason. This makes the defer
>>>>> probe reason available and avoids alerting userspace about something
>>>>> that is not necessarily an error.
>>>>>
>>>>> Suggested-by: AngeloGioacchino Del Regno <[email protected]>
>>>>> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
>>>>> ---
>>>>>    drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
>>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>>>>> index b73448cf349d..d447db912a61 100644
>>>>> --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
>>>>> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
>>>>> @@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device *dsi)
>>>>>        dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
>>>>>        of_node_put(dsi1);
>>>>> -    if (!dsi1_host) {
>>>>> -        dev_err(dev, "failed to find dsi host\n");
>>>>> -        return -EPROBE_DEFER;
>>>>> -    }
>>>>> +    if (!dsi1_host)
>>>>> +        return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
>>>>
>>>>         return dev_err_probe(dev, -EPROBE_DEFER,
>>>>                      "failed to find dsi host\n");
>>>>
>>>> With this addressed,
>>>
>>> I disagree. That's 87 columns, and the 80-col rule is long gone.
>>
>> It's still a maintainer's preference. I soft-enforce it in drivers I
>> maintain. In this case I'll let Neil decide, as he's listed as the
>> maintainer for drivers/gpu/drm/panel/.
>>
>
> Yes, that's right. Comes down to personal opinion.

There always was an exception for strings to go over the 80col, and I like it better as an one-liner.
So I'm in favor with the initial proposal.

Reviewed-by: Neil Armstrong <[email protected]>

Neil

>
> Cheers
>
>>> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>>>
>>>> Reviewed-by: Laurent Pinchart <[email protected]>
>>>>
>>>>>        /* register the second DSI device */
>>>>>        dsi1_device = mipi_dsi_device_register_full(dsi1_host, &info);
>>
>

2024-03-01 16:19:48

[permalink] [raw]

Subject: Re: [PATCH v2 0/9] drm: Switch from dev_err to dev_err_probe for missing DSI host error path

On Fri, Mar 01, 2024 at 08:34:31AM +0200, Laurent Pinchart wrote:
> Hi N?colas,
>
> On Thu, Feb 29, 2024 at 07:12:06PM -0500, N?colas F. R. A. Prado wrote:
> > This series changes every occurence of the following pattern:
> >
> > dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > if (!dsi_host) {
> > dev_err(dev, "failed to find dsi host\n");
> > return -EPROBE_DEFER;
> > }
> >
> > into
> >
> > dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > if (!dsi_host)
> > return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
> >
> > This registers the defer probe reason (so it can later be printed by the
> > driver core or checked on demand through the devices_deferred file in
> > debugfs) and prevents errors to be spammed in the kernel log every time
> > the driver retries to probe, unnecessarily alerting userspace about
> > something that is a normal part of the boot process.
>
> The idea is good, but I have a small issue with patches 1/9 to 7/9. They
> all patch a function that is called by the probe function. Calling
> dev_err_probe() in such functions is error-prone. I had to manually
> check when reviewing the patches that those functions were indeed called
> at probe time, and not through other code paths, and I also had to check
> that no callers were using dev_err_probe() in the error handling path,
> as that would have overridden the error message.
>
> Would there be a way to move the dev_err_probe() to the top-level ? I
> understand it's not always possible or convenient, but if it was doable
> in at least some of the drivers, I think it would be better. I'll let
> you be the judge.

Hey Laurent, thanks for the review.

I get where you're coming from, as I checked those things myself while writing
the patch. That said, I don't think moving dev_err_probe() to the top-level is a
good move for a few reasons:
* Keeping the log message as close to the source of the error makes it more
specific, and consequently, more useful.
* The original code already returned -EPROBE_DEFER, implying the function is
expected to be called only from the probe function.

With those points in mind, the only way I see to guarantee
dev_err_probe(...,-EPROBE_DEFER...) would only be called by probe, and that the
reason wouldn't be overriden, would be to move the entire code path of that
function that calls into dev_err_probe() up into the probe function. But if we
adopt this pattern consistently across the drivers in the tree, I think it would
drastically worsen readability and cancel out the benefits.

IMO the way forward with the API we have, is to make use of warnings and static
checkers to catch cases where dev_err_probe() is overriding a defer probe
reason, and where it's called outside of the probe function scope.

So I'm inclined to leave the patches as they are, but am happy to discuss this
further or other ideas.

Thanks,
N?colas

2024-03-01 21:44:48

[permalink] [raw]

Subject: Re: [PATCH v2 0/9] drm: Switch from dev_err to dev_err_probe for missing DSI host error path

On Fri, Mar 01, 2024 at 11:19:27AM -0500, Nícolas F. R. A. Prado wrote:
> On Fri, Mar 01, 2024 at 08:34:31AM +0200, Laurent Pinchart wrote:
> > Hi Nícolas,
> >
> > On Thu, Feb 29, 2024 at 07:12:06PM -0500, Nícolas F. R. A. Prado wrote:
> > > This series changes every occurence of the following pattern:
> > >
> > > dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > > if (!dsi_host) {
> > > dev_err(dev, "failed to find dsi host\n");
> > > return -EPROBE_DEFER;
> > > }
> > >
> > > into
> > >
> > > dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > > if (!dsi_host)
> > > return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
> > >
> > > This registers the defer probe reason (so it can later be printed by the
> > > driver core or checked on demand through the devices_deferred file in
> > > debugfs) and prevents errors to be spammed in the kernel log every time
> > > the driver retries to probe, unnecessarily alerting userspace about
> > > something that is a normal part of the boot process.
> >
> > The idea is good, but I have a small issue with patches 1/9 to 7/9. They
> > all patch a function that is called by the probe function. Calling
> > dev_err_probe() in such functions is error-prone. I had to manually
> > check when reviewing the patches that those functions were indeed called
> > at probe time, and not through other code paths, and I also had to check
> > that no callers were using dev_err_probe() in the error handling path,
> > as that would have overridden the error message.
> >
> > Would there be a way to move the dev_err_probe() to the top-level ? I
> > understand it's not always possible or convenient, but if it was doable
> > in at least some of the drivers, I think it would be better. I'll let
> > you be the judge.
>
> Hey Laurent, thanks for the review.
>
> I get where you're coming from, as I checked those things myself while writing
> the patch. That said, I don't think moving dev_err_probe() to the top-level is a
> good move for a few reasons:
> * Keeping the log message as close to the source of the error makes it more
> specific, and consequently, more useful.
> * The original code already returned -EPROBE_DEFER, implying the function is
> expected to be called only from the probe function.
>
> With those points in mind, the only way I see to guarantee
> dev_err_probe(...,-EPROBE_DEFER...) would only be called by probe, and that the
> reason wouldn't be overriden, would be to move the entire code path of that
> function that calls into dev_err_probe() up into the probe function. But if we
> adopt this pattern consistently across the drivers in the tree, I think it would
> drastically worsen readability and cancel out the benefits.
>
> IMO the way forward with the API we have, is to make use of warnings and static
> checkers to catch cases where dev_err_probe() is overriding a defer probe
> reason, and where it's called outside of the probe function scope.
>
> So I'm inclined to leave the patches as they are, but am happy to discuss this
> further or other ideas.

Thanks for checking and having taken the time to explain your rationale.
For the whole series,

Reviewed-by: Laurent Pinchart <[email protected]>

--
Regards,

Laurent Pinchart

2024-03-01 09:03:40

[permalink] [raw]

Subject: Re: [PATCH v2 0/9] drm: Switch from dev_err to dev_err_probe for missing DSI host error path

Il 01/03/24 01:12, Nícolas F. R. A. Prado ha scritto:
> This series changes every occurence of the following pattern:
>
> dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> if (!dsi_host) {
> dev_err(dev, "failed to find dsi host\n");
> return -EPROBE_DEFER;
> }
>
> into
>
> dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> if (!dsi_host)
> return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi host\n");
>
> This registers the defer probe reason (so it can later be printed by the
> driver core or checked on demand through the devices_deferred file in
> debugfs) and prevents errors to be spammed in the kernel log every time
> the driver retries to probe, unnecessarily alerting userspace about
> something that is a normal part of the boot process.
>
> I have omitted a Fixes: tag in the last patch, for the truly-nt35597
> panel, because it predates the dev_err_probe() helper.
>
> Changes in v2:
> - Added patches 2 onwards to fix all occurences of this pattern instead
> of just for the anx7625 driver
> - Link to v1: https://lore.kernel.org/r/20240226-anx7625-defer-log-no-dsi-host-v1-1-242b1af31884@collabora.com
>

Apart from patch [8/9], where you're going over 100 cols, this series looks good.

After fixing that, on v3, please feel free to add my....

Reviewed-by: AngeloGioacchino Del Regno <[email protected]>

..to all of the patches in this series (and the one that you'll fix as well).

Cheers!
Angelo

> ---
> Nícolas F. R. A. Prado (9):
> drm/bridge: anx7625: Don't log an error when DSI host can't be found
> drm/bridge: icn6211: Don't log an error when DSI host can't be found
> drm/bridge: lt8912b: Don't log an error when DSI host can't be found
> drm/bridge: lt9611: Don't log an error when DSI host can't be found
> drm/bridge: lt9611uxc: Don't log an error when DSI host can't be found
> drm/bridge: tc358775: Don't log an error when DSI host can't be found
> drm/bridge: dpc3433: Don't log an error when DSI host can't be found
> drm/panel: novatek-nt35950: Don't log an error when DSI host can't be found
> drm/panel: truly-nt35597: Don't log an error when DSI host can't be found
>
> drivers/gpu/drm/bridge/analogix/anx7625.c | 6 ++----
> drivers/gpu/drm/bridge/chipone-icn6211.c | 6 ++----
> drivers/gpu/drm/bridge/lontium-lt8912b.c | 6 ++----
> drivers/gpu/drm/bridge/lontium-lt9611.c | 6 ++----
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 6 ++----
> drivers/gpu/drm/bridge/tc358775.c | 6 ++----
> drivers/gpu/drm/bridge/ti-dlpc3433.c | 17 +++++++++--------
> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 6 ++----
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++----
> 9 files changed, 25 insertions(+), 40 deletions(-)
> ---
> base-commit: 2ae0a045e6814c8c1d676d6153c605a65746aa29
> change-id: 20240226-anx7625-defer-log-no-dsi-host-c3f9ccbcb287
>
> Best regards,

2024-03-08 15:06:26