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
next prev parent reply other threads:[~2024-03-04 10:00 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-29 18:10 [PATCH v2 00/11] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts Théo Lebrun
2024-02-29 18:10 ` [PATCH v2 01/11] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example Théo Lebrun
2024-02-29 19:26 ` Rob Herring
2024-03-01 15:11 ` Rob Herring
2024-03-01 15:47 ` Théo Lebrun
2024-02-29 18:10 ` [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema Théo Lebrun
2024-02-29 19:26 ` Rob Herring
2024-03-01 6:37 ` Krzysztof Kozlowski
2024-03-01 6:53 ` Guenter Roeck
2024-03-01 9:41 ` Théo Lebrun
2024-03-01 10:13 ` Krzysztof Kozlowski
2024-03-01 10:44 ` Théo Lebrun
2024-03-01 11:35 ` Krzysztof Kozlowski
2024-03-01 14:09 ` Théo Lebrun
2024-03-01 14:13 ` Krzysztof Kozlowski
2024-03-01 15:35 ` Rob Herring
2024-03-01 15:52 ` Théo Lebrun
2024-03-01 15:38 ` Rob Herring
2024-03-01 19:21 ` Guenter Roeck
2024-02-29 18:10 ` [PATCH v2 03/11] i2c: nomadik: rename private struct pointers from dev to priv Théo Lebrun
2024-03-02 0:16 ` [SPAM] " Andi Shyti
2024-03-04 9:13 ` Wolfram Sang
2024-02-29 18:10 ` [PATCH v2 04/11] i2c: nomadik: simplify IRQ masking logic Théo Lebrun
2024-03-02 0:39 ` [SPAM] " Andi Shyti
2024-03-04 9:46 ` Théo Lebrun
2024-02-29 18:10 ` [PATCH v2 05/11] i2c: nomadik: use bitops helpers Théo Lebrun
2024-03-02 1:31 ` Andi Shyti
2024-03-04 10:00 ` Théo Lebrun [this message]
2024-02-29 18:10 ` [PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer Théo Lebrun
2024-03-04 9:18 ` Wolfram Sang
2024-03-04 10:14 ` Théo Lebrun
2024-03-04 11:37 ` Wolfram Sang
2024-03-04 13:54 ` [SPAM] " Andi Shyti
2024-03-04 14:32 ` Théo Lebrun
2024-03-04 15:09 ` Andi Shyti
2024-02-29 18:10 ` [PATCH v2 07/11] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout Théo Lebrun
2024-03-04 9:23 ` Wolfram Sang
2024-03-04 13:55 ` Andi Shyti
2024-02-29 18:10 ` [PATCH v2 08/11] i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree Théo Lebrun
2024-02-29 21:04 ` Linus Walleij
2024-03-04 9:25 ` Wolfram Sang
2024-03-04 13:57 ` Andi Shyti
2024-02-29 18:10 ` [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller Théo Lebrun
2024-02-29 21:08 ` Linus Walleij
2024-03-04 9:27 ` Wolfram Sang
2024-03-04 10:25 ` Théo Lebrun
2024-03-04 14:08 ` Andi Shyti
2024-03-04 14:53 ` Théo Lebrun
2024-02-29 18:10 ` [PATCH v2 10/11] MIPS: mobileye: eyeq5: add 5 I2C controller nodes Théo Lebrun
2024-02-29 21:09 ` Linus Walleij
2024-02-29 18:10 ` [PATCH v2 11/11] MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor Théo Lebrun
2024-02-29 21:09 ` Linus Walleij
2024-03-06 1:49 ` [PATCH v2 00/11] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts Andi Shyti
2024-03-06 9:34 ` Théo Lebrun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CZKVMZC3BEXV.380JV1IP5RYFG@bootlin.com \
--to=theo.lebrun@bootlin.com \
--cc=andi.shyti@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregory.clement@bootlin.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=tawfik.bayouk@mobileye.com \
--cc=thomas.petazzoni@bootlin.com \
--cc=tsbogend@alpha.franken.de \
--cc=vladimir.kondratiev@mobileye.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).