On 04/04/2024 09:13, Anand Moon wrote:
> Use devm_regulator_bulk_get_enable() instead of open coded
> 'devm_regulator_get(), regulator_enable(), regulator_disable().
I fail to see how did you replace open-coded suspend/resume paths.
>
> Signed-off-by: Anand Moon <[email protected]>
> ---
> V2: no changes, did not find any regression in pm suspend/resume.
No, that's not equivalent code. No explanation in commit msg.
You already got comments on this and nothing improved. You just entirely
ignored received comments. That's not how it works.
I don't think you understand the code and Linux driver model. This patch
repeats several previous attempts with similar issues: no logic behind a
change.
NAK.
Best regards,
Krzysztof
Hi Krzysztof,
On Thu, 4 Apr 2024 at 12:53, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 04/04/2024 09:13, Anand Moon wrote:
> > Use devm_regulator_bulk_get_enable() instead of open coded
> > 'devm_regulator_get(), regulator_enable(), regulator_disable().
>
> I fail to see how did you replace open-coded suspend/resume paths.
>
> >
> > Signed-off-by: Anand Moon <[email protected]>
> > ---
> > V2: no changes, did not find any regression in pm suspend/resume.
>
> No, that's not equivalent code. No explanation in commit msg.
>
> You already got comments on this and nothing improved. You just entirely
> ignored received comments. That's not how it works.
>
> I don't think you understand the code and Linux driver model. This patch
> repeats several previous attempts with similar issues: no logic behind a
> change.
>
> NAK.
devm_regulator_get_enable and devm_regulator_bulk_get_enable
both remove the dependency from the driver to handle the regulator_enabled
and regulator_disabled. ie this removes the regulator from the driver structure.
Since these functions set devm_add_action to disable the regulator when the
resource is not used.
ret = devm_add_action(dev, devm_regulator_bulk_disable, devres);
if (!ret)
return 0;
>
> Best regards,
> Krzysztof
>
if you feel it's incorrect, I will drop this patch..
Thanks
-Anand
On 04/04/2024 09:38, Anand Moon wrote:
> Hi Krzysztof,
>
> On Thu, 4 Apr 2024 at 12:53, Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 04/04/2024 09:13, Anand Moon wrote:
>>> Use devm_regulator_bulk_get_enable() instead of open coded
>>> 'devm_regulator_get(), regulator_enable(), regulator_disable().
>>
>> I fail to see how did you replace open-coded suspend/resume paths.
>>
>>>
>>> Signed-off-by: Anand Moon <[email protected]>
>>> ---
>>> V2: no changes, did not find any regression in pm suspend/resume.
>>
>> No, that's not equivalent code. No explanation in commit msg.
>>
>> You already got comments on this and nothing improved. You just entirely
>> ignored received comments. That's not how it works.
>>
>> I don't think you understand the code and Linux driver model. This patch
>> repeats several previous attempts with similar issues: no logic behind a
>> change.
>>
>> NAK.
>
> devm_regulator_get_enable and devm_regulator_bulk_get_enable
> both remove the dependency from the driver to handle the regulator_enabled
> and regulator_disabled. ie this removes the regulator from the driver structure.
Not true. Please do not paste some generic knowledge and assume reviewer
knows it. Instead provide proof.
>
> Since these functions set devm_add_action to disable the regulator when the
> resource is not used.
>
> ret = devm_add_action(dev, devm_regulator_bulk_disable, devres);
> if (!ret)
> return 0;
Listen, you already got comments on this at v1. Address previous
comments instead of repeating something unrelated. We should not have
the same discussion twice.
Best regards,
Krzysztof