Re: [PATCH v2 05/11] i2c: nomadik: use bitops helpers

From: "Théo Lebrun" <theo.lebrun@bootlin.com>
To: "Andi Shyti" <andi.shyti@kernel.org>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-i2c@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-mips@vger.kernel.org>,
	"Gregory Clement" <gregory.clement@bootlin.com>,
	"Vladimir Kondratiev" <vladimir.kondratiev@mobileye.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Tawfik Bayouk" <tawfik.bayouk@mobileye.com>
Subject: Re: [PATCH v2 05/11] i2c: nomadik: use bitops helpers
Date: Mon, 04 Mar 2024 11:00:45 +0100	[thread overview]
Message-ID: <CZKVMZC3BEXV.380JV1IP5RYFG@bootlin.com> (raw)
In-Reply-To: <3kooaexx6vhlfwoojcpmnyhagupqwppwenjh4k7ucxbvlfpjn6@e3b7c3ocu6kc>

Hello,

On Sat Mar 2, 2024 at 2:31 AM CET, Andi Shyti wrote:

[...]

> > @@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
> >  }
> >  
> >  /* enable peripheral, master mode operation */
> > -#define DEFAULT_I2C_REG_CR	((1 << 1) | I2C_CR_PE)
> > +#define DEFAULT_I2C_REG_CR	(FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)
>
> 0b01?

OM is a two-bit field. We want to put 0b01 in that. We do not declare
constants for its 4 potential values. Values are:

 - 0b00 slave mode
 - 0b01 master mode
 - 0b10 master/slave mode
 - 0b11 reserved

To me the comment above DEFAULT_I2C_REG_CR is enough to show that we go
into master mode. We could declare all values as constants but only
0b01 would get used.

>
> >  /**
> >   * load_i2c_mcr_reg() - load the MCR register
> > @@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
> >  	u32 mcr = 0;
> >  	unsigned short slave_adr_3msb_bits;
> >  
> > -	mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
> > +	mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
> >  
> >  	if (unlikely(flags & I2C_M_TEN)) {
> >  		/* 10-bit address transaction */
> > -		mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
> > +		mcr |= FIELD_PREP(I2C_MCR_AM, 2);
> >  		/*
> >  		 * Get the top 3 bits.
> >  		 * EA10 represents extended address in MCR. This includes
> >  		 * the extension (MSB bits) of the 7 bit address loaded
> >  		 * in A7
> >  		 */
> > -		slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
> > +		slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
> > +						priv->cli.slave_adr);
>
> This looks like the only one not having a define. Shall we give a
> definition to GENMASK(9, 7)?

Indeed. What should it be named? It could be generic; this is about
getting the upper 3 bits from an extended (10-bit) I2C address?

> > -		mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
> > +		mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);
>
> ...
>
> > @@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
> >  	 * during the transaction.
> >  	 */
> >  	case I2C_IT_BERR:
> > +	{
> > +		u32 sr = readl(priv->virtbase + I2C_SR);
>
> please give a blank line after the variable definition.
>
> Besides, I'd prefer the assignment, when it is a bit more
> complex, in a different line, as well.

Will do.

> Rest looks OK, didn't see anything off.

Thanks for the review Andi!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com