Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v8 14/21] clk: tegra210: Add suspend and resume support
From: Dmitry Osipenko @ 2019-08-09 13:56 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
	marc.zyngier, linus.walleij, stefan, mark.rutland
  Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
	josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
	robh+dt, devicetree, rjw, viresh.kumar, linux-pm
In-Reply-To: <1565308020-31952-15-git-send-email-skomatineni@nvidia.com>

09.08.2019 2:46, Sowjanya Komatineni пишет:
> This patch adds support for clk: tegra210: suspend-resume.
> 
> All the CAR controller settings are lost on suspend when core
> power goes off.
> 
> This patch has implementation for saving and restoring all PLLs
> and clocks context during system suspend and resume to have the
> clocks back to same state for normal operation.
> 
> Clock driver suspend and resume are registered as syscore_ops as clocks
> restore need to happen before the other drivers resume to have all their
> clocks back to the same state as before suspend.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/clk/tegra/clk-tegra210.c | 103 +++++++++++++++++++++++++++++++++++++--
>  drivers/clk/tegra/clk.c          |  64 ++++++++++++++++++++++++
>  drivers/clk/tegra/clk.h          |   3 ++
>  3 files changed, 166 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
> index 998bf60b219a..8dd6f4f4debb 100644
> --- a/drivers/clk/tegra/clk-tegra210.c
> +++ b/drivers/clk/tegra/clk-tegra210.c
> @@ -9,13 +9,13 @@
>  #include <linux/clkdev.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/syscore_ops.h>
>  #include <linux/delay.h>
>  #include <linux/export.h>
>  #include <linux/mutex.h>
>  #include <linux/clk/tegra.h>
>  #include <dt-bindings/clock/tegra210-car.h>
>  #include <dt-bindings/reset/tegra210-car.h>
> -#include <linux/iopoll.h>
>  #include <linux/sizes.h>
>  #include <soc/tegra/pmc.h>
>  
> @@ -220,11 +220,15 @@
>  #define CLK_M_DIVISOR_SHIFT 2
>  #define CLK_M_DIVISOR_MASK 0x3
>  
> +#define CLK_MASK_ARM	0x44
> +#define MISC_CLK_ENB	0x48
> +
>  #define RST_DFLL_DVCO 0x2f4
>  #define DVFS_DFLL_RESET_SHIFT 0
>  
>  #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>  #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
> +#define CPU_SOFTRST_CTRL 0x380
>  
>  #define LVL2_CLK_GATE_OVRA 0xf8
>  #define LVL2_CLK_GATE_OVRC 0x3a0
> @@ -2825,6 +2829,7 @@ static int tegra210_enable_pllu(void)
>  	struct tegra_clk_pll_freq_table *fentry;
>  	struct tegra_clk_pll pllu;
>  	u32 reg;
> +	int ret;
>  
>  	for (fentry = pll_u_freq_table; fentry->input_rate; fentry++) {
>  		if (fentry->input_rate == pll_ref_freq)
> @@ -2853,9 +2858,14 @@ static int tegra210_enable_pllu(void)
>  	reg |= PLL_ENABLE;
>  	writel(reg, clk_base + PLLU_BASE);
>  
> -	readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
> -					  reg & PLL_BASE_LOCK, 2, 1000);
> -	if (!(reg & PLL_BASE_LOCK)) {
> +	/*
> +	 * During clocks resume, same PLLU init and enable sequence get
> +	 * executed. So, readx_poll_timeout_atomic can't be used here as it
> +	 * uses ktime_get() and timekeeping resume doesn't happen by that
> +	 * time. So, using tegra210_wait_for_mask for PLL LOCK.
> +	 */
> +	ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
> +	if (ret) {
>  		pr_err("Timed out waiting for PLL_U to lock\n");
>  		return -ETIMEDOUT;
>  	}
> @@ -3288,6 +3298,84 @@ static void tegra210_disable_cpu_clock(u32 cpu)
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> +/*
> + * This array lists mask values for each peripheral clk bank
> + * to mask out reserved bits during the clocks state restore
> + * on SC7 resume to prevent accidental writes to these reserved
> + * bits.
> + */
> +static u32 periph_clk_rsvd_mask[TEGRA210_CAR_BANK_COUNT] = {

Should be more natural to have a "valid_mask" instead of "rsvd_mask".

What's actually wrong with touching of the reserved bits? They must be NO-OP.. or the
reserved bits are actually some kind of "secret" bits? If those bits have some use-case
outside of Silicon HW (like FPGA simulation), then this doesn't matter for upstream and you
have to keep the workaround locally in the downstream kernel or whatever.

> +	0x23282006,
> +	0x782e0c18,
> +	0x0c012c05,
> +	0x003e7304,
> +	0x86c04800,
> +	0xc0199000,
> +	0x03e03800,
> +};
> +
> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) + ((_off) * 4))
> +#define car_writel(_val, _base, _off) \
> +		writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
> +
> +static u32 spare_reg_ctx, misc_clk_enb_ctx, clk_msk_arm_ctx;
> +static u32 cpu_softrst_ctx[3];
> +
> +static int tegra210_clk_suspend(void)
> +{
> +	unsigned int i;
> +
> +	clk_save_context();
> +
> +	/*
> +	 * Save the bootloader configured clock registers SPARE_REG0,
> +	 * MISC_CLK_ENB, CLK_MASK_ARM, CPU_SOFTRST_CTRL.
> +	 */
> +	spare_reg_ctx = readl_relaxed(clk_base + SPARE_REG0);
> +	misc_clk_enb_ctx = readl_relaxed(clk_base + MISC_CLK_ENB);
> +	clk_msk_arm_ctx = readl_relaxed(clk_base + CLK_MASK_ARM);
> +
> +	for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> +		cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
> +
> +	tegra_clk_periph_suspend();
> +	return 0;
> +}
> +
> +static void tegra210_clk_resume(void)
> +{
> +	unsigned int i;
> +
> +	tegra_clk_osc_resume(clk_base);
> +
> +	/*
> +	 * Restore the bootloader configured clock registers SPARE_REG0,
> +	 * MISC_CLK_ENB, CLK_MASK_ARM, CPU_SOFTRST_CTRL from saved context.
> +	 */
> +	writel_relaxed(spare_reg_ctx, clk_base + SPARE_REG0);
> +	writel_relaxed(misc_clk_enb_ctx, clk_base + MISC_CLK_ENB);
> +	writel_relaxed(clk_msk_arm_ctx, clk_base + CLK_MASK_ARM);
> +
> +	for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> +		car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
> +
> +	fence_udelay(5, clk_base);
> +
> +	/* enable all the clocks before changing the clock sources */
> +	tegra_clk_periph_force_on(periph_clk_rsvd_mask);

Why clocks need to be enabled before changing the sources?

> +	/* wait for all writes to happen to have all the clocks enabled */
> +	wmb();

fence_udelay() has exactly the same barrier at the very beginning of readl(), no need to
duplicate it here.

> +	fence_udelay(2, clk_base);
> +
> +	/* restore PLLs and all peripheral clock rates */
> +	tegra210_init_pllu();

Why USB PLL need to be restored at first?

> +	clk_restore_context();
> +
> +	/* restore all peripheral clocks enable and reset state */
> +	tegra_clk_periph_resume();
> +}

[snip]

^ permalink raw reply

* Re: [PATCH v3 02/21] ARM: dts: imx7-colibri: disable HS400
From: Marcel Ziswiler @ 2019-08-09 14:05 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: Stefan Agner, s.hauer@pengutronix.de,
	linux-kernel@vger.kernel.org, linux-imx@nxp.com,
	kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190807082556.5013-3-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Force HS200 by masking bit 63 of the SDHCI capability register.
> The i.MX ESDHC driver uses SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400. With
> that the stack checks bit 63 to descide whether HS400 is available.
> Using sdhci-caps-mask allows to mask bit 63. The stack then selects
> HS200 as operating mode.
> 
> This prevents rare communication errors with minimal effect on
> performance:
> 	sdhci-esdhc-imx 30b60000.usdhc: warning! HS400 strobe DLL
> 		status REF not lock!
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx7-colibri.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> b/arch/arm/boot/dts/imx7-colibri.dtsi
> index f1c1971f2160..f7c9ce5bed47 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -325,6 +325,7 @@
>  	vmmc-supply = <&reg_module_3v3>;
>  	vqmmc-supply = <&reg_DCDC3>;
>  	non-removable;
> +	sdhci-caps-mask = <0x80000000 0x0>;
>  };
>  
>  &iomuxc {

^ permalink raw reply

* Re: [PATCH v3 03/21] ARM: dts: imx7-colibri: prepare module device tree for FlexCAN
From: Marcel Ziswiler @ 2019-08-09 14:07 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: linux-imx@nxp.com, s.hauer@pengutronix.de, kernel@pengutronix.de,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <20190807082556.5013-4-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> Prepare FlexCAN use on SODIMM 55/63 178/188. Those SODIMM pins are
> compatible for CAN bus use with several modules from the Colibri
> family.
> Add Better drivestrength and also add flexcan2.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx7-colibri.dtsi | 35 ++++++++++++++++++++++++---
> --
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> b/arch/arm/boot/dts/imx7-colibri.dtsi
> index f7c9ce5bed47..52046085ce6f 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -117,6 +117,18 @@
>  	fsl,magic-packet;
>  };
>  
> +&flexcan1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_flexcan1>;
> +	status = "disabled";
> +};
> +
> +&flexcan2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_flexcan2>;
> +	status = "disabled";
> +};
> +
>  &gpmi {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_gpmi_nand>;
> @@ -330,12 +342,11 @@
>  
>  &iomuxc {
>  	pinctrl-names = "default";
> -	pinctrl-0 = <&pinctrl_gpio1 &pinctrl_gpio2 &pinctrl_gpio3
> &pinctrl_gpio4>;
> +	pinctrl-0 = <&pinctrl_gpio1 &pinctrl_gpio2 &pinctrl_gpio3
> &pinctrl_gpio4
> +		     &pinctrl_gpio7>;
>  
>  	pinctrl_gpio1: gpio1-grp {
>  		fsl,pins = <
> -			MX7D_PAD_ENET1_RGMII_RD3__GPIO7_IO3	0x74
> /* SODIMM 55 */
> -			MX7D_PAD_ENET1_RGMII_RD2__GPIO7_IO2	0x74
> /* SODIMM 63 */
>  			MX7D_PAD_SAI1_RX_SYNC__GPIO6_IO16	0x14 /*
> SODIMM 77 */
>  			MX7D_PAD_EPDC_DATA09__GPIO2_IO9		0x14
> /* SODIMM 89 */
>  			MX7D_PAD_EPDC_DATA08__GPIO2_IO8		0x74
> /* SODIMM 91 */
> @@ -416,6 +427,13 @@
>  		>;
>  	};
>  
> +	pinctrl_gpio7: gpio7-grp { /* Alternatively CAN1 */
> +		fsl,pins = <
> +			MX7D_PAD_ENET1_RGMII_RD3__GPIO7_IO3	0x14
> /* SODIMM 55 */
> +			MX7D_PAD_ENET1_RGMII_RD2__GPIO7_IO2	0x14
> /* SODIMM 63 */
> +		>;
> +	};
> +
>  	pinctrl_i2c1_int: i2c1-int-grp { /* PMIC / TOUCH */
>  		fsl,pins = <
>  			MX7D_PAD_GPIO1_IO13__GPIO1_IO13	0x79
> @@ -459,10 +477,17 @@
>  		>;
>  	};
>  
> +	pinctrl_flexcan1: flexcan1-grp {
> +		fsl,pins = <
> +			MX7D_PAD_ENET1_RGMII_RD3__FLEXCAN1_TX	0x79
> /* SODIMM 55 */
> +			MX7D_PAD_ENET1_RGMII_RD2__FLEXCAN1_RX	0x79
> /* SODIMM 63 */
> +		>;
> +	};
> +
>  	pinctrl_flexcan2: flexcan2-grp {
>  		fsl,pins = <
> -			MX7D_PAD_GPIO1_IO14__FLEXCAN2_RX	0x59
> -			MX7D_PAD_GPIO1_IO15__FLEXCAN2_TX	0x59
> +			MX7D_PAD_GPIO1_IO14__FLEXCAN2_RX	0x79 /*
> SODIMM 188 */
> +			MX7D_PAD_GPIO1_IO15__FLEXCAN2_TX	0x79 /*
> SODIMM 178 */
>  		>;
>  	};

^ permalink raw reply

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
From: Rob Herring @ 2019-08-09 14:08 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Harish Jenny K N, Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan
In-Reply-To: <CACRpkdZ+vXG-mGjn0Tt5gyGowAuxiCSQNdjEPGTP9qj23CwkSw@mail.gmail.com>

On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Jul 10, 2019 at 10:28 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
> > On 09/07/19 9:38 PM, Rob Herring wrote:
>
> > >> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
> > >
> > > We already define the active state of GPIOs in the consumers. If
> > > there's an inverter in the middle, the consumer active state is simply
> > > inverted. I don't agree that that is a hack as Linus said without some
> > > reasoning why an inverter needs to be modeled in DT. Anything about
> > > what 'userspace' needs is not a reason. That's a Linux thing that has
> > > little to do with hardware description.
>
> There is some level of ambition here which is inherently a bit fuzzy
> around the edges. ("How long is the coast of Britain?" comes to mind.)
>
> Surely the intention of device tree is not to recreate the schematic
> in all detail. What we want is a model of the hardware that will
> suffice for the operating system usecases.
>
> But sometimes the DTS files will become confusing: why is this
> component using GPIO_ACTIVE_LOW when another system
> doesn't have that flag? If there is an explicit inverter, the
> DTS gets more readable for a human.
>
> But arguable that is case for adding inverters as syntactic
> sugar in the DTS compiler instead...

If you really want something more explicit, then add a new GPIO
'inverted' flag. Then a device can always have the same HIGH/LOW flag.
That also solves the abstract it for userspace problem.

> > Yes we are talking about the hardware level inversions here.
> > The usecase is for those without the gpio consumer driver.
> > The usecase started with the concept of allowing an abstraction
> > of the underlying hardware for the userland controlling program
> > such that this program does not care whether the GPIO lines
> > are inverted or not physically. In other words, a single userland
> > controlling program can work unmodified across a variety of
> > hardware platforms with the device tree mapping the logical
> > to physical relationship of the GPIO hardware.
> > I totally understand anything about what 'userspace' needs is
> > not a reason, but this is not restricted to userspace alone as
> > kernel drivers may need this just as much. Also we are
> > just modelling/describing the hardware state in the device tree.
>
> The kernel also has a need to model inverters and it has come
> up from time to time, but I don't remember these instances
> right off the top of my head.

The only thing I can think of is an inverter needing its power supply
turned on. Seems a bit silly to have such fine grained control, but
who knows.

> I am not sure userspace needs are of zero concerns either.

No, but kernel vs. userspace is all a black box from a DT perspective
and not a distinction that we can design bindings around.

> Sure, for anything reimplementing what I have listed in
> Documentation/driver-api/gpio/drivers-on-gpio.rst
> it is just abuse of the ABI, but things like industrial control
> systems and other one-offs have this need to run the
> same binary unmodified for measuring the trigger level
> of water in some tank or so, they can't create kernel
> drivers for that kind of stuff.

The userspace interface already passes the flags for the gpio lines,
why can't a userspace program honor them? You can't have it both ways:
low level GPIO access and abstracted to not care about the details.

Rob

^ permalink raw reply

* Re: [v4 2/6] media: platform: dwc: Add MIPI CSI-2 controller driver
From: Sakari Ailus @ 2019-08-09 14:10 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: mchehab, davem, gregkh, Jonathan.Cameron, robh, nicolas.ferre,
	paulmck, mark.rutland, kishon, devicetree, linux-media,
	linux-kernel, Joao.Pinto
In-Reply-To: <1560280855-18085-3-git-send-email-luis.oliveira@synopsys.com>

Hi Luis,

On Tue, Jun 11, 2019 at 09:20:51PM +0200, Luis Oliveira wrote:
> Add the Synopsys MIPI CSI-2 controller driver. This
> controller driver is divided in platform functions and core functions.
> This way it serves as platform for future DesignWare drivers.
> 
> Signed-off-by: Luis Oliveira <luis.oliveira@synopsys.com>
> ---
> Changelog
> v3-v4
> - fix v4l2_fwnode_endpoint bad initialization @eugen
> - removed extra lines and fixed coding style issues
> 
>  MAINTAINERS                               |   8 +
>  drivers/media/platform/Kconfig            |   1 +
>  drivers/media/platform/Makefile           |   2 +
>  drivers/media/platform/dwc/Kconfig        |  19 +
>  drivers/media/platform/dwc/Makefile       |   9 +
>  drivers/media/platform/dwc/dw-csi-plat.c  | 475 +++++++++++++++++++++++
>  drivers/media/platform/dwc/dw-csi-plat.h  |  97 +++++
>  drivers/media/platform/dwc/dw-csi-sysfs.c | 624 ++++++++++++++++++++++++++++++
>  drivers/media/platform/dwc/dw-mipi-csi.c  | 569 +++++++++++++++++++++++++++
>  drivers/media/platform/dwc/dw-mipi-csi.h  | 287 ++++++++++++++
>  include/media/dwc/dw-mipi-csi-pltfrm.h    | 104 +++++
>  11 files changed, 2195 insertions(+)
>  create mode 100644 drivers/media/platform/dwc/Kconfig
>  create mode 100644 drivers/media/platform/dwc/Makefile
>  create mode 100644 drivers/media/platform/dwc/dw-csi-plat.c
>  create mode 100644 drivers/media/platform/dwc/dw-csi-plat.h
>  create mode 100644 drivers/media/platform/dwc/dw-csi-sysfs.c
>  create mode 100644 drivers/media/platform/dwc/dw-mipi-csi.c
>  create mode 100644 drivers/media/platform/dwc/dw-mipi-csi.h
>  create mode 100644 include/media/dwc/dw-mipi-csi-pltfrm.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16a97ba..6ffe4fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15187,6 +15187,14 @@ S:	Maintained
>  F:	drivers/dma/dwi-axi-dmac/
>  F:	Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
>  
> +SYNOPSYS DESIGNWARE MIPI DPHY CSI-2 HOST DRIVER
> +M:	Luis Oliveira <luis.oliveira@synopsys.com>
> +L:	linux-media@vger.kernel.org
> +T:	git git://linuxtv.org/media_tree.git
> +S:	Maintained
> +F:	drivers/media/platform/dwc
> +F:	Documentation/devicetree/bindings/media/snps,dw-csi.txt
> +
>  SYNOPSYS DESIGNWARE DMAC DRIVER
>  M:	Viresh Kumar <vireshk@kernel.org>
>  R:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 8a19654..b6fb139 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -147,6 +147,7 @@ source "drivers/media/platform/xilinx/Kconfig"
>  source "drivers/media/platform/rcar-vin/Kconfig"
>  source "drivers/media/platform/atmel/Kconfig"
>  source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
> +source "drivers/media/platform/dwc/Kconfig"
>  
>  config VIDEO_TI_CAL
>  	tristate "TI CAL (Camera Adaptation Layer) driver"
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 7cbbd92..4807caf 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -101,3 +101,5 @@ obj-y					+= meson/
>  obj-y					+= cros-ec-cec/
>  
>  obj-$(CONFIG_VIDEO_SUN6I_CSI)		+= sunxi/sun6i-csi/
> +
> +obj-y					+= dwc/
> diff --git a/drivers/media/platform/dwc/Kconfig b/drivers/media/platform/dwc/Kconfig
> new file mode 100644
> index 0000000..508ac21
> --- /dev/null
> +++ b/drivers/media/platform/dwc/Kconfig
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +#  Synopsys DWC Platform drivers
> +#	Drivers here are currently for MIPI CSI-2 support
> +
> +config DWC_MIPI_CSI2_HOST
> +	tristate "Synopsys DesignWare CSI-2 Host Controller support"
> +	select VIDEO_DEV
> +	select VIDEO_V4L2
> +	select VIDEO_V4L2_SUBDEV_API

Please depend on the above instead.

> +	select V4L2_FWNODE
> +	help
> +	  This selects the DesignWare MIPI CSI-2 host controller support. This
> +	  controller gives access to control a CSI-2 receiver acting as a V4L2
> +	  subdevice.
> +
> +	  If you have a controller with this interface, say Y.
> +
> +	   If unsure, say N.

You might mention the module name.

> diff --git a/drivers/media/platform/dwc/Makefile b/drivers/media/platform/dwc/Makefile
> new file mode 100644
> index 0000000..057f137
> --- /dev/null
> +++ b/drivers/media/platform/dwc/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for Synopsys DWC Platform drivers
> +#
> +dw-csi-objs := dw-csi-plat.o dw-mipi-csi.o
> +ifeq ($(CONFIG_DWC_MIPI_TC_DPHY_GEN3),y)
> +	dw-csi-objs += dw-csi-sysfs.o
> +endif
> +obj-$(CONFIG_DWC_MIPI_CSI2_HOST) += dw-csi.o
> diff --git a/drivers/media/platform/dwc/dw-csi-plat.c b/drivers/media/platform/dwc/dw-csi-plat.c
> new file mode 100644
> index 0000000..9828d55
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw-csi-plat.c
> @@ -0,0 +1,475 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018-2019 Synopsys, Inc. and/or its affiliates.
> + *
> + * Synopsys DesignWare MIPI CSI-2 Host controller driver.
> + * Platform driver
> + *
> + * Author: Luis Oliveira <luis.oliveira@synopsys.com>
> + */
> +
> +#include <media/dwc/dw-dphy-data.h>
> +
> +#include "dw-csi-plat.h"
> +
> +const struct mipi_dt csi_dt[] = {

Make this static or use a common prefix that somehow resembles the name
name of the driver.

> +	{
> +		.hex = CSI_2_YUV420_8,
> +		.name = "YUV420_8bits",
> +	}, {
> +		.hex = CSI_2_YUV420_10,
> +		.name = "YUV420_10bits",
> +	}, {
> +		.hex = CSI_2_YUV420_8_LEG,
> +		.name = "YUV420_8bits_LEGACY",
> +	}, {
> +		.hex = CSI_2_YUV420_8_SHIFT,
> +		.name = "YUV420_8bits_SHIFT",
> +	}, {
> +		.hex = CSI_2_YUV420_10_SHIFT,
> +		.name = "YUV420_10bits_SHIFT",
> +	}, {
> +		.hex = CSI_2_YUV422_8,
> +		.name = "YUV442_8bits",
> +	}, {
> +		.hex = CSI_2_YUV422_10,
> +		.name = "YUV442_10bits",
> +	}, {
> +		.hex = CSI_2_RGB444,
> +		.name = "RGB444",
> +	}, {
> +		.hex = CSI_2_RGB555,
> +		.name = "RGB555",
> +	}, {
> +		.hex = CSI_2_RGB565,
> +		.name = "RGB565",
> +	}, {
> +		.hex = CSI_2_RGB666,
> +		.name = "RGB666",
> +	}, {
> +		.hex = CSI_2_RGB888,
> +		.name = "RGB888",
> +	}, {
> +		.hex = CSI_2_RAW6,
> +		.name = "RAW6",
> +	}, {
> +		.hex = CSI_2_RAW7,
> +		.name = "RAW7",
> +	}, {
> +		.hex = CSI_2_RAW8,
> +		.name = "RAW8",
> +	}, {
> +		.hex = CSI_2_RAW10,
> +		.name = "RAW10",
> +	}, {
> +		.hex = CSI_2_RAW12,
> +		.name = "RAW12",
> +	}, {
> +		.hex = CSI_2_RAW14,
> +		.name = "RAW14",
> +	}, {
> +		.hex = CSI_2_RAW16,
> +		.name = "RAW16",
> +	},
> +};
> +
> +static struct mipi_fmt *
> +find_dw_mipi_csi_format(struct v4l2_mbus_framefmt *mf)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dw_mipi_csi_formats); i++)
> +		if (mf->code == dw_mipi_csi_formats[i].mbus_code)
> +			return &dw_mipi_csi_formats[i];
> +
> +	return NULL;
> +}
> +
> +static int dw_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd,
> +				      struct v4l2_subdev_pad_config *cfg,
> +				      struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index >= ARRAY_SIZE(dw_mipi_csi_formats))
> +		return -EINVAL;
> +
> +	if (code->index != 0)
> +		return -EINVAL;

