Hi,
On Wed, Mar 31, 2021 at 4:27 AM Kalyan Thota kalyan_t@codeaurora.org wrote:
@@ -294,6 +294,9 @@ static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms) struct icc_path *path1; struct drm_device *dev = dpu_kms->dev;
if (!dpu_supports_bw_scaling(dev))return 0;path0 = of_icc_get(dev->dev, "mdp0-mem"); path1 = of_icc_get(dev->dev, "mdp1-mem");
Instead of hard coding a check for specific SoC compatible strings, why not just check to see if path0 and/or path1 are ERR_PTR(-ENODEV)? Then change dpu_supports_bw_scaling() to just return:
!IS_ERR(dpu_kms->path[0])
It also seems like it would be nice if you did something if you got an error other than -ENODEV. Right now this function returns it but the caller ignores it? At least spit an error message out?
@@ -154,6 +154,15 @@ struct vsync_info {
#define to_dpu_global_state(x) container_of(x, struct dpu_global_state, base)
+/**
- dpu_supports_bw_scaling: returns true for drivers that support bw scaling.
- @dev: Pointer to drm_device structure
- */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev) +{
return of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss");
See above, but I think this would be better as:
return !IS_ERR(dpu_kms->path[0]);
Specifically, I don't think of_device_is_compatible() is really designed as something to call a lot. It's doing a whole bunch of data structure parsing / string comparisons. It's OK-ish during probe (though better to use the of_match_table), but you don't want to call it on every runtime suspend / runtime resume.