Thanks for the tip! Will submit patch V2 soon.
On Thu, Dec 20, 2018 at 11:07 PM Laurent Pinchart <
laurent.pinch...@ideasonboard.com> wrote:
> 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 <k...@umn.edu>
> > ---
> > 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
>
>
>
>
--
Kangjie Lu
Assistant Professor
Department of Computer Science and Engineering
University of Minnesota
Personal homepage <https://www-users.cs.umn.edu/~kjlu>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
