From: "Thierry Reding" <thierry.reding@gmail.com>
To: "Marc Dietrich" <marvin24@gmx.de>, <linux-staging@lists.linux.dev>
Cc: <linux-tegra@vger.kernel.org>, <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 3/4] staging: nvec: make i2c controller register writes robust
Date: Fri, 05 Apr 2024 16:33:23 +0200 [thread overview]
Message-ID: <D0C9H5M9NHBE.2AB0FZ1PDYZO4@gmail.com> (raw)
In-Reply-To: <20240405140906.77831-4-marvin24@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 3933 bytes --]
On Fri Apr 5, 2024 at 4:09 PM CEST, Marc Dietrich wrote:
> The i2c controller needs to read back the data written to its registers.
> This way we can avoid the long delay in the interrupt handler.
s/i2c/I2C/ perhaps? Also, looking at the preceding patches it looks to
me like the reason why we can drop the udelay() call is not just because
we read back, but also because we actually wait for completion. If so,
maybe that should also be mentioned.
>
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
> drivers/staging/nvec/nvec.c | 39 +++++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index 282a664c9176..9914c30b6933 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -565,6 +565,20 @@ static void nvec_tx_set(struct nvec_chip *nvec)
> (uint)nvec->tx->size, nvec->tx->data[1]);
> }
>
> +/**
> + * i2c_writel - savely write to an i2c client controller register
"safely", also "I2C"
> + @ val: value to be written
> + @ reg: register to write to
The formatting here looks odd. I think this is supposed to be:
* @val:
* @reg:
Thierry
> + */
> +
> +static void i2c_writel(u32 val, void *reg)
> +{
> + writel_relaxed(val, reg);
> +
> + /* read back register to make sure that register writes completed */
> + readl_relaxed(reg);
> +}
> +
> /**
> * nvec_interrupt - Interrupt handler
> * @irq: The IRQ
> @@ -599,7 +613,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
> if ((status & RNW) == 0) {
> received = readl(nvec->base + I2C_SL_RCVD);
> if (status & RCVD)
> - writel(0, nvec->base + I2C_SL_RCVD);
> + i2c_writel(0, nvec->base + I2C_SL_RCVD);
> }
>
> if (status == (I2C_SL_IRQ | RCVD))
> @@ -691,7 +705,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>
> /* Send data if requested, but not on end of transmission */
> if ((status & (RNW | END_TRANS)) == RNW)
> - writel(to_send, nvec->base + I2C_SL_RCVD);
> + i2c_writel(to_send, nvec->base + I2C_SL_RCVD);
>
> /* If we have send the first byte */
> if (status == (I2C_SL_IRQ | RNW | RCVD))
> @@ -708,15 +722,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
> status & RCVD ? " RCVD" : "",
> status & RNW ? " RNW" : "");
>
> - /*
> - * TODO: replace the udelay with a read back after each writel above
> - * in order to work around a hardware issue, see i2c-tegra.c
> - *
> - * Unfortunately, this change causes an intialisation issue with the
> - * touchpad, which needs to be fixed first.
> - */
> - udelay(100);
> -
> return IRQ_HANDLED;
> }
>
> @@ -732,15 +737,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
>
> val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
> (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
> - writel(val, nvec->base + I2C_CNFG);
> + i2c_writel(val, nvec->base + I2C_CNFG);
>
> clk_set_rate(nvec->i2c_clk, 8 * 80000);
>
> - writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
> - writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
> + i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
> + i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
>
> - writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
> - writel(0, nvec->base + I2C_SL_ADDR2);
> + i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
> + i2c_writel(0, nvec->base + I2C_SL_ADDR2);
>
> enable_irq(nvec->irq);
> }
> @@ -749,7 +754,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
> static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
> {
> disable_irq(nvec->irq);
> - writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
> + i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
> clk_disable_unprepare(nvec->i2c_clk);
> }
> #endif
> --
> 2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2024-04-05 14:33 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-05 14:09 [PATCH 0/4] Improve robustnes during initialization Marc Dietrich
2024-04-05 14:09 ` [PATCH 1/4] staging: nvec: make keyboard init synchronous Marc Dietrich
2024-04-05 14:38 ` Thierry Reding
2024-04-05 15:15 ` Dan Carpenter
2024-04-05 15:19 ` Dan Carpenter
2024-04-06 12:26 ` Marc Dietrich
2024-04-05 14:09 ` [PATCH 2/4] staging: nvec: make touchpad " Marc Dietrich
2024-04-05 14:40 ` Thierry Reding
2024-04-06 12:24 ` Marc Dietrich
2024-04-05 14:09 ` [PATCH 3/4] staging: nvec: make i2c controller register writes robust Marc Dietrich
2024-04-05 14:33 ` Thierry Reding [this message]
2024-04-05 15:21 ` Dan Carpenter
2024-04-05 14:09 ` [PATCH 4/4] staging: nvec: update TODO Marc Dietrich
2024-04-05 14:41 ` [PATCH 0/4] Improve robustnes during initialization Thierry Reding
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=D0C9H5M9NHBE.2AB0FZ1PDYZO4@gmail.com \
--to=thierry.reding@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux-tegra@vger.kernel.org \
--cc=marvin24@gmx.de \
/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).