| Message ID | c14058c4b7018556a78455ffef484a7ebe4d8ea2.1666357434.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | Use devm helpers for regulator get and enable | expand |
On 21/10/2022 15:18, 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; > > meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev, > "hdmitx_apb"); Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
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. > > meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev, > "hdmitx_apb");
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. > >> >> meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev, >> "hdmitx_apb"); > Neil
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. >>> --- //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
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). Yours, -- Matti
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 >
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; meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev, "hdmitx_apb");
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(-)