Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
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
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

  parent reply index

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 publically 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

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git