[PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Jani Nikula]

The expectation is that the struct drm_device based logging helpers get
passed an actual struct drm_device pointer rather than some random
struct pointer where you can dereference the ->dev member.

Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
current usage, but drops "[drm] *ERROR*" prefix from logging.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Looks like it's possible to hunt down the struct drm_device in most of
these cases, if that's desired. This was the simplest change.

Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: Andy Yan <andy.yan@rock-chips.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index e7a6669c46b0..f737e7d46e66 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 
@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->ref_clk)) {
 		ret = PTR_ERR(hdmi->ref_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get reference clock\n");
+			dev_err(hdmi->dev, "failed to get reference clock\n");
 		return ret;
 	}
 
@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 	if (IS_ERR(hdmi->grf_clk)) {
 		ret = PTR_ERR(hdmi->grf_clk);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get grf clock\n");
+			dev_err(hdmi->dev, "failed to get grf clock\n");
 		return ret;
 	}
 
@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 
 	ret = clk_prepare_enable(hdmi->grf_clk);
 	if (ret < 0) {
-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
 
 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
 	if (ret != 0)
-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
 
 	clk_disable_unprepare(hdmi->grf_clk);
-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
 }
 
 static int
@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	ret = rockchip_hdmi_parse_dt(hdmi);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "Unable to parse OF data\n");
+			dev_err(hdmi->dev, "Unable to parse OF data\n");
 		return ret;
 	}
 
@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy\n");
+			dev_err(hdmi->dev, "failed to get phy\n");
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index f41151d49fca..3d1dddb34603 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 	if (drm) {
 		changed = drm_helper_hpd_irq_event(drm);
 		if (changed)
-			drm_dbg(hdmi, "connector status changed\n");
+			dev_dbg(hdmi->dev, "connector status changed\n");
 	}
 }
 
@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0) {
-		drm_err(hdmi, "Failed to match HDMI port ID\n");
+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
 		return hdmi->port_id;
 	}
 
@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
 		return PTR_ERR(hdmi->regmap);
 	}
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap)) {
-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
 		return PTR_ERR(hdmi->vo_regmap);
 	}
 
 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
 	if (ret < 0) {
-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
 		return ret;
 	}
 
@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hdmi->enable_gpio)) {
 		ret = PTR_ERR(hdmi->enable_gpio);
-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
 		return ret;
 	}
 
@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
 		if (ret != -EPROBE_DEFER)
-			drm_err(hdmi, "failed to get phy: %d\n", ret);
+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
 		return ret;
 	}
 
@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector)) {
 		ret = PTR_ERR(connector);
-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
 		return ret;
 	}
 
-- 
2.39.5

Re:[PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Andy Yan]

Hi，

At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@intel.com> wrote:
>The expectation is that the struct drm_device based logging helpers get
>passed an actual struct drm_device pointer rather than some random
>struct pointer where you can dereference the ->dev member.
>
>Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
>current usage, but drops "[drm] *ERROR*" prefix from logging.

Frankly, I prefer the original version of the log.
It is a platform driver, so it should use its own device.
It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging

>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
>---
>
>Looks like it's possible to hunt down the struct drm_device in most of
>these cases, if that's desired. This was the simplest change.
>
>Cc: Sandy Huang <hjc@rock-chips.com>
>Cc: "Heiko Stübner" <heiko@sntech.de>
>Cc: Andy Yan <andy.yan@rock-chips.com>
>Cc: dri-devel@lists.freedesktop.org
>Cc: linux-arm-kernel@lists.infradead.org
>Cc: linux-rockchip@lists.infradead.org
>---
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>index e7a6669c46b0..f737e7d46e66 100644
>--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
> 
> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> 	if (IS_ERR(hdmi->regmap)) {
>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
> 		return PTR_ERR(hdmi->regmap);
> 	}
> 
>@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
> 	if (IS_ERR(hdmi->ref_clk)) {
> 		ret = PTR_ERR(hdmi->ref_clk);
> 		if (ret != -EPROBE_DEFER)
>-			drm_err(hdmi, "failed to get reference clock\n");
>+			dev_err(hdmi->dev, "failed to get reference clock\n");
> 		return ret;
> 	}
> 
>@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
> 	if (IS_ERR(hdmi->grf_clk)) {
> 		ret = PTR_ERR(hdmi->grf_clk);
> 		if (ret != -EPROBE_DEFER)
>-			drm_err(hdmi, "failed to get grf clock\n");
>+			dev_err(hdmi->dev, "failed to get grf clock\n");
> 		return ret;
> 	}
> 
>@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
> 
> 	ret = clk_prepare_enable(hdmi->grf_clk);
> 	if (ret < 0) {
>-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
>+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
> 		return;
> 	}
> 
> 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
> 	if (ret != 0)
>-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
>+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
> 
> 	clk_disable_unprepare(hdmi->grf_clk);
>-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
> }
> 
> static int
>@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
> 	ret = rockchip_hdmi_parse_dt(hdmi);
> 	if (ret) {
> 		if (ret != -EPROBE_DEFER)
>-			drm_err(hdmi, "Unable to parse OF data\n");
>+			dev_err(hdmi->dev, "Unable to parse OF data\n");
> 		return ret;
> 	}
> 
>@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
> 	if (IS_ERR(hdmi->phy)) {
> 		ret = PTR_ERR(hdmi->phy);
> 		if (ret != -EPROBE_DEFER)
>-			drm_err(hdmi, "failed to get phy\n");
>+			dev_err(hdmi->dev, "failed to get phy\n");
> 		return ret;
> 	}
> 
>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>index f41151d49fca..3d1dddb34603 100644
>--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
> 	if (drm) {
> 		changed = drm_helper_hpd_irq_event(drm);
> 		if (changed)
>-			drm_dbg(hdmi, "connector status changed\n");
>+			dev_dbg(hdmi->dev, "connector status changed\n");
> 	}
> }
> 
>@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> 		}
> 	}
> 	if (hdmi->port_id < 0) {
>-		drm_err(hdmi, "Failed to match HDMI port ID\n");
>+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
> 		return hdmi->port_id;
> 	}
> 
>@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
> 						       "rockchip,grf");
> 	if (IS_ERR(hdmi->regmap)) {
>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
> 		return PTR_ERR(hdmi->regmap);
> 	}
> 
> 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
> 							  "rockchip,vo-grf");
> 	if (IS_ERR(hdmi->vo_regmap)) {
>-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
>+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
> 		return PTR_ERR(hdmi->vo_regmap);
> 	}
> 
> 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
> 	if (ret < 0) {
>-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
>+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
> 		return ret;
> 	}
> 
>@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> 						    GPIOD_OUT_HIGH);
> 	if (IS_ERR(hdmi->enable_gpio)) {
> 		ret = PTR_ERR(hdmi->enable_gpio);
>-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
>+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
> 		return ret;
> 	}
> 
>@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> 	if (IS_ERR(hdmi->phy)) {
> 		ret = PTR_ERR(hdmi->phy);
> 		if (ret != -EPROBE_DEFER)
>-			drm_err(hdmi, "failed to get phy: %d\n", ret);
>+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
> 		return ret;
> 	}
> 
>@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
> 	connector = drm_bridge_connector_init(drm, encoder);
> 	if (IS_ERR(connector)) {
> 		ret = PTR_ERR(connector);
>-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
>+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
> 		return ret;
> 	}
> 
>-- 
>2.39.5
>
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re:[PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Jani Nikula]