One of the two conditions makes no sense. Is it the latter?

> +
> +	code->code = dw_mipi_csi_formats[code->index].mbus_code;
> +	return 0;
> +}
> +
> +static struct mipi_fmt *
> +dw_mipi_csi_try_format(struct v4l2_mbus_framefmt *mf)
> +{
> +	struct mipi_fmt *fmt;
> +
> +	fmt = find_dw_mipi_csi_format(mf);
> +	if (!fmt)
> +		fmt = &dw_mipi_csi_formats[0];
> +

You could return &dw_mipi_csi_formats[0] in find_dw_mipi_csi_format() and
omit this function.

> +	mf->code = fmt->mbus_code;
> +
> +	return fmt;
> +}
> +
> +static struct v4l2_mbus_framefmt *
> +dw_mipi_csi_get_format(struct dw_csi *dev, struct v4l2_subdev_pad_config *cfg,
> +		       enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return cfg ? v4l2_subdev_get_try_format(&dev->sd,
> +							cfg,
> +							0) : NULL;
> +	dev_dbg(dev->dev,
> +		"%s got v4l2_mbus_pixelcode. 0x%x\n", __func__,
> +		dev->format.code);
> +	dev_dbg(dev->dev,
> +		"%s got width. 0x%x\n", __func__,
> +		dev->format.width);
> +	dev_dbg(dev->dev,
> +		"%s got height. 0x%x\n", __func__,
> +		dev->format.height);

I'd just omit these debug prints in a driver. But adding them to the
framework might make sense. We don't have a lot of debug prints dealing
with user parameters in there. OTOH the common test programs largely do the
same already.

> +	return &dev->format;
> +}
> +
> +static int
> +dw_mipi_csi_set_fmt(struct v4l2_subdev *sd,
> +		    struct v4l2_subdev_pad_config *cfg,
> +		    struct v4l2_subdev_format *fmt)
> +{
> +	struct dw_csi *dev = sd_to_mipi_csi_dev(sd);
> +	struct mipi_fmt *dev_fmt;
> +	struct v4l2_mbus_framefmt *mf = dw_mipi_csi_get_format(dev, cfg,
> +							       fmt->which);
> +	int i;

unsigned int

> +
> +	dev_fmt = dw_mipi_csi_try_format(&fmt->format);
> +
> +	if (dev_fmt) {

Can dev_fmt be NULL?

> +		*mf = fmt->format;
> +		if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +			dev->fmt = dev_fmt;
> +		dev->fmt->mbus_code = mf->code;
> +		dw_mipi_csi_set_ipi_fmt(dev);
> +	}
> +
> +	if (fmt->format.width > 0 && fmt->format.height > 0) {
> +		dw_mipi_csi_fill_timings(dev, fmt);
> +	} else {
> +		dev_vdbg(dev->dev, "%s unacceptable values 0x%x.\n",
> +			 __func__, fmt->format.width);
> +		dev_vdbg(dev->dev, "%s unacceptable values 0x%x.\n",
> +			 __func__, fmt->format.height);

Instead of returning an error here, a driver needs to adjust them to make
them valid.

> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(csi_dt); i++)
> +		if (csi_dt[i].hex == dev->ipi_dt) {
> +			dev_vdbg(dev->dev, "Using data type %s\n",
> +				 csi_dt[i].name);
> +		}

Braces not needed here.

> +	return 0;
> +}
> +
> +static int
> +dw_mipi_csi_get_fmt(struct v4l2_subdev *sd,
> +		    struct v4l2_subdev_pad_config *cfg,
> +		    struct v4l2_subdev_format *fmt)
> +{
> +	struct dw_csi *dev = sd_to_mipi_csi_dev(sd);
> +	struct v4l2_mbus_framefmt *mf;
> +
> +	mf = dw_mipi_csi_get_format(dev, cfg, fmt->which);
> +	if (!mf)
> +		return -EINVAL;
> +
> +	mutex_lock(&dev->lock);
> +	fmt->format = *mf;
> +	mutex_unlock(&dev->lock);
> +
> +	return 0;
> +}
> +
> +static int
> +dw_mipi_csi_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct dw_csi *dev = sd_to_mipi_csi_dev(sd);
> +
> +	dev_vdbg(dev->dev, "%s: on=%d\n", __func__, on);
> +
> +	if (on) {
> +		dw_mipi_csi_hw_stdby(dev);
> +		dw_mipi_csi_start(dev);
> +	} else {
> +		phy_power_off(dev->phy);
> +		dw_mipi_csi_mask_irq_power_off(dev);
> +		/* reset data type */
> +		dev->ipi_dt = 0x0;
> +	}
> +	return 0;
> +}
> +
> +static int
> +dw_mipi_csi_log_status(struct v4l2_subdev *sd)
> +{
> +	struct dw_csi *dev = sd_to_mipi_csi_dev(sd);
> +
> +	dw_mipi_csi_dump(dev);
> +
> +	return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_VIDEO_ADV_DEBUG)
> +static int
> +dw_mipi_csi_g_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *reg)
> +{
> +	struct dw_csi *dev = sd_to_mipi_csi_dev(sd);
> +
> +	dev_vdbg(dev->dev, "%s: reg=%llu\n", __func__, reg->reg);
> +	reg->val = dw_mipi_csi_read(dev, reg->reg);
> +
> +	return 0;
> +}
> +#endif
> +static int dw_mipi_csi_init_cfg(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct v4l2_mbus_framefmt *format =
> +	    v4l2_subdev_get_try_format(sd, cfg, 0);
> +
> +	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->code = MEDIA_BUS_FMT_RGB888_1X24;
> +	format->field = V4L2_FIELD_NONE;
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_core_ops dw_mipi_csi_core_ops = {
> +	.s_power = dw_mipi_csi_s_power,
> +	.log_status = dw_mipi_csi_log_status,
> +#if IS_ENABLED(CONFIG_VIDEO_ADV_DEBUG)
> +	.g_register = dw_mipi_csi_g_register,
> +#endif
> +};
> +
> +static struct v4l2_subdev_pad_ops dw_mipi_csi_pad_ops = {
> +	.init_cfg = dw_mipi_csi_init_cfg,
> +	.enum_mbus_code = dw_mipi_csi_enum_mbus_code,
> +	.get_fmt = dw_mipi_csi_get_fmt,
> +	.set_fmt = dw_mipi_csi_set_fmt,
> +};
> +
> +static struct v4l2_subdev_ops dw_mipi_csi_subdev_ops = {
> +	.core = &dw_mipi_csi_core_ops,
> +	.pad = &dw_mipi_csi_pad_ops,
> +};
> +
> +static irqreturn_t dw_mipi_csi_irq1(int irq, void *dev_id)
> +{
> +	struct dw_csi *csi_dev = dev_id;
> +
> +	dw_mipi_csi_irq_handler(csi_dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int
> +dw_mipi_csi_parse_dt(struct platform_device *pdev, struct dw_csi *dev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
> +	int ret = 0;

ret is always assigned, no need to do that here.

> +
> +	if (of_property_read_u32(node, "snps,output-type",
> +				 &dev->hw.output))
> +		dev->hw.output = 2;

What does this do?

> +
> +	node = of_graph_get_next_endpoint(node, NULL);
> +	if (!node) {
> +		dev_err(&pdev->dev, "No port node at %pOF\n",
> +			pdev->dev.of_node);
> +		return -EINVAL;
> +	}
> +	/* Get port node and validate MIPI-CSI channel id. */
> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &ep);
> +	if (ret)
> +		goto err;
> +
> +	dev->index = ep.base.port - 1;

Why? Wouldn't index be -1 here?

> +	if (dev->index >= CSI_MAX_ENTITIES) {
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +	dev->hw.num_lanes = ep.bus.mipi_csi2.num_data_lanes;

A newline would be nice here.

> +err:
> +	of_node_put(node);

And here.

> +	return ret;
> +}
> +
> +static const struct of_device_id dw_mipi_csi_of_match[];

Could you move the declaration here?

> +
> +static int dw_csi_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *of_id = NULL;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res = NULL;
> +	struct dw_csi *csi;
> +	struct v4l2_subdev *sd;
> +	int ret;
> +
> +	dev_vdbg(dev, "Probing started\n");

I'd guess that might be needed at development time, but please remove it
now.

> +
> +	/* Resource allocation */
> +	csi = devm_kzalloc(dev, sizeof(*csi), GFP_KERNEL);
> +	if (!csi)
> +		return -ENOMEM;
> +
> +	mutex_init(&csi->lock);

Remember to do mutex_destroy() on this in error handling and driver's
remove() function.

> +	spin_lock_init(&csi->slock);
> +	csi->dev = dev;
> +
> +	of_id = of_match_node(dw_mipi_csi_of_match, dev->of_node);
> +	if (!of_id)
> +		return -EINVAL;
> +
> +	ret = dw_mipi_csi_parse_dt(pdev, csi);
> +	if (ret < 0)
> +		return ret;
> +
> +	csi->phy = devm_of_phy_get(dev, dev->of_node, NULL);
> +	if (IS_ERR(csi->phy)) {
> +		dev_err(dev, "No DPHY available\n");
> +		return PTR_ERR(csi->phy);
> +	}
> +	/* Registers mapping */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENXIO;
> +
> +	csi->base_address = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(csi->base_address)) {
> +		dev_err(dev, "Base address not set.\n");
> +		return PTR_ERR(csi->base_address);
> +	}
> +
> +	csi->ctrl_irq_number = platform_get_irq(pdev, 0);
> +	if (csi->ctrl_irq_number < 0) {
> +		dev_err(dev, "irq number %d not set.\n", csi->ctrl_irq_number);
> +		ret = csi->ctrl_irq_number;
> +		goto end;
> +	}
> +
> +	csi->rst = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(csi->rst)) {
> +		dev_err(dev, "error getting reset control %d\n", ret);
> +		return PTR_ERR(csi->rst);
> +	}
> +
> +	ret = devm_request_irq(dev, csi->ctrl_irq_number,
> +			       dw_mipi_csi_irq1, IRQF_SHARED,
> +			       dev_name(dev), csi);
> +	if (ret) {
> +		dev_err(dev, "irq csi %s failed\n", of_id->name);
> +
> +		goto end;
> +	}
> +
> +	sd = &csi->sd;
> +	v4l2_subdev_init(sd, &dw_mipi_csi_subdev_ops);
> +	csi->sd.owner = THIS_MODULE;
> +
> +	snprintf(sd->name, sizeof(sd->name), "%s.%d",
> +		 "dw-csi", csi->index);
> +
> +	csi->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	csi->fmt = &dw_mipi_csi_formats[0];
> +	csi->format.code = dw_mipi_csi_formats[0].mbus_code;
> +
> +	sd->entity.function = MEDIA_ENT_F_IO_V4L;
> +	csi->pads[CSI_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +	csi->pads[CSI_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&csi->sd.entity,
> +				     CSI_PADS_NUM, csi->pads);
> +
> +	dev_vdbg(dev, "v4l2.name: %s\n", csi->v4l2_dev.name);

Please remove.

> +
> +	if (ret < 0) {
> +		dev_err(dev, "media entity init failed\n");
> +		goto end;
> +	}
> +
> +	v4l2_set_subdevdata(&csi->sd, pdev);
> +	platform_set_drvdata(pdev, &csi->sd);
> +	dev_set_drvdata(dev, sd);
> +
> +	if (csi->rst)
> +		reset_control_deassert(csi->rst);
> +#if IS_ENABLED(CONFIG_DWC_MIPI_TC_DPHY_GEN3)
> +	dw_csi_create_capabilities_sysfs(pdev);
> +#endif
> +	dw_mipi_csi_get_version(csi);
> +	dw_mipi_csi_specific_mappings(csi);
> +	dw_mipi_csi_mask_irq_power_off(csi);
> +
> +	dev_info(dev, "DW MIPI CSI-2 Host registered successfully HW v%u.%u\n",
> +		 csi->hw_version_major, csi->hw_version_minor);
> +
> +	phy_init(csi->phy);
> +
> +	return 0;
> +
> +end:
> +	media_entity_cleanup(&csi->sd.entity);
> +	v4l2_device_unregister(csi->vdev.v4l2_dev);
> +
> +	return ret;
> +}
> +
> +/**
> + * @short Exit routine - Exit point of the driver
> + * @param[in] pdev pointer to the platform device structure
> + * @return 0 on success
> + */
> +static int dw_csi_remove(struct platform_device *pdev)
> +{
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *mipi_csi = sd_to_mipi_csi_dev(sd);
> +
> +	dev_dbg(&pdev->dev, "Removing MIPI CSI-2 module\n");
> +
> +	if (mipi_csi->rst)
> +		reset_control_assert(mipi_csi->rst);
> +	media_entity_cleanup(&mipi_csi->sd.entity);
> +
> +	return 0;
> +}
> +
> +/**
> + * @short of_device_id structure
> + */
> +static const struct of_device_id dw_mipi_csi_of_match[] = {
> +	{ .compatible = "snps,dw-csi" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, dw_mipi_csi_of_match);
> +
> +/**
> + * @short Platform driver structure
> + */
> +static struct platform_driver dw_mipi_csi_driver = {
> +	.remove = dw_csi_remove,
> +	.probe = dw_csi_probe,
> +	.driver = {
> +		.name = "dw-csi",
> +		.owner = THIS_MODULE,

You can drop the owner field.

> +		.of_match_table = of_match_ptr(dw_mipi_csi_of_match),
> +	},
> +};
> +
> +module_platform_driver(dw_mipi_csi_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Luis Oliveira <luis.oliveira@synopsys.com>");
> +MODULE_DESCRIPTION("Synopsys DesignWare MIPI CSI-2 Host Platform driver");
> diff --git a/drivers/media/platform/dwc/dw-csi-plat.h b/drivers/media/platform/dwc/dw-csi-plat.h
> new file mode 100644
> index 0000000..e322592
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw-csi-plat.h
> @@ -0,0 +1,97 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018 Synopsys, Inc.
> + *
> + * Synopsys DesignWare MIPI CSI-2 Host controller driver.
> + * Supported bus formats
> + *
> + * Author: Luis Oliveira <Luis.Oliveira@synopsys.com>
> + */
> +
> +#ifndef _DW_CSI_PLAT_H__
> +#define _DW_CSI_PLAT_H__
> +
> +#include "dw-mipi-csi.h"
> +
> +/* Video formats supported by the MIPI CSI-2 */
> +struct mipi_fmt dw_mipi_csi_formats[] = {

Please move to the .c file. If you need this in multiple files, please use
a forward declaration. This also should be const.

> +	{
> +		/* RAW 8 */
> +		.mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> +		.depth = 8,
> +	}, {
> +		/* RAW 10 */
> +		.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> +		.depth = 10,
> +	}, {
> +		/* RAW 12 */
> +		.mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> +		.depth = 12,
> +	}, {
> +		/* RAW 14 */
> +		.mbus_code = MEDIA_BUS_FMT_SBGGR14_1X14,
> +		.depth = 14,
> +	}, {
> +		/* RAW 16 */
> +		.mbus_code = MEDIA_BUS_FMT_SBGGR16_1X16,
> +		.depth = 16,
> +	}, {
> +		/* RGB 666 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB666_1X18,
> +		.depth = 18,
> +	}, {
> +		/* RGB 565 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> +		.depth = 16,
> +	}, {
> +		/* BGR 565 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> +		.depth = 16,
> +	}, {
> +		/* RGB 555 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE,
> +		.depth = 16,
> +	}, {
> +		/* BGR 555 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE,
> +		.depth = 16,
> +	}, {
> +		/* RGB 444 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE,
> +		.depth = 16,
> +	}, {
> +		/* RGB 444 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE,
> +		.depth = 16,
> +	}, {
> +		/* RGB 888 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB888_2X12_LE,
> +		.depth = 24,
> +	}, {
> +		/* BGR 888 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB888_2X12_BE,
> +		.depth = 24,
> +	}, {
> +		/* BGR 888 */
> +		.mbus_code = MEDIA_BUS_FMT_RGB888_1X24,
> +		.depth = 24,
> +	}, {
> +		/* YUV 422 8-bit */
> +		.mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> +		.depth = 16,
> +	}, {
> +		/* YUV 422 10-bit */
> +		.mbus_code = MEDIA_BUS_FMT_VYUY10_2X10,
> +		.depth = 20,
> +	}, {
> +		/* YUV 420 8-bit LEGACY */
> +		.mbus_code = MEDIA_BUS_FMT_Y8_1X8,
> +		.depth = 8,
> +	}, {
> +		/* YUV 420 10-bit */
> +		.mbus_code = MEDIA_BUS_FMT_Y10_1X10,
> +		.depth = 10,
> +	},
> +};
> +
> +#endif	/* _DW_CSI_PLAT_H__ */
> diff --git a/drivers/media/platform/dwc/dw-csi-sysfs.c b/drivers/media/platform/dwc/dw-csi-sysfs.c
> new file mode 100644
> index 0000000..e8d2bb9
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw-csi-sysfs.c
> @@ -0,0 +1,624 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018-2019 Synopsys, Inc. and/or its affiliates.
> + *
> + * Synopsys DesignWare MIPI CSI-2 Host controller driver.
> + * SysFS components for the platform driver
> + *
> + * Author: Luis Oliveira <Luis.Oliveira@synopsys.com>
> + */
> +
> +#include "dw-mipi-csi.h"
> +
> +static ssize_t core_version_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "v.%d.%d*\n", csi_dev->hw_version_major,
> +		 csi_dev->hw_version_minor);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t n_lanes_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long lanes;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 10, &lanes);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (lanes > 8) {
> +		dev_err(dev, "Invalid number of lanes %lu\n", lanes);
> +		return count;
> +	}
> +
> +	dev_info(dev, "Lanes %lu\n", lanes);
> +	csi_dev->hw.num_lanes = lanes;
> +
> +	return count;
> +}
> +
> +static ssize_t n_lanes_show(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%d\n", csi_dev->hw.num_lanes);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t core_reset_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	/* Reset Controller and DPHY */
> +	phy_reset(csi_dev->phy);
> +	dw_mipi_csi_reset(csi_dev);
> +
> +	snprintf(buffer, 10, "Reset\n");
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t data_type_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long dt;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 16, &dt);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (dt < 0x18 || dt > 0x2F) {
> +		dev_err(dev, "Invalid data type %lx\n", dt);
> +		return count;
> +	}
> +
> +	dev_info(dev, "Data type 0x%lx\n", dt);
> +	csi_dev->ipi_dt = dt;
> +
> +	return count;
> +}
> +
> +static ssize_t data_type_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%x\n", csi_dev->ipi_dt);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t hsa_store(struct device *dev,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long hsa;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 16, &hsa);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (hsa > 0xFFF) {
> +		dev_err(dev, "Invalid HSA time %lx\n", hsa);
> +		return count;
> +	}
> +
> +	dev_info(dev, "HSA time 0x%lx\n", hsa);
> +	csi_dev->hw.hsa = hsa;
> +
> +	return count;
> +}
> +
> +static ssize_t hsa_show(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%x\n", csi_dev->hw.hsa);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t hbp_store(struct device *dev,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long hbp;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 16, &hbp);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (hbp > 0xFFF) {
> +		dev_err(dev, "Invalid HBP time %lx\n", hbp);
> +		return count;
> +	}
> +
> +	dev_info(dev, "HBP time 0x%lx\n", hbp);
> +	csi_dev->hw.hbp = hbp;
> +
> +	return count;
> +}
> +
> +static ssize_t hbp_show(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%x\n", csi_dev->hw.hbp);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t hsd_store(struct device *dev,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long hsd;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 16, &hsd);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (hsd > 0xFF) {
> +		dev_err(dev, "Invalid HSD time %lx\n", hsd);
> +		return count;
> +	}
> +
> +	dev_info(dev, "HSD time 0x%lx\n", hsd);
> +	csi_dev->hw.hsd = hsd;
> +
> +	return count;
> +}
> +
> +static ssize_t hsd_show(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%x\n", csi_dev->hw.hsd);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t vsa_store(struct device *dev,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long vsa;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 16, &vsa);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (vsa > 0x3FF) {
> +		dev_err(dev, "Invalid VSA period %lx\n", vsa);
> +		return count;
> +	}
> +
> +	dev_info(dev, "VSA period 0x%lx\n", vsa);
> +	csi_dev->hw.vsa = vsa;
> +
> +	return count;
> +}
> +
> +static ssize_t vsa_show(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%x\n", csi_dev->hw.vsa);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t vbp_store(struct device *dev,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long vbp;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 16, &vbp);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (vbp > 0x2FF) {
> +		dev_err(dev, "Invalid VBP period %lx\n", vbp);
> +		return count;
> +	}
> +
> +	dev_info(dev, "VBP period 0x%lx\n", vbp);
> +	csi_dev->hw.vbp = vbp;
> +
> +	return count;
> +}
> +
> +static ssize_t vbp_show(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%x\n", csi_dev->hw.vbp);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t vfp_store(struct device *dev,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long vfp;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 16, &vfp);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (vfp > 0x3ff) {
> +		dev_err(dev, "Invalid VFP period %lx\n", vfp);
> +		return count;
> +	}
> +
> +	dev_info(dev, "VFP period 0x%lx\n", vfp);
> +	csi_dev->hw.vfp = vfp;
> +
> +	return count;
> +}
> +
> +static ssize_t vfp_show(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%x\n", csi_dev->hw.vfp);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t virtual_channel_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long virtual_ch;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 10, &virtual_ch);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((signed int)virtual_ch < 0 || (signed int)virtual_ch > 8) {
> +		dev_err(dev, "Invalid Virtual Channel %lu\n", virtual_ch);
> +		return count;
> +	}
> +
> +	dev_info(dev, "Virtual Channel %lu\n", virtual_ch);
> +	csi_dev->hw.virtual_ch = virtual_ch;
> +
> +	return count;
> +}
> +
> +static ssize_t virtual_channel_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%d\n", csi_dev->hw.virtual_ch);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t ipi_color_mode_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long ipi_color_mode;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 10, &ipi_color_mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((signed int)ipi_color_mode < 0 || (signed int)ipi_color_mode > 1) {
> +		dev_err(dev,
> +			"Wrong Color Mode %lu, (48 bits -> 0 or 16 bits -> 1\n",
> +			ipi_color_mode);
> +		return count;
> +	}
> +
> +	dev_info(dev, "IPI Color mode %lu\n", ipi_color_mode);
> +	csi_dev->hw.ipi_color_mode = ipi_color_mode;
> +
> +	return count;
> +}
> +
> +static ssize_t ipi_color_mode_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%d\n", csi_dev->hw.ipi_color_mode);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t ipi_auto_flush_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long ipi_auto_flush;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 10, &ipi_auto_flush);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((signed int)ipi_auto_flush < 0 || (signed int)ipi_auto_flush > 1) {
> +		dev_err(dev,
> +			"Invalid Auto Flush Mode %lu, (No -> 0 or Yes -> 1\n",
> +			ipi_auto_flush);
> +		return count;
> +	}
> +
> +	dev_info(dev, "IPI Auto Flush %lu\n", ipi_auto_flush);
> +	csi_dev->hw.ipi_auto_flush = ipi_auto_flush;
> +
> +	return count;
> +}
> +
> +static ssize_t ipi_auto_flush_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%d\n", csi_dev->hw.ipi_auto_flush);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t ipi_timings_mode_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long ipi_mode;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 10, &ipi_mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((signed int)ipi_mode < 0 || (signed int)ipi_mode > 1) {
> +		dev_err(dev,
> +			"Invalid Timing Source %lu (Camera:0|Controller:1)\n",
> +			ipi_mode);
> +		return count;
> +	}
> +
> +	dev_info(dev, "IPI Color mode %lu\n", ipi_mode);
> +	csi_dev->hw.ipi_mode = ipi_mode;
> +
> +	return count;
> +}
> +
> +static ssize_t ipi_timings_mode_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%d\n", csi_dev->hw.ipi_mode);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static ssize_t output_type_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long output;
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	ret = kstrtoul(buf, 10, &output);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((signed int)output < 0 || (signed int)output > 1) {
> +		dev_err(dev,
> +			"Invalid Core output %lu to be used \
> +			(IPI-> 0 or IDI->1 or BOTH- 2\n",
> +			output);
> +		return count;
> +	}
> +
> +	dev_info(dev, "IPI Color mode %lu\n", output);
> +	csi_dev->hw.output = output;
> +
> +	return count;
> +}
> +
> +static ssize_t output_type_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct dw_csi *csi_dev = sd_to_mipi_csi_dev(sd);
> +
> +	char buffer[10];
> +
> +	snprintf(buffer, 10, "%d\n", csi_dev->hw.output);
> +
> +	return strlcpy(buf, buffer, PAGE_SIZE);
> +}
> +
> +static DEVICE_ATTR_RO(core_version);
> +static DEVICE_ATTR_RO(core_reset);
> +static DEVICE_ATTR_RW(n_lanes);
> +static DEVICE_ATTR_RW(data_type);
> +static DEVICE_ATTR_RW(hsa);
> +static DEVICE_ATTR_RW(hbp);
> +static DEVICE_ATTR_RW(hsd);
> +static DEVICE_ATTR_RW(vsa);
> +static DEVICE_ATTR_RW(vbp);
> +static DEVICE_ATTR_RW(vfp);
> +static DEVICE_ATTR_RW(virtual_channel);
> +static DEVICE_ATTR_RW(ipi_color_mode);
> +static DEVICE_ATTR_RW(ipi_auto_flush);
> +static DEVICE_ATTR_RW(ipi_timings_mode);
> +static DEVICE_ATTR_RW(output_type);
> +
> +int dw_csi_create_capabilities_sysfs(struct platform_device *pdev)
> +{
> +	device_create_file(&pdev->dev, &dev_attr_core_version);
> +	device_create_file(&pdev->dev, &dev_attr_core_reset);
> +	device_create_file(&pdev->dev, &dev_attr_n_lanes);
> +	device_create_file(&pdev->dev, &dev_attr_data_type);
> +	device_create_file(&pdev->dev, &dev_attr_hsa);
> +	device_create_file(&pdev->dev, &dev_attr_hbp);
> +	device_create_file(&pdev->dev, &dev_attr_hsd);
> +	device_create_file(&pdev->dev, &dev_attr_vsa);
> +	device_create_file(&pdev->dev, &dev_attr_vbp);
> +	device_create_file(&pdev->dev, &dev_attr_vfp);
> +	device_create_file(&pdev->dev, &dev_attr_virtual_channel);
> +	device_create_file(&pdev->dev, &dev_attr_ipi_color_mode);
> +	device_create_file(&pdev->dev, &dev_attr_ipi_auto_flush);
> +	device_create_file(&pdev->dev, &dev_attr_ipi_timings_mode);
> +	device_create_file(&pdev->dev, &dev_attr_output_type);
> +
> +	return 0;
> +}

