Re: [PATCH 4/6] spi: Add SPIBSC driver

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Chris,

On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@xxxxxxxxxxx> wrote:
> Add a driver for the SPIBSC controller in Renesas SoC devices.
>
> Signed-off-by: Chris Brandt <chris.brandt@xxxxxxxxxxx>

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@xxxxxxxxxxxxxx

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