Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*()

From: Vaittinen, Matti
Date: Mon Oct 24 2022 - 00:41:02 EST


On 10/21/22 18:29, Neil Armstrong wrote:
> Hi,
>
> On 21/10/2022 17:02, Laurent Pinchart wrote:
>> Hi Matti,
>>
>> On Fri, Oct 21, 2022 at 04:18:01PM +0300, Matti Vaittinen wrote:
>>> Simplify using the devm_regulator_get_enable_optional(). Also drop the
>>> seemingly unused struct member 'hdmi_supply'.
>>>
>>> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
>>>
>>> ---
>>> v3 => v4:
>>> - split meson part to own patch
>>>
>>> RFCv1 => v2:
>>> - Change also sii902x to use devm_regulator_bulk_get_enable()
>>>
>>> Please note - this is only compile-tested due to the lack of HW. Careful
>>> review and testing is _highly_ appreciated.
>>> ---

//Snip

>>
>> As noted in the review of the series that introduced
>> devm_regulator_get_enable_optional(), the right thing to do is to
>> implement runtime PM in this driver to avoid wasting power.
>
> While I agree, it's not really the same level of effort as this patch
> should be functionally equivalent.
>

Exactly. As I wrote, I do not have the HW to test this change. This
intends to bring no functional changes. It is just a minor
simplification while also making it harder to create a bug with the
regulator control. (Registering the devm_action and leaving the handle
to the regulator is more fragile than using this new API which does not
give user the handle).

I am in no way against someone further improving the functionality here
(by adding runtime PM) but this someone is not me. Yet, I don't see how
this patch prevents runtime PM being implemented? The first thing that
needs to be done is removing the devm-action assuming someone did
implement power-saving by disabling the regulator(s) at runtime. After
this patch is applied, the action removal is automatically a necessity
in order to get the handle for regulator control.

I know this helper is not preferred by all but it is still safer than
the current code which registers the action while allowing the regulator
control.

Yours
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~