For configuring the above, we do have (or at least will have for some, such
as VC) proper V4L2 APIs. Please use them, and postpone e.g. VC
configurability until we do.

> diff --git a/drivers/media/platform/dwc/dw-mipi-csi.c b/drivers/media/platform/dwc/dw-mipi-csi.c
> new file mode 100644
> index 0000000..d01418d
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw-mipi-csi.c
> @@ -0,0 +1,569 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018-2019 Synopsys, Inc. and/or its affiliates.
> + *
> + * Synopsys DesignWare MIPI CSI-2 Host controller driver
> + * Core MIPI CSI-2 functions
> + *
> + * Author: Luis Oliveira <Luis.Oliveira@synopsys.com>
> + */
> +
> +#include "dw-mipi-csi.h"
> +
> +static struct R_CSI2 reg = {

const. Same below.

> +	.VERSION = 0x00,
> +	.N_LANES = 0x04,
> +	.CTRL_RESETN = 0x08,
> +	.INTERRUPT = 0x0C,
> +	.DATA_IDS_1 = 0x10,
> +	.DATA_IDS_2 = 0x14,
> +	.IPI_MODE = 0x80,
> +	.IPI_VCID = 0x84,
> +	.IPI_DATA_TYPE = 0x88,
> +	.IPI_MEM_FLUSH = 0x8C,
> +	.IPI_HSA_TIME = 0x90,
> +	.IPI_HBP_TIME = 0x94,
> +	.IPI_HSD_TIME = 0x98,
> +	.IPI_HLINE_TIME = 0x9C,
> +	.IPI_SOFTRSTN = 0xA0,
> +	.IPI_ADV_FEATURES = 0xAC,
> +	.IPI_VSA_LINES = 0xB0,
> +	.IPI_VBP_LINES = 0xB4,
> +	.IPI_VFP_LINES = 0xB8,
> +	.IPI_VACTIVE_LINES = 0xBC,
> +	.INT_PHY_FATAL = 0xe0,
> +	.MASK_INT_PHY_FATAL = 0xe4,
> +	.FORCE_INT_PHY_FATAL = 0xe8,
> +	.INT_PKT_FATAL = 0xf0,
> +	.MASK_INT_PKT_FATAL = 0xf4,
> +	.FORCE_INT_PKT_FATAL = 0xf8,
> +	.INT_PHY = 0x110,
> +	.MASK_INT_PHY = 0x114,
> +	.FORCE_INT_PHY = 0x118,
> +	.INT_LINE = 0x130,
> +	.MASK_INT_LINE = 0x134,
> +	.FORCE_INT_LINE = 0x138,
> +	.INT_IPI = 0x140,
> +	.MASK_INT_IPI = 0x144,
> +	.FORCE_INT_IPI = 0x148,

Instead I'd add #defines for the register addresses: this is static data
after all.

> +};
> +
> +struct interrupt_type csi_int = {
> +	.PHY_FATAL = BIT(0),
> +	.PKT_FATAL = BIT(1),
> +	.PHY = BIT(16),
> +};
> +
> +#define dw_print(VAR) \
> +	dev_info(csi_dev->dev, "%s: 0x%x: %X\n", "#VAR#",\

csi_dev should be an argument for the macro. Also, use parentheses around
the macro arguments.

It'd be better to define this also next to the only user.

> +	VAR, dw_mipi_csi_read(csi_dev, VAR))
> +
> +void dw_mipi_csi_write_part(struct dw_csi *dev, u32 address, u32 data,
> +			    u8 shift, u8 width)
> +{
> +	u32 mask = (1 << width) - 1;
> +	u32 temp = dw_mipi_csi_read(dev, address);
> +
> +	temp &= ~(mask << shift);
> +	temp |= (data & mask) << shift;
> +	dw_mipi_csi_write(dev, address, temp);
> +}
> +
> +void dw_mipi_csi_reset(struct dw_csi *csi_dev)
> +{
> +	dw_mipi_csi_write(csi_dev, reg.CTRL_RESETN, 0);
> +	usleep_range(100, 200);
> +	dw_mipi_csi_write(csi_dev, reg.CTRL_RESETN, 1);
> +}
> +
> +int dw_mipi_csi_mask_irq_power_off(struct dw_csi *csi_dev)
> +{
> +	if (csi_dev->hw_version_major == 1) {
> +		/* set only one lane (lane 0) as active (ON) */
> +		dw_mipi_csi_write(csi_dev, reg.N_LANES, 0);
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_PHY_FATAL, 0);
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_PKT_FATAL, 0);
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_PHY, 0);
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_LINE, 0);
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_IPI, 0);
> +
> +		/* only for version 1.30 */
> +		if (csi_dev->hw_version_minor == 30)
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MASK_INT_FRAME_FATAL, 0);
> +
> +		dw_mipi_csi_write(csi_dev, reg.CTRL_RESETN, 0);
> +
> +		/* only for version 1.40 */
> +		if (csi_dev->hw_version_minor == 40) {
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_BNDRY_FRAME_FATAL, 0);
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_SEQ_FRAME_FATAL, 0);
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_CRC_FRAME_FATAL, 0);
> +			dw_mipi_csi_write(csi_dev, reg.MSK_PLD_CRC_FATAL, 0);
> +			dw_mipi_csi_write(csi_dev, reg.MSK_DATA_ID, 0);
> +			dw_mipi_csi_write(csi_dev, reg.MSK_ECC_CORRECT, 0);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int dw_mipi_csi_hw_stdby(struct dw_csi *csi_dev)
> +{
> +	if (csi_dev->hw_version_major == 1) {
> +		/* set only one lane (lane 0) as active (ON) */
> +		dw_mipi_csi_reset(csi_dev);
> +		dw_mipi_csi_write(csi_dev, reg.N_LANES, 0);
> +		phy_init(csi_dev->phy);
> +
> +		/* only for version 1.30 */
> +		if (csi_dev->hw_version_minor == 30)
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MASK_INT_FRAME_FATAL,
> +					  GENMASK(31, 0));
> +
> +		/* common */
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_PHY_FATAL,
> +				  GENMASK(8, 0));
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_PKT_FATAL,
> +				  GENMASK(1, 0));
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_PHY, GENMASK(23, 0));
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_LINE, GENMASK(23, 0));
> +		dw_mipi_csi_write(csi_dev, reg.MASK_INT_IPI, GENMASK(5, 0));
> +
> +		/* only for version 1.40 */
> +		if (csi_dev->hw_version_minor == 40) {
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_BNDRY_FRAME_FATAL,
> +					  GENMASK(31, 0));
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_SEQ_FRAME_FATAL,
> +					  GENMASK(31, 0));
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_CRC_FRAME_FATAL,
> +					  GENMASK(31, 0));
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_PLD_CRC_FATAL,
> +					  GENMASK(31, 0));
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_DATA_ID, GENMASK(31, 0));
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.MSK_ECC_CORRECT, GENMASK(31, 0));
> +		}
> +	}
> +	return 0;
> +}
> +
> +void dw_mipi_csi_set_ipi_fmt(struct dw_csi *csi_dev)
> +{
> +	struct device *dev = csi_dev->dev;
> +
> +	if (csi_dev->ipi_dt) {
> +		dw_mipi_csi_write(csi_dev, reg.IPI_DATA_TYPE, csi_dev->ipi_dt);
> +		switch (csi_dev->ipi_dt) {
> +		case CSI_2_YUV420_8:
> +		case CSI_2_YUV420_8_LEG:
> +		case CSI_2_YUV420_8_SHIFT:
> +		break;
> +		case CSI_2_YUV420_10:
> +		case CSI_2_YUV420_10_SHIFT:
> +		break;

This doesn't do anything.

> +		}
> +	} else {
> +		switch (csi_dev->fmt->mbus_code) {
> +		/* RGB 666 */
> +		case MEDIA_BUS_FMT_RGB666_1X18:
> +		csi_dev->ipi_dt =  CSI_2_RGB666;
> +		break;

Indentation. Same below.

> +		/* RGB 565 */
> +		case MEDIA_BUS_FMT_RGB565_2X8_BE:
> +		case MEDIA_BUS_FMT_RGB565_2X8_LE:
> +		csi_dev->ipi_dt = CSI_2_RGB565;
> +		break;
> +		/* RGB 555 */
> +		case MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE:
> +		case MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE:
> +		csi_dev->ipi_dt = CSI_2_RGB555;
> +		break;
> +		/* RGB 444 */
> +		case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
> +		case MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE:
> +		csi_dev->ipi_dt = CSI_2_RGB444;
> +		break;
> +		/* RGB 888 */
> +		break;
> +		case MEDIA_BUS_FMT_RGB888_2X12_LE:
> +		case MEDIA_BUS_FMT_RGB888_2X12_BE:
> +		csi_dev->ipi_dt = CSI_2_RGB888;
> +		break;
> +		/* RAW 10 */
> +		case MEDIA_BUS_FMT_SBGGR10_1X10:
> +		case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:
> +		csi_dev->ipi_dt = CSI_2_RAW10;
> +		break;
> +		/* RAW 12 */
> +		case MEDIA_BUS_FMT_SBGGR12_1X12:
> +		csi_dev->ipi_dt = CSI_2_RAW12;
> +		break;
> +		/* RAW 14 */
> +		case MEDIA_BUS_FMT_SBGGR14_1X14:
> +		csi_dev->ipi_dt = CSI_2_RAW14;
> +		break;
> +		/* RAW 16 */
> +		case MEDIA_BUS_FMT_SBGGR16_1X16:
> +		csi_dev->ipi_dt = CSI_2_RAW16;
> +		break;
> +		/* RAW 8 */
> +		case MEDIA_BUS_FMT_SBGGR8_1X8:
> +		csi_dev->ipi_dt = CSI_2_RAW8;
> +		break;
> +		/* YUV 422 8-bit */
> +		case MEDIA_BUS_FMT_YVYU8_2X8:
> +		csi_dev->ipi_dt = CSI_2_RAW8;
> +		break;
> +		/* YUV 422 10-bit */
> +		case MEDIA_BUS_FMT_VYUY8_1X16:
> +		csi_dev->ipi_dt = CSI_2_YUV422_8;
> +		break;
> +		/* YUV 420 8-bit LEGACY */
> +		case MEDIA_BUS_FMT_Y8_1X8:
> +		csi_dev->ipi_dt = CSI_2_RAW8;
> +		break;
> +		/* YUV 420 10-bit */
> +		case MEDIA_BUS_FMT_Y10_1X10:
> +		csi_dev->ipi_dt = CSI_2_RAW8;
> +		break;
> +		default:
> +		break;
> +		}
> +		dw_mipi_csi_write(csi_dev, reg.IPI_DATA_TYPE, csi_dev->ipi_dt);
> +	}
> +	dev_info(dev, "Selected IPI Data Type 0x%X\n", csi_dev->ipi_dt);

Looks like dev_dbg() would be more appropriate.

> +}
> +
> +void dw_mipi_csi_fill_timings(struct dw_csi *dev,
> +			      struct v4l2_subdev_format *fmt)
> +{
> +	/* expected values */
> +	dev->hw.virtual_ch = 0;
> +	dev->hw.ipi_color_mode = COLOR48;
> +	dev->hw.ipi_auto_flush = 1;
> +	dev->hw.ipi_mode = CAMERA_TIMING;
> +	dev->hw.ipi_cut_through = CTINACTIVE;
> +	dev->hw.ipi_adv_features = LINE_EVENT_SELECTION(EVSELAUTO);
> +	dev->hw.htotal = fmt->format.width + dev->hw.hsa +
> +			 dev->hw.hbp + dev->hw.hsd;
> +	dev->hw.vactive = fmt->format.height;
> +	dev->hw.output = 2;
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +		dev_dbg(dev->dev, "*********** timings *********\n");
> +		dev_dbg(dev->dev, "Horizontal Sync Active: %d\n", dev->hw.hsa);
> +		dev_dbg(dev->dev, "Horizontal Back Porch: %d\n", dev->hw.hbp);
> +		dev_dbg(dev->dev, "Horizontal Width: %d\n", fmt->format.width);
> +		dev_dbg(dev->dev, "Horizontal Total: %d\n", dev->hw.htotal);
> +		dev_dbg(dev->dev, "Vertical Sync Active: %d\n", dev->hw.vsa);
> +		dev_dbg(dev->dev, "Vertical Back Porch: %d\n", dev->hw.vbp);
> +		dev_dbg(dev->dev, "Vertical Front Porch: %d\n", dev->hw.vfp);
> +		dev_dbg(dev->dev, "Vertical Active: %d\n", dev->hw.vactive);
> +	}
> +}
> +
> +void dw_mipi_csi_start(struct dw_csi *csi_dev)
> +{
> +	struct device *dev = csi_dev->dev;
> +
> +	dw_mipi_csi_write(csi_dev, reg.N_LANES, (csi_dev->hw.num_lanes - 1));
> +	dev_info(dev, "number of lanes: %d\n", csi_dev->hw.num_lanes);

Ditto.

> +
> +	/* IPI Related Configuration */
> +	if (csi_dev->hw.output == IPI_OUT || csi_dev->hw.output == BOTH_OUT) {
> +		if (csi_dev->hw_version_major >= 1) {
> +			if (csi_dev->hw_version_minor >= 20)
> +				dw_mipi_csi_write(csi_dev,
> +						  reg.IPI_ADV_FEATURES,
> +						  csi_dev->hw.ipi_adv_features);
> +			if (csi_dev->hw_version_minor >= 30)
> +				dw_mipi_csi_write(csi_dev,
> +						  reg.IPI_SOFTRSTN, 0x1);
> +		}
> +		/*  address | data, | shift | width */
> +		dw_mipi_csi_write_part(csi_dev, reg.IPI_MODE, 1, 24, 1);
> +		dw_mipi_csi_write_part(csi_dev,
> +				       reg.IPI_MODE,
> +				       csi_dev->hw.ipi_mode,
> +				       0, 1);
> +		if (csi_dev->hw.ipi_mode == CAMERA_TIMING) {
> +			dw_mipi_csi_write(csi_dev,
> +					  reg.IPI_ADV_FEATURES,
> +					  LINE_EVENT_SELECTION(EVSELPROG) |
> +					  EN_VIDEO |
> +					  EN_LINE_START |
> +					  EN_NULL |
> +					  EN_BLANKING |
> +					  EN_EMBEDDED);
> +		}

No need for braces.

> +		dw_mipi_csi_write_part(csi_dev,
> +				       reg.IPI_MODE,
> +				       csi_dev->hw.ipi_color_mode,
> +				       8, 1);
> +		dw_mipi_csi_write_part(csi_dev,
> +				       reg.IPI_MODE,
> +				       csi_dev->hw.ipi_cut_through,
> +				       16, 1);
> +		dw_mipi_csi_write_part(csi_dev,
> +				       reg.IPI_VCID,
> +				       csi_dev->hw.virtual_ch,
> +				       0, 2);
> +		dw_mipi_csi_write_part(csi_dev,
> +				       reg.IPI_MEM_FLUSH,
> +				       csi_dev->hw.ipi_auto_flush,
> +				       8, 1);
> +
> +		dev_vdbg(dev, "*********** config *********\n");
> +		dev_vdbg(dev, "IPI enable: %s\n",
> +			 csi_dev->hw.output ? "YES" : "NO");
> +		dev_vdbg(dev, "video mode transmission type: %s timming\n",
> +			 csi_dev->hw.ipi_mode ? "controller" : "camera");
> +		dev_vdbg(dev, "Color Mode: %s\n",
> +			 csi_dev->hw.ipi_color_mode ? "16 bits" : "48 bits");
> +		dev_vdbg(dev, "Cut Through Mode: %s\n",
> +			 csi_dev->hw.ipi_cut_through ? "enable" : "disable");
> +		dev_vdbg(dev, "Virtual Channel: %d\n",
> +			 csi_dev->hw.virtual_ch);
> +		dev_vdbg(dev, "Auto-flush: %d\n",
> +			 csi_dev->hw.ipi_auto_flush);
> +		dw_mipi_csi_write(csi_dev, reg.IPI_SOFTRSTN, 1);
> +
> +		if (csi_dev->hw.ipi_mode == AUTO_TIMING)
> +			phy_power_on(csi_dev->phy);
> +
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_HSA_TIME, csi_dev->hw.hsa);
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_HBP_TIME, csi_dev->hw.hbp);
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_HSD_TIME, csi_dev->hw.hsd);
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_HLINE_TIME, csi_dev->hw.htotal);
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_VSA_LINES, csi_dev->hw.vsa);
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_VBP_LINES, csi_dev->hw.vbp);
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_VFP_LINES, csi_dev->hw.vfp);
> +		dw_mipi_csi_write(csi_dev,
> +				  reg.IPI_VACTIVE_LINES, csi_dev->hw.vactive);
> +	}
> +	phy_power_on(csi_dev->phy);
> +}
> +
> +int dw_mipi_csi_irq_handler(struct dw_csi *csi_dev)
> +{
> +	struct device *dev = csi_dev->dev;
> +	u32 global_int_status, i_sts;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&csi_dev->slock, flags);
> +	global_int_status = dw_mipi_csi_read(csi_dev, reg.INTERRUPT);
> +
> +	if (global_int_status & csi_int.PHY_FATAL) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.INT_PHY_FATAL);
> +		dev_err_ratelimited(dev, "int %08X: PHY FATAL: %08X\n",
> +				    reg.INT_PHY_FATAL, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.PKT_FATAL) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.INT_PKT_FATAL);
> +		dev_err_ratelimited(dev, "int %08X: PKT FATAL: %08X\n",
> +				    reg.INT_PKT_FATAL, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.FRAME_FATAL &&
> +	    csi_dev->hw_version_major == 1 &&
> +	    csi_dev->hw_version_minor == 30) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.INT_FRAME_FATAL);
> +		dev_err_ratelimited(dev, "int %08X: FRAME FATAL: %08X\n",
> +				    reg.INT_FRAME_FATAL, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.PHY) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.INT_PHY);
> +		dev_err_ratelimited(dev, "int %08X: PHY: %08X\n",
> +				    reg.INT_PHY, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.PKT &&
> +	    csi_dev->hw_version_major == 1 &&
> +	    csi_dev->hw_version_minor <= 30) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.INT_PKT);
> +		dev_err_ratelimited(dev, "int %08X: PKT: %08X\n",
> +				    reg.INT_PKT, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.LINE) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.INT_LINE);
> +		dev_err_ratelimited(dev, "int %08X: LINE: %08X\n",
> +				    reg.INT_LINE, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.IPI) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.INT_IPI);
> +		dev_err_ratelimited(dev, "int %08X: IPI: %08X\n",
> +				    reg.INT_IPI, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.BNDRY_FRAME_FATAL) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.ST_BNDRY_FRAME_FATAL);
> +		dev_err_ratelimited(dev,
> +				    "int %08X: ST_BNDRY_FRAME_FATAL: %08X\n",
> +				    reg.ST_BNDRY_FRAME_FATAL, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.SEQ_FRAME_FATAL) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.ST_SEQ_FRAME_FATAL);
> +		dev_err_ratelimited(dev,
> +				    "int %08X: ST_SEQ_FRAME_FATAL: %08X\n",
> +				    reg.ST_SEQ_FRAME_FATAL, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.CRC_FRAME_FATAL) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.ST_CRC_FRAME_FATAL);
> +		dev_err_ratelimited(dev,
> +				    "int %08X: ST_CRC_FRAME_FATAL: %08X\n",
> +				    reg.ST_CRC_FRAME_FATAL, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.PLD_CRC_FATAL) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.ST_PLD_CRC_FATAL);
> +		dev_err_ratelimited(dev,
> +				    "int %08X: ST_PLD_CRC_FATAL: %08X\n",
> +				    reg.ST_PLD_CRC_FATAL, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.DATA_ID) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.ST_DATA_ID);
> +		dev_err_ratelimited(dev, "int %08X: ST_DATA_ID: %08X\n",
> +				    reg.ST_DATA_ID, i_sts);
> +	}
> +
> +	if (global_int_status & csi_int.ECC_CORRECTED) {
> +		i_sts = dw_mipi_csi_read(csi_dev, reg.ST_ECC_CORRECT);
> +		dev_err_ratelimited(dev, "int %08X: ST_ECC_CORRECT: %08X\n",
> +				    reg.ST_ECC_CORRECT, i_sts);
> +	}
> +
> +	spin_unlock_irqrestore(&csi_dev->slock, flags);
> +
> +	return 1;
> +}
> +
> +void dw_mipi_csi_get_version(struct dw_csi *csi_dev)
> +{
> +	u32 hw_version;
> +
> +	hw_version = dw_mipi_csi_read(csi_dev, reg.VERSION);
> +	csi_dev->hw_version_major = (u8)((hw_version >> 24) - '0');
> +	csi_dev->hw_version_minor = (u8)((hw_version >> 16) - '0');
> +	csi_dev->hw_version_minor = csi_dev->hw_version_minor * 10;
> +	csi_dev->hw_version_minor += (u8)((hw_version >> 8) - '0');
> +}
> +
> +int dw_mipi_csi_specific_mappings(struct dw_csi *csi_dev)
> +{
> +	struct device *dev = csi_dev->dev;
> +
> +	if (csi_dev->hw_version_major == 1) {
> +		if (csi_dev->hw_version_minor == 30) {
> +			/*
> +			 * Hardware registers that were
> +			 * exclusive to version < 1.40
> +			 */
> +			reg.INT_FRAME_FATAL = 0x100;
> +			reg.MASK_INT_FRAME_FATAL = 0x104;
> +			reg.FORCE_INT_FRAME_FATAL = 0x108;
> +			reg.INT_PKT = 0x120;
> +			reg.MASK_INT_PKT = 0x124;
> +			reg.FORCE_INT_PKT = 0x128;
> +
> +			/* interrupt source present until this release */
> +			csi_int.PKT = BIT(17);
> +			csi_int.LINE = BIT(18);
> +			csi_int.IPI = BIT(19);
> +			csi_int.FRAME_FATAL = BIT(2);
> +
> +		} else if (csi_dev->hw_version_minor == 40) {
> +			/*
> +			 * HW registers that were added
> +			 * to version 1.40
> +			 */
> +			reg.ST_BNDRY_FRAME_FATAL = 0x280;
> +			reg.MSK_BNDRY_FRAME_FATAL = 0x284;
> +			reg.FORCE_BNDRY_FRAME_FATAL = 0x288;
> +			reg.ST_SEQ_FRAME_FATAL = 0x290;
> +			reg.MSK_SEQ_FRAME_FATAL	= 0x294;
> +			reg.FORCE_SEQ_FRAME_FATAL = 0x298;
> +			reg.ST_CRC_FRAME_FATAL = 0x2a0;
> +			reg.MSK_CRC_FRAME_FATAL	= 0x2a4;
> +			reg.FORCE_CRC_FRAME_FATAL = 0x2a8;
> +			reg.ST_PLD_CRC_FATAL = 0x2b0;
> +			reg.MSK_PLD_CRC_FATAL = 0x2b4;
> +			reg.FORCE_PLD_CRC_FATAL = 0x2b8;
> +			reg.ST_DATA_ID = 0x2c0;
> +			reg.MSK_DATA_ID = 0x2c4;
> +			reg.FORCE_DATA_ID = 0x2c8;
> +			reg.ST_ECC_CORRECT = 0x2d0;
> +			reg.MSK_ECC_CORRECT = 0x2d4;
> +			reg.FORCE_ECC_CORRECT = 0x2d8;
> +			reg.DATA_IDS_VC_1 = 0x0;
> +			reg.DATA_IDS_VC_2 = 0x0;
> +			reg.VC_EXTENSION = 0x0;

Please take this into account in the functions accessing the registers
instead.

Note that here reg is global, not specific to a given device.

> +
> +			/* interrupts map were changed */
> +			csi_int.LINE = BIT(17);
> +			csi_int.IPI = BIT(18);
> +			csi_int.BNDRY_FRAME_FATAL = BIT(2);
> +			csi_int.SEQ_FRAME_FATAL	= BIT(3);
> +			csi_int.CRC_FRAME_FATAL = BIT(4);
> +			csi_int.PLD_CRC_FATAL = BIT(5);
> +			csi_int.DATA_ID = BIT(6);
> +			csi_int.ECC_CORRECTED = BIT(7);
> +
> +		} else {
> +			dev_info(dev, "Version minor not supported.");
> +		}
> +	} else {
> +		dev_info(dev, "Version major not supported.");
> +	}
> +	return 0;
> +}
> +
> +void dw_mipi_csi_dump(struct dw_csi *csi_dev)
> +{
> +	dw_print(reg.VERSION);
> +	dw_print(reg.N_LANES);
> +	dw_print(reg.CTRL_RESETN);
> +	dw_print(reg.INTERRUPT);
> +	dw_print(reg.DATA_IDS_1);
> +	dw_print(reg.DATA_IDS_2);
> +	dw_print(reg.IPI_MODE);
> +	dw_print(reg.IPI_VCID);
> +	dw_print(reg.IPI_DATA_TYPE);
> +	dw_print(reg.IPI_MEM_FLUSH);
> +	dw_print(reg.IPI_HSA_TIME);
> +	dw_print(reg.IPI_HBP_TIME);
> +	dw_print(reg.IPI_HSD_TIME);
> +	dw_print(reg.IPI_HLINE_TIME);
> +	dw_print(reg.IPI_SOFTRSTN);
> +	dw_print(reg.IPI_ADV_FEATURES);
> +	dw_print(reg.IPI_VSA_LINES);
> +	dw_print(reg.IPI_VBP_LINES);
> +	dw_print(reg.IPI_VFP_LINES);
> +	dw_print(reg.IPI_VACTIVE_LINES);
> +	dw_print(reg.IPI_DATA_TYPE);
> +	dw_print(reg.VERSION);
> +	dw_print(reg.IPI_ADV_FEATURES);
> +}
> diff --git a/drivers/media/platform/dwc/dw-mipi-csi.h b/drivers/media/platform/dwc/dw-mipi-csi.h
> new file mode 100644
> index 0000000..6df3688
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw-mipi-csi.h
> @@ -0,0 +1,287 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018-2019 Synopsys, Inc. and/or its affiliates.
> + *
> + * Synopsys DesignWare MIPI CSI-2 Host controller driver
> + *
> + * Author: Luis Oliveira <Luis.Oliveira@synopsys.com>
> + */
> +
> +#ifndef _DW_MIPI_CSI_H__
> +#define _DW_MIPI_CSI_H__
> +
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/phy/phy.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/ratelimit.h>
> +#include <linux/reset.h>
> +#include <linux/videodev2.h>
> +#include <linux/wait.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/dwc/dw-mipi-csi-pltfrm.h>
> +
> +/* Advanced features */
> +#define IPI_DT_OVERWRITE BIT(0)
> +#define DATA_TYPE_OVERWRITE(dt) (((dt) & GENMASK(5, 0)) << 8)
> +#define LINE_EVENT_SELECTION(n) ((n) << 16)
> +
> +enum line_event {
> +	EVSELAUTO = 0,
> +	EVSELPROG = 1,
> +};
> +
> +#define EN_VIDEO BIT(17)
> +#define EN_LINE_START BIT(18)
> +#define EN_NULL BIT(19)
> +#define EN_BLANKING BIT(20)
> +#define EN_EMBEDDED BIT(21)

