Received-SPF: pass (google.com: domain of linux-kernel+bounces-90367-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Mon, 04 Mar 2024 11:00:45 +0100
Message-Id: <CZKVMZC3BEXV.380JV1IP5RYFG@bootlin.com>
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>
To: "Andi Shyti" <andi.shyti@kernel.org>
From: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>
Subject: Re: [PATCH v2 05/11] i2c: nomadik: use bitops helpers
References: <20240229-mbly-i2c-v2-0-b32ed18c098c@bootlin.com>
 <20240229-mbly-i2c-v2-5-b32ed18c098c@bootlin.com>
 <3kooaexx6vhlfwoojcpmnyhagupqwppwenjh4k7ucxbvlfpjn6@e3b7c3ocu6kc>
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)
> >  }
> > =20
> >  /* 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 *p=
riv, u16 flags)
> >  	u32 mcr =3D 0;
> >  	unsigned short slave_adr_3msb_bits;
> > =20
> > -	mcr |=3D GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
> > +	mcr |=3D FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
> > =20
> >  	if (unlikely(flags & I2C_M_TEN)) {
> >  		/* 10-bit address transaction */
> > -		mcr |=3D GEN_MASK(2, I2C_MCR_AM, 12);
> > +		mcr |=3D 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 =3D (priv->cli.slave_adr >> 7) & 0x7;
> > +		slave_adr_3msb_bits =3D 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 |=3D GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
> > +		mcr |=3D 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 =3D 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=C3=A9o Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com