[PATCH 2/5] drm/rockchip: stop passing non struct drm_device to drm_err() and friends
Andy Yan
andyshrk at 163.com
Wed Feb 26 02:36:26 PST 2025
Hi，
在 2025-02-26 16:33:21，"Thomas Zimmermann" <tzimmermann at suse.de> 写道：
>Hi
>
>Am 25.01.25 um 04:53 schrieb Andy Yan:
>>
>> 在 2025-01-24 19:43:07，"Jani Nikula" <jani.nikula at intel.com> 写道：
>>> On Fri, 24 Jan 2025, "Andy Yan" <andyshrk at 163.com> wrote:
>>>> Hi，
>>>>
>>>> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula at 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 at 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 at rock-chips.com>
>>>>> Cc: "Heiko Stübner" <heiko at sntech.de>
>>>>> Cc: Andy Yan <andy.yan at rock-chips.com>
>>>>> Cc: dri-devel at lists.freedesktop.org
>>>>> Cc: linux-arm-kernel at lists.infradead.org
>>>>> Cc: linux-rockchip at 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 at 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)
>
More information about the Linux-rockchip
mailing list