Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Wed, 5 Dec 2018 10:06:12 +0100 | Subject | Re: [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
| |