RE: [PATCH v5 4/4] media: i2c: ds90ub953: use guard() to simplify code

[G.N. Zhou (OSS)]

Hi Tomi,

Thanks for your review.

> -----Original Message-----
> From: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, March 11, 2026 4:45 PM
> To: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Frank Li
> <frank.li@xxxxxxx>; Vladimir Zapolskiy <vz@xxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; G.N. Zhou
> <guoniu.zhou@xxxxxxx>
> Subject: Re: [PATCH v5 4/4] media: i2c: ds90ub953: use guard() to simplify code
> 
> Hi,
> 
> On 28/02/2026 08:18, Guoniu Zhou wrote:
> > From: Guoniu Zhou <guoniu.zhou@xxxxxxx>
> >
> > Use guard() to simplify mutex locking. No functional change.
> 
> That's not strictly true, as the unlock will happen later with this patch. Still, this
> cleanup makes sense.
> 
> >
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > Signed-off-by: Guoniu Zhou <guoniu.zhou@xxxxxxx>
> > ---
> >  drivers/media/i2c/ds90ub953.c | 34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ds90ub953.c
> > b/drivers/media/i2c/ds90ub953.c index
> >
> a85c6a9b64070491db161ca1586179dba9c69cb0..3a459687aefce05ac025a51
> 7c8d4
> > cc6d76cd7293 100644
> > --- a/drivers/media/i2c/ds90ub953.c
> > +++ b/drivers/media/i2c/ds90ub953.c
> > @@ -113,20 +113,18 @@ static int ub953_read(struct ub953_data *priv, u8
> reg, u8 *val, int *err)
> >  	if (err && *err)
> >  		return *err;
> >
> > -	mutex_lock(&priv->reg_lock);
> > +	guard(mutex)(&priv->reg_lock);
> >
> >  	ret = regmap_read(priv->regmap, reg, &v);
> >  	if (ret) {
> >  		dev_err(&priv->client->dev, "Cannot read register
> 0x%02x: %d\n",
> >  			reg, ret);
> > -		goto out_unlock;
> > +		goto err;
> >  	}
> >
> >  	*val = v;
> >
> > -out_unlock:
> > -	mutex_unlock(&priv->reg_lock);
> > -
> > +err:
> 
> I think the label should be just "out", as this is not an error path (even if we
> jump to the label when handling an error). Similar comment to the below
> cases.
> 

Agree, will update in next version.

> >  	if (ret && err)
> >  		*err = ret;
> >
> > @@ -140,15 +138,13 @@ static int ub953_write(struct ub953_data *priv, u8
> reg, u8 val, int *err)
> >  	if (err && *err)
> >  		return *err;
> >
> > -	mutex_lock(&priv->reg_lock);
> > +	guard(mutex)(&priv->reg_lock);
> >
> >  	ret = regmap_write(priv->regmap, reg, val);
> >  	if (ret)
> >  		dev_err(&priv->client->dev,
> >  			"Cannot write register 0x%02x: %d\n", reg, ret);
> >
> > -	mutex_unlock(&priv->reg_lock);
> > -
> >  	if (ret && err)
> >  		*err = ret;
> >
> > @@ -185,18 +181,18 @@ static int ub953_read_ind(struct ub953_data
> *priv, u8 block, u8 reg, u8 *val,
> >  	if (err && *err)
> >  		return *err;
> >
> > -	mutex_lock(&priv->reg_lock);
> > +	guard(mutex)(&priv->reg_lock);
> >
> >  	ret = ub953_select_ind_reg_block(priv, block);
> >  	if (ret)
> > -		goto out_unlock;
> > +		goto err;
> >
> >  	ret = regmap_write(priv->regmap, UB953_REG_IND_ACC_ADDR, reg);
> >  	if (ret) {
> >  		dev_err(&priv->client->dev,
> >  			"Write to IND_ACC_ADDR failed when
> reading %u:0x%02x: %d\n",
> >  			block, reg, ret);
> > -		goto out_unlock;
> > +		goto err;
> >  	}
> >
> >  	ret = regmap_read(priv->regmap, UB953_REG_IND_ACC_DATA, &v);
> @@
> > -204,14 +200,12 @@ static int ub953_read_ind(struct ub953_data *priv, u8
> block, u8 reg, u8 *val,
> >  		dev_err(&priv->client->dev,
> >  			"Write to IND_ACC_DATA failed when
> reading %u:0x%02x: %d\n",
> >  			block, reg, ret);
> > -		goto out_unlock;
> > +		goto err;
> >  	}
> >
> >  	*val = v;
> >
> > -out_unlock:
> > -	mutex_unlock(&priv->reg_lock);
> > -
> > +err:
> >  	if (ret && err)
> >  		*err = ret;
> >
> > @@ -227,18 +221,18 @@ static int ub953_write_ind(struct ub953_data
> *priv, u8 block, u8 reg, u8 val,
> >  	if (err && *err)
> >  		return *err;
> >
> > -	mutex_lock(&priv->reg_lock);
> > +	guard(mutex)(&priv->reg_lock);
> >
> >  	ret = ub953_select_ind_reg_block(priv, block);
> >  	if (ret)
> > -		goto out_unlock;
> > +		goto err;
> >
> >  	ret = regmap_write(priv->regmap, UB953_REG_IND_ACC_ADDR, reg);
> >  	if (ret) {
> >  		dev_err(&priv->client->dev,
> >  			"Write to IND_ACC_ADDR failed when
> writing %u:0x%02x: %d\n",
> >  			block, reg, ret);
> > -		goto out_unlock;
> > +		goto err;
> >  	}
> >
> >  	ret = regmap_write(priv->regmap, UB953_REG_IND_ACC_DATA, val);
> @@
> > -248,9 +242,7 @@ static int ub953_write_ind(struct ub953_data *priv, u8
> block, u8 reg, u8 val,
> >  			block, reg, ret);
> >  	}
> >
> > -out_unlock:
> > -	mutex_unlock(&priv->reg_lock);
> > -
> > +err:
> >  	if (ret && err)
> >  		*err = ret;
> >
> >