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 <ma...@denx.de>
> Cc: Jonas Karlman <jo...@kwiboo.se>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Lucas Stach <l.st...@pengutronix.de>
> Cc: Marek Vasut <ma...@denx.de>
> Cc: Maxime Ripard <max...@cerno.tech>
> Cc: Neil Armstrong <narmstr...@baylibre.com>
> Cc: Robert Foss <robert.f...@linaro.org>
> Cc: Sam Ravnborg <s...@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;
> }