Please align the macro expansions to the same column.

> +#define IPI_SYNC_EVENT_MODE(n) ((n) << 24)
> +
> +enum sync_event {
> +	SYNCEVFSN = 0,
> +	SYNCEVFS = 1,
> +};
> +
> +/* DW MIPI CSI-2 register addresses*/
> +
> +struct R_CSI2 {
> +	u16 VERSION;

Please use lower case names for structs and struct fields.

> +	u16 N_LANES;
> +	u16 CTRL_RESETN;
> +	u16 INTERRUPT;
> +	u16 DATA_IDS_1;
> +	u16 DATA_IDS_2;
> +	u16 DATA_IDS_VC_1;
> +	u16 DATA_IDS_VC_2;
> +	u16 IPI_MODE;
> +	u16 IPI_VCID;
> +	u16 IPI_DATA_TYPE;
> +	u16 IPI_MEM_FLUSH;
> +	u16 IPI_HSA_TIME;
> +	u16 IPI_HBP_TIME;
> +	u16 IPI_HSD_TIME;
> +	u16 IPI_HLINE_TIME;
> +	u16 IPI_SOFTRSTN;
> +	u16 IPI_ADV_FEATURES;
> +	u16 IPI_VSA_LINES;
> +	u16 IPI_VBP_LINES;
> +	u16 IPI_VFP_LINES;
> +	u16 IPI_VACTIVE_LINES;
> +	u16 VC_EXTENSION;
> +	u16 INT_PHY_FATAL;
> +	u16 MASK_INT_PHY_FATAL;
> +	u16 FORCE_INT_PHY_FATAL;
> +	u16 INT_PKT_FATAL;
> +	u16 MASK_INT_PKT_FATAL;
> +	u16 FORCE_INT_PKT_FATAL;
> +	u16 INT_FRAME_FATAL;
> +	u16 MASK_INT_FRAME_FATAL;
> +	u16 FORCE_INT_FRAME_FATAL;
> +	u16 INT_PHY;
> +	u16 MASK_INT_PHY;
> +	u16 FORCE_INT_PHY;
> +	u16 INT_PKT;
> +	u16 MASK_INT_PKT;
> +	u16 FORCE_INT_PKT;
> +	u16 INT_LINE;
> +	u16 MASK_INT_LINE;
> +	u16 FORCE_INT_LINE;
> +	u16 INT_IPI;
> +	u16 MASK_INT_IPI;
> +	u16 FORCE_INT_IPI;
> +	u16 ST_BNDRY_FRAME_FATAL;
> +	u16 MSK_BNDRY_FRAME_FATAL;
> +	u16 FORCE_BNDRY_FRAME_FATAL;
> +	u16 ST_SEQ_FRAME_FATAL;
> +	u16 MSK_SEQ_FRAME_FATAL;
> +	u16 FORCE_SEQ_FRAME_FATAL;
> +	u16 ST_CRC_FRAME_FATAL;
> +	u16 MSK_CRC_FRAME_FATAL;
> +	u16 FORCE_CRC_FRAME_FATAL;
> +	u16 ST_PLD_CRC_FATAL;
> +	u16 MSK_PLD_CRC_FATAL;
> +	u16 FORCE_PLD_CRC_FATAL;
> +	u16 ST_DATA_ID;
> +	u16 MSK_DATA_ID;
> +	u16 FORCE_DATA_ID;
> +	u16 ST_ECC_CORRECT;
> +	u16 MSK_ECC_CORRECT;
> +	u16 FORCE_ECC_CORRECT;
> +};
> +
> +/* Interrupt Masks */
> +struct interrupt_type {
> +	u32 PHY_FATAL;
> +	u32 PKT_FATAL;
> +	u32 FRAME_FATAL;
> +	u32 PHY;
> +	u32 PKT;
> +	u32 LINE;
> +	u32 IPI;
> +	u32 BNDRY_FRAME_FATAL;
> +	u32 SEQ_FRAME_FATAL;
> +	u32 CRC_FRAME_FATAL;
> +	u32 PLD_CRC_FATAL;
> +	u32 DATA_ID;
> +	u32 ECC_CORRECTED;
> +};
> +
> +/* IPI Data Types */
> +enum data_type {
> +	CSI_2_YUV420_8 = 0x18,
> +	CSI_2_YUV420_10 = 0x19,
> +	CSI_2_YUV420_8_LEG = 0x1A,
> +	CSI_2_YUV420_8_SHIFT = 0x1C,
> +	CSI_2_YUV420_10_SHIFT = 0x1D,
> +	CSI_2_YUV422_8 = 0x1E,
> +	CSI_2_YUV422_10 = 0x1F,
> +	CSI_2_RGB444 = 0x20,
> +	CSI_2_RGB555 = 0x21,
> +	CSI_2_RGB565 = 0x22,
> +	CSI_2_RGB666 = 0x23,
> +	CSI_2_RGB888 = 0x24,
> +	CSI_2_RAW6 = 0x28,
> +	CSI_2_RAW7 = 0x29,
> +	CSI_2_RAW8 = 0x2A,
> +	CSI_2_RAW10 = 0x2B,
> +	CSI_2_RAW12 = 0x2C,
> +	CSI_2_RAW14 = 0x2D,
> +	CSI_2_RAW16 = 0x2E,
> +	CSI_2_RAW20 = 0x2F,
> +	USER_DEFINED_1 = 0x30,
> +	USER_DEFINED_2 = 0x31,
> +	USER_DEFINED_3 = 0x32,
> +	USER_DEFINED_4 = 0x33,
> +	USER_DEFINED_5 = 0x34,
> +	USER_DEFINED_6 = 0x35,
> +	USER_DEFINED_7 = 0x36,
> +	USER_DEFINED_8 = 0x37,
> +};
> +
> +/* DWC MIPI CSI-2 output types */
> +enum output {
> +	IPI_OUT = 0,
> +	IDI_OUT = 1,
> +	BOTH_OUT = 2
> +};
> +
> +/* IPI color components */
> +enum color_mode {
> +	COLOR48 = 0,
> +	COLOR16 = 1
> +};
> +
> +/* IPI cut through */
> +enum cut_through {
> +	CTINACTIVE = 0,
> +	CTACTIVE = 1
> +};
> +
> +/* IPI output types */
> +enum ipi_output {
> +	CAMERA_TIMING = 0,
> +	AUTO_TIMING = 1
> +};
> +
> +/* Format template */
> +struct mipi_fmt {
> +	u32 mbus_code;
> +	u8 depth;
> +};
> +
> +struct mipi_dt {
> +	u32 hex;
> +	char *name;
> +};
> +
> +/* CSI specific configuration */
> +struct csi_data {
> +	u32 num_lanes;
> +	u32 dphy_freq;
> +	u32 pclk;
> +	u32 fps;
> +	u32 bpp;
> +	u32 output;
> +	u32 ipi_mode;
> +	u32 ipi_adv_features;
> +	u32 ipi_cut_through;
> +	u32 ipi_color_mode;
> +	u32 ipi_auto_flush;
> +	u32 virtual_ch;
> +	u32 hsa;
> +	u32 hbp;
> +	u32 hsd;
> +	u32 htotal;
> +	u32 vsa;
> +	u32 vbp;
> +	u32 vfp;
> +	u32 vactive;
> +};
> +
> +/* Structure to embed device driver information */
> +struct dw_csi {
> +	struct v4l2_subdev sd;
> +	struct video_device vdev;
> +	struct v4l2_device v4l2_dev;
> +	struct device *dev;
> +	struct media_pad pads[CSI_PADS_NUM];
> +	struct mipi_fmt *fmt;
> +	struct v4l2_mbus_framefmt format;
> +	void __iomem *base_address;
> +	void __iomem *demo;
> +	void __iomem *csc;
> +	int ctrl_irq_number;
> +	int demosaic_irq;
> +	struct csi_data hw;
> +	struct reset_control *rst;
> +	struct phy *phy;
> +	struct dw_csih_pdata *config;
> +	struct mutex lock; /* protect resources sharing */
> +	spinlock_t slock; /* interrupt handling lock */
> +	u8 ipi_dt;
> +	u8 index;
> +	u8 hw_version_major;
> +	u16 hw_version_minor;
> +};
> +
> +static inline struct dw_csi *sd_to_mipi_csi_dev(struct v4l2_subdev *sdev)
> +{
> +	return container_of(sdev, struct dw_csi, sd);
> +}
> +
> +void dw_mipi_csi_reset(struct dw_csi *csi_dev);
> +int dw_mipi_csi_mask_irq_power_off(struct dw_csi *csi_dev);
> +int dw_mipi_csi_hw_stdby(struct dw_csi *csi_dev);
> +void dw_mipi_csi_set_ipi_fmt(struct dw_csi *csi_dev);
> +void dw_mipi_csi_start(struct dw_csi *csi_dev);
> +int dw_mipi_csi_irq_handler(struct dw_csi *csi_dev);
> +void dw_mipi_csi_get_version(struct dw_csi *csi_dev);
> +int dw_mipi_csi_specific_mappings(struct dw_csi *csi_dev);
> +void dw_mipi_csi_fill_timings(struct dw_csi *dev,
> +			      struct v4l2_subdev_format *fmt);
> +void dw_mipi_csi_dump(struct dw_csi *csi_dev);
> +
> +#if IS_ENABLED(CONFIG_DWC_MIPI_TC_DPHY_GEN3)
> +int dw_csi_create_capabilities_sysfs(struct platform_device *pdev);
> +#endif
> +
> +static inline void dw_mipi_csi_write(struct dw_csi *dev,
> +				     u32 address, u32 data)
> +{
> +	writel(data, dev->base_address + address);
> +}
> +
> +static inline u32 dw_mipi_csi_read(struct dw_csi *dev, u32 address)
> +{
> +	return readl(dev->base_address + address);
> +}
> +
> +#endif /*_DW_MIPI_CSI_H__ */
> diff --git a/include/media/dwc/dw-mipi-csi-pltfrm.h b/include/media/dwc/dw-mipi-csi-pltfrm.h
> new file mode 100644
> index 0000000..948db4e
> --- /dev/null
> +++ b/include/media/dwc/dw-mipi-csi-pltfrm.h
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018-2019 Synopsys, Inc. and/or its affiliates.
> + *
> + * Synopsys DesignWare MIPI CSI-2 Host media entities
> + *
> + * Author: Luis Oliveira <Luis.Oliveira@synopsys.com>
> + */
> +
> +#ifndef __DW_MIPI_CSI_PLTFRM_INCLUDES_H_
> +#define __DW_MIPI_CSI_PLTFRM_INCLUDES_H_
> +
> +#include <media/media-entity.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-mediabus.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define MAX_WIDTH	3280
> +#define MAX_HEIGHT	1852
> +
> +/* The subdevices' group IDs. */
> +#define GRP_ID_SENSOR		(10)
> +#define GRP_ID_CSI		(20)
> +#define GRP_ID_VIF		(30)
> +#define GRP_ID_VIDEODEV		(40)
> +
> +#define CSI_MAX_ENTITIES	(2)
> +#define VIF_MAX_ENTITIES	(2)
> +#define PLAT_MAX_SENSORS	(2)
> +
> +struct pdata_names {
> +	char *name;
> +};
> +
> +enum video_dev_pads {
> +	VIDEO_DEV_SD_PAD_SINK_VIF1,
> +	VIDEO_DEV_SD_PAD_SINK_VIF2,
> +	VIDEO_DEV_SD_PAD_SOURCE_DMA,
> +	VIDEO_DEV_SD_PADS_NUM,

These are unused; please remove.

> +};
> +
> +enum vif_pads {
> +	VIF_PAD_SINK_CSI,
> +	VIF_PAD_SOURCE_DMA,
> +	VIF_PADS_NUM,
> +};
> +
> +enum mipi_csi_pads {
> +	CSI_PAD_SINK,
> +	CSI_PAD_SOURCE,
> +	CSI_PADS_NUM,
> +};
> +
> +struct plat_csi_source_info {
> +	u16 flags;
> +	u16 mux_id;
> +};
> +
> +struct plat_csi_fmt {
> +	char *name;
> +	u32 mbus_code;
> +	u32 fourcc;
> +	u8 depth;
> +};
> +
> +struct plat_csi_media_pipeline;
> +
> +/*
> + * Media pipeline operations to be called from within a video node,  i.e. the
> + * last entity within the pipeline. Implemented by related media device driver.
> + */
> +struct plat_csi_media_pipeline_ops {
> +	int (*prepare)(struct plat_csi_media_pipeline *p,
> +		       struct media_entity *me);
> +	int (*unprepare)(struct plat_csi_media_pipeline *p);
> +	int (*open)(struct plat_csi_media_pipeline *p, struct media_entity *me,
> +		    bool resume);
> +	int (*close)(struct plat_csi_media_pipeline *p);
> +	int (*set_stream)(struct plat_csi_media_pipeline *p, bool state);
> +	int (*set_format)(struct plat_csi_media_pipeline *p,
> +			  struct v4l2_subdev_format *fmt);
> +};
> +
> +struct plat_csi_video_entity {
> +	struct video_device vdev;
> +	struct plat_csi_media_pipeline *pipe;
> +};
> +
> +struct plat_csi_media_pipeline {
> +	struct media_pipeline mp;
> +	const struct plat_csi_media_pipeline_ops *ops;
> +};

