lkml.org 
[lkml]   [2018]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
Hi Mason,

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

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

\
 
 \ /
  Last update: 2018-12-05 10:07    [W:0.173 / U:3.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site