linux-kernel - Re: [PATCH 12/13] media: i2c: ds90ub913: Add error handling to ub913_hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Zy31nf_B-O_UTXSo@smile.fi.intel.com>
Date: Fri, 8 Nov 2024 13:27:25 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jai Luthra <jai.luthra@...asonboard.com>
Subject: Re: [PATCH 12/13] media: i2c: ds90ub913: Add error handling to
 ub913_hw_init()

On Fri, Nov 08, 2024 at 11:34:09AM +0200, Tomi Valkeinen wrote:
> On 10/10/2024 17:04, Andy Shevchenko wrote:
> > On Fri, Oct 04, 2024 at 05:46:43PM +0300, Tomi Valkeinen wrote:
> > > Add error handling to ub913_hw_init() using a new helper function,
> > > ub913_update_bits().

...

> > > +	ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
> > > +				UB913_REG_GENERAL_CFG_PCLK_RISING,
> > > +				priv->pclk_polarity_rising ?
> > > +					UB913_REG_GENERAL_CFG_PCLK_RISING :
> > > +					0);
> > 
> > So, you can use regmap_set_bits() / regmap_clear_bits() instead of this
> > ternary. It also gives one parameter less to the regmap calls.
> 
> True... But is it better?

In my opinion yes, because it's clearer on what's going on.
It has no (semi-)hidden choice, so code wise it most likely
will be the same at the end. So we are speaking only about
C-level of readability.

> if (priv->pclk_polarity_rising)
> 	ret = regmap_set_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 			      UB913_REG_GENERAL_CFG_PCLK_RISING);
> else
> 	ret = regmap_clear_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 				UB913_REG_GENERAL_CFG_PCLK_RISING);
> 
> The call itself is more readable there, but then again, as we're setting the
> value of a bit, I dislike having if/else with two calls for a single
> assignment.

FTR, there was an attempt to add _assign() in similar way how it's done with
bitops (set/clear/assign) to regmap, but had been rejected by Mark. I don't
remember detail why, though.

> Using FIELD_PREP is perhaps a bit better than the ternary:
> 
> ret = ub913_update_bits(priv, UB913_REG_GENERAL_CFG,
> 			UB913_REG_GENERAL_CFG_PCLK_RISING,
> 			FIELD_PREP(UB913_REG_GENERAL_CFG_PCLK_RISING,
> 				   priv->pclk_polarity_rising));
> 
> I think I'd like best a function to set/clear a bitmask with a boolean:
> 
> ret = regmap_toggle_bits(priv->regmap, UB913_REG_GENERAL_CFG,
> 			 UB913_REG_GENERAL_CFG_PCLK_RISING,
> 			 priv->pclk_polarity_rising);
> 
> For now, I think I'll go with the FIELD_PREP() version. It's perhaps a bit
> better than the ternary.

Okay.

-- 
With Best Regards,
Andy Shevchenko