Hi Kangjie,
Thank you for the patch.
On Thursday, 20 December 2018 09:41:16 EET Kangjie Lu wrote:
> Both anx78xx_set_bits() and anx78xx_clear_bits() in the poweron process
> may fail. The fix inserts checks for their return values. If the poweron
> process fails, it calls anx78xx_poweroff().
>
> Signed-off-by: Kangjie Lu <kjlu@xxxxxxx>
> ---
> drivers/gpu/drm/bridge/analogix-anx78xx.c | 26 ++++++++++++++++-------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> b/drivers/gpu/drm/bridge/analogix-anx78xx.c index
> f8433c93f463..a57104c71739 100644
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> @@ -610,20 +610,20 @@ static int anx78xx_enable_interrupts(struct anx78xx
> *anx78xx) return 0;
> }
>
> -static void anx78xx_poweron(struct anx78xx *anx78xx)
> +static int anx78xx_poweron(struct anx78xx *anx78xx)
> {
> struct anx78xx_platform_data *pdata = &anx78xx->pdata;
> - int err;
> + int err = 0;
>
> if (WARN_ON(anx78xx->powered))
> - return;
> + return err;
You can return 0 here.
>
> if (pdata->dvdd10) {
> err = regulator_enable(pdata->dvdd10);
> if (err) {
> DRM_ERROR("Failed to enable DVDD10 regulator: %d\n",
> err);
> - return;
> + return err;
> }
>
> usleep_range(1000, 2000);
> @@ -638,12 +638,18 @@ static void anx78xx_poweron(struct anx78xx *anx78xx)
> gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
>
> /* Power on registers module */
> - anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
> + err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
> SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
> - anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,
> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2],
> SP_POWERDOWN_CTRL_REG, SP_REGISTER_PD | SP_TOTAL_PD);
If both functions fail with a different error code, this may result in a
meaningless error being returned. One option is to do
err = anx78xx_set_bits(...);
if (!err)
err = anx78xx_clear_bits(...);
The construct gets quickly ugly though, so I sometimes define register
accessors as taking an int * for the error code instead of returning it.
void write(..., int *status)
{
if (*status)
return;
*status = real_write(...);
}
and then use it as
int status = 0;
write(..., &status);
write(..., &status);
write(..., &status);
write(..., &status);
write(..., &status);
return status;
This may be overkill here.
> + if (err) {
> + anx78xx_poweroff(anx78xx);
> + return err;
> + }
>
> anx78xx->powered = true;
> +
> + return err;
And return 0 here too, removing the need to initialize the err variable to 0.
> }
>
> static void anx78xx_poweroff(struct anx78xx *anx78xx)
> @@ -1144,7 +1150,9 @@ static irqreturn_t anx78xx_hpd_threaded_handler(int
> irq, void *data) mutex_lock(&anx78xx->lock);
>
> /* Cable is pulled, power on the chip */
> - anx78xx_poweron(anx78xx);
> + err = anx78xx_poweron(anx78xx);
> + if (err)
> + DRM_ERROR("Failed to power on the chip: %d\n", err);
Wouldn't it be better to move the error message to the an78xx_poweron()
function ? That way it would also be printed in the probe() path.
> err = anx78xx_enable_interrupts(anx78xx);
> if (err)
> @@ -1379,7 +1387,9 @@ static int anx78xx_i2c_probe(struct i2c_client
> *client, }
>
> /* Look for supported chip ID */
> - anx78xx_poweron(anx78xx);
> + err = anx78xx_poweron(anx78xx);
> + if (err)
> + goto err_poweroff;
If poweron fails, doesn't it clean up after itself ? Do you need to call
poweroff here ?
> err = regmap_read(anx78xx->map[I2C_IDX_TX_P2], SP_DEVICE_IDL_REG,
> &idl);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
