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
| ||
|
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
Powered by blists - more mailing lists