Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Mason,

On Mon, Dec 3, 2018 at 10:19 AM Mason Yang <masonccyang@xxxxxxxxxxx> wrote:
> Add a driver for Renesas R-Car Gen3 RPC SPI controller.
>
> Signed-off-by: Mason Yang <masonccyang@xxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -528,6 +528,12 @@ config SPI_RSPI
>         help
>           SPI driver for Renesas RSPI and QSPI blocks.
>
> +config SPI_RENESAS_RPC
> +       tristate "Renesas R-Car Gen3 RPC SPI controller"
> +       depends on SUPERH || ARCH_RENESAS || COMPILE_TEST

So this driver is intended for SuperH SoCs, too?
If not, please drop the dependency.

> --- /dev/null
> +++ b/drivers/spi/spi-renesas-rpc.c

> +#ifdef CONFIG_RESET_CONTROLLER
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)

What's the purpose of the reset routine?
Given the #ifdef, is it optional or required?

> +{
> +       int i, ret;
> +
> +       ret = reset_control_reset(rpc->rstc);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < LOOP_TIMEOUT; i++) {
> +               ret = reset_control_status(rpc->rstc);
> +               if (ret == 0)
> +                       return 0;
> +               usleep_range(0, 1);
> +       }

Why do you need this loop?
The delay in cpg_mssr_reset() should be sufficient.

> +
> +       return -ETIMEDOUT;
> +}
> +#else
> +static int rpc_spi_do_reset(struct rpc_spi *rpc)
> +{
> +       return -ETIMEDOUT;
> +}
> +#endif

> +static int rpc_spi_transfer_one_message(struct spi_master *master,
> +                                       struct spi_message *msg)
> +{
> +       struct rpc_spi *rpc = spi_master_get_devdata(master);
> +       struct spi_transfer *t;
> +       int ret;
> +
> +       rpc_spi_transfer_setup(rpc, msg);
> +
> +       list_for_each_entry(t, &msg->transfers, transfer_list) {
> +               if (!list_is_last(&t->transfer_list, &msg->transfers))
> +                       continue;
> +               ret = rpc_spi_xfer_message(rpc, t);

rpc_spi_xfer_message() sounds like a bad name to me, given it operates
on a transfer, not on a message.

> +               if (ret)
> +                       goto out;
> +       }
> +
> +       msg->status = 0;
> +       msg->actual_length = rpc->totalxferlen;
> +out:
> +       spi_finalize_current_message(master);
> +       return 0;
> +}


> +static int rpc_spi_probe(struct platform_device *pdev)
> +{

> +       rpc->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +       if (IS_ERR(rpc->rstc))
> +               return PTR_ERR(rpc->rstc);

This will return an error if CONFIG_RESET_CONTROLLER is not set, hence
the #ifdef above is moot.

> +
> +       pm_runtime_enable(&pdev->dev);
> +       master->auto_runtime_pm = true;
> +
> +       master->num_chipselect = 1;
> +       master->mem_ops = &rpc_spi_mem_ops;
> +       master->transfer_one_message = rpc_spi_transfer_one_message;

Is there any reason you cannot use the standard
spi_transfer_one_message, i.e. provide spi_controller.transfer_one()
instead of spi_controller.transfer_one_message()?

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