From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Joel Stanley <joel@jms.id.au>
Cc: Eddie James <eajames@linux.ibm.com>,
linux-spi <linux-spi@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Mark Brown <broonie@kernel.org>, Andrew Jeffery <andrew@aj.id.au>,
linux-fsi@lists.ozlabs.org
Subject: Re: [PATCH v2] spi: Add FSI-attached SPI controller driver
Date: Tue, 25 Feb 2020 11:38:40 +0200 [thread overview]
Message-ID: <CAHp75VciUDKqxEQw_1bHDmVx19qTZWWjHdfgcJmm4iBwmyFJnA@mail.gmail.com> (raw)
In-Reply-To: <CACPK8XdmdksFctYk96x46XJcxe3yQD3HfAzC8gdF_GXWJHeu2A@mail.gmail.com>
On Tue, Feb 25, 2020 at 7:08 AM Joel Stanley <joel@jms.id.au> wrote:
> On Mon, 3 Feb 2020 at 22:30, Eddie James <eajames@linux.ibm.com> wrote:
...
> > +static int fsi_spi_data_in(u64 in, u8 *rx, int len)
> > +{
> > + int i;
> > + int num_bytes = min(len, 8);
> > +
> > + for (i = 0; i < num_bytes; ++i)
> > + rx[i] = (u8)(in >> (8 * ((num_bytes - 1) - i)));
> > +
> > + return num_bytes;
> > +}
> > +
> > +static int fsi_spi_data_out(u64 *out, const u8 *tx, int len)
> > +{
> > + int i;
> > + int num_bytes = min(len, 8);
> > +
> > + *out = 0ULL;
> > +
> > + for (i = 0; i < num_bytes; ++i)
> > + *out |= (u64)tx[i] << (8 * (8 - (i + 1)));
>
> Did this work with non-8 byte transfers? I think the second 8 should
> be num_bytes.
>
> The loop requires careful reading to check. I wonder if we could do
> this instead, which eliminates a lot duplicated loads and stores and
> is easier to read:
>
> uint8_t *outp = (uint8_t *)out;
>
> for (i = 0; i < num_bytes; ++i) {
> outp[num_bytes - (i + 1)] = tx[i];
> }
Have you had a chance to read my review of this? What do you think
about put_unaligned*()/get_unaligned*() instead of above?
> > + return num_bytes;
> > +}
...
> > +static int fsi_spi_transfer_init(struct fsi_spi *ctx)
> > +{
> > + int rc;
> > + bool reset = false;
> > + unsigned long end;
> > + u64 seq_state;
> > + u64 clock_cfg = 0ULL;
> > + u64 status = 0ULL;
> > + u64 wanted_clock_cfg = SPI_FSI_CLOCK_CFG_ECC_DISABLE |
> > + SPI_FSI_CLOCK_CFG_SCK_NO_DEL |
> > + FIELD_PREP(SPI_FSI_CLOCK_CFG_SCK_DIV, 4);
> > +
> > + end = jiffies + msecs_to_jiffies(SPI_FSI_INIT_TIMEOUT_MS);
> > + do {
> > + if (time_after(jiffies, end))
> > + return -ETIMEDOUT;
>
> How tightly does this loop spin?
>
> Should there be a delay inside of it?
>
> > +
> > + rc = fsi_spi_read_reg(ctx, SPI_FSI_STATUS, &status);
> > + if (rc)
> > + return rc;
> > +
> > + if (status & (SPI_FSI_STATUS_ANY_ERROR |
> > + SPI_FSI_STATUS_TDR_FULL |
> > + SPI_FSI_STATUS_RDR_FULL)) {
> > + if (reset)
> > + return -EIO;
> > +
> > + rc = fsi_spi_reset(ctx);
> > + if (rc)
> > + return rc;
> > +
> > + reset = true;
> > + continue;
> > + }
> > +
> > + seq_state = status & SPI_FSI_STATUS_SEQ_STATE;
> > + } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE));
>
> ../drivers/spi/spi-fsi.c: In function ‘fsi_spi_transfer_one_message’:
> ../drivers/spi/spi-fsi.c:363:11: warning: ‘seq_state’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> 363 | } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE));
> | ^~~~~~~~~
It's bogus warning, though, I think, easy to fix by reshuffling loop body.
> > + rc = fsi_spi_read_reg(ctx, SPI_FSI_CLOCK_CFG, &clock_cfg);
> > + if (rc)
> > + return rc;
> > +
> > + if ((clock_cfg & (SPI_FSI_CLOCK_CFG_MM_ENABLE |
> > + SPI_FSI_CLOCK_CFG_ECC_DISABLE |
> > + SPI_FSI_CLOCK_CFG_MODE |
> > + SPI_FSI_CLOCK_CFG_SCK_RECV_DEL |
> > + SPI_FSI_CLOCK_CFG_SCK_DIV)) != wanted_clock_cfg)
> > + rc = fsi_spi_write_reg(ctx, SPI_FSI_CLOCK_CFG,
> > + wanted_clock_cfg);
> > +
> > + return rc;
> > +}
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2020-02-25 9:38 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-03 22:30 [PATCH v2] spi: Add FSI-attached SPI controller driver Eddie James
[not found] ` <20200203223003.4567-1-eajames-tEXmvtCZX7AybS5Ee8rs3A@public.gmane.org>
2020-02-04 11:05 ` Andy Shevchenko
[not found] ` <CAHp75VfHAS46-kMyg7Ky4jn7zmkht6wd9ayCfn+rrO0R6aNr6A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-02-24 18:54 ` Eddie James
2020-02-25 5:08 ` Joel Stanley
2020-02-25 9:38 ` Andy Shevchenko [this message]
[not found] ` <CACPK8XdmdksFctYk96x46XJcxe3yQD3HfAzC8gdF_GXWJHeu2A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-02-25 19:45 ` Eddie James
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=CAHp75VciUDKqxEQw_1bHDmVx19qTZWWjHdfgcJmm4iBwmyFJnA@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=andrew@aj.id.au \
--cc=broonie@kernel.org \
--cc=eajames@linux.ibm.com \
--cc=joel@jms.id.au \
--cc=linux-fsi@lists.ozlabs.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
/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 NNTP newsgroup(s).