| Message ID | f42da4c9943a2f2a9de4272b7849e72236d4c3f9.1737644530.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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
在 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
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; > } >
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
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
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) >
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.
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; }
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(-)