linux-kernel - Re: [PATCH v3 04/29] media: iris: initialize power resources

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f88d8596-c6a0-356e-060e-81d68f038995@quicinc.com>
Date: Fri, 6 Sep 2024 16:51:56 +0530
From: Vikash Garodia <quic_vgarodia@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
        Dikshita Agarwal
	<quic_dikshita@...cinc.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        "Mauro Carvalho Chehab" <mchehab@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 04/29] media: iris: initialize power resources

Hi Krzysztof,

On 9/5/2024 5:27 PM, Krzysztof Kozlowski wrote:
> On 05/09/2024 13:53, Dikshita Agarwal wrote:
>>
>>
>> On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote:
>>> On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
>>>> From: Dikshita Agarwal <quic_dikshita@...cinc.com>
>>>>
>>>> Add support for initializing Iris "resources", which are clocks,
>>>> interconnects, power domains, reset clocks, and clock frequencies
>>>> used for iris hardware.
>>>>
>>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>>>> ---
>>>
>>> ...
>>>
>>>> +struct iris_platform_data sm8550_data = {
>>>> +	.icc_tbl = sm8550_icc_table,
>>>> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
>>>> +	.clk_rst_tbl = sm8550_clk_reset_table,
>>>> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table),
>>>> +	.pmdomain_tbl = sm8550_pmdomain_table,
>>>> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
>>>> +	.opp_pd_tbl = sm8550_opp_pd_table,
>>>> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
>>>> +	.clk_tbl = sm8550_clk_table,
>>>> +	.clk_tbl_size = ARRAY_SIZE(sm8550_clk_table),
>>>> +};
>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>>>> index 0a54fdaa1ab5..2616a31224f9 100644
>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>>>> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev)
>>>>  	if (core->irq < 0)
>>>>  		return core->irq;
>>>>  
>>>> +	core->iris_platform_data = of_device_get_match_data(core->dev);
>>>> +	if (!core->iris_platform_data) {
>>>> +		ret = -ENODEV;
>>>> +		dev_err_probe(core->dev, ret, "init platform failed\n");
>>>
>>> That's not even possible. I would suggest dropping entire if. But if yoi
>>> insist, then without this weird redundant code. return -EINVAL.
>>>
>> Its possible if platform data is not initialized and this is only place we
>> check it, there is no further NULL check for the same.
> 
> It is possible? Then point me to the code line. Or present some code
> flow leading to it.
Lets go with return -EINVAL in this if block.

> 
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = iris_init_resources(core);
>>>> +	if (ret) {
>>>> +		dev_err_probe(core->dev, ret, "init resource failed\n");
>>>> +		return ret;
>>>
>>> How many same errors are you printing? Not mentioning that syntax of
>>> dev_errp_rpboe is different...
>> We have these errors at multiple points to know at what point the probe
>> failed which is useful while debugging probe failures.
> 
> Duplicating is not helpful.
> 
>> But Sure we will revisit this code and fix the syntax of dev_err_probe.
> 
>>>
>>>
>>>> +	}
>>>> +
>>>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>>>  	if (ret)
>>>>  		return ret;
>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>>>  }
>>>>  
>>>>  static const struct of_device_id iris_dt_match[] = {
>>>> -	{ .compatible = "qcom,sm8550-iris", },
>>>> -	{ .compatible = "qcom,sm8250-venus", },
>>>> +	{
>>>> +		.compatible = "qcom,sm8550-iris",
>>>> +		.data = &sm8550_data,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "qcom,sm8250-venus",
>>>> +		.data = &sm8250_data,
>>>
>>> You just added this. No, please do not add code which is immediatly
>>> incorrect.
>> It's not incorrect, in earlier patch we only added the compatible strings
>> and with this patch introducing the platform data and APIs to get it.
> 
> It is incorrect to immediately remove it. You keep arguing on basic
> stuff. Sorry, but that is not how it works. If you add code and
> IMMEDIATELY remove it, then it means the code was not needed. Or was not
> correct. Choose one.
I think it is not removing it. It is adding platform data to compatibles
introduced in previous patch. Maybe it appears as if it is removing it.
> 
> ...
> 
>>>
>>> This should be just part of of main unit file, next to probe. It is
>>> unusual to see probe parts not next to probe. Sorry, that's wrong.
>>>
>> All the APIs handling(init/enable/disable) the different resources (PM
>> domains, OPP, clocks, buses) are kept in this iris_resource.c file hence
>> this API to init all those resources is kept here to not load iris_probe.c
>> file.
> 
> You introduce your own coding style and as an argument you use just "I
> do this".
> 
> The expected is to see resource initialization next to probe. Repeating
> what your code does, is not helping me to understand your design choice.
I see your point and it would be good to have the resources initialized as part
of probe.

> Best regards,
> Krzysztof

Regards,
Vikash