Some of the above appears to be unused. Such as the pipeline ops. Please
remove what is unused by the driver.

> +
> +static inline struct plat_csi_video_entity
> +*vdev_to_plat_csi_video_entity(struct video_device *vdev)
> +{
> +	return container_of(vdev, struct plat_csi_video_entity, vdev);
> +}
> +
> +#define plat_csi_pipeline_call(ent, op, args...)			  \
> +	(!(ent) ? -ENOENT : (((ent)->pipe->ops && (ent)->pipe->ops->op) ? \
> +	(ent)->pipe->ops->op(((ent)->pipe), ##args) : -ENOIOCTLCMD))	  \

Same here.

> +
> +#endif /* __DW_MIPI_CSI_PLTFRM_INCLUDES_H_ */

-- 
Regards,

Sakari Ailus

^ permalink raw reply

* Re: [PATCH v3 04/21] ARM: dts: imx7-colibri: Add sleep mode to ethernet
From: Marcel Ziswiler @ 2019-08-09 14:20 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	linux-imx@nxp.com
In-Reply-To: <20190807082556.5013-5-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> Add sleep pinmux to the fec so it can properly sleep.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx7-colibri.dtsi | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> b/arch/arm/boot/dts/imx7-colibri.dtsi
> index 52046085ce6f..a8d992f3e897 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -101,8 +101,9 @@
>  };
>  
>  &fec1 {
> -	pinctrl-names = "default";
> +	pinctrl-names = "default", "sleep";
>  	pinctrl-0 = <&pinctrl_enet1>;
> +	pinctrl-1 = <&pinctrl_enet1_sleep>;
>  	clocks = <&clks IMX7D_ENET_AXI_ROOT_CLK>,
>  		<&clks IMX7D_ENET_AXI_ROOT_CLK>,
>  		<&clks IMX7D_ENET1_TIME_ROOT_CLK>,
> @@ -463,6 +464,22 @@
>  		>;
>  	};
>  
> +	pinctrl_enet1_sleep: enet1sleepgrp {
> +		fsl,pins = <
> +			MX7D_PAD_ENET1_RGMII_RX_CTL__GPIO7_IO4		
> 0x0
> +			MX7D_PAD_ENET1_RGMII_RD0__GPIO7_IO0		
> 0x0
> +			MX7D_PAD_ENET1_RGMII_RD1__GPIO7_IO1		
> 0x0
> +			MX7D_PAD_ENET1_RGMII_RXC__GPIO7_IO5		
> 0x0
> +
> +			MX7D_PAD_ENET1_RGMII_TX_CTL__GPIO7_IO10		
> 0x0
> +			MX7D_PAD_ENET1_RGMII_TD0__GPIO7_IO6		
> 0x0
> +			MX7D_PAD_ENET1_RGMII_TD1__GPIO7_IO7		
> 0x0
> +			MX7D_PAD_GPIO1_IO12__GPIO1_IO12			
> 0x0
> +			MX7D_PAD_SD2_CD_B__GPIO5_IO9			
> 0x0
> +			MX7D_PAD_SD2_WP__GPIO5_IO10			
> 0x0
> +		>;
> +	};
> +
>  	pinctrl_ecspi3_cs: ecspi3-cs-grp {
>  		fsl,pins = <
>  			MX7D_PAD_I2C2_SDA__GPIO4_IO11		0x14

^ permalink raw reply

* Re: [Patch V6 7/8] usb: gadget: Add UDC driver for tegra XUSB device mode controller
From: kbuild test robot @ 2019-08-09 14:28 UTC (permalink / raw)
  To: Nagarjuna Kristam
  Cc: kbuild-all, balbi, gregkh, thierry.reding, jonathanh,
	mark.rutland, robh+dt, devicetree, linux-tegra, linux-usb
In-Reply-To: <1565257046-9890-8-git-send-email-nkristam@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 5132 bytes --]

Hi Nagarjuna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190809]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nagarjuna-Kristam/Tegra-XUSB-gadget-driver-support/20190809-042626
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:332:0,
                    from include/linux/kernel.h:15,
                    from include/linux/clk.h:13,
                    from drivers/usb/gadget/udc/tegra_xudc.c:9:
   drivers/usb/gadget/udc/tegra_xudc.c: In function 'tegra_xudc_ep0_standard_req':
>> include/linux/dynamic_debug.h:122:52: warning: this statement may fall through [-Wimplicit-fallthrough=]
    #define __dynamic_func_call(id, fmt, func, ...) do { \
                                                       ^
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
     ^~~~~~~~~~~~~~~~~~
   include/linux/device.h:1509:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~
>> drivers/usb/gadget/udc/tegra_xudc.c:2394:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(xudc->dev, "USB_REQ_SET_CONFIGURATION\n");
      ^~~~~~~
   drivers/usb/gadget/udc/tegra_xudc.c:2400:2: note: here
     default:
     ^~~~~~~
--
   In file included from include/linux/printk.h:332:0,
                    from include/linux/kernel.h:15,
                    from include/linux/clk.h:13,
                    from drivers/usb//gadget/udc/tegra_xudc.c:9:
   drivers/usb//gadget/udc/tegra_xudc.c: In function 'tegra_xudc_ep0_standard_req':
>> include/linux/dynamic_debug.h:122:52: warning: this statement may fall through [-Wimplicit-fallthrough=]
    #define __dynamic_func_call(id, fmt, func, ...) do { \
                                                       ^
   include/linux/dynamic_debug.h:143:2: note: in expansion of macro '__dynamic_func_call'
     __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:157:2: note: in expansion of macro '_dynamic_func_call'
     _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
     ^~~~~~~~~~~~~~~~~~
   include/linux/device.h:1509:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~
   drivers/usb//gadget/udc/tegra_xudc.c:2394:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(xudc->dev, "USB_REQ_SET_CONFIGURATION\n");
      ^~~~~~~
   drivers/usb//gadget/udc/tegra_xudc.c:2400:2: note: here
     default:
     ^~~~~~~

vim +/dev_dbg +2394 drivers/usb/gadget/udc/tegra_xudc.c

  2365	
  2366	static int tegra_xudc_ep0_standard_req(struct tegra_xudc *xudc,
  2367					      struct usb_ctrlrequest *ctrl)
  2368	{
  2369		int ret;
  2370	
  2371		switch (ctrl->bRequest) {
  2372		case USB_REQ_GET_STATUS:
  2373			dev_dbg(xudc->dev, "USB_REQ_GET_STATUS\n");
  2374			ret = tegra_xudc_ep0_get_status(xudc, ctrl);
  2375			break;
  2376		case USB_REQ_SET_ADDRESS:
  2377			dev_dbg(xudc->dev, "USB_REQ_SET_ADDRESS\n");
  2378			ret = tegra_xudc_ep0_set_address(xudc, ctrl);
  2379			break;
  2380		case USB_REQ_SET_SEL:
  2381			dev_dbg(xudc->dev, "USB_REQ_SET_SEL\n");
  2382			ret = tegra_xudc_ep0_set_sel(xudc, ctrl);
  2383			break;
  2384		case USB_REQ_SET_ISOCH_DELAY:
  2385			dev_dbg(xudc->dev, "USB_REQ_SET_ISOCH_DELAY\n");
  2386			ret = tegra_xudc_ep0_set_isoch_delay(xudc, ctrl);
  2387			break;
  2388		case USB_REQ_CLEAR_FEATURE:
  2389		case USB_REQ_SET_FEATURE:
  2390			dev_dbg(xudc->dev, "USB_REQ_CLEAR/SET_FEATURE\n");
  2391			ret = tegra_xudc_ep0_set_feature(xudc, ctrl);
  2392			break;
  2393		case USB_REQ_SET_CONFIGURATION:
> 2394			dev_dbg(xudc->dev, "USB_REQ_SET_CONFIGURATION\n");
  2395			/*
  2396			 * In theory we need to clear RUN bit before status stage of
  2397			 * deconfig request sent, but this seems to be causing problems.
  2398			 * Clear RUN once all endpoints are disabled instead.
  2399			 */
  2400		default:
  2401			ret = tegra_xudc_ep0_delegate_req(xudc, ctrl);
  2402			break;
  2403		}
  2404	
  2405		return ret;
  2406	}
  2407	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71385 bytes --]

