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