On Fri, 24 Jan 2025, "Andy Yan" <andyshrk@163.com> wrote:
> Hi，
>
> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@intel.com> wrote:
>>The expectation is that the struct drm_device based logging helpers get
>>passed an actual struct drm_device pointer rather than some random
>>struct pointer where you can dereference the ->dev member.
>>
>>Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
>>current usage, but drops "[drm] *ERROR*" prefix from logging.
>
> Frankly, I prefer the original version of the log.
> It is a platform driver, so it should use its own device.
> It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging

If you need to do struct device based logging that is not the same
device as the struct drm_device dev member, you need to use dev_err()
and friends. You can't and must not use drm_err() and friends.

It's as simple as that.

The current drm_err(hdmi, ...) usage is simply abuse of the macros, and
must stop.


BR,
Jani.


>
>>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>
>>---
>>
>>Looks like it's possible to hunt down the struct drm_device in most of
>>these cases, if that's desired. This was the simplest change.
>>
>>Cc: Sandy Huang <hjc@rock-chips.com>
>>Cc: "Heiko Stübner" <heiko@sntech.de>
>>Cc: Andy Yan <andy.yan@rock-chips.com>
>>Cc: dri-devel@lists.freedesktop.org
>>Cc: linux-arm-kernel@lists.infradead.org
>>Cc: linux-rockchip@lists.infradead.org
>>---
>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
>> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
>> 2 files changed, 16 insertions(+), 16 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>index e7a6669c46b0..f737e7d46e66 100644
>>--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>> 
>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>> 	if (IS_ERR(hdmi->regmap)) {
>>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>> 		return PTR_ERR(hdmi->regmap);
>> 	}
>> 
>>@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>> 	if (IS_ERR(hdmi->ref_clk)) {
>> 		ret = PTR_ERR(hdmi->ref_clk);
>> 		if (ret != -EPROBE_DEFER)
>>-			drm_err(hdmi, "failed to get reference clock\n");
>>+			dev_err(hdmi->dev, "failed to get reference clock\n");
>> 		return ret;
>> 	}
>> 
>>@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>> 	if (IS_ERR(hdmi->grf_clk)) {
>> 		ret = PTR_ERR(hdmi->grf_clk);
>> 		if (ret != -EPROBE_DEFER)
>>-			drm_err(hdmi, "failed to get grf clock\n");
>>+			dev_err(hdmi->dev, "failed to get grf clock\n");
>> 		return ret;
>> 	}
>> 
>>@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>> 
>> 	ret = clk_prepare_enable(hdmi->grf_clk);
>> 	if (ret < 0) {
>>-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
>>+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
>> 		return;
>> 	}
>> 
>> 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
>> 	if (ret != 0)
>>-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
>>+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
>> 
>> 	clk_disable_unprepare(hdmi->grf_clk);
>>-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>> }
>> 
>> static int
>>@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>> 	ret = rockchip_hdmi_parse_dt(hdmi);
>> 	if (ret) {
>> 		if (ret != -EPROBE_DEFER)
>>-			drm_err(hdmi, "Unable to parse OF data\n");
>>+			dev_err(hdmi->dev, "Unable to parse OF data\n");
>> 		return ret;
>> 	}
>> 
>>@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>> 	if (IS_ERR(hdmi->phy)) {
>> 		ret = PTR_ERR(hdmi->phy);
>> 		if (ret != -EPROBE_DEFER)
>>-			drm_err(hdmi, "failed to get phy\n");
>>+			dev_err(hdmi->dev, "failed to get phy\n");
>> 		return ret;
>> 	}
>> 
>>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>index f41151d49fca..3d1dddb34603 100644
>>--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
>> 	if (drm) {
>> 		changed = drm_helper_hpd_irq_event(drm);
>> 		if (changed)
>>-			drm_dbg(hdmi, "connector status changed\n");
>>+			dev_dbg(hdmi->dev, "connector status changed\n");
>> 	}
>> }
>> 
>>@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>> 		}
>> 	}
>> 	if (hdmi->port_id < 0) {
>>-		drm_err(hdmi, "Failed to match HDMI port ID\n");
>>+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
>> 		return hdmi->port_id;
>> 	}
>> 
>>@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>> 						       "rockchip,grf");
>> 	if (IS_ERR(hdmi->regmap)) {
>>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>> 		return PTR_ERR(hdmi->regmap);
>> 	}
>> 
>> 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>> 							  "rockchip,vo-grf");
>> 	if (IS_ERR(hdmi->vo_regmap)) {
>>-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
>>+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
>> 		return PTR_ERR(hdmi->vo_regmap);
>> 	}
>> 
>> 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
>> 	if (ret < 0) {
>>-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
>>+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
>> 		return ret;
>> 	}
>> 
>>@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>> 						    GPIOD_OUT_HIGH);
>> 	if (IS_ERR(hdmi->enable_gpio)) {
>> 		ret = PTR_ERR(hdmi->enable_gpio);
>>-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
>>+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
>> 		return ret;
>> 	}
>> 
>>@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>> 	if (IS_ERR(hdmi->phy)) {
>> 		ret = PTR_ERR(hdmi->phy);
>> 		if (ret != -EPROBE_DEFER)
>>-			drm_err(hdmi, "failed to get phy: %d\n", ret);
>>+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
>> 		return ret;
>> 	}
>> 
>>@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>> 	connector = drm_bridge_connector_init(drm, encoder);
>> 	if (IS_ERR(connector)) {
>> 		ret = PTR_ERR(connector);
>>-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
>>+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
>> 		return ret;
>> 	}
>> 
>>-- 
>>2.39.5
>>
>>
>>_______________________________________________
>>Linux-rockchip mailing list
>>Linux-rockchip@lists.infradead.org
>>http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
Jani Nikula, Intel

Re:Re:[PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Andy Yan]

