| Message ID | 20210304155144.1.Ic9c04f960190faad5290738b2a35d73661862735@changeid (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/3] drm/bridge: ti-sn65dsi86: Simplify refclk handling | expand |
Hey Douglas, Thanks for submitting this cleanup, it looks good to me. Reviewed-by: Robert Foss <robert.foss@linaro.org> On Fri, 5 Mar 2021 at 00:53, Douglas Anderson <dianders@chromium.org> wrote: > > The clock framework makes it simple to deal with an optional clock. > You can call clk_get_optional() and if the clock isn't specified it'll > just return NULL without complaint. It's valid to pass NULL to > enable/disable/prepare/unprepare. Let's make use of this to simplify > things a tiny bit. > > NOTE: this makes things look a tad bit asymmetric now since we check > for NULL before clk_prepare_enable() but not for > clk_disable_unprepare(). This seemed OK to me. We already have to > check for NULL in the enable case anyway so why not avoid the extra > call? I think this is fine. Since the refclk != NULL check in ti_sn_bridge_set_refclk_freq is in order to determine other behaviour, the asymmetry is required. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index f27306c51e4d..942019842ff4 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -1261,14 +1261,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > return ret; > } > > - pdata->refclk = devm_clk_get(pdata->dev, "refclk"); > - if (IS_ERR(pdata->refclk)) { > - ret = PTR_ERR(pdata->refclk); > - if (ret == -EPROBE_DEFER) > - return ret; > - DRM_DEBUG_KMS("refclk not found\n"); > - pdata->refclk = NULL; > - } > + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); > + if (IS_ERR(pdata->refclk)) > + return PTR_ERR(pdata->refclk); > > ret = ti_sn_bridge_parse_dsi_host(pdata); > if (ret) > -- > 2.30.1.766.gb4fecdf3b7-goog >
On Thu 04 Mar 17:51 CST 2021, Douglas Anderson wrote: > The clock framework makes it simple to deal with an optional clock. > You can call clk_get_optional() and if the clock isn't specified it'll > just return NULL without complaint. It's valid to pass NULL to > enable/disable/prepare/unprepare. Let's make use of this to simplify > things a tiny bit. > > NOTE: this makes things look a tad bit asymmetric now since we check > for NULL before clk_prepare_enable() but not for > clk_disable_unprepare(). This seemed OK to me. We already have to > check for NULL in the enable case anyway so why not avoid the extra > call? > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index f27306c51e4d..942019842ff4 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -1261,14 +1261,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > return ret; > } > > - pdata->refclk = devm_clk_get(pdata->dev, "refclk"); > - if (IS_ERR(pdata->refclk)) { > - ret = PTR_ERR(pdata->refclk); > - if (ret == -EPROBE_DEFER) > - return ret; > - DRM_DEBUG_KMS("refclk not found\n"); > - pdata->refclk = NULL; > - } > + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); > + if (IS_ERR(pdata->refclk)) > + return PTR_ERR(pdata->refclk); > > ret = ti_sn_bridge_parse_dsi_host(pdata); > if (ret) > -- > 2.30.1.766.gb4fecdf3b7-goog >
Quoting Douglas Anderson (2021-03-04 15:51:59) > The clock framework makes it simple to deal with an optional clock. > You can call clk_get_optional() and if the clock isn't specified it'll > just return NULL without complaint. It's valid to pass NULL to > enable/disable/prepare/unprepare. Let's make use of this to simplify > things a tiny bit. > > NOTE: this makes things look a tad bit asymmetric now since we check > for NULL before clk_prepare_enable() but not for > clk_disable_unprepare(). This seemed OK to me. We already have to > check for NULL in the enable case anyway so why not avoid the extra > call? > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Hi Douglas, Thank you for the patch. On Thu, Mar 04, 2021 at 03:51:59PM -0800, Douglas Anderson wrote: > The clock framework makes it simple to deal with an optional clock. > You can call clk_get_optional() and if the clock isn't specified it'll > just return NULL without complaint. It's valid to pass NULL to > enable/disable/prepare/unprepare. Let's make use of this to simplify > things a tiny bit. > > NOTE: this makes things look a tad bit asymmetric now since we check > for NULL before clk_prepare_enable() but not for > clk_disable_unprepare(). This seemed OK to me. We already have to > check for NULL in the enable case anyway so why not avoid the extra > call? > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index f27306c51e4d..942019842ff4 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -1261,14 +1261,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > return ret; > } > > - pdata->refclk = devm_clk_get(pdata->dev, "refclk"); > - if (IS_ERR(pdata->refclk)) { > - ret = PTR_ERR(pdata->refclk); > - if (ret == -EPROBE_DEFER) > - return ret; > - DRM_DEBUG_KMS("refclk not found\n"); > - pdata->refclk = NULL; > - } > + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); > + if (IS_ERR(pdata->refclk)) > + return PTR_ERR(pdata->refclk); > > ret = ti_sn_bridge_parse_dsi_host(pdata); > if (ret)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index f27306c51e4d..942019842ff4 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1261,14 +1261,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; } - pdata->refclk = devm_clk_get(pdata->dev, "refclk"); - if (IS_ERR(pdata->refclk)) { - ret = PTR_ERR(pdata->refclk); - if (ret == -EPROBE_DEFER) - return ret; - DRM_DEBUG_KMS("refclk not found\n"); - pdata->refclk = NULL; - } + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); + if (IS_ERR(pdata->refclk)) + return PTR_ERR(pdata->refclk); ret = ti_sn_bridge_parse_dsi_host(pdata); if (ret)
The clock framework makes it simple to deal with an optional clock. You can call clk_get_optional() and if the clock isn't specified it'll just return NULL without complaint. It's valid to pass NULL to enable/disable/prepare/unprepare. Let's make use of this to simplify things a tiny bit. NOTE: this makes things look a tad bit asymmetric now since we check for NULL before clk_prepare_enable() but not for clk_disable_unprepare(). This seemed OK to me. We already have to check for NULL in the enable case anyway so why not avoid the extra call? Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)