<div dir="ltr">Thanks for the tip! Will submit patch V2 soon.<br></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 20, 2018 at 11:07 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Kangjie,<br>
<br>
Thank you for the patch.<br>
<br>
On Thursday, 20 December 2018 09:41:16 EET Kangjie Lu wrote:<br>
> Both anx78xx_set_bits() and anx78xx_clear_bits() in the poweron process<br>
> may fail. The fix inserts checks for their return values. If the poweron<br>
> process fails, it calls anx78xx_poweroff().<br>
> <br>
> Signed-off-by: Kangjie Lu <<a href="mailto:kjlu@umn.edu" target="_blank">kjlu@umn.edu</a>><br>
> ---<br>
>  drivers/gpu/drm/bridge/analogix-anx78xx.c | 26 ++++++++++++++++-------<br>
>  1 file changed, 18 insertions(+), 8 deletions(-)<br>
> <br>
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c<br>
> b/drivers/gpu/drm/bridge/analogix-anx78xx.c index<br>
> f8433c93f463..a57104c71739 100644<br>
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c<br>
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c<br>
> @@ -610,20 +610,20 @@ static int anx78xx_enable_interrupts(struct anx78xx<br>
> *anx78xx) return 0;<br>
>  }<br>
> <br>
> -static void anx78xx_poweron(struct anx78xx *anx78xx)<br>
> +static int anx78xx_poweron(struct anx78xx *anx78xx)<br>
>  {<br>
>       struct anx78xx_platform_data *pdata = &anx78xx->pdata;<br>
> -     int err;<br>
> +     int err = 0;<br>
> <br>
>       if (WARN_ON(anx78xx->powered))<br>
> -             return;<br>
> +             return err;<br>
<br>
You can return 0 here.<br>
<br>
> <br>
>       if (pdata->dvdd10) {<br>
>               err = regulator_enable(pdata->dvdd10);<br>
>               if (err) {<br>
>                       DRM_ERROR("Failed to enable DVDD10 regulator: %d\n",<br>
>                                 err);<br>
> -                     return;<br>
> +                     return err;<br>
>               }<br>
> <br>
>               usleep_range(1000, 2000);<br>
> @@ -638,12 +638,18 @@ static void anx78xx_poweron(struct anx78xx *anx78xx)<br>
>       gpiod_set_value_cansleep(pdata->gpiod_reset, 0);<br>
> <br>
>       /* Power on registers module */<br>
> -     anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,<br>
> +     err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,<br>
> SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);<br>
> -     anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], SP_POWERDOWN_CTRL_REG,<br>
> +     err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2],<br>
> SP_POWERDOWN_CTRL_REG, SP_REGISTER_PD | SP_TOTAL_PD);<br>
<br>
If both functions fail with a different error code, this may result in a <br>
meaningless error being returned. One option is to do<br>
<br>
        err = anx78xx_set_bits(...);<br>
        if (!err)<br>
                err = anx78xx_clear_bits(...);<br>
<br>
The construct gets quickly ugly though, so I sometimes define register <br>
accessors as taking an int * for the error code instead of returning it.<br>
<br>
void write(..., int *status)<br>
{<br>
        if (*status)<br>
                return;<br>
<br>
        *status = real_write(...);<br>
}<br>
<br>
and then use it as<br>
<br>
        int status = 0;<br>
<br>
        write(..., &status);<br>
        write(..., &status);<br>
        write(..., &status);<br>
        write(..., &status);<br>
        write(..., &status);<br>
<br>
        return status;<br>
<br>
This may be overkill here.<br>
<br>
> +     if (err) {<br>
> +             anx78xx_poweroff(anx78xx);<br>
> +             return err;<br>
> +     }<br>
> <br>
>       anx78xx->powered = true;<br>
> +<br>
> +     return err;<br>
<br>
And return 0 here too, removing the need to initialize the err variable to 0.<br>
<br>
>  }<br>
> <br>
>  static void anx78xx_poweroff(struct anx78xx *anx78xx)<br>
> @@ -1144,7 +1150,9 @@ static irqreturn_t anx78xx_hpd_threaded_handler(int<br>
> irq, void *data) mutex_lock(&anx78xx->lock);<br>
> <br>
>       /* Cable is pulled, power on the chip */<br>
> -     anx78xx_poweron(anx78xx);<br>
> +     err = anx78xx_poweron(anx78xx);<br>
> +     if (err)<br>
> +             DRM_ERROR("Failed to power on the chip: %d\n", err);<br>
<br>
Wouldn't it be better to move the error message to the an78xx_poweron() <br>
function ? That way it would also be printed in the probe() path.<br>
<br>
>       err = anx78xx_enable_interrupts(anx78xx);<br>
>       if (err)<br>
> @@ -1379,7 +1387,9 @@ static int anx78xx_i2c_probe(struct i2c_client<br>
> *client, }<br>
> <br>
>       /* Look for supported chip ID */<br>
> -     anx78xx_poweron(anx78xx);<br>
> +     err = anx78xx_poweron(anx78xx);<br>
> +     if (err)<br>
> +             goto err_poweroff;<br>
<br>
If poweron fails, doesn't it clean up after itself ? Do you need to call <br>
poweroff here ?<br>
<br>
>       err = regmap_read(anx78xx->map[I2C_IDX_TX_P2], SP_DEVICE_IDL_REG,<br>
>                         &idl);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
<br>
<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div dir="ltr"><div><span style="font-size:12.8px">Kangjie Lu</span><br></div><div>Assistant Professor</div><div>Department of Computer Science and Engineering</div><div>University of Minnesota<br></div><div><a href="https://www-users.cs.umn.edu/~kjlu" target="_blank">Personal homepage</a></div></div></div></div></div></div></div></div>