在 2025-01-24 19:43:07，"Jani Nikula" <jani.nikula@intel.com> 写道：
>On Fri, 24 Jan 2025, "Andy Yan" <andyshrk@163.com> wrote:
>> Hi，
>>
>> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@intel.com> wrote:
>>>The expectation is that the struct drm_device based logging helpers get
>>>passed an actual struct drm_device pointer rather than some random
>>>struct pointer where you can dereference the ->dev member.
>>>
>>>Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
>>>current usage, but drops "[drm] *ERROR*" prefix from logging.
>>
>> Frankly, I prefer the original version of the log.
>> It is a platform driver, so it should use its own device.
>> It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging
>
>If you need to do struct device based logging that is not the same
>device as the struct drm_device dev member, you need to use dev_err()
>and friends. You can't and must not use drm_err() and friends.
>
>It's as simple as that.
>
>The current drm_err(hdmi, ...) usage is simply abuse of the macros, and
>must stop.

Perhaps when you initially designed this macros, you intended it to accept only drm_device, 
but your code implementation didn't enforce this restriction at the beginning.
If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0], 
as neither drm_err nor dev_err can maintain consistency with the original log of this driver.
Alternatively, as suggested by Sam  in the initial submission of your patch 5 years ago, 
there should also be a macro similar to drm_dev_info(device *, ..).[1]


[0]https://lore.kernel.org/linux-rockchip/20240813-dw-hdmi-rockchip-cleanup-v1-1-b3e73b5f4fd6@collabora.com/
[1]https://lore.kernel.org/dri-devel/20191212215303.GA11520@ravnborg.org/

>
>
>BR,
>Jani.
>
>
>>
>>>
>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>
>>>---
>>>
>>>Looks like it's possible to hunt down the struct drm_device in most of
>>>these cases, if that's desired. This was the simplest change.
>>>
>>>Cc: Sandy Huang <hjc@rock-chips.com>
>>>Cc: "Heiko Stübner" <heiko@sntech.de>
>>>Cc: Andy Yan <andy.yan@rock-chips.com>
>>>Cc: dri-devel@lists.freedesktop.org
>>>Cc: linux-arm-kernel@lists.infradead.org
>>>Cc: linux-rockchip@lists.infradead.org
>>>---
>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
>>> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
>>> 2 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>index e7a6669c46b0..f737e7d46e66 100644
>>>--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>> 
>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>>> 	if (IS_ERR(hdmi->regmap)) {
>>>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>> 		return PTR_ERR(hdmi->regmap);
>>> 	}
>>> 
>>>@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>> 	if (IS_ERR(hdmi->ref_clk)) {
>>> 		ret = PTR_ERR(hdmi->ref_clk);
>>> 		if (ret != -EPROBE_DEFER)
>>>-			drm_err(hdmi, "failed to get reference clock\n");
>>>+			dev_err(hdmi->dev, "failed to get reference clock\n");
>>> 		return ret;
>>> 	}
>>> 
>>>@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>> 	if (IS_ERR(hdmi->grf_clk)) {
>>> 		ret = PTR_ERR(hdmi->grf_clk);
>>> 		if (ret != -EPROBE_DEFER)
>>>-			drm_err(hdmi, "failed to get grf clock\n");
>>>+			dev_err(hdmi->dev, "failed to get grf clock\n");
>>> 		return ret;
>>> 	}
>>> 
>>>@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>>> 
>>> 	ret = clk_prepare_enable(hdmi->grf_clk);
>>> 	if (ret < 0) {
>>>-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
>>>+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
>>> 		return;
>>> 	}
>>> 
>>> 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
>>> 	if (ret != 0)
>>>-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
>>>+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
>>> 
>>> 	clk_disable_unprepare(hdmi->grf_clk);
>>>-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>>+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>> }
>>> 
>>> static int
>>>@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>> 	ret = rockchip_hdmi_parse_dt(hdmi);
>>> 	if (ret) {
>>> 		if (ret != -EPROBE_DEFER)
>>>-			drm_err(hdmi, "Unable to parse OF data\n");
>>>+			dev_err(hdmi->dev, "Unable to parse OF data\n");
>>> 		return ret;
>>> 	}
>>> 
>>>@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>> 	if (IS_ERR(hdmi->phy)) {
>>> 		ret = PTR_ERR(hdmi->phy);
>>> 		if (ret != -EPROBE_DEFER)
>>>-			drm_err(hdmi, "failed to get phy\n");
>>>+			dev_err(hdmi->dev, "failed to get phy\n");
>>> 		return ret;
>>> 	}
>>> 
>>>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>index f41151d49fca..3d1dddb34603 100644
>>>--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
>>> 	if (drm) {
>>> 		changed = drm_helper_hpd_irq_event(drm);
>>> 		if (changed)
>>>-			drm_dbg(hdmi, "connector status changed\n");
>>>+			dev_dbg(hdmi->dev, "connector status changed\n");
>>> 	}
>>> }
>>> 
>>>@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>> 		}
>>> 	}
>>> 	if (hdmi->port_id < 0) {
>>>-		drm_err(hdmi, "Failed to match HDMI port ID\n");
>>>+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
>>> 		return hdmi->port_id;
>>> 	}
>>> 
>>>@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> 						       "rockchip,grf");
>>> 	if (IS_ERR(hdmi->regmap)) {
>>>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>> 		return PTR_ERR(hdmi->regmap);
>>> 	}
>>> 
>>> 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> 							  "rockchip,vo-grf");
>>> 	if (IS_ERR(hdmi->vo_regmap)) {
>>>-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
>>>+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
>>> 		return PTR_ERR(hdmi->vo_regmap);
>>> 	}
>>> 
>>> 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
>>> 	if (ret < 0) {
>>>-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
>>>+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
>>> 		return ret;
>>> 	}
>>> 
>>>@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>> 						    GPIOD_OUT_HIGH);
>>> 	if (IS_ERR(hdmi->enable_gpio)) {
>>> 		ret = PTR_ERR(hdmi->enable_gpio);
>>>-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
>>>+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
>>> 		return ret;
>>> 	}
>>> 
>>>@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>> 	if (IS_ERR(hdmi->phy)) {
>>> 		ret = PTR_ERR(hdmi->phy);
>>> 		if (ret != -EPROBE_DEFER)
>>>-			drm_err(hdmi, "failed to get phy: %d\n", ret);
>>>+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
>>> 		return ret;
>>> 	}
>>> 
>>>@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>> 	connector = drm_bridge_connector_init(drm, encoder);
>>> 	if (IS_ERR(connector)) {
>>> 		ret = PTR_ERR(connector);
>>>-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
>>>+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
>>> 		return ret;
>>> 	}
>>> 
>>>-- 
>>>2.39.5
>>>
>>>
>>>_______________________________________________
>>>Linux-rockchip mailing list
>>>Linux-rockchip@lists.infradead.org
>>>http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>-- 
>Jani Nikula, Intel

Re:Re:[PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Jani Nikula]

