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@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. >>> --- >>> 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). Yours, -- Matti -- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~