linux-kernel - Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

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: <20250731140409.00000029@huawei.com>
Date: Thu, 31 Jul 2025 14:04:09 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Dixit Parmar <dixitparmar19@...il.com>
CC: Jonathan Cameron <jic23@...nel.org>, David Lechner
	<dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, "Krzysztof
 Kozlowski" <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
	<devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D
 3D Magentic sensor


Please crop to the remaining discussion points.

> > > > > +
> > > > > +#define TLV493D_DATA_X_GET(b)	\
> > > > > +	sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > > > > +			(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)    
> > > > 
> > > > These are odd enough I'd make them c functions rather than macros. Burn a few lines
> > > > for better readability. 
> > > >     
> > > I saw this kind of data retrival and formation from registers as macros so I sticked to
> > > it. Having all these as function will also require a seperate function
> > > for each channel coz the masks and the layout of the bits changes over
> > > the register. Do you still recommend it as c functions?  
> > 
> > Is it more than 4 short functions?  I'd burn the few lines that costs.
> > 
> > s32 tlv493d_data_y_get(u8 *buff)
> > {
> > 	u16 val = FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> > 		  FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> > 
> > 	return sign_extend32(val, 11);
> > }  
> Okay.
> Will a single function with channel as arguments will be better?

IIRC I gave that a go as my first try before falling back to this. 
You either need a look up table, or you need to pass
a lot of parameters.  In the end it felt simpler to just have 4 small functions.

If you can come up with a clean and readable way of doing so, go for it!

Jonathan