^ permalink raw reply

* Re: [v4 3/6] media: platform: dwc: Add MIPI CSI-2 platform data
From: Sakari Ailus @ 2019-08-09 14:39 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: mchehab, davem, gregkh, Jonathan.Cameron, robh, nicolas.ferre,
	paulmck, mark.rutland, kishon, devicetree, linux-media,
	linux-kernel, Joao.Pinto
In-Reply-To: <1560280855-18085-4-git-send-email-luis.oliveira@synopsys.com>

Hi Luis,

On Tue, Jun 11, 2019 at 09:20:52PM +0200, Luis Oliveira wrote:
> This allows the driver loading via platform data which makes the driver
> not device tree dependent.
> 
> Signed-off-by: Luis Oliveira <luis.oliveira@synopsys.com>

Platform data? I thought we ceased to add support for platform data long
time ago.

-- 
Regards,

Sakari Ailus

^ permalink raw reply

* [PATCH] dt-bindings: arm: Extend SCMI to support new reset protocol
From: Sudeep Holla @ 2019-08-09 14:40 UTC (permalink / raw)
  To: devicetree, Rob Herring; +Cc: Sudeep Holla, linux-kernel

SCMIv2.0 adds a new Reset Management Protocol to manage various reset
states a given device or domain can enter. Extend the existing SCMI
bindings to add reset protocol support by re-using the reset bindings
for both reset providers and consumers.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../devicetree/bindings/arm/arm,scmi.txt        | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Hi Rob,

I am posting this separately to avoid reposting the driver patches that
are already reviewed/asked. I need your ack to take the changes for v5.4
I might have messed up something that it got missed from your patchworks
Full series @[1]

Regards,
Sudeep

[1] https://lore.kernel.org/lkml/20190806170208.6787-4-sudeep.holla@arm.com/

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index 317a2fc3667a..083dbf96ee00 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -73,6 +73,16 @@ SCMI provides an API to access the various sensors on the SoC.
 			 as used by the firmware. Refer to  platform details
 			 for your implementation for the IDs to use.

+Reset signal bindings for the reset domains based on SCMI Message Protocol
+------------------------------------------------------------
+
+This binding for the SCMI reset domain providers uses the generic reset
+signal binding[5].
+
+Required properties:
+ - #reset-cells : Should be 1. Contains the reset domain ID value used
+		  by SCMI commands.
+
 SRAM and Shared Memory for SCMI
 -------------------------------

@@ -93,6 +103,7 @@ Each sub-node represents the reserved area for SCMI.
 [2] Documentation/devicetree/bindings/power/power_domain.txt
 [3] Documentation/devicetree/bindings/thermal/thermal.txt
 [4] Documentation/devicetree/bindings/sram/sram.txt
+[5] Documentation/devicetree/bindings/reset/reset.txt

 Example:

@@ -152,6 +163,11 @@ firmware {
 			reg = <0x15>;
 			#thermal-sensor-cells = <1>;
 		};
+
+		scmi_reset: protocol@16 {
+			reg = <0x16>;
+			#reset-cells = <1>;
+		};
 	};
 };

@@ -166,6 +182,7 @@ hdlcd@7ff60000 {
 	reg = <0 0x7ff60000 0 0x1000>;
 	clocks = <&scmi_clk 4>;
 	power-domains = <&scmi_devpd 1>;
+	resets = <&scmi_reset 10>;
 };

 thermal-zones {
--
2.17.1

^ permalink raw reply related

* Re: [PATCH 2/2] pwm: sprd: Add Spreadtrum PWM support
From: Uwe Kleine-König @ 2019-08-09 14:41 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Thierry Reding, Rob Herring, Mark Rutland, Orson Zhai,
	Chunyan Zhang, Vincent Guittot, linux-pwm, DTML, LKML, kernel
In-Reply-To: <CAMz4kuLQsrBWjta1s=ZRPgxUd0_+_f-GbJV138tccuMLg2XCLA@mail.gmail.com>

Hello Baolin,

On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > +{
> > > +     struct sprd_pwm_chip *spc =
> > > +             container_of(chip, struct sprd_pwm_chip, chip);
> > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > +     u64 div, tmp;
> > > +     u32 prescale, duty;
> > > +     int ret;
> > > +
> > > +     /*
> > > +      * NOTE: the clocks to PWM channel has to be enabled first before
> > > +      * writing to the registers.
> > > +      */
> > > +     if (!chn->clk_enabled) {
> > > +             ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> >
> > Do you really need to enable all 8 clocks to configure a single channel?
> 
> We have 4 channels, and each channel use 2 clocks, so here only enable
> 2 clocks, see SPRD_PWM_NUM_CLKS.

Ah, I thought SPRD_PWM_NUM_CLKS was 8.

> > > +             if (ret) {
> > > +                     dev_err(spc->dev, "failed to enable pwm%u clock\n",
> > > +                             pwm->hwpwm);
> > > +                     return ret;
> > > +             }
> > > +
> > > +             chn->clk_enabled = true;
> > > +     }
> > > +
> > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> >
> > @Baolin: until we're there that there are framework requirements how to
> > round, please document at least how you do it here. Also describing the
> > underlying concepts would be good for the driver reader.
> >
> > Something like:
> >
> > /*
> >  * The hardware provides a counter that is feed by the source clock.
> >  * The period length is (PRESCALE + 1) * MOD counter steps.
> >  * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> >  *
> >  * To keep the maths simple we're always using MOD = MOD_MAX.
> >  * The value for PRESCALE is selected such that the resulting period
> >  * gets the maximal length not bigger than the requested one with the
> >  * given settings (MOD = MOD_MAX and input clock).
> >  */
> 
> Yes, totally agree with you. I will add some documentation for our
> controller's setting.
> 
> >
> > @Thierry: having a framework guideline here would be good. Or still
> > better a guideline and a debug setting that notices drivers stepping out
> > of line.
> >
> > I assume using MOD = 0 is forbidden?
> >
> > > +     /*
> > > +      * According to the datasheet, the period_ns calculation formula
> > > +      * should be:
> > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > +      *
> > > +      * Then we can get the prescale formula:
> > > +      * prescale = (period_ns * clk_rate) / (10^9 * mod) -1
> > > +      */
> > > +     tmp = chn->clk_rate * period_ns;
> > > +     div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> >
> > Please use NSEC_PER_SEC instead of 1000000000ULL.
> 
> Sure.
> 
> >
> > > +     prescale = div64_u64(tmp, div) - 1;
> >
> > If tmp is smaller than div you end up with prescale = 0xffffffff which
> > should be catched. What if prescale > 0xffff?
> 
> Usually we can not meet this case, but you are right, the prescale has
> a limit according to the register definition. So will add a validation
> here.
> 
> >
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> >
> > You're losing precision here as you always use SPRD_PWM_MOD_MAX, right?
> > (Just for my understanding, if this simpler approach is good enough
> > here that's fine.)
> 
> Yes, SPRD_PWM_MOD_MAX is good enough.

ok.
 
> >
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_LOW, SPRD_PWM_REG_MSK);
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_HIGH, SPRD_PWM_REG_MSK);
> >
> > Please describe these two registers in a short comment.
> 
> Sure.
> 
> >
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> >
> > Is the configuration here atomic in the sense that the write of DUTY
> > above only gets effective when PRESCALE is written? If not, please add
> 
> Yes.
> 
> > a describing paragraph at the top of the driver similar to what is
> > written in pwm-sifive.c. When the PWM is already running before, how
> > does a configuration change effects the output? Is the currently running
> > period completed?
> 
> Anyway, I'll add some comments to explain how it works.

I'd apreciate if you'd stick to the format in pwm-sifive.c to make it
easier to grep for that.
 
> > > +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +                            struct pwm_state *state)
> > > +{
> > > +     struct sprd_pwm_chip *spc =
> > > +             container_of(chip, struct sprd_pwm_chip, chip);
> > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > +     u32 enabled, duty, prescale;
> > > +     u64 tmp;
> > > +     int ret;
> > > +
> > > +     ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > > +     if (ret) {
> > > +             dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> > > +                     pwm->hwpwm);
> > > +             return;
> > > +     }
> > > +
> > > +     chn->clk_enabled = true;
> > > +
> > > +     duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > +     prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > +     enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > +
> > > +     /*
> > > +      * According to the datasheet, the period_ns and duty_ns calculation
> > > +      * formula should be:
> > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > +      * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > +      */
> > > +     tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > +     state->period = div64_u64(tmp, chn->clk_rate);
> >
> > This is not idempotent. If you apply the configuration that is returned
> > here this shouldn't result in a reconfiguration.
> 
> Since we may configure the  PWM in bootloader, so in kernel part we
> should get current PWM state to avoid reconfiguration if state
> configuration are same.

This is also important as some consumers might do something like:

	state = pwm_get_state(mypwm)
	if (something):
		state->duty = 0
	else:
		state->duty = state->period / 2
	pwm_set_state(mypwm, state)

and then period shouldn't get smaller in each step.
(This won't happen as of now because the PWM framework caches the last
state that was set and returns this for pwm_get_state. Still getting
this right would be good.)