On Sat, 25 Jan 2025, "Andy Yan" <andyshrk@163.com> wrote:
> 在 2025-01-24 19:43:07，"Jani Nikula" <jani.nikula@intel.com> 写道：
>>On Fri, 24 Jan 2025, "Andy Yan" <andyshrk@163.com> wrote:
>>> Hi，
>>>
>>> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@intel.com> wrote:
>>>>The expectation is that the struct drm_device based logging helpers get
>>>>passed an actual struct drm_device pointer rather than some random
>>>>struct pointer where you can dereference the ->dev member.
>>>>
>>>>Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
>>>>current usage, but drops "[drm] *ERROR*" prefix from logging.
>>>
>>> Frankly, I prefer the original version of the log.
>>> It is a platform driver, so it should use its own device.
>>> It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging
>>
>>If you need to do struct device based logging that is not the same
>>device as the struct drm_device dev member, you need to use dev_err()
>>and friends. You can't and must not use drm_err() and friends.
>>
>>It's as simple as that.
>>
>>The current drm_err(hdmi, ...) usage is simply abuse of the macros, and
>>must stop.
>
> Perhaps when you initially designed this macros, you intended it to accept only drm_device, 
> but your code implementation didn't enforce this restriction at the beginning.
> If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0], 
> as neither drm_err nor dev_err can maintain consistency with the original log of this driver.
> Alternatively, as suggested by Sam  in the initial submission of your patch 5 years ago, 
> there should also be a macro similar to drm_dev_info(device *, ..).[1]

