| Message ID | 1555313834-15107-1-git-send-email-masonccyang@mxic.com.tw |
|---|---|
| Headers | show |
| Series | mfd: Add Renesas R-Car Gen3 RPC-IF MFD & SPI driver | expand |
The patch name is somewhat misleading -- there's no "MFD controller". On 04/15/2019 10:37 AM, Mason Yang wrote: > Add a driver for Renesas R-Car Gen3 RPC-IF MFD controller. > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> [...] > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 26ad646..7914349 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -978,6 +978,15 @@ config MFD_RDC321X > southbridge which provides access to GPIOs and Watchdog using the > southbridge PCI device configuration space. > > +config MFD_RENESAS_RPC > + tristate "Renesas R-Car Gen3 RPC-IF MFD driver" > + select MFD_CORE > + depends on ARCH_RENESAS > + help > + This supports for Renesas R-Car Gen3 RPC-IF multifunction device "For" node needed here. > + controller which provides either SPI host controller or HyperFlash. > + You have to select individual components under the corresponding menu. > + > config MFD_RT5033 > tristate "Richtek RT5033 Power Management IC" > depends on I2C [...] > diff --git a/drivers/mfd/renesas-rpc.c b/drivers/mfd/renesas-rpc.c > new file mode 100644 > index 0000000..a565f31 > --- /dev/null > +++ b/drivers/mfd/renesas-rpc.c > @@ -0,0 +1,128 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp. > +// Copyright (C) 2019 Macronix International Co., Ltd. > +// > +// R-Car Gen3 RPC-IF MFD driver > +// > +// Author: > +// Mason Yang <masonccyang@mxic.com.tw> > +// > + > +#include <linux/mfd/renesas-rpc.h> > +#include <linux/mfd/core.h> > + > +static const struct mfd_cell rpc_hf_ctlr = { > + .name = "rpc-hf", > + .of_compatible = "renesas,rcar-rpc-hf", Don't need this line any more... > +}; > + > +static const struct mfd_cell rpc_spi_ctlr = { > + .name = "rpc-spi", > + .of_compatible = "renesas,rcar-rpc-spi", ... and this too. [...] > +static int rpc_mfd_probe(struct platform_device *pdev) > +{ > + struct device_node *flash; > + const struct mfd_cell *cell; > + struct resource *res; > + struct rpc_mfd *rpc; > + void __iomem *base; > + int ret; > + > + flash = of_get_next_child(pdev->dev.of_node, NULL); > + if (!flash) { > + dev_warn(&pdev->dev, "no flash node found\n"); > + return -ENODEV; > + } > + > + ret = of_device_is_compatible(flash, "jedec,spi-nor"); > + if (ret) { > + cell = &rpc_spi_ctlr; > + } else { > + ret = of_device_is_compatible(flash, "cfi-flash"); > + if (ret) { > + cell = &rpc_hf_ctlr; > + } else { > + dev_warn(&pdev->dev, "no jedec spi/cfi device found\n"); > + return -ENODEV; > + } > + } This definitely should look simpler, like below: if (of_device_is_compatible(flash, "jedec,spi-nor")) { cell = &rpc_spi_ctlr; } else if (of_device_is_compatible(flash, "cfi-flash")) { cell = &rpc_hf_ctlr; } else { dev_warn(&pdev->dev, "unknown flash type\n"); return -ENODEV; } [...] > + ret = devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL); > + > + return ret; return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL); [...] > diff --git a/include/linux/mfd/renesas-rpc.h b/include/linux/mfd/renesas-rpc.h > new file mode 100644 > index 0000000..61ada14 > --- /dev/null > +++ b/include/linux/mfd/renesas-rpc.h > @@ -0,0 +1,153 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp. > +// Copyright (C) 2019 Macronix International Co., Ltd. > +// > +// R-Car Gen3 RPC-IF MFD driver > +// > +// Author: > +// Mason Yang <masonccyang@mxic.com.tw> > +// > + > +#ifndef __MFD_RENESAS_RPC_H > +#define __MFD_RENESAS_RPC_H > + > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/log2.h> > +#include <linux/iopoll.h> > +#include <linux/module.h> > +#include <linux/mtd/mtd.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/of.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> NAK. I have already told you that these #include's are only needed in the drivers, not in this header. My idea is to contain all hardware manipulation in the MFD driver, with SPI/HF drivers calling that code via a set of APIs declared in this header. The registers (declared below) would end up only needed by that common MFD driver... [...] MBR, Sergei
On 04/15/2019 10:37 AM, Mason Yang wrote: > Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller. > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> This patch looked good to me (you seem to have incorporated another patch of mine here :-)). Since the patch is already carrying my signoff, I wouldn't add a Reviewed-by: tag. However, I believe that all manipulations with the RPC-IF hardware need to be moved to the MFD driver, to be called from the SPI and HF subdrivers... doesn't seem an easy task though... MBR, Sergei
On 04/16/2019 04:19 AM, masonccyang@mxic.com.tw wrote: >> Re: [PATCH v10 1/3] mfd: Add Renesas R-Car Gen3 RPC-IF MFD controller driver >> >> The patch name is somewhat misleading -- there's no "MFD controller". > > Can't get your point ? > > RPC-IF support SPI and HF, it's a MFD controller and > that's why I patch it to MFD as Marek's comments. I'd like the "controller" word dropped, as MFD stands for multi-function device anyway. "Controller" seems to be superfluous here... >> On 04/15/2019 10:37 AM, Mason Yang wrote: >> >> > Add a driver for Renesas R-Car Gen3 RPC-IF MFD controller. >> > >> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> >> [...] >> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> > index 26ad646..7914349 100644 >> > --- a/drivers/mfd/Kconfig >> > +++ b/drivers/mfd/Kconfig >> > @@ -978,6 +978,15 @@ config MFD_RDC321X >> > southbridge which provides access to GPIOs and Watchdog using the >> > southbridge PCI device configuration space. >> > >> > +config MFD_RENESAS_RPC >> > + tristate "Renesas R-Car Gen3 RPC-IF MFD driver" >> > + select MFD_CORE >> > + depends on ARCH_RENESAS >> > + help >> > + This supports for Renesas R-Car Gen3 RPC-IF multifunction device >> >> "For" node needed here. > > ? Sorry, that was a typo: s/node/not/. >> [...] >> > diff --git a/include/linux/mfd/renesas-rpc.h b/include/linux/mfd/ >> renesas-rpc.h >> > new file mode 100644 >> > index 0000000..61ada14 >> > --- /dev/null >> > +++ b/include/linux/mfd/renesas-rpc.h >> > @@ -0,0 +1,153 @@ >> > +// SPDX-License-Identifier: GPL-2.0 >> > +// >> > +// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp. >> > +// Copyright (C) 2019 Macronix International Co., Ltd. >> > +// >> > +// R-Car Gen3 RPC-IF MFD driver >> > +// >> > +// Author: >> > +// Mason Yang <masonccyang@mxic.com.tw> >> > +// >> > + >> > +#ifndef __MFD_RENESAS_RPC_H >> > +#define __MFD_RENESAS_RPC_H >> > + >> > +#include <linux/clk.h> >> > +#include <linux/io.h> >> > +#include <linux/log2.h> >> > +#include <linux/iopoll.h> >> > +#include <linux/module.h> >> > +#include <linux/mtd/mtd.h> >> > +#include <linux/platform_device.h> >> > +#include <linux/pm_runtime.h> >> > +#include <linux/of.h> >> > +#include <linux/regmap.h> >> > +#include <linux/reset.h> >> >> NAK. I have already told you that these #include's are only needed in the >> drivers, not in this header. > > why ? Why what? As a rule of thumb, we #include only headers necessary to build the file in question. > I think these #include files are still needed in HyperFlash driver. Hopefully not. Though that doesn't really matter much. > i.e,. clk controller, mtd and so on. > >> >> My idea is to contain all hardware manipulation in the MFD driver, >> with SPI/HF drivers calling that code via a set of APIs declared in >> this header. >> The registers (declared below) would end up only needed by that >> common MFD driver... > > best regards, > Mason MBR, Sergei
On 04/17/2019 09:21 AM, masonccyang@mxic.com.tw wrote: >> >> Re: [PATCH v10 1/3] mfd: Add Renesas R-Car Gen3 RPC-IF MFD >> controller driver >> >> >> >> The patch name is somewhat misleading -- there's no "MFD controller". >> > >> > Can't get your point ? >> > >> > RPC-IF support SPI and HF, it's a MFD controller and >> > that's why I patch it to MFD as Marek's comments. >> >> I'd like the "controller" word dropped, as MFD stands for multi- >> function device >> anyway. "Controller" seems to be superfluous here... > > okay, will rename to: > > [PATCH v11 1/3] mfd: Add Renesas R-Car Gen3 RPC-IF MFD driver Thanx! >> >> On 04/15/2019 10:37 AM, Mason Yang wrote: >> >> >> >> > Add a driver for Renesas R-Car Gen3 RPC-IF MFD controller. >> >> > >> >> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> >> >> [...] >> >> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> >> > index 26ad646..7914349 100644 >> >> > --- a/drivers/mfd/Kconfig >> >> > +++ b/drivers/mfd/Kconfig >> >> > @@ -978,6 +978,15 @@ config MFD_RDC321X >> >> > southbridge which provides access to GPIOs and Watchdog using the >> >> > southbridge PCI device configuration space. >> >> > >> >> > +config MFD_RENESAS_RPC >> >> > + tristate "Renesas R-Car Gen3 RPC-IF MFD driver" >> >> > + select MFD_CORE >> >> > + depends on ARCH_RENESAS >> >> > + help >> >> > + This supports for Renesas R-Car Gen3 RPC-IF multifunction device >> >> >> >> "For" node needed here. >> > >> > ? >> >> Sorry, that was a typo: s/node/not/. > > Oops, still don't know what do you mean to ? > > Will change to: > help > This supports for Renesas R-Car Gen3 RPC-IF multifunction device > which provides either SPI host or HyperFlash. > You have to select individual components under the corresponding menu. > > is it ok ? Should be "This supports Renesas..." [...] MBR, Sergei
Hi, v10 patch including: 1) Address range for > 64M byte flash. 2) Removed dirmap_write due to WBUF 256 bytes transfer issue. 3) Dummy bytes setting according to spi-nor.c layer. v9 patch is for RPC MFD driver and RPC SPI driver. v8 patch including: 1) Supported SoC-specific values in DTS. 2) Rename device node name as flash. v7 patch is according to Geert and Sergei's comments: 1) Add all R-Car Gen3 model in dts. 2) patch rpc-if child node search. 3) minror coding style. v6 patch is accroding to Geert, Marek and Sergei's comments: 1) spi_controller for new code. 2) "renesas,rcar-gen3-rpc" instead of "renesas,r8a77995-rpc." 3) patch external address read mode w/o u64 readq(). 4) patch dts for write buffer & drop "renesas,rpc-mode". 5) coding style and so on. v5 patch is accroding to Sergei's comments: 1) Read 6 bytes ID from Sergei's patch. 2) regmap_update_bits(). 3) C++ style comment. v4 patch is according to Sergei's comments including: 1) Drop soc_device_match(). 2) Drop unused RPC registers. 3) Use ilog2() instead of fls(). 4) Patch read 6 bytes ID w/ one command. 5) Coding style and so on. v3 patch is according to Marek and Geert's comments including: 1) soc_device_mach() to set up RPC_PHYCNT_STRTIM. 2) get_unaligned(). 3) rpc-mode for rpi-spi-flash or rpc-hyperflash. 4) coding style and so on. v2 patch including: 1) remove RPC clock enable/dis-able control, 2) patch run time PM. 3) add RPC module software reset, 4) add regmap. 5) other coding style and so on. thanks for your review. best regards, Mason Mason Yang (3): mfd: Add Renesas R-Car Gen3 RPC-IF MFD controller driver spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF controller bindings .../devicetree/bindings/mfd/mfd-renesas-rpc.txt | 37 ++ drivers/mfd/Kconfig | 9 + drivers/mfd/Makefile | 1 + drivers/mfd/renesas-rpc.c | 128 +++++ drivers/spi/Kconfig | 6 + drivers/spi/Makefile | 1 + drivers/spi/spi-renesas-rpc.c | 561 +++++++++++++++++++++ include/linux/mfd/renesas-rpc.h | 153 ++++++ 8 files changed, 896 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt create mode 100644 drivers/mfd/renesas-rpc.c create mode 100644 drivers/spi/spi-renesas-rpc.c create mode 100644 include/linux/mfd/renesas-rpc.h