> > > +     tmp = (prescale + 1) * 1000000000ULL * duty;
> > > +     state->duty_cycle = div64_u64(tmp, chn->clk_rate);
> > > +
> > > +     state->enabled = !!enabled;
> > > +
> > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > +     if (!enabled) {
> > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > +             chn->clk_enabled = false;
> > > +     }
> > > +}
> > > +
> > > +static const struct pwm_ops sprd_pwm_ops = {
> > > +     .config = sprd_pwm_config,
> > > +     .enable = sprd_pwm_enable,
> > > +     .disable = sprd_pwm_disable,
> > > +     .get_state = sprd_pwm_get_state,
> > > +     .owner = THIS_MODULE,
> > > +};
> > > +
> > > +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> > > +{
> > > +     struct clk *clk_parent, *clk_pwm;
> > > +     int ret, i, clk_index = 0;
> > > +
> > > +     clk_parent = devm_clk_get(spc->dev, "source");
> > > +     if (IS_ERR(clk_parent)) {
> > > +             dev_err(spc->dev, "failed to get source clock\n");
> > > +             return PTR_ERR(clk_parent);
> > > +     }
> > > +
> > > +     for (i = 0; i < SPRD_PWM_NUM; i++) {
> > > +             struct sprd_pwm_chn *chn = &spc->chn[i];
> > > +             int j;
> > > +
> > > +             for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
> > > +                     chn->clks[j].id = sprd_pwm_clks[clk_index++];
> > > +
> > > +             ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks);
> > > +             if (ret) {
> > > +                     if (ret == -ENOENT)
> > > +                             break;
> >
> > devm_clk_bulk_get_optional ? I'm sure you don't want to get all 8 clocks
> > 8 times.
> 
> This is not optional, each channel has 2 required clocks, and we have
> 4 channels. (SPRD_PWM_NUM_CLKS == 2)

I see. Currently it is not possible to use channel 2 if there are no
clocks for channel 0, right? There is no hardware related problem here,
just a shortcoming of the driver?

> > > +
> > > +                     dev_err(spc->dev, "failed to get channel clocks\n");
> > > +                     return ret;
> > > +             }
> > > +
> > > +             clk_pwm = chn->clks[1].clk;
> >
> > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > all? You're not using i in the loop at all, this doesn't look right.
> 
> Like I said above, each channel has 2 clocks: enable clock and pwm
> clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> which is used to set the source clock. I know this's not easy to read,
> so do you have any good suggestion?

Not sure this is easily possible to rework to make this clearer.
 
Do these clks have different uses? e.g. one to enable register access
and the other to enable the pwm output? If so just using
devm_clk_bulk_get isn't the right thing because you should be able know
if clks[0] or clks[1] is the one you need to enable the output (or
register access).

> > > +             if (!clk_set_parent(clk_pwm, clk_parent))
> > > +                     chn->clk_rate = clk_get_rate(clk_pwm);
> > > +             else
> > > +                     chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> >
> > I don't know all the clock framework details, but I think there are
> > better ways to ensure that a given clock is used as parent for another
> > given clock. Please read the chapter about "Assigned clock parents and
> > rates" in the clock bindings and check if this could be used for the
> > purpose here and so simplify the driver.
> 
> Actually there are many other drivers set the parent clock like this,
> and we want a default clock if failed to set the parent clock.

These might be older than the clk framework capabilities, or the
reviewers didn't pay attention to this detail; both shouldn't be a
reason to not make it better here.

> > > +     return 0;
> > > +}
> > > +
> > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > +{
> > > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > +     int i;
> > > +
> > > +     for (i = 0; i < spc->num_pwms; i++)
> > > +             pwm_disable(&spc->chip.pwms[i]);
> >
> > This is wrong. You must not call pwm_disable here. It's the consumer's
> > responsibility to disable the hardware.
> 
> Emm, I saw some drivers did like this, like pwm-spear.c.
> Could you elaborate on what's the problem if the driver issues pwm_disable?

This is a function to be called from code that called pwm_get before
(i.e. pwm consumers). This just doesn't explode because up to now the
PWM framework is only a thin wrapper around the lowlevel driver
callbacks.

The reasoning is similar to what I wrote in commit
f82d15e223403b05fffb33ba792b87a8620f6fee. I'd like to have a PWM_DEBUG
setting that catches such problems but the discussion with Thierry to
even document the expectations is stuck, see
https://patchwork.ozlabs.org/patch/1021566/.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* Re: [v4 4/6] dt-bindings: phy: Document the Synopsys MIPI DPHY Rx bindings
From: Sakari Ailus @ 2019-08-09 14:42 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: mchehab, davem, gregkh, Jonathan.Cameron, robh, nicolas.ferre,
	paulmck, mark.rutland, kishon, devicetree, linux-media,
	linux-kernel, Joao.Pinto
In-Reply-To: <1560280855-18085-5-git-send-email-luis.oliveira@synopsys.com>

Hi Luis,

On Tue, Jun 11, 2019 at 09:20:53PM +0200, Luis Oliveira wrote:
> Add device-tree bindings documentation for SNPS DesignWare MIPI D-PHY in
> RX mode.
> 
> Signed-off-by: Luis Oliveira <luis.oliveira@synopsys.com>
> ---
> Changelog
> v3-v4
> - @Laurent I know I told you I could remove the snps,dphy-frequency on V3 but
>   it is really useful for me here. I removed all other the proprietary
>   properties except this one. Do you still think it must be removed?

Yes. DT is the wrong place for runtime configuration. You get that
information using the V4L2_CID_LINK_FREQ control on the upstream
sub-device.

> - Frequency units @Rob
> 
>  .../devicetree/bindings/phy/snps,dw-dphy-rx.txt    | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/snps,dw-dphy-rx.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/snps,dw-dphy-rx.txt b/Documentation/devicetree/bindings/phy/snps,dw-dphy-rx.txt
> new file mode 100644
> index 0000000..50603e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/snps,dw-dphy-rx.txt
> @@ -0,0 +1,29 @@
> +Synopsys DesignWare MIPI Rx D-PHY block details
> +
> +Description
> +-----------
> +
> +The Synopsys MIPI D-PHY controller supports MIPI-DPHY in receiver mode.
> +Please refer to phy-bindings.txt for more information.
> +
> +Required properties:
> +- compatible		: Shall be "snps,dw-dphy-rx".
> +- #phy-cells		: Must be 1.
> +- bus-width		: Size of the test interface data bus (8 bits->8 or
> +			  12bits->12).

Hmm. This seems like runtime configuration as well.

> +- snps,dphy-frequency	: Frequency at which D-PHY should start, configurable.
> +			  Check Synopsys databook. (-kHz)
> +- reg			: Test interface register. This correspondes to the
> +			  physical base address of the controller and size of
> +			  the device memory mapped registers; Check Synopsys
> +			  databook.

Is this just for testing purposes or not?

> +
> +Example:
> +
> +	mipi_dphy_rx1: dphy@d00003040 {
> +		compatible = "snps,dw-dphy-rx";
> +		#phy-cells = <1>;
> +		bus-width = <12>;
> +		snps,dphy-frequency = <300000>;
> +		reg = <0xd0003040 0x20>;
> +	};

-- 
Regards,

Sakari Ailus

^ permalink raw reply

* Re: [v4 4/6] dt-bindings: phy: Document the Synopsys MIPI DPHY Rx bindings
From: Sakari Ailus @ 2019-08-09 14:45 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: mchehab, davem, gregkh, Jonathan.Cameron, robh, nicolas.ferre,
	paulmck, mark.rutland, kishon, devicetree, linux-media,
	linux-kernel, Joao.Pinto
In-Reply-To: <20190809144252.GD864@valkosipuli.retiisi.org.uk>

On Fri, Aug 09, 2019 at 05:42:52PM +0300, Sakari Ailus wrote:
> Hi Luis,
> 
> On Tue, Jun 11, 2019 at 09:20:53PM +0200, Luis Oliveira wrote:
> > Add device-tree bindings documentation for SNPS DesignWare MIPI D-PHY in
> > RX mode.
> > 
> > Signed-off-by: Luis Oliveira <luis.oliveira@synopsys.com>
> > ---
> > Changelog
> > v3-v4
> > - @Laurent I know I told you I could remove the snps,dphy-frequency on V3 but
> >   it is really useful for me here. I removed all other the proprietary
> >   properties except this one. Do you still think it must be removed?
> 
> Yes. DT is the wrong place for runtime configuration. You get that
> information using the V4L2_CID_LINK_FREQ control on the upstream
> sub-device.

And the PHY driver itself gets that in its configuration (struct
phy_configure_opts_mipi_dphy through the configure op).

-- 
Sakari Ailus

^ permalink raw reply

* Re: [PATCH v3 05/21] ARM: dts: add recovery for I2C for iMX7
From: Marcel Ziswiler @ 2019-08-09 14:45 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: s.hauer@pengutronix.de, linux-kernel@vger.kernel.org,
	Oleksandr Suvorov, linux-imx@nxp.com, kernel@pengutronix.de,
	linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190807082556.5013-6-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> 
> - add recovery mode for applicable i2c buses for
>   Colibri iMX7 module.
> 
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx7-colibri.dtsi | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> b/arch/arm/boot/dts/imx7-colibri.dtsi
> index a8d992f3e897..2480623c92ff 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -140,8 +140,12 @@
>  
>  &i2c1 {
>  	clock-frequency = <100000>;
> -	pinctrl-names = "default";
> +	pinctrl-names = "default", "gpio";
>  	pinctrl-0 = <&pinctrl_i2c1 &pinctrl_i2c1_int>;
> +	pinctrl-1 = <&pinctrl_i2c1_recovery &pinctrl_i2c1_int>;
> +	scl-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> +	sda-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>;
> +
>  	status = "okay";
>  
>  	codec: sgtl5000@a {
> @@ -242,8 +246,11 @@
>  
>  &i2c4 {
>  	clock-frequency = <100000>;
> -	pinctrl-names = "default";
> +	pinctrl-names = "default", "gpio";
>  	pinctrl-0 = <&pinctrl_i2c4>;
> +	pinctrl-1 = <&pinctrl_i2c4_recovery>;
> +	scl-gpios = <&gpio7 8 GPIO_ACTIVE_HIGH>;
> +	sda-gpios = <&gpio7 9 GPIO_ACTIVE_HIGH>;
>  };
>  
>  &lcdif {
> @@ -540,6 +547,13 @@
>  		>;
>  	};
>  
> +	pinctrl_i2c4_recovery: i2c4-recoverygrp {
> +		fsl,pins = <
> +			MX7D_PAD_ENET1_RGMII_TD2__GPIO7_IO8	0x400
> 0007f
> +			MX7D_PAD_ENET1_RGMII_TD3__GPIO7_IO9	0x400
> 0007f
> +		>;
> +	};
> +
>  	pinctrl_lcdif_dat: lcdif-dat-grp {
>  		fsl,pins = <
>  			MX7D_PAD_LCD_DATA00__LCD_DATA0		0x79
> @@ -740,6 +754,13 @@
>  		>;
>  	};
>  
> +	pinctrl_i2c1_recovery: i2c1-recoverygrp {
> +		fsl,pins = <
> +			MX7D_PAD_LPSR_GPIO1_IO04__GPIO1_IO4	0x400
> 0007f
> +			MX7D_PAD_LPSR_GPIO1_IO05__GPIO1_IO5	0x400
> 0007f
> +		>;
> +	};
> +
>  	pinctrl_cd_usdhc1: usdhc1-cd-grp {
>  		fsl,pins = <
>  			MX7D_PAD_LPSR_GPIO1_IO00__GPIO1_IO0	0x59
> /* CD */

^ permalink raw reply

* Re: [PATCH v3 06/21] ARM: dts: imx7-colibri: add GPIO wakeup key
From: Marcel Ziswiler @ 2019-08-09 14:46 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: Stefan Agner, s.hauer@pengutronix.de,
	linux-kernel@vger.kernel.org, linux-imx@nxp.com,
	kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190807082556.5013-7-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add wakeup GPIO key which is able to wake the system from sleep
> modes (e.g. Suspend-to-Memory).
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi | 14 ++++++++++++++
>  arch/arm/boot/dts/imx7-colibri.dtsi         |  7 ++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> index 3f2746169181..d4dbc4fc1adf 100644
> --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> @@ -52,6 +52,20 @@
>  		clock-frequency = <16000000>;
>  	};
>  
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpiokeys>;
> +
> +		power {
> +			label = "Wake-Up";
> +			gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
> +			linux,code = <KEY_WAKEUP>;
> +			debounce-interval = <10>;
> +			gpio-key,wakeup;
> +		};
> +	};
> +
>  	panel: panel {
>  		compatible = "edt,et057090dhu";
>  		backlight = <&bl>;
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> b/arch/arm/boot/dts/imx7-colibri.dtsi
> index 2480623c92ff..16d1a1ed1aff 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -741,12 +741,17 @@
>  
>  	pinctrl_gpio_lpsr: gpio1-grp {
>  		fsl,pins = <
> -			MX7D_PAD_LPSR_GPIO1_IO01__GPIO1_IO1	0x59
>  			MX7D_PAD_LPSR_GPIO1_IO02__GPIO1_IO2	0x59
>  			MX7D_PAD_LPSR_GPIO1_IO03__GPIO1_IO3	0x59
>  		>;
>  	};
>  
> +	pinctrl_gpiokeys: gpiokeysgrp {
> +		fsl,pins = <
> +			MX7D_PAD_LPSR_GPIO1_IO01__GPIO1_IO1	0x19
> +		>;
> +	};
> +
>  	pinctrl_i2c1: i2c1-grp {
>  		fsl,pins = <
>  			MX7D_PAD_LPSR_GPIO1_IO05__I2C1_SDA	0x400
> 0007f

^ permalink raw reply

* Re: [v4 4/6] dt-bindings: phy: Document the Synopsys MIPI DPHY Rx bindings
From: Sakari Ailus @ 2019-08-09 14:47 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: mchehab, davem, gregkh, Jonathan.Cameron, robh, nicolas.ferre,
	paulmck, mark.rutland, kishon, devicetree, linux-media,
	linux-kernel, Joao.Pinto
In-Reply-To: <20190809144252.GD864@valkosipuli.retiisi.org.uk>

On Fri, Aug 09, 2019 at 05:42:52PM +0300, Sakari Ailus wrote:
> > +The Synopsys MIPI D-PHY controller supports MIPI-DPHY in receiver mode.
> > +Please refer to phy-bindings.txt for more information.
> > +
> > +Required properties:
> > +- compatible		: Shall be "snps,dw-dphy-rx".
> > +- #phy-cells		: Must be 1.
> > +- bus-width		: Size of the test interface data bus (8 bits->8 or
> > +			  12bits->12).
> 
> Hmm. This seems like runtime configuration as well.

And it's for the parallel busses.

I'd like to see the bindings that have the other interface described as
well if there are parameters that affect device configuration there.

-- 
Sakari Ailus

^ permalink raw reply

* Re: [PATCH v3 07/21] ARM: dts: imx7-colibri: fix 1.8V/UHS support
From: Marcel Ziswiler @ 2019-08-09 14:48 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, Stefan Agner, linux-kernel@vger.kernel.org,
	linux-imx@nxp.com
In-Reply-To: <20190807082556.5013-8-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add pinmuxing and do not specify voltage restrictions for the usdhc
> instance available on the modules edge connector. This allows to use
> SD-cards with higher transfer modes if supported by the carrier
> board.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3:
> - Add new commit message from Stefan's proposal on ML
> 
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx7-colibri.dtsi | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> b/arch/arm/boot/dts/imx7-colibri.dtsi
> index 16d1a1ed1aff..67f5e0c87fdc 100644
> --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> @@ -326,7 +326,6 @@
>  &usdhc1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_usdhc1 &pinctrl_cd_usdhc1>;
> -	no-1-8-v;
>  	cd-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
>  	disable-wp;
>  	vqmmc-supply = <&reg_LDO2>;
> @@ -671,6 +670,28 @@
>  		>;
>  	};
>  
> +	pinctrl_usdhc1_100mhz: usdhc1grp_100mhz {
> +		fsl,pins = <
> +			MX7D_PAD_SD1_CMD__SD1_CMD	0x5a
> +			MX7D_PAD_SD1_CLK__SD1_CLK	0x1a
> +			MX7D_PAD_SD1_DATA0__SD1_DATA0	0x5a
> +			MX7D_PAD_SD1_DATA1__SD1_DATA1	0x5a
> +			MX7D_PAD_SD1_DATA2__SD1_DATA2	0x5a
> +			MX7D_PAD_SD1_DATA3__SD1_DATA3	0x5a
> +		>;
> +	};
> +
> +	pinctrl_usdhc1_200mhz: usdhc1grp_200mhz {
> +		fsl,pins = <
> +			MX7D_PAD_SD1_CMD__SD1_CMD	0x5b
> +			MX7D_PAD_SD1_CLK__SD1_CLK	0x1b
> +			MX7D_PAD_SD1_DATA0__SD1_DATA0	0x5b
> +			MX7D_PAD_SD1_DATA1__SD1_DATA1	0x5b
> +			MX7D_PAD_SD1_DATA2__SD1_DATA2	0x5b
> +			MX7D_PAD_SD1_DATA3__SD1_DATA3	0x5b
> +		>;
> +	};
> +
>  	pinctrl_usdhc3: usdhc3grp {
>  		fsl,pins = <
>  			MX7D_PAD_SD3_CMD__SD3_CMD		0x59

^ permalink raw reply

* Re: [PATCH 2/6] arm64: dts: ti: k3-j721e: Add gpio nodes in main domain
From: Keerthy @ 2019-08-09 14:53 UTC (permalink / raw)
  To: Lokesh Vutla, Tero Kristo, Nishanth Menon, linus.walleij
  Cc: linux-gpio, Rob Herring, Linux ARM Mailing List,
	Device Tree Mailing List
In-Reply-To: <20190809082947.30590-3-lokeshvutla@ti.com>



On 09/08/19 1:59 PM, Lokesh Vutla wrote:
> There are 8 instances of gpio modules in main domain divided into 2 groups:
> - Group1: gpio0, gpio2, gpio4, gpio6
> - Group2: gpio1, gpio3, gpio5, gpio7
> 
> Groups are created to provide protection between two different processor
> virtual worlds. There are x gpio lines coming out of each group. Each module
> in a group has equal x gpio lines pinned out. There is a top level mux for
> selecting the module instance for each pin coming out of group. Exactly
> one module can be selected to control the corresponding pin. This muxing
> can be controlled along the pad mux configuration registers.
> 
> Group1 pins out 128 lines(8 banks). Group 2 pins out 36 lines(2 banks).
> 
> Add DT nodes for each module instance in the main domain. Users should
> make sure that correct gpio instance is selected in their pad configuration.

Reviewed-by: Keerthy <j-keerthy@ti.com>

> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>   arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 132 ++++++++++++++++++++++
>   1 file changed, 132 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> index 01661c22c39d..199bc9a00b20 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> @@ -240,4 +240,136 @@
>   		clocks = <&k3_clks 286 0>;
>   		clock-names = "fclk";
>   	};
> +
> +	main_gpio0: gpio@600000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00600000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <105 0>, <105 1>, <105 2>, <105 3>,
> +			     <105 4>, <105 5>, <105 6>, <105 7>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <128>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 105 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 105 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	main_gpio1: gpio@601000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00601000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <106 0>, <106 1>, <106 2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <36>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 106 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 106 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	main_gpio2: gpio@610000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00610000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <107 0>, <107 1>, <107 2>, <107 3>,
> +			     <107 4>, <107 5>, <107 6>, <107 7>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <128>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 107 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 107 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	main_gpio3: gpio@611000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00611000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <108 0>, <108 1>, <108 2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <36>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 108 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 108 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	main_gpio4: gpio@620000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00620000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <109 0>, <109 1>, <109 2>, <109 3>,
> +			     <109 4>, <109 5>, <109 6>, <109 7>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <128>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 109 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 109 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	main_gpio5: gpio@621000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00621000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <110 0>, <110 1>, <110 2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <36>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 110 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 110 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	main_gpio6: gpio@630000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00630000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <111 0>, <111 1>, <111 2>, <111 3>,
> +			     <111 4>, <111 5>, <111 6>, <111 7>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <128>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 111 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 111 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	main_gpio7: gpio@631000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x00631000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&main_gpio_intr>;
> +		interrupts = <112 0>, <112 1>, <112 2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <36>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 112 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 112 0>;
> +		clock-names = "gpio";
> +	};
>   };
> 

^ permalink raw reply

* Re: [PATCH 3/6] arm64: dts: ti: k3-j721e: Add gpio nodes in wakeup domain
From: Keerthy @ 2019-08-09 14:54 UTC (permalink / raw)
  To: Lokesh Vutla, Tero Kristo, Nishanth Menon, linus.walleij
  Cc: linux-gpio, Rob Herring, Linux ARM Mailing List,
	Device Tree Mailing List
In-Reply-To: <20190809082947.30590-4-lokeshvutla@ti.com>



On 09/08/19 1:59 PM, Lokesh Vutla wrote:
> Similar to the gpio groups in main domain, there is one gpio group
> in wakup domain with 2 module instances in it. This gpio group pins
> out 84 lines(6 banks). Add DT node for these 2 gpio module instances.

Reviewed-by: Keerthy <j-keerthy@ti.com>

> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>   .../boot/dts/ti/k3-j721e-mcu-wakeup.dtsi      | 34 +++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
> index e616c2481f51..555dc7b7aedc 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
> @@ -87,4 +87,38 @@
>   		ti,sci-dst-id = <14>;
>   		ti,sci-rm-range-girq = <0x5>;
>   	};
> +
> +	wkup_gpio0: gpio@42110000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x42110000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&wkup_gpio_intr>;
> +		interrupts = <113 0>, <113 1>, <113 2>,
> +			     <113 3>, <113 4>, <113 5>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <84>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 113 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 113 0>;
> +		clock-names = "gpio";
> +	};
> +
> +	wkup_gpio1: gpio@42100000 {
> +		compatible = "ti,j721e-gpio", "ti,keystone-gpio";
> +		reg = <0x0 0x42100000 0x0 0x100>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&wkup_gpio_intr>;
> +		interrupts = <114 0>, <114 1>, <114 2>,
> +			     <114 3>, <114 4>, <114 5>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		ti,ngpio = <84>;
> +		ti,davinci-gpio-unbanked = <0>;
> +		power-domains = <&k3_pds 114 TI_SCI_PD_EXCLUSIVE>;
> +		clocks = <&k3_clks 114 0>;
> +		clock-names = "gpio";
> +	};
>   };
> 