Commit 1b8f576c6958 ("drm/rockchip: dw_hdmi: Use modern drm_device based
logging") does not revert cleanly, and even if it did, DRM_DEV_ERROR()
is deprecated in favour of drm_err() or dev_err(). I'm using the latter.

Ack for applying the patch at hand as-is?

BR,
Jani.




>
>
> [0]https://lore.kernel.org/linux-rockchip/20240813-dw-hdmi-rockchip-cleanup-v1-1-b3e73b5f4fd6@collabora.com/
> [1]https://lore.kernel.org/dri-devel/20191212215303.GA11520@ravnborg.org/
>
>>
>>
>>BR,
>>Jani.
>>
>>
>>>
>>>>
>>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>
>>>>---
>>>>
>>>>Looks like it's possible to hunt down the struct drm_device in most of
>>>>these cases, if that's desired. This was the simplest change.
>>>>
>>>>Cc: Sandy Huang <hjc@rock-chips.com>
>>>>Cc: "Heiko Stübner" <heiko@sntech.de>
>>>>Cc: Andy Yan <andy.yan@rock-chips.com>
>>>>Cc: dri-devel@lists.freedesktop.org
>>>>Cc: linux-arm-kernel@lists.infradead.org
>>>>Cc: linux-rockchip@lists.infradead.org
>>>>---
>>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
>>>> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
>>>> 2 files changed, 16 insertions(+), 16 deletions(-)
>>>>
>>>>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>index e7a6669c46b0..f737e7d46e66 100644
>>>>--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>@@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>> 
>>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>>>> 	if (IS_ERR(hdmi->regmap)) {
>>>>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>>> 		return PTR_ERR(hdmi->regmap);
>>>> 	}
>>>> 
>>>>@@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>> 	if (IS_ERR(hdmi->ref_clk)) {
>>>> 		ret = PTR_ERR(hdmi->ref_clk);
>>>> 		if (ret != -EPROBE_DEFER)
>>>>-			drm_err(hdmi, "failed to get reference clock\n");
>>>>+			dev_err(hdmi->dev, "failed to get reference clock\n");
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>@@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>> 	if (IS_ERR(hdmi->grf_clk)) {
>>>> 		ret = PTR_ERR(hdmi->grf_clk);
>>>> 		if (ret != -EPROBE_DEFER)
>>>>-			drm_err(hdmi, "failed to get grf clock\n");
>>>>+			dev_err(hdmi->dev, "failed to get grf clock\n");
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>@@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>>>> 
>>>> 	ret = clk_prepare_enable(hdmi->grf_clk);
>>>> 	if (ret < 0) {
>>>>-		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
>>>>+		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
>>>> 		return;
>>>> 	}
>>>> 
>>>> 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
>>>> 	if (ret != 0)
>>>>-		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
>>>>+		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
>>>> 
>>>> 	clk_disable_unprepare(hdmi->grf_clk);
>>>>-	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>>>+	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>>> }
>>>> 
>>>> static int
>>>>@@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>>> 	ret = rockchip_hdmi_parse_dt(hdmi);
>>>> 	if (ret) {
>>>> 		if (ret != -EPROBE_DEFER)
>>>>-			drm_err(hdmi, "Unable to parse OF data\n");
>>>>+			dev_err(hdmi->dev, "Unable to parse OF data\n");
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>@@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>>> 	if (IS_ERR(hdmi->phy)) {
>>>> 		ret = PTR_ERR(hdmi->phy);
>>>> 		if (ret != -EPROBE_DEFER)
>>>>-			drm_err(hdmi, "failed to get phy\n");
>>>>+			dev_err(hdmi->dev, "failed to get phy\n");
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>>index f41151d49fca..3d1dddb34603 100644
>>>>--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>>+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>>@@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
>>>> 	if (drm) {
>>>> 		changed = drm_helper_hpd_irq_event(drm);
>>>> 		if (changed)
>>>>-			drm_dbg(hdmi, "connector status changed\n");
>>>>+			dev_dbg(hdmi->dev, "connector status changed\n");
>>>> 	}
>>>> }
>>>> 
>>>>@@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 		}
>>>> 	}
>>>> 	if (hdmi->port_id < 0) {
>>>>-		drm_err(hdmi, "Failed to match HDMI port ID\n");
>>>>+		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
>>>> 		return hdmi->port_id;
>>>> 	}
>>>> 
>>>>@@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>> 						       "rockchip,grf");
>>>> 	if (IS_ERR(hdmi->regmap)) {
>>>>-		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>>+		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>>> 		return PTR_ERR(hdmi->regmap);
>>>> 	}
>>>> 
>>>> 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>> 							  "rockchip,vo-grf");
>>>> 	if (IS_ERR(hdmi->vo_regmap)) {
>>>>-		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
>>>>+		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
>>>> 		return PTR_ERR(hdmi->vo_regmap);
>>>> 	}
>>>> 
>>>> 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
>>>> 	if (ret < 0) {
>>>>-		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
>>>>+		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>@@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 						    GPIOD_OUT_HIGH);
>>>> 	if (IS_ERR(hdmi->enable_gpio)) {
>>>> 		ret = PTR_ERR(hdmi->enable_gpio);
>>>>-		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
>>>>+		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>@@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 	if (IS_ERR(hdmi->phy)) {
>>>> 		ret = PTR_ERR(hdmi->phy);
>>>> 		if (ret != -EPROBE_DEFER)
>>>>-			drm_err(hdmi, "failed to get phy: %d\n", ret);
>>>>+			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>@@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 	connector = drm_bridge_connector_init(drm, encoder);
>>>> 	if (IS_ERR(connector)) {
>>>> 		ret = PTR_ERR(connector);
>>>>-		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
>>>>+		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>> 
>>>>-- 
>>>>2.39.5
>>>>
>>>>
>>>>_______________________________________________
>>>>Linux-rockchip mailing list
>>>>Linux-rockchip@lists.infradead.org
>>>>http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>-- 
>>Jani Nikula, Intel

-- 
Jani Nikula, Intel

Re: [PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Thomas Zimmermann]

Hi

Am 25.01.25 um 04:53 schrieb Andy Yan:
>
> 在 2025-01-24 19:43:07，"Jani Nikula" <jani.nikula@intel.com> 写道：
>> On Fri, 24 Jan 2025, "Andy Yan" <andyshrk@163.com> wrote:
>>> Hi，
>>>
>>> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@intel.com> wrote:
>>>> The expectation is that the struct drm_device based logging helpers get
>>>> passed an actual struct drm_device pointer rather than some random
>>>> struct pointer where you can dereference the ->dev member.
>>>>
>>>> Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
>>>> current usage, but drops "[drm] *ERROR*" prefix from logging.
>>> Frankly, I prefer the original version of the log.
>>> It is a platform driver, so it should use its own device.
>>> It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging
>> If you need to do struct device based logging that is not the same
>> device as the struct drm_device dev member, you need to use dev_err()
>> and friends. You can't and must not use drm_err() and friends.
>>
>> It's as simple as that.
>>
>> The current drm_err(hdmi, ...) usage is simply abuse of the macros, and
>> must stop.
> Perhaps when you initially designed this macros, you intended it to accept only drm_device,
> but your code implementation didn't enforce this restriction at the beginning.

C'mon. Are we really arguing about type safety now?

Patch 5 adds a little getter function that does the type check on the 
function call's argument.


> If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0],
> as neither drm_err nor dev_err can maintain consistency with the original log of this driver.
> Alternatively, as suggested by Sam  in the initial submission of your patch 5 years ago,
> there should also be a macro similar to drm_dev_info(device *, ..).[1]

DRM code tends to keep a reference to the drm_device somewhere and 
fetches it if necessary. For this patch, it should be possible to fetch 
the DRM device from struct rockchip_hdmi easily. Just do

   drm_err(rockchip_hdmi_drm_dev(hdmi), "...");

This would resolve the problem without new logging functions and keep 
the "[drm]" prefix to the messages.

Best regards
Thomas


>
>
> [0]https://lore.kernel.org/linux-rockchip/20240813-dw-hdmi-rockchip-cleanup-v1-1-b3e73b5f4fd6@collabora.com/
> [1]https://lore.kernel.org/dri-devel/20191212215303.GA11520@ravnborg.org/
>
>>
>> BR,
>> Jani.
>>
>>
>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>
>>>> ---
>>>>
>>>> Looks like it's possible to hunt down the struct drm_device in most of
>>>> these cases, if that's desired. This was the simplest change.
>>>>
>>>> Cc: Sandy Huang <hjc@rock-chips.com>
>>>> Cc: "Heiko Stübner" <heiko@sntech.de>
>>>> Cc: Andy Yan <andy.yan@rock-chips.com>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-rockchip@lists.infradead.org
>>>> ---
>>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
>>>> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
>>>> 2 files changed, 16 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>> index e7a6669c46b0..f737e7d46e66 100644
>>>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>> @@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>>
>>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>>>> 	if (IS_ERR(hdmi->regmap)) {
>>>> -		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>> +		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>>> 		return PTR_ERR(hdmi->regmap);
>>>> 	}
>>>>
>>>> @@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>> 	if (IS_ERR(hdmi->ref_clk)) {
>>>> 		ret = PTR_ERR(hdmi->ref_clk);
>>>> 		if (ret != -EPROBE_DEFER)
>>>> -			drm_err(hdmi, "failed to get reference clock\n");
>>>> +			dev_err(hdmi->dev, "failed to get reference clock\n");
>>>> 		return ret;
>>>> 	}
>>>>
>>>> @@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>> 	if (IS_ERR(hdmi->grf_clk)) {
>>>> 		ret = PTR_ERR(hdmi->grf_clk);
>>>> 		if (ret != -EPROBE_DEFER)
>>>> -			drm_err(hdmi, "failed to get grf clock\n");
>>>> +			dev_err(hdmi->dev, "failed to get grf clock\n");
>>>> 		return ret;
>>>> 	}
>>>>
>>>> @@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>>>>
>>>> 	ret = clk_prepare_enable(hdmi->grf_clk);
>>>> 	if (ret < 0) {
>>>> -		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
>>>> +		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
>>>> 		return;
>>>> 	}
>>>>
>>>> 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
>>>> 	if (ret != 0)
>>>> -		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
>>>> +		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
>>>>
>>>> 	clk_disable_unprepare(hdmi->grf_clk);
>>>> -	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>>> +	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>>> }
>>>>
>>>> static int
>>>> @@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>>> 	ret = rockchip_hdmi_parse_dt(hdmi);
>>>> 	if (ret) {
>>>> 		if (ret != -EPROBE_DEFER)
>>>> -			drm_err(hdmi, "Unable to parse OF data\n");
>>>> +			dev_err(hdmi->dev, "Unable to parse OF data\n");
>>>> 		return ret;
>>>> 	}
>>>>
>>>> @@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>>> 	if (IS_ERR(hdmi->phy)) {
>>>> 		ret = PTR_ERR(hdmi->phy);
>>>> 		if (ret != -EPROBE_DEFER)
>>>> -			drm_err(hdmi, "failed to get phy\n");
>>>> +			dev_err(hdmi->dev, "failed to get phy\n");
>>>> 		return ret;
>>>> 	}
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>> index f41151d49fca..3d1dddb34603 100644
>>>> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>> @@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
>>>> 	if (drm) {
>>>> 		changed = drm_helper_hpd_irq_event(drm);
>>>> 		if (changed)
>>>> -			drm_dbg(hdmi, "connector status changed\n");
>>>> +			dev_dbg(hdmi->dev, "connector status changed\n");
>>>> 	}
>>>> }
>>>>
>>>> @@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 		}
>>>> 	}
>>>> 	if (hdmi->port_id < 0) {
>>>> -		drm_err(hdmi, "Failed to match HDMI port ID\n");
>>>> +		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
>>>> 		return hdmi->port_id;
>>>> 	}
>>>>
>>>> @@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>> 						       "rockchip,grf");
>>>> 	if (IS_ERR(hdmi->regmap)) {
>>>> -		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>> +		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>>> 		return PTR_ERR(hdmi->regmap);
>>>> 	}
>>>>
>>>> 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>> 							  "rockchip,vo-grf");
>>>> 	if (IS_ERR(hdmi->vo_regmap)) {
>>>> -		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
>>>> +		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
>>>> 		return PTR_ERR(hdmi->vo_regmap);
>>>> 	}
>>>>
>>>> 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
>>>> 	if (ret < 0) {
>>>> -		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
>>>> +		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>>
>>>> @@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 						    GPIOD_OUT_HIGH);
>>>> 	if (IS_ERR(hdmi->enable_gpio)) {
>>>> 		ret = PTR_ERR(hdmi->enable_gpio);
>>>> -		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
>>>> +		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>>
>>>> @@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 	if (IS_ERR(hdmi->phy)) {
>>>> 		ret = PTR_ERR(hdmi->phy);
>>>> 		if (ret != -EPROBE_DEFER)
>>>> -			drm_err(hdmi, "failed to get phy: %d\n", ret);
>>>> +			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>>
>>>> @@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>> 	connector = drm_bridge_connector_init(drm, encoder);
>>>> 	if (IS_ERR(connector)) {
>>>> 		ret = PTR_ERR(connector);
>>>> -		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
>>>> +		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
>>>> 		return ret;
>>>> 	}
>>>>
>>>> -- 
>>>> 2.39.5
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-rockchip mailing list
>>>> Linux-rockchip@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>> -- 
>> Jani Nikula, Intel

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re:Re: [PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Andy Yan]

Hi，

在 2025-02-26 16:33:21，"Thomas Zimmermann" <tzimmermann@suse.de> 写道：
>Hi
>
>Am 25.01.25 um 04:53 schrieb Andy Yan:
>>
>> 在 2025-01-24 19:43:07，"Jani Nikula" <jani.nikula@intel.com> 写道：
>>> On Fri, 24 Jan 2025, "Andy Yan" <andyshrk@163.com> wrote:
>>>> Hi，
>>>>
>>>> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@intel.com> wrote:
>>>>> The expectation is that the struct drm_device based logging helpers get
>>>>> passed an actual struct drm_device pointer rather than some random
>>>>> struct pointer where you can dereference the ->dev member.
>>>>>
>>>>> Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
>>>>> current usage, but drops "[drm] *ERROR*" prefix from logging.
>>>> Frankly, I prefer the original version of the log.
>>>> It is a platform driver, so it should use its own device.
>>>> It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging
>>> If you need to do struct device based logging that is not the same
>>> device as the struct drm_device dev member, you need to use dev_err()
>>> and friends. You can't and must not use drm_err() and friends.
>>>
>>> It's as simple as that.
>>>
>>> The current drm_err(hdmi, ...) usage is simply abuse of the macros, and
>>> must stop.
>> Perhaps when you initially designed this macros, you intended it to accept only drm_device,
>> but your code implementation didn't enforce this restriction at the beginning.
>
>C'mon. Are we really arguing about type safety now?
>
>Patch 5 adds a little getter function that does the type check on the 
>function call's argument.
>
>
>> If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0],
>> as neither drm_err nor dev_err can maintain consistency with the original log of this driver.
>> Alternatively, as suggested by Sam  in the initial submission of your patch 5 years ago,
>> there should also be a macro similar to drm_dev_info(device *, ..).[1]
>
>DRM code tends to keep a reference to the drm_device somewhere and 
>fetches it if necessary. For this patch, it should be possible to fetch 
>the DRM device from struct rockchip_hdmi easily. Just do
>
>   drm_err(rockchip_hdmi_drm_dev(hdmi), "...");
>
>This would resolve the problem without new logging functions and keep 
>the "[drm]" prefix to the messages.

Yes, this keeps the "[drm]" prefix to the log messages, but it also changed hdmi
device from drm device in the log messages.
For more efficient debugging, it is preferable for log entries to explicitly specify which device generated them,
especially in DRM systems where multiple devices(bridge/encoder) may be present."

And I'm ok with this PATCH. However, I would also like to know your and Jani's opinions on whether we can consider
adding an API similar to drm_dev_info，as Sam suggested before. Of course, this could be left for future implementation
by others.



>
>Best regards
>Thomas
>
>
>>
>>
>> [0]https://lore.kernel.org/linux-rockchip/20240813-dw-hdmi-rockchip-cleanup-v1-1-b3e73b5f4fd6@collabora.com/
>> [1]https://lore.kernel.org/dri-devel/20191212215303.GA11520@ravnborg.org/
>>
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Looks like it's possible to hunt down the struct drm_device in most of
>>>>> these cases, if that's desired. This was the simplest change.
>>>>>
>>>>> Cc: Sandy Huang <hjc@rock-chips.com>
>>>>> Cc: "Heiko Stübner" <heiko@sntech.de>
>>>>> Cc: Andy Yan <andy.yan@rock-chips.com>
>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-rockchip@lists.infradead.org
>>>>> ---
>>>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
>>>>> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
>>>>> 2 files changed, 16 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>> index e7a6669c46b0..f737e7d46e66 100644
>>>>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>>>> @@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>>>
>>>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>>>>> 	if (IS_ERR(hdmi->regmap)) {
>>>>> -		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>>> +		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>>>> 		return PTR_ERR(hdmi->regmap);
>>>>> 	}
>>>>>
>>>>> @@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>>> 	if (IS_ERR(hdmi->ref_clk)) {
>>>>> 		ret = PTR_ERR(hdmi->ref_clk);
>>>>> 		if (ret != -EPROBE_DEFER)
>>>>> -			drm_err(hdmi, "failed to get reference clock\n");
>>>>> +			dev_err(hdmi->dev, "failed to get reference clock\n");
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> @@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>>> 	if (IS_ERR(hdmi->grf_clk)) {
>>>>> 		ret = PTR_ERR(hdmi->grf_clk);
>>>>> 		if (ret != -EPROBE_DEFER)
>>>>> -			drm_err(hdmi, "failed to get grf clock\n");
>>>>> +			dev_err(hdmi->dev, "failed to get grf clock\n");
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> @@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>>>>>
>>>>> 	ret = clk_prepare_enable(hdmi->grf_clk);
>>>>> 	if (ret < 0) {
>>>>> -		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
>>>>> +		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
>>>>> 		return;
>>>>> 	}
>>>>>
>>>>> 	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
>>>>> 	if (ret != 0)
>>>>> -		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
>>>>> +		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
>>>>>
>>>>> 	clk_disable_unprepare(hdmi->grf_clk);
>>>>> -	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>>>> +	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>>>>> }
>>>>>
>>>>> static int
>>>>> @@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>>>> 	ret = rockchip_hdmi_parse_dt(hdmi);
>>>>> 	if (ret) {
>>>>> 		if (ret != -EPROBE_DEFER)
>>>>> -			drm_err(hdmi, "Unable to parse OF data\n");
>>>>> +			dev_err(hdmi->dev, "Unable to parse OF data\n");
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> @@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>>>> 	if (IS_ERR(hdmi->phy)) {
>>>>> 		ret = PTR_ERR(hdmi->phy);
>>>>> 		if (ret != -EPROBE_DEFER)
>>>>> -			drm_err(hdmi, "failed to get phy\n");
>>>>> +			dev_err(hdmi->dev, "failed to get phy\n");
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>>> index f41151d49fca..3d1dddb34603 100644
>>>>> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
>>>>> @@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
>>>>> 	if (drm) {
>>>>> 		changed = drm_helper_hpd_irq_event(drm);
>>>>> 		if (changed)
>>>>> -			drm_dbg(hdmi, "connector status changed\n");
>>>>> +			dev_dbg(hdmi->dev, "connector status changed\n");
>>>>> 	}
>>>>> }
>>>>>
>>>>> @@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>>> 		}
>>>>> 	}
>>>>> 	if (hdmi->port_id < 0) {
>>>>> -		drm_err(hdmi, "Failed to match HDMI port ID\n");
>>>>> +		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
>>>>> 		return hdmi->port_id;
>>>>> 	}
>>>>>
>>>>> @@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>>> 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>>> 						       "rockchip,grf");
>>>>> 	if (IS_ERR(hdmi->regmap)) {
>>>>> -		drm_err(hdmi, "Unable to get rockchip,grf\n");
>>>>> +		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>>>>> 		return PTR_ERR(hdmi->regmap);
>>>>> 	}
>>>>>
>>>>> 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>>> 							  "rockchip,vo-grf");
>>>>> 	if (IS_ERR(hdmi->vo_regmap)) {
>>>>> -		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
>>>>> +		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
>>>>> 		return PTR_ERR(hdmi->vo_regmap);
>>>>> 	}
>>>>>
>>>>> 	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
>>>>> 	if (ret < 0) {
>>>>> -		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
>>>>> +		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> @@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>>> 						    GPIOD_OUT_HIGH);
>>>>> 	if (IS_ERR(hdmi->enable_gpio)) {
>>>>> 		ret = PTR_ERR(hdmi->enable_gpio);
>>>>> -		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
>>>>> +		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> @@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>>> 	if (IS_ERR(hdmi->phy)) {
>>>>> 		ret = PTR_ERR(hdmi->phy);
>>>>> 		if (ret != -EPROBE_DEFER)
>>>>> -			drm_err(hdmi, "failed to get phy: %d\n", ret);
>>>>> +			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> @@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>>>>> 	connector = drm_bridge_connector_init(drm, encoder);
>>>>> 	if (IS_ERR(connector)) {
>>>>> 		ret = PTR_ERR(connector);
>>>>> -		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
>>>>> +		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
>>>>> 		return ret;
>>>>> 	}
>>>>>
>>>>> -- 
>>>>> 2.39.5
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Linux-rockchip mailing list
>>>>> Linux-rockchip@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>> -- 
>>> Jani Nikula, Intel
>
>-- 
>--
>Thomas Zimmermann
>Graphics Driver Developer
>SUSE Software Solutions Germany GmbH
>Frankenstrasse 146, 90461 Nuernberg, Germany
>GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>HRB 36809 (AG Nuernberg)
>

Re:Re: [PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Jani Nikula]

On Wed, 26 Feb 2025, "Andy Yan" <andyshrk@163.com> wrote:
> Hi，
>
> 在 2025-02-26 16:33:21，"Thomas Zimmermann" <tzimmermann@suse.de> 写道：
>>Hi
>>
>>Am 25.01.25 um 04:53 schrieb Andy Yan:
>>>
>>> 在 2025-01-24 19:43:07，"Jani Nikula" <jani.nikula@intel.com> 写道：
>>>> On Fri, 24 Jan 2025, "Andy Yan" <andyshrk@163.com> wrote:
>>>>> Hi，
>>>>>
>>>>> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@intel.com> wrote:
>>>>>> The expectation is that the struct drm_device based logging helpers get
>>>>>> passed an actual struct drm_device pointer rather than some random
>>>>>> struct pointer where you can dereference the ->dev member.
>>>>>>
>>>>>> Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
>>>>>> current usage, but drops "[drm] *ERROR*" prefix from logging.
>>>>> Frankly, I prefer the original version of the log.
>>>>> It is a platform driver, so it should use its own device.
>>>>> It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging
>>>> If you need to do struct device based logging that is not the same
>>>> device as the struct drm_device dev member, you need to use dev_err()
>>>> and friends. You can't and must not use drm_err() and friends.
>>>>
>>>> It's as simple as that.
>>>>
>>>> The current drm_err(hdmi, ...) usage is simply abuse of the macros, and
>>>> must stop.
>>> Perhaps when you initially designed this macros, you intended it to accept only drm_device,
>>> but your code implementation didn't enforce this restriction at the beginning.
>>
>>C'mon. Are we really arguing about type safety now?
>>
>>Patch 5 adds a little getter function that does the type check on the 
>>function call's argument.
>>
>>
>>> If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0],
>>> as neither drm_err nor dev_err can maintain consistency with the original log of this driver.
>>> Alternatively, as suggested by Sam  in the initial submission of your patch 5 years ago,
>>> there should also be a macro similar to drm_dev_info(device *, ..).[1]
>>
>>DRM code tends to keep a reference to the drm_device somewhere and 
>>fetches it if necessary. For this patch, it should be possible to fetch 
>>the DRM device from struct rockchip_hdmi easily. Just do
>>
>>   drm_err(rockchip_hdmi_drm_dev(hdmi), "...");
>>
>>This would resolve the problem without new logging functions and keep 
>>the "[drm]" prefix to the messages.
>
> Yes, this keeps the "[drm]" prefix to the log messages, but it also changed hdmi
> device from drm device in the log messages.
> For more efficient debugging, it is preferable for log entries to explicitly specify which device generated them,
> especially in DRM systems where multiple devices(bridge/encoder) may be present."
>
> And I'm ok with this PATCH. However, I would also like to know your and Jani's opinions on whether we can consider
> adding an API similar to drm_dev_info，as Sam suggested before. Of course, this could be left for future implementation
> by others.

I'm not at all thrilled about adding loads of variations of drm specific
debug macros. They just multiply.

The way around that would be to use _Generic() to allow passing either
struct drm_device (which would use drm->dev) or a more specific struct
device to be used for dev based logging. I'd be supportive of that.

However, this patch series is a dependency for implementing generics at
a later time. I'd like to start here instead of piling on more work.

Furthermore, there's been talk about drm display object based
logging. The generics could later be expanded to allow for example:

	drm_err(connector, "Fail\n");

to translate to:

	dev_err(connector->dev->dev, "[CONNECTOR:%d:%s] Fail\n",
		connector->base.id, connector->name);

This would simplify a lot of connector based logging. Similarly for
other objects.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends

[Louis Chauvet]

Le 23/01/2025 à 16:09, Jani Nikula a écrit :
> The expectation is that the struct drm_device based logging helpers get
> passed an actual struct drm_device pointer rather than some random
> struct pointer where you can dereference the ->dev member.
> 
> Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches
> current usage, but drops "[drm] *ERROR*" prefix from logging.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
> 
> Looks like it's possible to hunt down the struct drm_device in most of
> these cases, if that's desired. This was the simplest change.
> 
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: Andy Yan <andy.yan@rock-chips.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-rockchip@lists.infradead.org
> ---
>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    | 16 ++++++++--------
>   drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 16 ++++++++--------
>   2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index e7a6669c46b0..f737e7d46e66 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -203,7 +203,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>   
>   	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>   	if (IS_ERR(hdmi->regmap)) {
> -		drm_err(hdmi, "Unable to get rockchip,grf\n");
> +		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>   		return PTR_ERR(hdmi->regmap);
>   	}
>   
> @@ -214,7 +214,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>   	if (IS_ERR(hdmi->ref_clk)) {
>   		ret = PTR_ERR(hdmi->ref_clk);
>   		if (ret != -EPROBE_DEFER)
> -			drm_err(hdmi, "failed to get reference clock\n");
> +			dev_err(hdmi->dev, "failed to get reference clock\n");
>   		return ret;
>   	}
>   
> @@ -222,7 +222,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>   	if (IS_ERR(hdmi->grf_clk)) {
>   		ret = PTR_ERR(hdmi->grf_clk);
>   		if (ret != -EPROBE_DEFER)
> -			drm_err(hdmi, "failed to get grf clock\n");
> +			dev_err(hdmi->dev, "failed to get grf clock\n");
>   		return ret;
>   	}
>   
> @@ -302,16 +302,16 @@ static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
>   
>   	ret = clk_prepare_enable(hdmi->grf_clk);
>   	if (ret < 0) {
> -		drm_err(hdmi, "failed to enable grfclk %d\n", ret);
> +		dev_err(hdmi->dev, "failed to enable grfclk %d\n", ret);
>   		return;
>   	}
>   
>   	ret = regmap_write(hdmi->regmap, hdmi->chip_data->lcdsel_grf_reg, val);
>   	if (ret != 0)
> -		drm_err(hdmi, "Could not write to GRF: %d\n", ret);
> +		dev_err(hdmi->dev, "Could not write to GRF: %d\n", ret);
>   
>   	clk_disable_unprepare(hdmi->grf_clk);
> -	drm_dbg(hdmi, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
> +	dev_dbg(hdmi->dev, "vop %s output to hdmi\n", ret ? "LIT" : "BIG");
>   }
>   
>   static int
> @@ -574,7 +574,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>   	ret = rockchip_hdmi_parse_dt(hdmi);
>   	if (ret) {
>   		if (ret != -EPROBE_DEFER)
> -			drm_err(hdmi, "Unable to parse OF data\n");
> +			dev_err(hdmi->dev, "Unable to parse OF data\n");
>   		return ret;
>   	}
>   
> @@ -582,7 +582,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>   	if (IS_ERR(hdmi->phy)) {
>   		ret = PTR_ERR(hdmi->phy);
>   		if (ret != -EPROBE_DEFER)
> -			drm_err(hdmi, "failed to get phy\n");
> +			dev_err(hdmi->dev, "failed to get phy\n");
>   		return ret;
>   	}
>   
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index f41151d49fca..3d1dddb34603 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -242,7 +242,7 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
>   	if (drm) {
>   		changed = drm_helper_hpd_irq_event(drm);
>   		if (changed)
> -			drm_dbg(hdmi, "connector status changed\n");
> +			dev_dbg(hdmi->dev, "connector status changed\n");
>   	}
>   }
>   
> @@ -472,7 +472,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>   		}
>   	}
>   	if (hdmi->port_id < 0) {
> -		drm_err(hdmi, "Failed to match HDMI port ID\n");
> +		dev_err(hdmi->dev, "Failed to match HDMI port ID\n");
>   		return hdmi->port_id;
>   	}
>   
> @@ -496,20 +496,20 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>   	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>   						       "rockchip,grf");
>   	if (IS_ERR(hdmi->regmap)) {
> -		drm_err(hdmi, "Unable to get rockchip,grf\n");
> +		dev_err(hdmi->dev, "Unable to get rockchip,grf\n");
>   		return PTR_ERR(hdmi->regmap);
>   	}
>   
>   	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
>   							  "rockchip,vo-grf");
>   	if (IS_ERR(hdmi->vo_regmap)) {
> -		drm_err(hdmi, "Unable to get rockchip,vo-grf\n");
> +		dev_err(hdmi->dev, "Unable to get rockchip,vo-grf\n");
>   		return PTR_ERR(hdmi->vo_regmap);
>   	}
>   
>   	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
>   	if (ret < 0) {
> -		drm_err(hdmi, "Failed to get clocks: %d\n", ret);
> +		dev_err(hdmi->dev, "Failed to get clocks: %d\n", ret);
>   		return ret;
>   	}
>   
> @@ -517,7 +517,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>   						    GPIOD_OUT_HIGH);
>   	if (IS_ERR(hdmi->enable_gpio)) {
>   		ret = PTR_ERR(hdmi->enable_gpio);
> -		drm_err(hdmi, "Failed to request enable GPIO: %d\n", ret);
> +		dev_err(hdmi->dev, "Failed to request enable GPIO: %d\n", ret);
>   		return ret;
>   	}
>   
> @@ -525,7 +525,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>   	if (IS_ERR(hdmi->phy)) {
>   		ret = PTR_ERR(hdmi->phy);
>   		if (ret != -EPROBE_DEFER)
> -			drm_err(hdmi, "failed to get phy: %d\n", ret);
> +			dev_err(hdmi->dev, "failed to get phy: %d\n", ret);
>   		return ret;
>   	}
>   
> @@ -564,7 +564,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>   	connector = drm_bridge_connector_init(drm, encoder);
>   	if (IS_ERR(connector)) {
>   		ret = PTR_ERR(connector);
> -		drm_err(hdmi, "failed to init bridge connector: %d\n", ret);
> +		dev_err(hdmi->dev, "failed to init bridge connector: %d\n", ret);
>   		return ret;
>   	}
>   

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com