| Message ID | 20220518233602.248185-3-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/bridge: tc358767: Handle dsi_lanes == 0 as invalid | expand |
Am Donnerstag, dem 19.05.2022 um 01:36 +0200 schrieb Marek Vasut: > The Refclk may be supplied by SoC clock output instead of crystal > oscillator, make sure the clock are enabled before any other action > is performed with the bridge chip, otherwise it may either fail to > operate at all, or miss reset GPIO toggle. > > Fixes: 7caff0fc4296e ("drm/bridge: tc358767: Add DPI to eDP bridge driver") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Marek Vasut <marex@denx.de> > Cc: Maxime Ripard <maxime@cerno.tech> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > --- > drivers/gpu/drm/bridge/tc358767.c | 51 ++++++++++++++++++++----------- > 1 file changed, 33 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index a8bb6de9e600a..4b15acbc73c4e 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -2055,10 +2055,24 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > if (ret) > return ret; > > + tc->refclk = devm_clk_get(dev, "ref"); > + if (IS_ERR(tc->refclk)) { > + ret = PTR_ERR(tc->refclk); > + dev_err(dev, "Failed to get refclk: %d\n", ret); > + return ret; > + } > + > + clk_prepare_enable(tc->refclk); > + > + /* tRSTW = 100 cycles , at 13 MHz that is ~7.69 us */ > + usleep_range(10, 15); > + > /* Shut down GPIO is optional */ > tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH); > - if (IS_ERR(tc->sd_gpio)) > - return PTR_ERR(tc->sd_gpio); > + if (IS_ERR(tc->sd_gpio)) { > + ret = PTR_ERR(tc->sd_gpio); > + goto err_clk; > + } > > if (tc->sd_gpio) { > gpiod_set_value_cansleep(tc->sd_gpio, 0); > @@ -2067,26 +2081,21 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > > /* Reset GPIO is optional */ > tc->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > - if (IS_ERR(tc->reset_gpio)) > - return PTR_ERR(tc->reset_gpio); > + if (IS_ERR(tc->reset_gpio)) { > + ret = PTR_ERR(tc->reset_gpio); > + goto err_clk; > + } > > if (tc->reset_gpio) { > gpiod_set_value_cansleep(tc->reset_gpio, 1); > usleep_range(5000, 10000); > } > > - tc->refclk = devm_clk_get(dev, "ref"); > - if (IS_ERR(tc->refclk)) { > - ret = PTR_ERR(tc->refclk); > - dev_err(dev, "Failed to get refclk: %d\n", ret); > - return ret; > - } > - > tc->regmap = devm_regmap_init_i2c(client, &tc_regmap_config); > if (IS_ERR(tc->regmap)) { > ret = PTR_ERR(tc->regmap); > dev_err(dev, "Failed to initialize regmap: %d\n", ret); > - return ret; > + goto err_clk; > } > > ret = of_property_read_u32(dev->of_node, "toshiba,hpd-pin", > @@ -2096,7 +2105,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > } else { > if (tc->hpd_pin < 0 || tc->hpd_pin > 1) { > dev_err(dev, "failed to parse HPD number\n"); > - return ret; > + goto err_clk; > } > } > > @@ -2110,7 +2119,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > "tc358767-irq", tc); > if (ret) { > dev_err(dev, "failed to register dp interrupt\n"); > - return ret; > + goto err_clk; > } > > tc->have_irq = true; > @@ -2119,12 +2128,13 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); > if (ret) { > dev_err(tc->dev, "can not read device ID: %d\n", ret); > - return ret; > + goto err_clk; > } > > if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) { > dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev); > - return -EINVAL; > + ret = -EINVAL; > + goto err_clk; > } > > tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ > @@ -2164,7 +2174,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */ > ret = tc_aux_link_setup(tc); > if (ret) > - return ret; > + goto err_clk; > } > > tc->bridge.of_node = dev->of_node; > @@ -2176,11 +2186,15 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > ret = tc_mipi_dsi_host_attach(tc); > if (ret) { > drm_bridge_remove(&tc->bridge); > - return ret; > + goto err_clk; > } > } > > return 0; > + > +err_clk: > + clk_disable_unprepare(tc->refclk); > + return ret; That's a lot of error paths to disable a effectively always-on clock when something goes wrong. Could you maybe simplify this by using devm_add_action_or_reset for the clock disable? Seems like lots of watchdog drivers do this already if you need some inspiration. Regards, Lucas > } > > static int tc_remove(struct i2c_client *client) > @@ -2188,6 +2202,7 @@ static int tc_remove(struct i2c_client *client) > struct tc_data *tc = i2c_get_clientdata(client); > > drm_bridge_remove(&tc->bridge); > + clk_disable_unprepare(tc->refclk); > > return 0; > }
On 5/19/22 09:57, Lucas Stach wrote: > Am Donnerstag, dem 19.05.2022 um 01:36 +0200 schrieb Marek Vasut: >> The Refclk may be supplied by SoC clock output instead of crystal >> oscillator, make sure the clock are enabled before any other action >> is performed with the bridge chip, otherwise it may either fail to >> operate at all, or miss reset GPIO toggle. >> >> Fixes: 7caff0fc4296e ("drm/bridge: tc358767: Add DPI to eDP bridge driver") >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Jonas Karlman <jonas@kwiboo.se> >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Marek Vasut <marex@denx.de> >> Cc: Maxime Ripard <maxime@cerno.tech> >> Cc: Neil Armstrong <narmstrong@baylibre.com> >> Cc: Robert Foss <robert.foss@linaro.org> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> --- >> drivers/gpu/drm/bridge/tc358767.c | 51 ++++++++++++++++++++----------- >> 1 file changed, 33 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c >> index a8bb6de9e600a..4b15acbc73c4e 100644 >> --- a/drivers/gpu/drm/bridge/tc358767.c >> +++ b/drivers/gpu/drm/bridge/tc358767.c >> @@ -2055,10 +2055,24 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) >> if (ret) >> return ret; >> >> + tc->refclk = devm_clk_get(dev, "ref"); >> + if (IS_ERR(tc->refclk)) { >> + ret = PTR_ERR(tc->refclk); >> + dev_err(dev, "Failed to get refclk: %d\n", ret); >> + return ret; >> + } >> + >> + clk_prepare_enable(tc->refclk); >> + >> + /* tRSTW = 100 cycles , at 13 MHz that is ~7.69 us */ >> + usleep_range(10, 15); >> + >> /* Shut down GPIO is optional */ >> tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH); >> - if (IS_ERR(tc->sd_gpio)) >> - return PTR_ERR(tc->sd_gpio); >> + if (IS_ERR(tc->sd_gpio)) { >> + ret = PTR_ERR(tc->sd_gpio); >> + goto err_clk; >> + } >> >> if (tc->sd_gpio) { >> gpiod_set_value_cansleep(tc->sd_gpio, 0); >> @@ -2067,26 +2081,21 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) >> >> /* Reset GPIO is optional */ >> tc->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); >> - if (IS_ERR(tc->reset_gpio)) >> - return PTR_ERR(tc->reset_gpio); >> + if (IS_ERR(tc->reset_gpio)) { >> + ret = PTR_ERR(tc->reset_gpio); >> + goto err_clk; >> + } >> >> if (tc->reset_gpio) { >> gpiod_set_value_cansleep(tc->reset_gpio, 1); >> usleep_range(5000, 10000); >> } >> >> - tc->refclk = devm_clk_get(dev, "ref"); >> - if (IS_ERR(tc->refclk)) { >> - ret = PTR_ERR(tc->refclk); >> - dev_err(dev, "Failed to get refclk: %d\n", ret); >> - return ret; >> - } >> - >> tc->regmap = devm_regmap_init_i2c(client, &tc_regmap_config); >> if (IS_ERR(tc->regmap)) { >> ret = PTR_ERR(tc->regmap); >> dev_err(dev, "Failed to initialize regmap: %d\n", ret); >> - return ret; >> + goto err_clk; >> } >> >> ret = of_property_read_u32(dev->of_node, "toshiba,hpd-pin", >> @@ -2096,7 +2105,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) >> } else { >> if (tc->hpd_pin < 0 || tc->hpd_pin > 1) { >> dev_err(dev, "failed to parse HPD number\n"); >> - return ret; >> + goto err_clk; >> } >> } >> >> @@ -2110,7 +2119,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) >> "tc358767-irq", tc); >> if (ret) { >> dev_err(dev, "failed to register dp interrupt\n"); >> - return ret; >> + goto err_clk; >> } >> >> tc->have_irq = true; >> @@ -2119,12 +2128,13 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) >> ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); >> if (ret) { >> dev_err(tc->dev, "can not read device ID: %d\n", ret); >> - return ret; >> + goto err_clk; >> } >> >> if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) { >> dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev); >> - return -EINVAL; >> + ret = -EINVAL; >> + goto err_clk; >> } >> >> tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ >> @@ -2164,7 +2174,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) >> if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */ >> ret = tc_aux_link_setup(tc); >> if (ret) >> - return ret; >> + goto err_clk; >> } >> >> tc->bridge.of_node = dev->of_node; >> @@ -2176,11 +2186,15 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) >> ret = tc_mipi_dsi_host_attach(tc); >> if (ret) { >> drm_bridge_remove(&tc->bridge); >> - return ret; >> + goto err_clk; >> } >> } >> >> return 0; >> + >> +err_clk: >> + clk_disable_unprepare(tc->refclk); >> + return ret; > > That's a lot of error paths to disable a effectively always-on clock > when something goes wrong. Could you maybe simplify this by using > devm_add_action_or_reset for the clock disable? Seems like lots of > watchdog drivers do this already if you need some inspiration. I sent V2 as a separate patch.
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index a8bb6de9e600a..4b15acbc73c4e 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -2055,10 +2055,24 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) if (ret) return ret; + tc->refclk = devm_clk_get(dev, "ref"); + if (IS_ERR(tc->refclk)) { + ret = PTR_ERR(tc->refclk); + dev_err(dev, "Failed to get refclk: %d\n", ret); + return ret; + } + + clk_prepare_enable(tc->refclk); + + /* tRSTW = 100 cycles , at 13 MHz that is ~7.69 us */ + usleep_range(10, 15); + /* Shut down GPIO is optional */ tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH); - if (IS_ERR(tc->sd_gpio)) - return PTR_ERR(tc->sd_gpio); + if (IS_ERR(tc->sd_gpio)) { + ret = PTR_ERR(tc->sd_gpio); + goto err_clk; + } if (tc->sd_gpio) { gpiod_set_value_cansleep(tc->sd_gpio, 0); @@ -2067,26 +2081,21 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) /* Reset GPIO is optional */ tc->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); - if (IS_ERR(tc->reset_gpio)) - return PTR_ERR(tc->reset_gpio); + if (IS_ERR(tc->reset_gpio)) { + ret = PTR_ERR(tc->reset_gpio); + goto err_clk; + } if (tc->reset_gpio) { gpiod_set_value_cansleep(tc->reset_gpio, 1); usleep_range(5000, 10000); } - tc->refclk = devm_clk_get(dev, "ref"); - if (IS_ERR(tc->refclk)) { - ret = PTR_ERR(tc->refclk); - dev_err(dev, "Failed to get refclk: %d\n", ret); - return ret; - } - tc->regmap = devm_regmap_init_i2c(client, &tc_regmap_config); if (IS_ERR(tc->regmap)) { ret = PTR_ERR(tc->regmap); dev_err(dev, "Failed to initialize regmap: %d\n", ret); - return ret; + goto err_clk; } ret = of_property_read_u32(dev->of_node, "toshiba,hpd-pin", @@ -2096,7 +2105,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) } else { if (tc->hpd_pin < 0 || tc->hpd_pin > 1) { dev_err(dev, "failed to parse HPD number\n"); - return ret; + goto err_clk; } } @@ -2110,7 +2119,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) "tc358767-irq", tc); if (ret) { dev_err(dev, "failed to register dp interrupt\n"); - return ret; + goto err_clk; } tc->have_irq = true; @@ -2119,12 +2128,13 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); if (ret) { dev_err(tc->dev, "can not read device ID: %d\n", ret); - return ret; + goto err_clk; } if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) { dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev); - return -EINVAL; + ret = -EINVAL; + goto err_clk; } tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ @@ -2164,7 +2174,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */ ret = tc_aux_link_setup(tc); if (ret) - return ret; + goto err_clk; } tc->bridge.of_node = dev->of_node; @@ -2176,11 +2186,15 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) ret = tc_mipi_dsi_host_attach(tc); if (ret) { drm_bridge_remove(&tc->bridge); - return ret; + goto err_clk; } } return 0; + +err_clk: + clk_disable_unprepare(tc->refclk); + return ret; } static int tc_remove(struct i2c_client *client) @@ -2188,6 +2202,7 @@ static int tc_remove(struct i2c_client *client) struct tc_data *tc = i2c_get_clientdata(client); drm_bridge_remove(&tc->bridge); + clk_disable_unprepare(tc->refclk); return 0; }
The Refclk may be supplied by SoC clock output instead of crystal oscillator, make sure the clock are enabled before any other action is performed with the bridge chip, otherwise it may either fail to operate at all, or miss reset GPIO toggle. Fixes: 7caff0fc4296e ("drm/bridge: tc358767: Add DPI to eDP bridge driver") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Marek Vasut <marex@denx.de> Cc: Maxime Ripard <maxime@cerno.tech> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Robert Foss <robert.foss@linaro.org> Cc: Sam Ravnborg <sam@ravnborg.org> --- drivers/gpu/drm/bridge/tc358767.c | 51 ++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 18 deletions(-)