Hi,
On Wed, Mar 31, 2021 at 4:27 AM Kalyan Thota <kalyan_t@xxxxxxxxxxxxxx> 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.
