From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Chris Brandt <chris.brandt@renesas.com>
Cc: Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
linux-spi <linux-spi@vger.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
linux-clk <linux-clk@vger.kernel.org>,
Mason Yang <masonccyang@mxic.com.tw>,
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Subject: Re: [PATCH 4/6] spi: Add SPIBSC driver
Date: Tue, 3 Dec 2019 19:28:18 +0100 [thread overview]
Message-ID: <CAMuHMdVAZOa7OmT0s=RsVJsny9NujDzpdg4T+QoUXGe0kJjOTw@mail.gmail.com> (raw)
In-Reply-To: <20191203034519.5640-5-chris.brandt@renesas.com>
Hi Chris,
On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> Add a driver for the SPIBSC controller in Renesas SoC devices.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Thanks for your patch!
> --- /dev/null
> +++ b/drivers/spi/spi-spibsc.c
> @@ -0,0 +1,609 @@
> +// SPDX-License-Identifier: GPL-2.0
GPL-2.0-only
> +static void spibsc_print_data(struct spibsc_priv *sbsc, u8 tx, const u8 *buf,
> + int len)
> +{
> +#if defined(DEBUG_PRINT_DATA)
> + char line_buffer[3*16+1];
> + int i, line_index = 0;
> +
> + if (tx)
> + pr_debug("spibsc: send data: ");
> + else
> + pr_debug("spibsc: recv data: ");
> +
> + for (i = 0; i < len; ) {
> + sprintf(line_buffer + line_index, " %02X", buf[i]);
> + line_index += 3;
> + i++;
> + if (i % 16 == 0) {
> + pr_debug(line_buffer);
> + line_index = 0;
> + }
> + }
> + if (i % 16 != 0)
> + pr_debug(line_buffer);
> + else
> + pr_debug("\n");
> +#endif
print_hex_dump_debug()?
> +}
> +
> +static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc)
> +{
> + int t = 256 * 100000;
> +
> + while (t--) {
> + if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND)
> + return 0;
> + ndelay(1);
> + }
So this may busy loop for up to 25.6 ms? Oops...
Can you use the interrupt (SPIHF) instead, signalling a completion?
> +
> + dev_err(sbsc->dev, "Timeout waiting for TEND\n");
> + return -ETIMEDOUT;
> +}
> + /* Use 'Option Data' for 3rd-6th bytes */
> + switch (tx_len) {
> + case 6:
> + dropr |= DROPR_OPD0(tx_data[5]);
> + opde |= (1 << 0);
Both checkpatch and gcc tell you to add fallthrough coments.
> + case 5:
> + dropr |= DROPR_OPD1(tx_data[4]);
> + opde |= (1 << 1);
> + case 4:
> + dropr |= DROPR_OPD2(tx_data[3]);
> + opde |= (1 << 2);
> + case 3:
> + dropr |= DROPR_OPD3(tx_data[2]);
> + opde |= (1 << 3);
> + drenr |= DRENR_OPDE(opde);
> + }
> +static int spibsc_transfer_one_message(struct spi_controller *master,
> + struct spi_message *msg)
> +{
> + struct spibsc_priv *sbsc = spi_controller_get_devdata(master);
> + struct spi_transfer *t, *t_last;
> + u8 tx_data[MAX_CMD_LEN];
> + int tx_only;
> + u8 tx_len;
> + int ret;
> +
> + t_last = list_last_entry(&msg->transfers, struct spi_transfer,
> + transfer_list);
> + /* defaults */
> + ret = 0;
> + sbsc->last_xfer = 0;
> + tx_only = 1;
> +
> + /* Analyze the messages */
> + t = list_first_entry(&msg->transfers, struct spi_transfer,
> + transfer_list);
> + if (t->rx_buf) {
> + dev_dbg(sbsc->dev, "Cannot Rx without Tx first!\n");
> + return -EIO;
> + }
> + list_for_each_entry(t, &msg->transfers, transfer_list) {
> + if (t->rx_buf) {
> + tx_only = 0;
> + if (t != t_last) {
> + dev_dbg(sbsc->dev, "RX transaction is not the last transaction!\n");
> + return -EIO;
> + }
> + }
> + if (t->cs_change) {
> + dev_err(sbsc->dev, "cs_change not supported");
> + return -EIO;
> + }
> + }
If you would do the checks above in .prepare_message()
(like e.g. rspi does)...
> +
> + /* Tx Only (SPI Mode is used) */
> + if (tx_only == 1) {
> +
> + dev_dbg(sbsc->dev, "%s: TX only\n", __func__);
> +
> + /* Initialize for SPI Mode */
> + spibsc_write(sbsc, CMNCR, CMNCR_INIT);
> +
> + /* Send messages */
> + list_for_each_entry(t, &msg->transfers, transfer_list) {
> +
> + if (t == t_last)
> + sbsc->last_xfer = 1;
> +
> + ret = spibsc_send_data(sbsc, t->tx_buf, t->len);
> + if (ret)
> + break;
> +
> + msg->actual_length += t->len;
> + }
> +
> + /* Done */
> + msg->status = ret;
> + spi_finalize_current_message(master);
> + return ret;
> + }
> +
> + /* Tx, then RX (Data Read Mode is used) */
> + dev_dbg(sbsc->dev, "%s: Tx then Rx\n", __func__);
> +
> + /* Buffer up the transmit portion (cmd + addr) so we can send it all at
> + * once
> + */
> + tx_len = 0;
> + list_for_each_entry(t, &msg->transfers, transfer_list) {
> + if (t->tx_buf) {
> + if ((tx_len + t->len) > sizeof(tx_data)) {
> + dev_dbg(sbsc->dev, "Command too big (%d)\n",
> + tx_len + t->len);
> + return -EIO;
> + }
> + memcpy(tx_data + tx_len, t->tx_buf, t->len);
> + tx_len += t->len;
> + }
> +
> + if (t->rx_buf)
> + ret = spibsc_send_recv_data(sbsc, tx_data, tx_len,
> + t->rx_buf, t->len);
> +
> + msg->actual_length += t->len;
> + }
> +
> + msg->status = ret;
> + spi_finalize_current_message(master);
... can't you just use .transfer_one(), instead of duplicating the logic
from spi_transfer_one_message()?
Or is there some special detail that's not obvious to the casual reviewer?
> +static const struct of_device_id of_spibsc_match[] = {
> + { .compatible = "renesas,spibsc"},
> + { .compatible = "renesas,r7s72100-spibsc", .data = (void *)HAS_SPBCR},
> + { .compatible = "renesas,r7s9210-spibsc"},
Do you need to match against all 3 in the driver?
Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR?
If not, the fallback to "renesas,spibsc" is not valid for RZ/A1.
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_spibsc_match);
> +
> +static struct platform_driver spibsc_driver = {
> + .probe = spibsc_probe,
> + .remove = spibsc_remove,
> + .driver = {
> + .name = "spibsc",
> + .owner = THIS_MODULE,
> + .of_match_table = of_spibsc_match,
> + },
> +};
> +module_platform_driver(spibsc_driver);
> +
> +MODULE_DESCRIPTION("Renesas SPIBSC SPI Flash driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Chris Brandt");
> --
> 2.23.0
>
--
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
next prev parent reply other threads:[~2019-12-03 18:28 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-03 3:45 [PATCH 0/6] spi: Add Renesas SPIBSC controller Chris Brandt
2019-12-03 3:45 ` [PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support Chris Brandt
2019-12-03 18:32 ` Geert Uytterhoeven
2019-12-03 18:46 ` Chris Brandt
2019-12-03 18:51 ` Geert Uytterhoeven
2019-12-03 3:45 ` [PATCH 2/6] ARM: dts: r7s72100: Add SPIBSC clocks Chris Brandt
2019-12-03 18:42 ` Geert Uytterhoeven
2019-12-03 18:57 ` Chris Brandt
2019-12-03 19:12 ` Geert Uytterhoeven
2019-12-04 8:38 ` Lee Jones
2019-12-04 9:03 ` Geert Uytterhoeven
2019-12-04 9:47 ` Lee Jones
2019-12-04 11:00 ` Chris Brandt
2019-12-03 3:45 ` [PATCH 3/6] clk: renesas: r7s9210: Add SPIBSC clock Chris Brandt
2019-12-03 18:49 ` Geert Uytterhoeven
2019-12-03 19:09 ` Chris Brandt
2019-12-03 20:40 ` Geert Uytterhoeven
2019-12-04 3:09 ` Chris Brandt
2019-12-03 3:45 ` [PATCH 4/6] spi: Add SPIBSC driver Chris Brandt
2019-12-03 14:19 ` Mark Brown
2019-12-03 15:00 ` Chris Brandt
2019-12-03 18:29 ` Geert Uytterhoeven
2019-12-04 11:25 ` Mark Brown
2019-12-04 12:14 ` Geert Uytterhoeven
2019-12-04 15:51 ` Chris Brandt
2019-12-04 16:49 ` Mark Brown
2019-12-03 18:28 ` Geert Uytterhoeven [this message]
2019-12-04 11:18 ` Mark Brown
2019-12-04 22:12 ` Chris Brandt
2019-12-03 3:45 ` [PATCH 5/6] ARM: dts: r7s9210: Add SPIBSC Device support Chris Brandt
2019-12-03 18:59 ` Geert Uytterhoeven
2019-12-03 22:38 ` Chris Brandt
2019-12-04 7:57 ` Geert Uytterhoeven
2019-12-04 11:04 ` Chris Brandt
2019-12-03 3:45 ` [PATCH 6/6] dt-bindings: spi: Document Renesas SPIBSC bindings Chris Brandt
2019-12-03 9:14 ` Sergei Shtylyov
2019-12-03 13:27 ` Chris Brandt
2019-12-03 16:04 ` Sergei Shtylyov
2019-12-03 16:35 ` Chris Brandt
2019-12-03 18:57 ` Geert Uytterhoeven
2019-12-03 20:39 ` Geert Uytterhoeven
2019-12-04 2:54 ` Chris Brandt
2019-12-04 8:56 ` Geert Uytterhoeven
2019-12-04 13:31 ` Chris Brandt
2019-12-05 15:48 ` Geert Uytterhoeven
2019-12-05 16:00 ` Chris Brandt
2019-12-04 8:40 ` Geert Uytterhoeven
2019-12-03 22:33 ` Chris Brandt
2019-12-04 8:43 ` Geert Uytterhoeven
2019-12-04 11:19 ` Chris Brandt
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='CAMuHMdVAZOa7OmT0s=RsVJsny9NujDzpdg4T+QoUXGe0kJjOTw@mail.gmail.com' \
--to=geert@linux-m68k.org \
--cc=broonie@kernel.org \
--cc=chris.brandt@renesas.com \
--cc=devicetree@vger.kernel.org \
--cc=geert+renesas@glider.be \
--cc=linux-clk@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=masonccyang@mxic.com.tw \
--cc=mturquette@baylibre.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=sergei.shtylyov@cogentembedded.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 NNTP newsgroup(s).