Re: [PATCH 2/5] drm/msm/dpu: Integrate interconnect API in MDSS

[Georgi Djakov <georgi.djakov@linaro.org>]

On 5/13/19 17:47, Sean Paul wrote:
> On Wed, May 08, 2019 at 01:42:12PM -0700, Rob Clark wrote:
>> From: Jayant Shekhar <jshekhar@codeaurora.org>
>>
>> The interconnect framework is designed to provide a
>> standard kernel interface to control the settings of
>> the interconnects on a SoC.
>>
>> The interconnect API uses a consumer/provider-based model,
>> where the providers are the interconnect buses and the
>> consumers could be various drivers.
>>
>> MDSS is one of the interconnect consumers which uses the
>> interconnect APIs to get the path between endpoints and
>> set its bandwidth requirement for the given interconnected
>> path.
>>
>> Changes in v2:
>> 	- Remove error log and unnecessary check (Jordan Crouse)
>>
>> Changes in v3:
>> 	- Code clean involving variable name change, removal
>> 	  of extra paranthesis and variables (Matthias Kaehlcke)
>>
>> Changes in v4:
>> 	- Add comments, spacings, tabs, proper port name
>> 	  and icc macro (Georgi Djakov)
>>
>> Changes in v5:
>> 	- Commit text and parenthesis alignment (Georgi Djakov)
>>
>> Changes in v6:
>> 	- Change to new icc_set API's (Doug Anderson)
>>
>> Changes in v7:
>> 	- Fixed a typo
>>
>> Signed-off-by: Sravanthi Kollukuduru <skolluku@codeaurora.org>
>> Signed-off-by: Jayant Shekhar <jshekhar@codeaurora.org>
>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 49 ++++++++++++++++++++++--
>>  1 file changed, 45 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>> index 7316b4ab1b85..e3c56ccd7357 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
>> @@ -4,11 +4,15 @@
>>   */
>>  
>>  #include "dpu_kms.h"
>> +#include <linux/interconnect.h>
>>  
>>  #define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base)
>>  
>>  #define HW_INTR_STATUS			0x0010
>>  
>> +/* Max BW defined in KBps */
>> +#define MAX_BW				6800000
>> +
>>  struct dpu_irq_controller {
>>  	unsigned long enabled_mask;
>>  	struct irq_domain *domain;
>> @@ -21,8 +25,30 @@ struct dpu_mdss {
>>  	u32 hwversion;
>>  	struct dss_module_power mp;
>>  	struct dpu_irq_controller irq_controller;
>> +	struct icc_path *path[2];
>> +	u32 num_paths;
>>  };
>>  
>> +static int dpu_mdss_parse_data_bus_icc_path(struct drm_device *dev,
>> +						struct dpu_mdss *dpu_mdss)
>> +{
>> +	struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
>> +	struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
>> +
>> +	if (IS_ERR(path0))
> 
> of_icc_get can also return NULL, it looks like we also want to guard against
> this case and keep num_paths == 0.

of_icc_get() returns NULL when the interconnect API is not enabled or the DT
properties are not present. In this case, passing a NULL path to the icc
functions is just a nop. So it should be fine either way.

Acked-by: Georgi Djakov <georgi.djakov@linaro.org>

Thanks,
Georgi