Messages in this thread | | | Date | Wed, 16 Nov 2022 13:23:00 +0100 | From | "" <> | Subject | Re: [PATCH v4 1/4] gpu: drm: meson: Use devm_regulator_*get_enable*() |
| |
On 16/11/2022 13:02, Vaittinen, Matti wrote: > Hi dee Ho Laurent & All, > > 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@gmail.com> >>>> >>>> --- >>>> 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. >>>> --- >>>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++-------------------- >>>> 1 file changed, 3 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c >>>> b/drivers/gpu/drm/meson/meson_dw_hdmi.c >>>> index 5cd2b2ebbbd3..7642f740272b 100644 >>>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c >>>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c >>>> @@ -140,7 +140,6 @@ struct meson_dw_hdmi { >>>> struct reset_control *hdmitx_apb; >>>> struct reset_control *hdmitx_ctrl; >>>> struct reset_control *hdmitx_phy; >>>> - struct regulator *hdmi_supply; >>>> u32 irq_stat; >>>> struct dw_hdmi *hdmi; >>>> struct drm_bridge *bridge; >>>> @@ -665,11 +664,6 @@ static void meson_dw_hdmi_init(struct >>>> meson_dw_hdmi *meson_dw_hdmi) >>>> } >>>> -static void meson_disable_regulator(void *data) >>>> -{ >>>> - regulator_disable(data); >>>> -} >>>> - >>>> static void meson_disable_clk(void *data) >>>> { >>>> clk_disable_unprepare(data); >>>> @@ -723,20 +717,9 @@ static int meson_dw_hdmi_bind(struct device >>>> *dev, struct device *master, >>>> meson_dw_hdmi->data = match; >>>> dw_plat_data = &meson_dw_hdmi->dw_plat_data; >>>> - meson_dw_hdmi->hdmi_supply = devm_regulator_get_optional(dev, >>>> "hdmi"); >>>> - if (IS_ERR(meson_dw_hdmi->hdmi_supply)) { >>>> - if (PTR_ERR(meson_dw_hdmi->hdmi_supply) == -EPROBE_DEFER) >>>> - return -EPROBE_DEFER; >>>> - meson_dw_hdmi->hdmi_supply = NULL; >>>> - } else { >>>> - ret = regulator_enable(meson_dw_hdmi->hdmi_supply); >>>> - if (ret) >>>> - return ret; >>>> - ret = devm_add_action_or_reset(dev, meson_disable_regulator, >>>> - meson_dw_hdmi->hdmi_supply); >>>> - if (ret) >>>> - return ret; >>>> - } >>>> + ret = devm_regulator_get_enable_optional(dev, "hdmi"); >>>> + if (ret != -ENODEV) >>>> + return ret; >>> >>> 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. > > While we all agree that power savings gained by runtime PM would be > great - I don't think we should stop minor improvements while waiting > for that to magically happen ;) > > I like the saying I first heard from Jonathan Cameron - "Don't let the > perfect be an enemy of good". > > After that being said, should I re-spin this or just drop it? (I am > cleaning up my local git from old stuff, and these are about to vanish > from my books).
I'm ok with you, please re-spin it.
Neil
> > Yours, > -- Matti >
| |