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: <76ffb882-10f9-4737-afa2-9bb60248835d@kernel.org>
Date: Thu, 5 Sep 2024 13:57:23 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Dikshita Agarwal <quic_dikshita@...cinc.com>,
 Vikash Garodia <quic_vgarodia@...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

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.

>>> +		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.

...

>>
>> 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.

Best regards,
Krzysztof