Re: [PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[Yury Norov <yury.norov@gmail.com>]

On Wed, Dec 10, 2025 at 10:18:42AM +0000, David Laight wrote:
> On Wed, 10 Dec 2025 10:41:02 +0100
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:
> > > On Wed, 10 Dec 2025 06:56:17 +0100
> > > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> > >   
> > > > $subject has typo: thunderblot -> thunderbolt ;-)
> > > > 
> > > > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:  
> > > > > From: David Laight <david.laight.linux@gmail.com>
> > > > > 
> > > > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > > > parameter, this can't be used with bifields.
> > > > > 
> > > > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > > > to force the type to int.
> > > > > 
> > > > > So add a zero here.
> > > > > 
> > > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > > ---
> > > > >  drivers/thunderbolt/tb.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > > > index e96474f17067..7ca2b5a0f01e 100644
> > > > > --- a/drivers/thunderbolt/tb.h
> > > > > +++ b/drivers/thunderbolt/tb.h
> > > > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > > > >   */
> > > > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > > > >  {
> > > > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);    
> > > > 
> > > > Can't this use a cast instead? If not then can you also add a comment here
> > > > because next someone will send a patch "fixing" the unnecessary addition.  
> > > 
> > > A cast can do other (possibly incorrect) conversions, adding zero is never going
> > > to so any 'damage' - even if it looks a bit odd.
> > > 
> > > Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> > > just do:
> > > 	/* The major version is in the top 3 bits */
> > > 	return sw->config.thunderbolt_version > 5;  
> > 
> > You mean 
> > 
> > 	return sw->config.thunderbolt_version >> 5;
> > 
> > ?
> > 
> > Yes that works but I prefer then:
> > 
> > 	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;
> 
> I've put that in for the next version (without the comment line).

FIELD_GET() is here exactly to let people to not opencode this
error-prone bit manipulation. So, let's continue using it.

David, can you explain in details why this code needs to be fixed? Why
and when typecast wouldn't work so that you have to use an ugly '+0'
hack, or even drop the FIELD_GET().

My current understanding is that the existing FIELD_GET()
implementation works well with any data types, including bitfields,
and what you suggested in this series - does not.

If it's correct, I don't think that switching to your version is
well-justified.

Thanks,
Yury