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
