| Message ID | df0096b5aea2a18d1540cde379c5abf589ccd7c4.1669799805.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 30/11/2022 10:23, Matti Vaittinen wrote: > Simplify using the devm_regulator_get_enable_optional(). Also drop the > now unused struct member 'hdmi_supply'. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > Martin Blumenstingl <martin.blumenstingl@googlemail.com> Missing Acked-by, I'll add it while applying. Neil > > --- > v4 resend 2: > Respinning unchanged code with the commit title changed as wa suggested > by Robert Foss and commit message changed as was suggested by Martin. > > I am doing a clean-up for my local git and encountered this one. > Respinning as it seems this one fell through the cracks. > --- > 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");
On 12/1/22 10:38, Neil Armstrong wrote: > On 30/11/2022 10:23, Matti Vaittinen wrote: >> Simplify using the devm_regulator_get_enable_optional(). Also drop the >> now unused struct member 'hdmi_supply'. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > Missing Acked-by, I'll add it while applying. Oh, well spotted. I should've been more careful.. Sorry and thanks for sorting it out! --Matti
Hi Matti, On 30.11.2022 10:23, Matti Vaittinen wrote: > Simplify using the devm_regulator_get_enable_optional(). Also drop the > now unused struct member 'hdmi_supply'. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > v4 resend 2: > Respinning unchanged code with the commit title changed as wa suggested > by Robert Foss and commit message changed as was suggested by Martin. > > I am doing a clean-up for my local git and encountered this one. > Respinning as it seems this one fell through the cracks. > --- > 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) The above line should be "if (ret < 0)", otherwise it breaks hdmi support. I've noticed this once this change has been merged to linux-next and all my Amlogic Meson based boards failed to initialize HDMI. Is it possible to fix this in drm tree or do I need to send the incremental fixup? > + return ret; > > meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev, > "hdmitx_apb"); Best regards
On 09/01/2023 14:46, Marek Szyprowski wrote: > Hi Matti, > > On 30.11.2022 10:23, Matti Vaittinen wrote: >> Simplify using the devm_regulator_get_enable_optional(). Also drop the >> now unused struct member 'hdmi_supply'. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> --- >> v4 resend 2: >> Respinning unchanged code with the commit title changed as wa suggested >> by Robert Foss and commit message changed as was suggested by Martin. >> >> I am doing a clean-up for my local git and encountered this one. >> Respinning as it seems this one fell through the cracks. >> --- >> 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) > > The above line should be "if (ret < 0)", otherwise it breaks hdmi support. > > I've noticed this once this change has been merged to linux-next and all > my Amlogic Meson based boards failed to initialize HDMI. Is it possible > to fix this in drm tree or do I need to send the incremental fixup? Nop, please send an incremental fix and I'll apply it asap. Thanks, Neil > >> + return ret; >> >> meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev, >> "hdmitx_apb"); > > Best regards
Hi Marek & All, ma 9. tammik. 2023 klo 17.23 Neil Armstrong (neil.armstrong@linaro.org) kirjoitti: > > On 09/01/2023 14:46, Marek Szyprowski wrote: > > Hi Matti, > > > > On 30.11.2022 10:23, Matti Vaittinen wrote: > >> Simplify using the devm_regulator_get_enable_optional(). Also drop the > >> now unused struct member 'hdmi_supply'. > >> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > >> Martin Blumenstingl <martin.blumenstingl@googlemail.com> > >> --- > >> v4 resend 2: > >> Respinning unchanged code with the commit title changed as wa suggested > >> by Robert Foss and commit message changed as was suggested by Martin. > >> > >> I am doing a clean-up for my local git and encountered this one. > >> Respinning as it seems this one fell through the cracks. > >> --- > >> 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) > > > > The above line should be "if (ret < 0)", otherwise it breaks hdmi support. > > Sorry for the trouble and thanks for pointing this out! I believe the condition I was looking for was if (ret && ret != -ENODEV). At least in my eyes the use of regulator_get_optional() sounds like the -ENODEV should not be treated as an error. The obvious mistake is regarding return value 0 as such. Thanks for spotting this! > > I've noticed this once this change has been merged to linux-next and all > > my Amlogic Meson based boards failed to initialize HDMI. Is it possible > > to fix this in drm tree or do I need to send the incremental fixup? > > Nop, please send an incremental fix and I'll apply it asap. Thanks for cleaning-up the mess I caused Marek! Please, let me know if you want me to cook the fix. 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 now unused struct member 'hdmi_supply'. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- v4 resend 2: Respinning unchanged code with the commit title changed as wa suggested by Robert Foss and commit message changed as was suggested by Martin. I am doing a clean-up for my local git and encountered this one. Respinning as it seems this one fell through the cracks. --- drivers/gpu/drm/meson/meson_dw_hdmi.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)