^ permalink raw reply

* Re: [PATCH 4/6] arm64: dts: ti: k3-j721e-common-proc-board: Disable unused gpio modules
From: Keerthy @ 2019-08-09 14:55 UTC (permalink / raw)
  To: Lokesh Vutla, Tero Kristo, Nishanth Menon, linus.walleij
  Cc: linux-gpio, Rob Herring, Linux ARM Mailing List,
	Device Tree Mailing List
In-Reply-To: <20190809082947.30590-5-lokeshvutla@ti.com>



On 09/08/19 1:59 PM, Lokesh Vutla wrote:
> There are 10 gpio instances inside SoC with 3 groups as below:
> - Group1: main_gpio0, main_gpio2, main_gpio4, main_gpio6
> - Group2: main_gpio1, main_gpio3, main_gpio5, main_gpio7
> - Group3: wkup_gpio0, wkup_gpio1
> 
> Only one instance can be used in each group at a time. So use main_gpio0,
> main_gpio1 and wkup_gpio0 for the current linux context and mark other
> gpio nodes as disabled.

Reviewed-by: Keerthy <j-keerthy@ti.com>

> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>   .../dts/ti/k3-j721e-common-proc-board.dts     | 28 +++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> index 63b47b839388..509579ca3db2 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> @@ -52,3 +52,31 @@
>   	/* UART not brought out */
>   	status = "disabled";
>   };
> +
> +&main_gpio2 {
> +	status = "disabled";
> +};
> +
> +&main_gpio3 {
> +	status = "disabled";
> +};
> +
> +&main_gpio4 {
> +	status = "disabled";
> +};
> +
> +&main_gpio5 {
> +	status = "disabled";
> +};
> +
> +&main_gpio6 {
> +	status = "disabled";
> +};
> +
> +&main_gpio7 {
> +	status = "disabled";
> +};
> +
> +&wkup_gpio1 {
> +	status = "disabled";
> +};
> 

^ permalink raw reply

* Re: [PATCH v3 08/21] ARM: dts: imx7-colibri: Add touch controllers
From: Marcel Ziswiler @ 2019-08-09 14:56 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	linux-imx@nxp.com
In-Reply-To: <20190807082556.5013-9-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> Add touch controller that is connected over an I2C bus.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3:
> - Fix commit message
> 
> Changes in v2:
> - Deleted touchrevolution downstream stuff
> - Use generic node name
> - Better comment
> 
>  arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi | 24
> +++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> index d4dbc4fc1adf..576dec9ff81c 100644
> --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> @@ -145,6 +145,21 @@
>  &i2c4 {
>  	status = "okay";
>  
> +	/*
> +	 * Touchscreen is using SODIMM 28/30, also used for PWM<B>,
> PWM<C>,
> +	 * aka pwm2, pwm3. so if you enable touchscreen, disable the
> pwms
> +	 */
> +	touchscreen@4a {
> +		compatible = "atmel,maxtouch";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpiotouch>;
> +		reg = <0x4a>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <9 IRQ_TYPE_EDGE_FALLING>;		/*
> SODIMM 28 */
> +		reset-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>;	/*
> SODIMM 30 */
> +		status = "disabled";
> +	};
> +
>  	/* M41T0M6 real time clock on carrier board */
>  	rtc: m41t0m6@68 {
>  		compatible = "st,m41t0";
> @@ -200,3 +215,12 @@
>  	vmmc-supply = <&reg_3v3>;
>  	status = "okay";
>  };
> +
> +&iomuxc {
> +	pinctrl_gpiotouch: touchgpios {
> +		fsl,pins = <
> +			MX7D_PAD_GPIO1_IO09__GPIO1_IO9		0x74
> +			MX7D_PAD_GPIO1_IO10__GPIO1_IO10		0x14
> +		>;
> +	};
> +};

^ permalink raw reply

* Re: [PATCH 6/6] arm64: dts: k3-j721e: Add gpio-keys on common processor board
From: Keerthy @ 2019-08-09 14:56 UTC (permalink / raw)
  To: Lokesh Vutla, Tero Kristo, Nishanth Menon, linus.walleij
  Cc: linux-gpio, Rob Herring, Linux ARM Mailing List,
	Device Tree Mailing List
In-Reply-To: <20190809082947.30590-7-lokeshvutla@ti.com>



On 09/08/19 1:59 PM, Lokesh Vutla wrote:
> From: Nikhil Devshatwar <nikhil.nd@ti.com>
> 
> Common processor board for K3 J721E platform has two push buttons
> namely SW10 and SW11.
> Add a gpio-keys device node to model them as input keys in Linux.
> Add required pinmux nodes to set GPIO pins as input.

Reviewed-by: Keerthy <j-keerthy@ti.com>

> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>   .../dts/ti/k3-j721e-common-proc-board.dts     | 37 +++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> index 509579ca3db2..d2894d55fbbe 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> @@ -6,12 +6,49 @@
>   /dts-v1/;
>   
>   #include "k3-j721e-som-p0.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
>   
>   / {
>   	chosen {
>   		stdout-path = "serial2:115200n8";
>   		bootargs = "console=ttyS2,115200n8 earlycon=ns16550a,mmio32,0x02800000";
>   	};
> +
> +	gpio_keys: gpio-keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sw10_button_pins_default &sw11_button_pins_default>;
> +
> +		sw10: sw10 {
> +			label = "GPIO Key USER1";
> +			linux,code = <BTN_0>;
> +			gpios = <&main_gpio0 0 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		sw11: sw11 {
> +			label = "GPIO Key USER2";
> +			linux,code = <BTN_1>;
> +			gpios = <&wkup_gpio0 7 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +};
> +
> +&main_pmx0 {
> +	sw10_button_pins_default: sw10_button_pins_default {
> +		pinctrl-single,pins = <
> +			J721E_IOPAD(0x0, PIN_INPUT, 7) /* (AC18) EXTINTn.GPIO0_0 */
> +		>;
> +	};
> +};
> +
> +&wkup_pmx0 {
> +	sw11_button_pins_default: sw11_button_pins_default {
> +		pinctrl-single,pins = <
> +			J721E_WKUP_IOPAD(0xcc, PIN_INPUT, 7) /* (G28) WKUP_GPIO0_7 */
> +		>;
> +	};
>   };
>   
>   &wkup_uart0 {
> 

^ permalink raw reply

* Re: [PATCH v3 09/21] ARM: dts: imx6qdl-colibri: add phy to fec
From: Marcel Ziswiler @ 2019-08-09 14:59 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	linux-imx@nxp.com
In-Reply-To: <20190807082556.5013-10-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> Add the phy-node and mdio bus to the fec-node, represented as is on
> hardware.
> This commit includes micrel,led-mode that is set to the default
> value, prepared for someone who wants to change this.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx6qdl-colibri.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> index 1beac22266ed..019dda6b88ad 100644
> --- a/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> @@ -140,7 +140,18 @@
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_enet>;
>  	phy-mode = "rmii";
> +	phy-handle = <&ethphy>;
>  	status = "okay";
> +
> +	mdio {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ethphy: ethernet-phy@0 {
> +			reg = <0>;
> +			micrel,led-mode = <0>;
> +		};
> +	};
>  };
>  
>  &hdmi {

^ permalink raw reply

* Re: [PATCH v3 10/21] ARM: dts: imx6qdl-colibri: Add missing pin declaration in iomuxc
From: Marcel Ziswiler @ 2019-08-09 15:00 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	linux-imx@nxp.com
In-Reply-To: <20190807082556.5013-11-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> This adds the muxing for the optional pins usb-oc (overcurrent) and
> usb-id.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/boot/dts/imx6qdl-colibri.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> index 019dda6b88ad..9a63debab0b5 100644
> --- a/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> @@ -615,6 +615,13 @@
>  		>;
>  	};
>  
> +	pinctrl_usbh_oc_1: usbh_oc-1 {
> +		fsl,pins = <
> +			/* USBH_OC */
> +			MX6QDL_PAD_EIM_D30__GPIO3_IO30		0x1b0
> b0
> +		>;
> +	};
> +
>  	pinctrl_spdif: spdifgrp {
>  		fsl,pins = <
>  			MX6QDL_PAD_GPIO_17__SPDIF_OUT 0x1b0b0
> @@ -681,6 +688,13 @@
>  		>;
>  	};
>  
> +	pinctrl_usbc_id_1: usbc_id-1 {
> +		fsl,pins = <
> +			/* USBC_ID */
> +			MX6QDL_PAD_NANDF_D2__GPIO2_IO02		0x1b0
> b0
> +		>;
> +	};
> +
>  	pinctrl_usdhc1: usdhc1grp {
>  		fsl,pins = <
>  			MX6QDL_PAD_SD1_CMD__SD1_CMD	0x17071

^ permalink raw reply

* Re: [PATCH v3 11/21] ARM: dts: imx6qdl-apalis: Add sleep state to can interfaces
From: Marcel Ziswiler @ 2019-08-09 15:02 UTC (permalink / raw)
  To: Max Krummenacher, stefan@agner.ch, Philippe Schenker,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	michal.vokac@ysoft.com, shawnguo@kernel.org, festevam@gmail.com,
	robh+dt@kernel.org
  Cc: linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	linux-imx@nxp.com
In-Reply-To: <20190807082556.5013-12-philippe.schenker@toradex.com>

On Wed, 2019-08-07 at 08:26 +0000, Philippe Schenker wrote:
> This patch prepares the devicetree for the new Ixora V1.2 where we
> are
> able to turn off the supply of the can transceiver. This implies to
> use
> a sleep state on transmission pins in order to prevent backfeeding.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
> 
> Changes in v3: None
> Changes in v2:
> - Changed commit title to '...imx6qdl-apalis:...'
> 
>  arch/arm/boot/dts/imx6qdl-apalis.dtsi | 27 +++++++++++++++++++++--
> ----
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
> b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
> index 7c4ad541c3f5..59ed2e4a1fd1 100644
> --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
> @@ -148,14 +148,16 @@
>  };
>  
>  &can1 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&pinctrl_flexcan1>;
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&pinctrl_flexcan1_default>;
> +	pinctrl-1 = <&pinctrl_flexcan1_sleep>;
>  	status = "disabled";
>  };
>  
>  &can2 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&pinctrl_flexcan2>;
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&pinctrl_flexcan2_default>;
> +	pinctrl-1 = <&pinctrl_flexcan2_sleep>;
>  	status = "disabled";
>  };
>  
> @@ -599,19 +601,32 @@
>  		>;
>  	};
>  
> -	pinctrl_flexcan1: flexcan1grp {
> +	pinctrl_flexcan1_default: flexcan1defgrp {
>  		fsl,pins = <
>  			MX6QDL_PAD_GPIO_7__FLEXCAN1_TX 0x1b0b0
>  			MX6QDL_PAD_GPIO_8__FLEXCAN1_RX 0x1b0b0
>  		>;
>  	};
>  
> -	pinctrl_flexcan2: flexcan2grp {
> +	pinctrl_flexcan1_sleep: flexcan1slpgrp {
> +		fsl,pins = <
> +			MX6QDL_PAD_GPIO_7__GPIO1_IO07 0x0
> +			MX6QDL_PAD_GPIO_8__GPIO1_IO08 0x0
> +		>;
> +	};
> +
> +	pinctrl_flexcan2_default: flexcan2defgrp {
>  		fsl,pins = <
>  			MX6QDL_PAD_KEY_COL4__FLEXCAN2_TX 0x1b0b0
>  			MX6QDL_PAD_KEY_ROW4__FLEXCAN2_RX 0x1b0b0
>  		>;
>  	};
> +	pinctrl_flexcan2_sleep: flexcan2slpgrp {
> +		fsl,pins = <
> +			MX6QDL_PAD_KEY_COL4__GPIO4_IO14 0x0
> +			MX6QDL_PAD_KEY_ROW4__GPIO4_IO15 0x0
> +		>;
> +	};
>  
>  	pinctrl_gpio_bl_on: gpioblon {
>  		fsl,pins = <

^ permalink raw reply

* Re: [PATCH v2 2/2] spi: npcm-fiu: add NPCM FIU controller driver
From: Boris Brezillon @ 2019-08-09 15:25 UTC (permalink / raw)
  To: Tomer Maimon
  Cc: Mark Brown, Rob Herring, Mark Rutland, Vignesh Raghavendra,
	Boris Brezillon, Avi Fishman, Tali Perry, Patrick Venture,
	Nancy Yuen, Benjamin Fair, linux-spi, devicetree,
	OpenBMC Maillist, Linux Kernel Mailing List
In-Reply-To: <CAP6Zq1iW0C0FDOoqmn5r_xk5HQFWw+GgLfeapvt-8mB50N2Vvg@mail.gmail.com>

On Fri, 9 Aug 2019 18:26:23 +0300
Tomer Maimon <tmaimon77@gmail.com> wrote:

> Hi Boris,
> 
> Thanks a lot for your comment.
> 
> On Thu, 8 Aug 2019 at 18:32, Boris Brezillon <boris.brezillon@collabora.com>
> wrote:
> 
> > On Thu,  8 Aug 2019 16:14:48 +0300
> > Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> >  
> > > +
> > > +static const struct spi_controller_mem_ops npcm_fiu_mem_ops = {
> > > +     .exec_op = npcm_fiu_exec_op,  
> >
> > No npcm_supports_op()? That's suspicious, especially after looking at
> > the npcm_fiu_exec_op() (and the functions called from there) where the
> > requested ->buswidth seems to be completely ignored...
> >
> > Sorry but I do not fully understand it, do you mean a support for the  
> buswidth?
> If yes it been done in the UMA functions as follow:
> 
>                 uma_cfg |= ilog2(op->cmd.buswidth);
>                 uma_cfg |= ilog2(op->addr.buswidth) <<
>                         NPCM_FIU_UMA_CFG_ADBPCK_SHIFT;
>                 uma_cfg |= ilog2(op->data.buswidth) <<
>                         NPCM_FIU_UMA_CFG_WDBPCK_SHIFT;
>                 uma_cfg |= op->addr.nbytes << NPCM_FIU_UMA_CFG_ADDSIZ_SHIFT;
>                 regmap_write(fiu->regmap, NPCM_FIU_UMA_ADDR, op->addr.val);
>

Hm, the default supports_op() implementation might be just fine for
your use case. But there's one thing you still need to check: the
number of addr cycles (or address size as you call it in this driver).
Looks like your IP is limited to 4 address cycles, if I'm right, you
should reject any operation that have op->addr.nbytes > 4. I also
wonder if there's a limitation on the data size you can have on a
single transfer. If there's one you should implement ->adjust_op() too.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox