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
