Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v1 1/2] dt-bindings: add vendor prefix for logic technologies limited
From: Philippe Schenker @ 2019-09-23  9:07 UTC (permalink / raw)
  To: marcel@ziswiler.com, dri-devel@lists.freedesktop.org
  Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, heiko@sntech.de,
	Marcel Ziswiler, j.bauer@endrich.com,
	linux-kernel@vger.kernel.org, mripard@kernel.org,
	info@logictechno.com, robh+dt@kernel.org,
	laurent.pinchart@ideasonboard.com, hverkuil-cisco@xs4all.nl,
	shawnguo@kernel.org, linux-arm-kernel@lists.infradead.org,
	icenowy@aosc.io
In-Reply-To: <20190920075411.15735-1-marcel@ziswiler.com>

On Fri, 2019-09-20 at 09:54 +0200, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Add vendor prefix for Logic Technologies Limited [1] which is a
> Chinese
> display manufacturer e.g. distributed by German Endrich Bauelemente
> Vertriebs GmbH [2].
> 
> [1] https://logictechno.com/contact-us/
> [2] 
> https://www.endrich.com/isi50_isi30_tft-displays/lt170410-1whc_isi30
> 
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>

> 
> ---
> 
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 967e78c5ec0a..1441146f394f 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -541,6 +541,8 @@ patternProperties:
>      description: Linear Technology Corporation
>    "^logicpd,.*":
>      description: Logic PD, Inc.
> +  "^logictechno,.*":
> +    description: Logic Technologies Limited
>    "^longcheer,.*":
>      description: Longcheer Technology (Shanghai) Co., Ltd.
>    "^lsi,.*":
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 02/10] ASoC: samsung: arndale: Add missing OF node dereferencing
From: Krzysztof Kozlowski @ 2019-09-23  9:08 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: devicetree, alsa-devel, linux-samsung-soc, ckeepax, b.zolnierkie,
	sbkim73, patches, lgirdwood, robh+dt, broonie, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <20190920130218.32690-3-s.nawrocki@samsung.com>

On Fri, Sep 20, 2019 at 03:02:11PM +0200, Sylwester Nawrocki wrote:
> Ensure there is no OF node references kept when the driver
> is removed/unbound.
> 
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v1:
>  - rebased to beginning of the series
> ---
>  sound/soc/samsung/arndale_rt5631.c | 34 ++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v1 2/2] drm/panel: simple: add display timings for logic technologies displays
From: Philippe Schenker @ 2019-09-23  9:08 UTC (permalink / raw)
  To: marcel@ziswiler.com, dri-devel@lists.freedesktop.org
  Cc: devicetree@vger.kernel.org, Marcel Ziswiler, j.bauer@endrich.com,
	linux-kernel@vger.kernel.org, airlied@linux.ie,
	info@logictechno.com, thierry.reding@gmail.com, daniel@ffwll.ch,
	sam@ravnborg.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190920075411.15735-2-marcel@ziswiler.com>

On Fri, 2019-09-20 at 09:54 +0200, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Add display timings for the following 3 display panels manufactured by
> Logic Technologies Limited:
> 
> - LT161010-2NHC e.g. as found in the Toradex Capacitive Touch Display
>   7" Parallel [1]
> - LT161010-2NHR e.g. as found in the Toradex Resistive Touch Display
> 7"
>   Parallel [2]
> - LT170410-2WHC e.g. as found in the Toradex Capacitive Touch Display
>   10.1" LVDS [3]
> 
> Those panels may also be distributed by Endrich Bauelemente Vertriebs
> GmbH [4].
> 
> [1] 
> https://docs.toradex.com/104497-7-inch-parallel-capacitive-touch-display-800x480-datasheet.pdf
> [2] 
> https://docs.toradex.com/104498-7-inch-parallel-resistive-touch-display-800x480.pdf
> [3] 
> https://docs.toradex.com/105952-10-1-inch-lvds-capacitive-touch-display-1280x800-datasheet.pdf
> [4] 
> https://www.endrich.com/isi50_isi30_tft-displays/lt170410-1whc_isi30
> 
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>

> 
> ---
> 
>  drivers/gpu/drm/panel/panel-simple.c | 65
> ++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index 28fa6ba7b767..42bd0de25167 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -2034,6 +2034,62 @@ static const struct panel_desc lg_lp129qe = {
>  	},
>  };
>  
> +static const struct display_timing logictechno_lt161010_2nh_timing =
> {
> +	.pixelclock = { 26400000, 33300000, 46800000 },
> +	.hactive = { 800, 800, 800 },
> +	.hfront_porch = { 16, 210, 354 },
> +	.hback_porch = { 46, 46, 46 },
> +	.hsync_len = { 1, 20, 40 },
> +	.vactive = { 480, 480, 480 },
> +	.vfront_porch = { 7, 22, 147 },
> +	.vback_porch = { 23, 23, 23 },
> +	.vsync_len = { 1, 10, 20 },
> +	.flags = DISPLAY_FLAGS_HSYNC_LOW | DISPLAY_FLAGS_VSYNC_LOW |
> +		 DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_PIXDATA_POSEDGE |
> +		 DISPLAY_FLAGS_SYNC_POSEDGE,
> +};
> +
> +static const struct panel_desc logictechno_lt161010_2nh = {
> +	.timings = &logictechno_lt161010_2nh_timing,
> +	.num_timings = 1,
> +	.size = {
> +		.width = 154,
> +		.height = 86,
> +	},
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH |
> +		     DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> +		     DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE,
> +};
> +
> +static const struct display_timing logictechno_lt170410_2whc_timing =
> {
> +	.pixelclock = { 68900000, 71100000, 7340000 },
> +	.hactive = { 1280, 1280, 1280 },
> +	.hfront_porch = { 23, 60, 71 },
> +	.hback_porch = { 23, 60, 71 },
> +	.hsync_len = { 15, 40, 47 },
> +	.vactive = { 800, 800, 800 },
> +	.vfront_porch = { 5, 7, 10 },
> +	.vback_porch = { 5, 7, 10 },
> +	.vsync_len = { 6, 9, 12 },
> +	.flags = DISPLAY_FLAGS_HSYNC_LOW | DISPLAY_FLAGS_VSYNC_LOW |
> +		 DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_PIXDATA_POSEDGE |
> +		 DISPLAY_FLAGS_SYNC_POSEDGE,
> +};
> +
> +static const struct panel_desc logictechno_lt170410_2whc = {
> +	.timings = &logictechno_lt170410_2whc_timing,
> +	.num_timings = 1,
> +	.size = {
> +		.width = 217,
> +		.height = 136,
> +	},
> +	.bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH |
> +		     DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> +		     DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE,
> +};
> +
>  static const struct drm_display_mode mitsubishi_aa070mc01_mode = {
>  	.clock = 30400,
>  	.hdisplay = 800,
> @@ -3264,6 +3320,15 @@ static const struct of_device_id
> platform_of_match[] = {
>  	}, {
>  		.compatible = "lg,lp129qe",
>  		.data = &lg_lp129qe,
> +	}, {
> +		.compatible = "logictechno,lt161010-2nhc",
> +		.data = &logictechno_lt161010_2nh,
> +	}, {
> +		.compatible = "logictechno,lt161010-2nhr",
> +		.data = &logictechno_lt161010_2nh,
> +	}, {
> +		.compatible = "logictechno,lt170410-2whc",
> +		.data = &logictechno_lt170410_2whc,
>  	}, {
>  		.compatible = "mitsubishi,aa070mc01-ca1",
>  		.data = &mitsubishi_aa070mc01,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 04/10] ASoC: wm8994: Add support for MCLKn clock gating
From: Krzysztof Kozlowski @ 2019-09-23  9:09 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: devicetree, alsa-devel, linux-samsung-soc, ckeepax, b.zolnierkie,
	sbkim73, patches, lgirdwood, robh+dt, broonie, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <20190920130218.32690-5-s.nawrocki@samsung.com>

On Fri, Sep 20, 2019 at 03:02:13PM +0200, Sylwester Nawrocki wrote:
> As an intermediate step before covering the clocking subsystem
> of the CODEC entirely by the clk API add handling of external CODEC's
> master clocks in DAPM events when the AIFn clocks are sourced directly
> from MCLKn; when FLLn are used we enable/disable respective MCLKn
> before/after FLLn is enabled/disabled.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v1:
>  - addressed review comments from Charles
> ---
>  sound/soc/codecs/wm8994.c | 108 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 104 insertions(+), 4 deletions(-)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 03/10] ASoC: wm8994: Add support for setting MCLK clock rate
From: Sylwester Nawrocki @ 2019-09-23  9:10 UTC (permalink / raw)
  To: krzk, ckeepax
  Cc: devicetree, alsa-devel, linux-samsung-soc, b.zolnierkie, sbkim73,
	patches, lgirdwood, robh+dt, broonie, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <20190920130218.32690-4-s.nawrocki@samsung.com>

On 9/20/19 15:02, Sylwester Nawrocki wrote:
> Extend the set_sysclk() handler so we also set frequency of the MCLK1,
> MCLK2 clocks through clk API when those clocks are specified in DT.
> 
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Sorry, I squashed other patch to this one but forgot to remove above tags, 
not sure if those still stand as there was rather significant change in 
the patch. 

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v1:
>  - getting of clocks moved from MFD to the CODEC's driver probe()
> ---

-- 
Regards,
Sylwester

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v3 3/3] MAINTAINERS: Add entry for IMX290 CMOS image sensor driver
From: Sakari Ailus @ 2019-09-23  9:07 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: devicetree, c.barrett, linux-kernel, a.brela, robh+dt, mchehab,
	linux-arm-kernel, linux-media
In-Reply-To: <20190830091943.22646-4-manivannan.sadhasivam@linaro.org>

On Fri, Aug 30, 2019 at 02:49:43PM +0530, Manivannan Sadhasivam wrote:
> Add MAINTAINERS entry for Sony IMX290 CMOS image sensor driver.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f7c84004187d..0ee261fca602 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14962,6 +14962,14 @@ S:	Maintained
>  F:	drivers/media/i2c/imx274.c
>  F:	Documentation/devicetree/bindings/media/i2c/imx274.txt
>  
> +SONY IMX290 SENSOR DRIVER
> +M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> +L:	linux-media@vger.kernel.org
> +T:	git git://linuxtv.org/media_tree.git
> +S:	Maintained
> +F:	drivers/media/i2c/imx290.c
> +F:	Documentation/devicetree/bindings/media/i2c/imx290.txt
> +
>  SONY IMX319 SENSOR DRIVER
>  M:	Bingbu Cao <bingbu.cao@intel.com>
>  L:	linux-media@vger.kernel.org

Please squash this change to the first patch.

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH] arm64: errata: Update stale comment
From: Thierry Reding @ 2019-09-23  9:12 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon; +Cc: Marc Zyngier, linux-kernel, linux-arm-kernel

From: Thierry Reding <treding@nvidia.com>

Commit 73f381660959 ("arm64: Advertise mitigation of Spectre-v2, or lack
thereof") renamed the caller of the install_bp_hardening_cb() function
but forgot to update a comment, which can be confusing when trying to
follow the code flow.

Fixes: 73f381660959 ("arm64: Advertise mitigation of Spectre-v2, or lack thereof")
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 arch/arm64/kernel/cpu_errata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 1e43ba5c79b7..f593f4cffc0d 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -128,8 +128,8 @@ static void install_bp_hardening_cb(bp_hardening_cb_t fn,
 	int cpu, slot = -1;
 
 	/*
-	 * enable_smccc_arch_workaround_1() passes NULL for the hyp_vecs
-	 * start/end if we're a guest. Skip the hyp-vectors work.
+	 * detect_harden_bp_fw() passes NULL for the hyp_vecs start/end if
+	 * we're a guest. Skip the hyp-vectors work.
 	 */
 	if (!hyp_vecs_start) {
 		__this_cpu_write(bp_hardening_data.fn, fn);
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH v2 04/10] ASoC: wm8994: Add support for MCLKn clock gating
From: Sylwester Nawrocki @ 2019-09-23  9:13 UTC (permalink / raw)
  To: Charles Keepax
  Cc: devicetree, alsa-devel, linux-samsung-soc, b.zolnierkie, sbkim73,
	patches, broonie, lgirdwood, krzk, robh+dt, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <20190923085105.GN10204@ediswmail.ad.cirrus.com>

On 9/23/19 10:51, Charles Keepax wrote:
> On Fri, Sep 20, 2019 at 03:02:13PM +0200, Sylwester Nawrocki wrote:
>> As an intermediate step before covering the clocking subsystem
>> of the CODEC entirely by the clk API add handling of external CODEC's
>> master clocks in DAPM events when the AIFn clocks are sourced directly
>> from MCLKn; when FLLn are used we enable/disable respective MCLKn
>> before/after FLLn is enabled/disabled.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> Looks good to me:
> 
> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks a lot for reviewing!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 07/10] ASoC: samsung: arndale: Add support for WM1811 CODEC
From: Krzysztof Kozlowski @ 2019-09-23  9:16 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: devicetree, alsa-devel, linux-samsung-soc, ckeepax, b.zolnierkie,
	sbkim73, patches, lgirdwood, robh+dt, broonie, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <20190920130218.32690-8-s.nawrocki@samsung.com>

On Fri, Sep 20, 2019 at 03:02:16PM +0200, Sylwester Nawrocki wrote:
> The Arndale boards come with different types of the audio daughter
> board.  In order to support the WM1811 one we add new definition of
> an ASoC card which will be registered when the driver matches on
> "samsung,arndale-wm1811" compatible.  There is no runtime detection of
> the audio daughter board type at the moment, compatible string of the
> audio card needs to be adjusted in DT, e.g. by the bootloader,
> depending on actual audio board (CODEC) used.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v1:
>  - removed unneeded __maybe_used attribute
> ---
>  sound/soc/samsung/Kconfig          |  2 +
>  sound/soc/samsung/arndale_rt5631.c | 85 +++++++++++++++++++++++++-----
>  2 files changed, 74 insertions(+), 13 deletions(-)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH 1/1] ARM: dts: imx28: add poweroff support
From: Sébastien Szymanski @ 2019-09-23  9:17 UTC (permalink / raw)
  To: linux-arm-kernel, devicetree, NXP Linux Team,
	Pengutronix Kernel Team
  Cc: Mark Rutland, Fabio Estevam, Sascha Hauer, Julien Boibessot,
	Rob Herring, Shawn Guo

This add poweroff support using the generic syscon-poweroff driver.

Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
---
 arch/arm/boot/dts/imx28.dtsi | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index e14d8ef0158b..37fa8131a022 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -1114,8 +1114,16 @@
 			};
 
 			power: power@80044000 {
+				compatible = "syscon", "simple-mfd";
 				reg = <0x80044000 0x2000>;
-				status = "disabled";
+
+				mxs_poweroff: mxs-poweroff {
+					compatible = "syscon-poweroff";
+					regmap = <&power>;
+					offset = <0x104>;
+					value = <0x3E770001>;
+					status = "disabled";
+				};
 			};
 
 			saif1: saif@80046000 {
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH v2 08/10] ASoC: samsung: Rename Arndale card driver
From: Krzysztof Kozlowski @ 2019-09-23  9:17 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: devicetree, alsa-devel, linux-samsung-soc, ckeepax, b.zolnierkie,
	sbkim73, patches, lgirdwood, robh+dt, broonie, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <20190920130218.32690-9-s.nawrocki@samsung.com>

On Fri, Sep 20, 2019 at 03:02:17PM +0200, Sylwester Nawrocki wrote:
> Rename arndale_rt5631.c to just arnddale.c as we support other CODECs
> than RT5631.  While at it replace spaces in Kconfig with tabs.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v1:
>  - new patch.
> ---
>  sound/soc/samsung/Kconfig                         | 10 +++++-----
>  sound/soc/samsung/Makefile                        |  4 ++--
>  sound/soc/samsung/{arndale_rt5631.c => arndale.c} |  0
>  3 files changed, 7 insertions(+), 7 deletions(-)
>  rename sound/soc/samsung/{arndale_rt5631.c => arndale.c} (100%)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 1/4] pwm: mxs: implement ->apply
From: Uwe Kleine-König @ 2019-09-23  9:17 UTC (permalink / raw)
  To: Rasmus Villemoes, Russell King
  Cc: devicetree, linux-pwm, Shawn Guo, Sascha Hauer, linux-kernel,
	Rob Herring, Thierry Reding, NXP Linux Team,
	Pengutronix Kernel Team, Fabio Estevam, linux-clk,
	linux-arm-kernel
In-Reply-To: <a6407644-0b5b-ba46-9435-0d14be9066a5@rasmusvillemoes.dk>

Hello,

[expanded the recipents to include RMK and the clk list]

On Mon, Sep 23, 2019 at 11:04:39AM +0200, Rasmus Villemoes wrote:
> On 23/09/2019 10.24, Uwe Kleine-König wrote:
> > Also there is a bug already in .config: You are not supposed to call
> > clk_get_rate if the clk might be off.
> 
> Interesting, I didn't know that. So the prepare_enable logic needs to be
> moved before we start computing the period/duty cycles. Do you know why
> it has apparently worked so far? I would have thought such a rule would
> be enforced by the clock framework, or at least produced a warning.

FTR: This is documented in the kerneldoc code comment to clk_get_rate in
include/linux/clk.h.

Assuming this is relevant, it might indeed make sense to add a
WARN_ONCE for this.

Best regards
Uwe

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v3 2/3] media: i2c: Add IMX290 CMOS image sensor driver
From: Sakari Ailus @ 2019-09-23  9:22 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: devicetree, c.barrett, linux-kernel, a.brela, robh+dt, mchehab,
	linux-arm-kernel, linux-media
In-Reply-To: <20190830091943.22646-3-manivannan.sadhasivam@linaro.org>

Hi Manivannan,

On Fri, Aug 30, 2019 at 02:49:42PM +0530, Manivannan Sadhasivam wrote:
> Add driver for Sony IMX290 CMOS image sensor driver. The driver only
> supports I2C interface for programming and MIPI CSI-2 for sensor output.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/imx290.c | 881 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 893 insertions(+)
>  create mode 100644 drivers/media/i2c/imx290.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 79ce9ec6fc1b..4ebb80b18748 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -595,6 +595,17 @@ config VIDEO_IMX274
>  	  This is a V4L2 sensor driver for the Sony IMX274
>  	  CMOS image sensor.
>  
> +config VIDEO_IMX290
> +	tristate "Sony IMX290 sensor support"
> +	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +	depends on MEDIA_CAMERA_SUPPORT

Please drop this line. It will be redundant very soon.

> +	help
> +	  This is a Video4Linux2 sensor driver for the Sony
> +	  IMX290 camera sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called imx290.
> +
>  config VIDEO_IMX319
>  	tristate "Sony IMX319 sensor support"
>  	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index fd4ea86dedd5..04411ddb4922 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -111,6 +111,7 @@ obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
>  obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
>  obj-$(CONFIG_VIDEO_IMX258)	+= imx258.o
>  obj-$(CONFIG_VIDEO_IMX274)	+= imx274.o
> +obj-$(CONFIG_VIDEO_IMX290)	+= imx290.o
>  obj-$(CONFIG_VIDEO_IMX319)	+= imx319.o
>  obj-$(CONFIG_VIDEO_IMX355)	+= imx355.o
>  obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> new file mode 100644
> index 000000000000..db5bb0d69eb8
> --- /dev/null
> +++ b/drivers/media/i2c/imx290.c
> @@ -0,0 +1,881 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sony IMX290 CMOS Image Sensor Driver
> + *
> + * Copyright (C) 2019 FRAMOS GmbH.
> + *
> + * Copyright (C) 2019 Linaro Ltd.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define IMX290_STANDBY 0x3000
> +#define IMX290_REGHOLD 0x3001
> +#define IMX290_XMSTA 0x3002
> +#define IMX290_GAIN 0x3014
> +
> +#define IMX290_DEFAULT_LINK_FREQ 445500000
> +
> +static const char * const imx290_supply_name[] = {
> +	"vdda",
> +	"vddd",
> +	"vdddo",
> +};
> +
> +#define IMX290_NUM_SUPPLIES ARRAY_SIZE(imx290_supply_name)
> +
> +struct imx290_regval {
> +	u16 reg;
> +	u8 val;
> +};
> +
> +struct imx290_mode {
> +	u32 width;
> +	u32 height;
> +	u32 pixel_rate;
> +	u32 link_freq_index;
> +
> +	const struct imx290_regval *data;
> +	u32 data_size;
> +};
> +
> +struct imx290 {
> +	struct device *dev;
> +	struct clk *xclk;
> +	struct regmap *regmap;
> +
> +	struct v4l2_subdev sd;
> +	struct v4l2_fwnode_endpoint ep;
> +	struct media_pad pad;
> +	struct v4l2_mbus_framefmt current_format;
> +	const struct imx290_mode *current_mode;
> +
> +	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
> +	struct gpio_desc *rst_gpio;
> +
> +	struct v4l2_ctrl_handler ctrls;
> +	struct v4l2_ctrl *link_freq;
> +	struct v4l2_ctrl *pixel_rate;
> +
> +	struct mutex lock;
> +};
> +
> +struct imx290_pixfmt {
> +	u32 code;
> +};
> +
> +static const struct imx290_pixfmt imx290_formats[] = {
> +	{ MEDIA_BUS_FMT_SRGGB10_1X10 },

You have a single format here. You don't need the entire array, do you?

Unless you have plans to add more, that is.

> +};
> +
> +static struct regmap_config imx290_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +static const struct imx290_regval imx290_global_init_settings[] = {
> +	{ 0x3007, 0x00 },
> +	{ 0x3009, 0x00 },
> +	{ 0x3018, 0x65 },
> +	{ 0x3019, 0x04 },
> +	{ 0x301a, 0x00 },
> +	{ 0x3443, 0x03 },
> +	{ 0x3444, 0x20 },
> +	{ 0x3445, 0x25 },
> +	{ 0x3407, 0x03 },
> +	{ 0x303a, 0x0c },
> +	{ 0x3040, 0x00 },
> +	{ 0x3041, 0x00 },
> +	{ 0x303c, 0x00 },
> +	{ 0x303d, 0x00 },
> +	{ 0x3042, 0x9c },
> +	{ 0x3043, 0x07 },
> +	{ 0x303e, 0x49 },
> +	{ 0x303f, 0x04 },
> +	{ 0x304b, 0x0a },
> +	{ 0x300f, 0x00 },
> +	{ 0x3010, 0x21 },
> +	{ 0x3012, 0x64 },
> +	{ 0x3016, 0x09 },
> +	{ 0x3070, 0x02 },
> +	{ 0x3071, 0x11 },
> +	{ 0x309b, 0x10 },
> +	{ 0x309c, 0x22 },
> +	{ 0x30a2, 0x02 },
> +	{ 0x30a6, 0x20 },
> +	{ 0x30a8, 0x20 },
> +	{ 0x30aa, 0x20 },
> +	{ 0x30ac, 0x20 },
> +	{ 0x30b0, 0x43 },
> +	{ 0x3119, 0x9e },
> +	{ 0x311c, 0x1e },
> +	{ 0x311e, 0x08 },
> +	{ 0x3128, 0x05 },
> +	{ 0x313d, 0x83 },
> +	{ 0x3150, 0x03 },
> +	{ 0x317e, 0x00 },
> +	{ 0x32b8, 0x50 },
> +	{ 0x32b9, 0x10 },
> +	{ 0x32ba, 0x00 },
> +	{ 0x32bb, 0x04 },
> +	{ 0x32c8, 0x50 },
> +	{ 0x32c9, 0x10 },
> +	{ 0x32ca, 0x00 },
> +	{ 0x32cb, 0x04 },
> +	{ 0x332c, 0xd3 },
> +	{ 0x332d, 0x10 },
> +	{ 0x332e, 0x0d },
> +	{ 0x3358, 0x06 },
> +	{ 0x3359, 0xe1 },
> +	{ 0x335a, 0x11 },
> +	{ 0x3360, 0x1e },
> +	{ 0x3361, 0x61 },
> +	{ 0x3362, 0x10 },
> +	{ 0x33b0, 0x50 },
> +	{ 0x33b2, 0x1a },
> +	{ 0x33b3, 0x04 },
> +};
> +
> +static const struct imx290_regval imx290_1080p_settings[] = {
> +	/* mode settings */
> +	{ 0x3007, 0x00 },
> +	{ 0x303a, 0x0c },
> +	{ 0x3414, 0x0a },
> +	{ 0x3472, 0x80 },
> +	{ 0x3473, 0x07 },
> +	{ 0x3418, 0x38 },
> +	{ 0x3419, 0x04 },
> +	{ 0x3012, 0x64 },
> +	{ 0x3013, 0x00 },
> +	{ 0x305c, 0x18 },
> +	{ 0x305d, 0x03 },
> +	{ 0x305e, 0x20 },
> +	{ 0x305f, 0x01 },
> +	{ 0x315e, 0x1a },
> +	{ 0x3164, 0x1a },
> +	{ 0x3480, 0x49 },
> +	/* data rate settings */
> +	{ 0x3009, 0x01 },
> +	{ 0x3405, 0x10 },
> +	{ 0x3446, 0x57 },
> +	{ 0x3447, 0x00 },
> +	{ 0x3448, 0x37 },
> +	{ 0x3449, 0x00 },
> +	{ 0x344a, 0x1f },
> +	{ 0x344b, 0x00 },
> +	{ 0x344c, 0x1f },
> +	{ 0x344d, 0x00 },
> +	{ 0x344e, 0x1f },
> +	{ 0x344f, 0x00 },
> +	{ 0x3450, 0x77 },
> +	{ 0x3451, 0x00 },
> +	{ 0x3452, 0x1f },
> +	{ 0x3453, 0x00 },
> +	{ 0x3454, 0x17 },
> +	{ 0x3455, 0x00 },
> +	{ 0x301c, 0x98 },
> +	{ 0x301d, 0x08 },
> +};
> +
> +static const struct imx290_regval imx290_720p_settings[] = {
> +	/* mode settings */
> +	{ 0x3007, 0x10 },
> +	{ 0x303a, 0x06 },
> +	{ 0x3414, 0x04 },
> +	{ 0x3472, 0x00 },
> +	{ 0x3473, 0x05 },
> +	{ 0x3418, 0xd0 },
> +	{ 0x3419, 0x02 },
> +	{ 0x3012, 0x64 },
> +	{ 0x3013, 0x00 },
> +	{ 0x305c, 0x20 },
> +	{ 0x305d, 0x00 },
> +	{ 0x305e, 0x20 },
> +	{ 0x305f, 0x01 },
> +	{ 0x315e, 0x1a },
> +	{ 0x3164, 0x1a },
> +	{ 0x3480, 0x49 },
> +	/* data rate settings */
> +	{ 0x3009, 0x01 },
> +	{ 0x3405, 0x10 },
> +	{ 0x3446, 0x4f },
> +	{ 0x3447, 0x00 },
> +	{ 0x3448, 0x2f },
> +	{ 0x3449, 0x00 },
> +	{ 0x344a, 0x17 },
> +	{ 0x344b, 0x00 },
> +	{ 0x344c, 0x17 },
> +	{ 0x344d, 0x00 },
> +	{ 0x344e, 0x17 },
> +	{ 0x344f, 0x00 },
> +	{ 0x3450, 0x57 },
> +	{ 0x3451, 0x00 },
> +	{ 0x3452, 0x17 },
> +	{ 0x3453, 0x00 },
> +	{ 0x3454, 0x17 },
> +	{ 0x3455, 0x00 },
> +	{ 0x301c, 0xe4 },
> +	{ 0x301d, 0x0c },
> +};
> +
> +static const struct imx290_regval imx290_10bit_settings[] = {
> +	{ 0x3005, 0x00},
> +	{ 0x3046, 0x00},
> +	{ 0x3129, 0x1d},
> +	{ 0x317c, 0x12},
> +	{ 0x31ec, 0x37},
> +	{ 0x3441, 0x0a},
> +	{ 0x3442, 0x0a},
> +	{ 0x300a, 0x3c},
> +	{ 0x300b, 0x00},
> +};
> +
> +/* supported link frequencies */
> +static const s64 imx290_link_freq[] = {
> +	IMX290_DEFAULT_LINK_FREQ,
> +};
> +
> +/* Mode configs */
> +static const struct imx290_mode imx290_modes[] = {
> +	{
> +		.width = 1920,
> +		.height = 1080,
> +		.data = imx290_1080p_settings,
> +		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.pixel_rate = 178200000,
> +		.link_freq_index = 0,
> +	},
> +	{
> +		.width = 1280,
> +		.height = 720,
> +		.data = imx290_720p_settings,
> +		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.pixel_rate = 178200000,
> +		.link_freq_index = 0,
> +	},
> +};
> +
> +static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> +{
> +	return container_of(_sd, struct imx290, sd);
> +}
> +
> +static inline int imx290_read_reg(struct imx290 *imx290, u16 addr, u8 *value)
> +{
> +	u32 regval;

I don't remember for sure what I asked previously, but feel free to make
this unsigned int again. The third argument of regmap_read() is of that
type.

> +	int ret;
> +
> +	ret = regmap_read(imx290->regmap, addr, &regval);
> +	if (ret) {
> +		dev_err(imx290->dev, "I2C read failed for addr: %x\n", addr);
> +		return ret;
> +	}
> +
> +	*value = regval & 0xff;
> +
> +	return 0;
> +}
> +
> +static int imx290_write_reg(struct imx290 *imx290, u16 addr, u8 value)
> +{
> +	int ret;
> +
> +	ret = regmap_write(imx290->regmap, addr, value);
> +	if (ret) {
> +		dev_err(imx290->dev, "I2C write failed for addr: %x\n", addr);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int imx290_set_register_array(struct imx290 *imx290,
> +				     const struct imx290_regval *settings,
> +				     unsigned int num_settings)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < num_settings; ++i, ++settings) {
> +		ret = imx290_write_reg(imx290, settings->reg, settings->val);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Settle time is 10ms for all registers */
> +		msleep(10);
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx290_write_buffered_reg(struct imx290 *imx290, u16 address_low,
> +				     u8 nr_regs, u32 value)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	ret = imx290_write_reg(imx290, IMX290_REGHOLD, 0x01);
> +	if (ret) {
> +		dev_err(imx290->dev, "Error setting hold register\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < nr_regs; i++) {
> +		ret = imx290_write_reg(imx290, address_low + i,
> +				       (u8)(value >> (i * 8)));
> +		if (ret) {
> +			dev_err(imx290->dev, "Error writing buffered registers\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = imx290_write_reg(imx290, IMX290_REGHOLD, 0x00);
> +	if (ret) {
> +		dev_err(imx290->dev, "Error setting hold register\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int imx290_set_gain(struct imx290 *imx290, u32 value)
> +{
> +	int ret;
> +
> +	u32 adjusted_value = (value * 10) / 3;

What's the purpose of this? Why not to use the value directly?

> +
> +	ret = imx290_write_buffered_reg(imx290, IMX290_GAIN, 1, adjusted_value);
> +	if (ret)
> +		dev_err(imx290->dev, "Unable to write gain\n");
> +
> +	return ret;
> +}
> +
> +static int imx290_set_power_on(struct imx290 *imx290)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(imx290->xclk);

Please move the code from this function to the runtime PM runtime suspend
callback. The same for imx290_set_power_off().

> +	if (ret) {
> +		dev_err(imx290->dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(IMX290_NUM_SUPPLIES,
> +				    imx290->supplies);

Fits on a single line.

> +	if (ret) {
> +		dev_err(imx290->dev, "Failed to enable regulators\n");
> +		goto xclk_off;
> +	}
> +
> +	usleep_range(1, 2);
> +	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
> +	usleep_range(30000, 31000);
> +
> +	return 0;
> +
> +xclk_off:
> +	clk_disable_unprepare(imx290->xclk);
> +	return ret;
> +}
> +
> +/* Stop streaming */
> +static int imx290_stop_streaming(struct imx290 *imx290)
> +{
> +	int ret;
> +
> +	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x01);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(30);
> +
> +	return imx290_write_reg(imx290, IMX290_XMSTA, 0x01);
> +}
> +
> +static void imx290_set_power_off(struct imx290 *imx290)
> +{
> +	clk_disable_unprepare(imx290->xclk);
> +	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
> +	regulator_bulk_disable(IMX290_NUM_SUPPLIES, imx290->supplies);
> +}
> +
> +static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx290 *imx290 = container_of(ctrl->handler,
> +					     struct imx290, ctrls);
> +	int ret = 0;
> +
> +	/* V4L2 controls values will be applied only when power is already up */
> +	if (!pm_runtime_get_if_in_use(imx290->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_GAIN:
> +		ret = imx290_set_gain(imx290, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put(imx290->dev);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops imx290_ctrl_ops = {
> +	.s_ctrl = imx290_set_ctrl,
> +};
> +
> +static int imx290_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(imx290_formats))
> +		return -EINVAL;
> +
> +	code->code = imx290_formats[code->index].code;
> +
> +	return 0;
> +}
> +
> +static int imx290_get_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_pad_config *cfg,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	struct imx290 *imx290 = to_imx290(sd);
> +	struct v4l2_mbus_framefmt *framefmt;
> +
> +	mutex_lock(&imx290->lock);
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +		framefmt = v4l2_subdev_get_try_format(&imx290->sd, cfg,
> +						      fmt->pad);
> +	else
> +		framefmt = &imx290->current_format;
> +
> +	fmt->format = *framefmt;
> +
> +	mutex_unlock(&imx290->lock);
> +
> +	return 0;
> +}
> +
> +static int imx290_set_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_pad_config *cfg,
> +		      struct v4l2_subdev_format *fmt)
> +{
> +	struct imx290 *imx290 = to_imx290(sd);
> +	const struct imx290_mode *mode;
> +	struct v4l2_mbus_framefmt *format;
> +	int i, ret = 0;

unsigned int i

> +
> +	mutex_lock(&imx290->lock);
> +
> +	mode = v4l2_find_nearest_size(imx290_modes,
> +				      ARRAY_SIZE(imx290_modes),
> +				      width, height,
> +				      fmt->format.width, fmt->format.height);
> +
> +	fmt->format.width = mode->width;
> +	fmt->format.height = mode->height;
> +
> +	for (i = 0; i < ARRAY_SIZE(imx290_formats); i++)
> +		if (imx290_formats[i].code == fmt->format.code)
> +			break;
> +
> +	if (i >= ARRAY_SIZE(imx290_formats))
> +		i = 0;
> +
> +	fmt->format.code = imx290_formats[i].code;
> +	fmt->format.field = V4L2_FIELD_NONE;
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		format = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> +	} else {
> +		format = &imx290->current_format;
> +		__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> +		__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, mode->pixel_rate);
> +
> +		imx290->current_mode = mode;
> +	}
> +
> +	*format = fmt->format;
> +
> +	mutex_unlock(&imx290->lock);
> +
> +	return ret;
> +}
> +
> +static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
> +				  struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct v4l2_subdev_format fmt = { 0 };
> +
> +	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> +	fmt.format.width = 1920;
> +	fmt.format.height = 1080;
> +
> +	imx290_set_fmt(subdev, cfg, &fmt);
> +
> +	return 0;
> +}
> +
> +static int imx290_write_current_format(struct imx290 *imx290,
> +				       struct v4l2_mbus_framefmt *format)
> +{
> +	int ret;
> +
> +	switch (format->code) {
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
> +					ARRAY_SIZE(imx290_10bit_settings));

Indentation.

> +		if (ret < 0) {
> +			dev_err(imx290->dev, "Could not set format registers\n");
> +			return ret;
> +		}
> +		break;
> +	default:
> +		dev_err(imx290->dev, "Unknown pixel format\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Start streaming */
> +static int imx290_start_streaming(struct imx290 *imx290)
> +{
> +	int ret;
> +
> +	/* Set init register settings */
> +	ret = imx290_set_register_array(imx290, imx290_global_init_settings,
> +				ARRAY_SIZE(imx290_global_init_settings));
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set init registers\n");
> +		return ret;
> +	}
> +
> +	/* Set current frame format */
> +	ret = imx290_write_current_format(imx290, &imx290->current_format);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set frame format\n");
> +		return ret;
> +	}
> +
> +	/* Apply default values of current mode */
> +	ret = imx290_set_register_array(imx290, imx290->current_mode->data,
> +					imx290->current_mode->data_size);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set current mode\n");
> +		return ret;
> +	}
> +
> +	/* Apply customized values from user */
> +	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> +	if (ret) {
> +		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
> +		return ret;
> +	}
> +
> +	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x00);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(30);
> +
> +	/* Start streaming */
> +	return imx290_write_reg(imx290, IMX290_XMSTA, 0x00);
> +}
> +
> +static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct imx290 *imx290 = to_imx290(sd);
> +	int ret = 0;
> +
> +	if (enable) {
> +		ret = pm_runtime_get_sync(imx290->dev);
> +		if (ret < 0) {
> +			pm_runtime_put_noidle(imx290->dev);
> +			goto unlock_and_return;
> +		}
> +
> +		ret = imx290_start_streaming(imx290);
> +		if (ret) {
> +			dev_err(imx290->dev, "Start stream failed\n");
> +			pm_runtime_put(imx290->dev);
> +			goto unlock_and_return;
> +		}
> +	} else {
> +		imx290_stop_streaming(imx290);
> +		pm_runtime_put(imx290->dev);
> +	}
> +
> +unlock_and_return:
> +
> +	return ret;
> +}
> +
> +static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < IMX290_NUM_SUPPLIES; i++)
> +		imx290->supplies[i].supply = imx290_supply_name[i];
> +
> +	return devm_regulator_bulk_get(dev, IMX290_NUM_SUPPLIES,
> +				       imx290->supplies);
> +}
> +
> +static int __maybe_unused imx290_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx290 *imx290 = to_imx290(sd);
> +
> +	return imx290_set_power_on(imx290);
> +}
> +
> +static int __maybe_unused imx290_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx290 *imx290 = to_imx290(sd);
> +
> +	imx290_set_power_off(imx290);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops imx290_pm_ops = {
> +	SET_RUNTIME_PM_OPS(imx290_runtime_suspend,
> +			   imx290_runtime_resume, NULL)

Fits on a single line.

> +};
> +
> +static const struct v4l2_subdev_video_ops imx290_video_ops = {
> +	.s_stream = imx290_set_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
> +	.init_cfg = imx290_entity_init_cfg,
> +	.enum_mbus_code = imx290_enum_mbus_code,
> +	.get_fmt = imx290_get_fmt,
> +	.set_fmt = imx290_set_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops imx290_subdev_ops = {
> +	.video = &imx290_video_ops,
> +	.pad = &imx290_pad_ops,
> +};
> +
> +static const struct media_entity_operations imx290_subdev_entity_ops = {
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static int imx290_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	struct imx290 *imx290;
> +	u32 xclk_freq;
> +	int ret;
> +
> +	imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL);
> +	if (!imx290)
> +		return -ENOMEM;
> +
> +	imx290->dev = dev;
> +	imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config);
> +	if (IS_ERR(imx290->regmap)) {
> +		dev_err(dev, "Unable to initialize I2C\n");
> +		return -ENODEV;
> +	}
> +
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "Endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &imx290->ep);

You're missing v4l2_fwnode_endpoint_free() below, both in error handling as
for the successful case.

> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(dev, "Parsing endpoint node failed\n");
> +		return ret;
> +	}
> +
> +	if (!imx290->ep.nr_of_link_frequencies) {
> +		dev_err(dev, "link-frequency property not found in DT\n");
> +		return -EINVAL;
> +	}
> +
> +	if (imx290->ep.link_frequencies[0] != IMX290_DEFAULT_LINK_FREQ) {
> +		dev_err(dev, "Unsupported link frequency\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Only CSI2 is supported for now */
> +	if (imx290->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(dev, "Unsupported bus type, should be CSI2\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Set default mode to max resolution */
> +	imx290->current_mode = &imx290_modes[0];
> +
> +	/* get system clock (xclk) */
> +	imx290->xclk = devm_clk_get(dev, "xclk");
> +	if (IS_ERR(imx290->xclk)) {
> +		dev_err(dev, "Could not get xclk");
> +		return PTR_ERR(imx290->xclk);
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);

You're using the fwnode API elsewhere in the driver; please use
fwnode_property_read_u32() here, too.

> +	if (ret) {
> +		dev_err(dev, "Could not get xclk frequency\n");
> +		return ret;
> +	}
> +
> +	/* external clock must be 37.125 MHz */
> +	if (xclk_freq != 37125000) {
> +		dev_err(dev, "External clock frequency %u is not supported\n",
> +			xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = clk_set_rate(imx290->xclk, xclk_freq);
> +	if (ret) {
> +		dev_err(dev, "Could not set xclk frequency\n");
> +		return ret;
> +	}
> +
> +	ret = imx290_get_regulators(dev, imx290);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot get regulators\n");
> +		return ret;
> +	}
> +
> +	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
> +	if (IS_ERR(imx290->rst_gpio)) {
> +		dev_err(dev, "Cannot get reset gpio\n");
> +		return PTR_ERR(imx290->rst_gpio);
> +	}
> +
> +	mutex_init(&imx290->lock);
> +
> +	v4l2_ctrl_handler_init(&imx290->ctrls, 3);
> +
> +	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> +			  V4L2_CID_GAIN, 0, 72, 1, 0);
> +	imx290->link_freq = v4l2_ctrl_new_int_menu(&imx290->ctrls,
> +					&imx290_ctrl_ops,
> +					V4L2_CID_LINK_FREQ,
> +					ARRAY_SIZE(imx290_link_freq) - 1,
> +					0, imx290_link_freq);
> +	if (imx290->link_freq)
> +		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE, 1,
> +					       INT_MAX, 1,
> +					       imx290_modes[0].pixel_rate);
> +
> +	imx290->sd.ctrl_handler = &imx290->ctrls;
> +
> +	if (imx290->ctrls.error) {
> +		dev_err(dev, "Control initialization error %d\n",
> +			imx290->ctrls.error);
> +		ret = imx290->ctrls.error;
> +		goto free_ctrl;
> +	}
> +
> +	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> +	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	imx290->sd.dev = &client->dev;
> +	imx290->sd.entity.ops = &imx290_subdev_entity_ops;
> +	imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	imx290->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
> +	if (ret < 0) {
> +		dev_err(dev, "Could not register media entity\n");
> +		goto free_ctrl;
> +	}
> +
> +	ret = v4l2_async_register_subdev(&imx290->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "Could not register v4l2 device\n");
> +		goto free_entity;
> +	}
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);

Here, runtime PM's idea of your device's power state is that it's powered
on. You'll need to manually call the function doing that before you're
here. Otherewise there's a chance that the device will be powered off when
the first use tries to access it. See e.g. the smiapp driver for an
example.

> +	pm_runtime_idle(dev);
> +
> +	return 0;
> +
> +free_entity:
> +	media_entity_cleanup(&imx290->sd.entity);
> +free_ctrl:
> +	v4l2_ctrl_handler_free(&imx290->ctrls);
> +	mutex_destroy(&imx290->lock);
> +
> +	return ret;
> +}
> +
> +static int imx290_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx290 *imx290 = to_imx290(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +
> +	mutex_destroy(&imx290->lock);
> +
> +	pm_runtime_disable(imx290->dev);
> +	if (!pm_runtime_status_suspended(imx290->dev))
> +		imx290_set_power_off(imx290);
> +	pm_runtime_set_suspended(imx290->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx290_of_match[] = {
> +	{ .compatible = "sony,imx290" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx290_of_match);
> +
> +static struct i2c_driver imx290_i2c_driver = {
> +	.probe_new  = imx290_probe,
> +	.remove = imx290_remove,
> +	.driver = {
> +		.name  = "imx290",
> +		.pm = &imx290_pm_ops,
> +		.of_match_table = of_match_ptr(imx290_of_match),
> +	},
> +};
> +
> +module_i2c_driver(imx290_i2c_driver);
> +
> +MODULE_DESCRIPTION("Sony IMX290 CMOS Image Sensor Driver");
> +MODULE_AUTHOR("FRAMOS GmbH");
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
> +MODULE_LICENSE("GPL v2");

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 03/10] ASoC: wm8994: Add support for setting MCLK clock rate
From: Charles Keepax @ 2019-09-23  9:25 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: devicetree, alsa-devel, linux-samsung-soc, b.zolnierkie, sbkim73,
	patches, broonie, lgirdwood, krzk, robh+dt, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <7334ce45-f192-4421-aa3d-d142582153ef@samsung.com>

On Mon, Sep 23, 2019 at 11:10:48AM +0200, Sylwester Nawrocki wrote:
> On 9/20/19 15:02, Sylwester Nawrocki wrote:
> > Extend the set_sysclk() handler so we also set frequency of the MCLK1,
> > MCLK2 clocks through clk API when those clocks are specified in DT.
> > 
> > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Sorry, I squashed other patch to this one but forgot to remove above tags, 
> not sure if those still stand as there was rather significant change in 
> the patch. 
> 

I am happy with my tag being there.

Thanks,
Charles

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 0/5] provide the XTAL clock via OF on Meson8/8b/8m2
From: Jerome Brunet @ 2019-09-23  9:29 UTC (permalink / raw)
  To: Martin Blumenstingl, narmstrong, robh+dt, mark.rutland,
	linux-amlogic, devicetree, khilman
  Cc: Martin Blumenstingl, linux-kernel, linux-arm-kernel, linux-clk
In-Reply-To: <20190921151223.768842-1-martin.blumenstingl@googlemail.com>

On Sat 21 Sep 2019 at 17:12, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> So far the HHI clock controller has been providing the XTAL clock on
> Amlogic Meson8/Meson8b/Meson8m2 SoCs.
> This is not correct because the XTAL is actually a crystal on the
> boards and the SoC has a dedicated input for it.
>
> This updates the dt-bindings of the HHI clock controller and defines
> a fixed-clock in meson.dtsi (along with switching everything over to
> use this clock).
> The clock driver needs three updates to use this:
> - patch #2 uses clk_hw_set_parent in the CPU clock notifier. This drops
>   the explicit reference to CLKID_XTAL while at the same time making
>   the code much easier (thanks to Neil for providing this new method
>   as part of the G12A CPU clock bringup!)
> - patch #3 ensures that the clock driver doesn't rely on it's internal
>   XTAL clock while not losing support for older .dtbs that don't have
>   the XTAL clock input yet
> - with patch #4 the clock controller's own XTAL clock is not registered
>   anymore when a clock input is provided via OF
>
> This series is a functional no-op. It's main goal is to better represent
> how the actual hardware looks like.

I'm a bit unsure about this series.

On one hand, I totally agree with you ... having the xtal in DT is the
right way to do it ... when done from the start

On the other hand, things have been this way for years, they are working
and going for xtal in DT does not solve any pending issue. Doing this
means adding complexity in the driver to support both methods. It is
also quite a significant change in DT :/

I'll defer this one to Kevin

>
>
> Martin Blumenstingl (5):
>   dt-bindings: clock: meson8b: add the clock inputs
>   clk: meson: meson8b: use clk_hw_set_parent in the CPU clock notifier
>   clk: meson: meson8b: change references to the XTAL clock to use the
>     name
>   clk: meson: meson8b: don't register the XTAL clock when provided via
>     OF
>   ARM: dts: meson: provide the XTAL clock using a fixed-clock
>
>  .../bindings/clock/amlogic,meson8b-clkc.txt   |   5 +
>  arch/arm/boot/dts/meson.dtsi                  |   7 ++
>  arch/arm/boot/dts/meson6.dtsi                 |   7 --
>  arch/arm/boot/dts/meson8.dtsi                 |  15 +--
>  arch/arm/boot/dts/meson8b-ec100.dts           |   2 +-
>  arch/arm/boot/dts/meson8b-mxq.dts             |   2 +-
>  arch/arm/boot/dts/meson8b-odroidc1.dts        |   2 +-
>  arch/arm/boot/dts/meson8b.dtsi                |  15 +--
>  drivers/clk/meson/meson8b.c                   | 106 +++++++++---------
>  9 files changed, 87 insertions(+), 74 deletions(-)
>
> -- 
> 2.23.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 4/5] clk: meson: meson8b: don't register the XTAL clock when provided via OF
From: Jerome Brunet @ 2019-09-23  9:31 UTC (permalink / raw)
  To: Martin Blumenstingl, narmstrong, robh+dt, mark.rutland,
	linux-amlogic, devicetree, khilman
  Cc: Martin Blumenstingl, linux-kernel, linux-arm-kernel, linux-clk
In-Reply-To: <20190921151223.768842-5-martin.blumenstingl@googlemail.com>

On Sat 21 Sep 2019 at 17:12, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> The XTAL clock is an actual crystal on the PCB. Thus the meson8b clock
> driver should not register the XTAL clock - instead it should be
> provided via .dts and then passed to the clock controller.
>
> Skip the registration of the XTAL clock if a parent clock is provided
> via OF. Fall back to registering the XTAL clock if this is not the case
> to keep support for old .dtbs.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/meson8b.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index b785b67baf2b..15ec14fde2a0 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -3682,10 +3682,16 @@ static void __init meson8b_clkc_init_common(struct device_node *np,
>  		meson8b_clk_regmaps[i]->map = map;
>  
>  	/*
> -	 * register all clks
> -	 * CLKID_UNUSED = 0, so skip it and start with CLKID_XTAL = 1
> +	 * always skip CLKID_UNUSED and also skip XTAL if the .dtb provides the
> +	 * XTAL clock as input.
>  	 */
> -	for (i = CLKID_XTAL; i < CLK_NR_CLKS; i++) {
> +	if (of_clk_get_parent_count(np))

If we are going for this, I'd prefer if could explicity check for the
clock named "xtal" instead of just checking if DT has clocks.

> +		i = CLKID_PLL_FIXED;
> +	else
> +		i = CLKID_XTAL;
> +
> +	/* register all clks */
> +	for (; i < CLK_NR_CLKS; i++) {
>  		/* array might be sparse */
>  		if (!clk_hw_onecell_data->hws[i])
>  			continue;
> -- 
> 2.23.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] seccomp: remove unused arg from secure_computing()
From: Borislav Petkov @ 2019-09-23  9:49 UTC (permalink / raw)
  To: Christian Brauner, Andy Lutomirski
  Cc: linux-s390, Will Drewry, Kees Cook, linux-parisc, x86, linux-um,
	linux-kernel, Oleg Nesterov, Thomas Gleixner, linux-arm-kernel
In-Reply-To: <20190920131907.6886-1-christian.brauner@ubuntu.com>

On Fri, Sep 20, 2019 at 03:19:09PM +0200, Christian Brauner wrote:
> While touching seccomp code I realized that the struct seccomp_data
> argument to secure_computing() seems to be unused by all current
> callers. So let's remove it unless there is some subtlety I missed.
> Note, I only tested this on x86.

What was amluto thinking in

2f275de5d1ed ("seccomp: Add a seccomp_data parameter secure_computing()")

?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v6 1/2] dt-bindings: display/bridge: Add binding for NWL mipi dsi host controller
From: Guido Günther @ 2019-09-23  9:56 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Mark Rutland, devicetree, Jernej Skrabec, Pengutronix Kernel Team,
	Sam Ravnborg, Neil Armstrong, David Airlie, Fabio Estevam,
	Sascha Hauer, Jonas Karlman, linux-kernel, dri-devel, Rob Herring,
	Arnd Bergmann, NXP Linux Team, Daniel Vetter, Robert Chiras,
	Lee Jones, Shawn Guo, linux-arm-kernel, Laurent Pinchart
In-Reply-To: <18619804-ffe8-f3a5-aa54-ab590b3a83c0@samsung.com>

Hi,
On Mon, Sep 23, 2019 at 10:59:34AM +0200, Andrzej Hajda wrote:
> On 22.09.2019 18:47, Guido Günther wrote:
> > The Northwest Logic MIPI DSI IP core can be found in NXPs i.MX8 SoCs.
> >
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > Tested-by: Robert Chiras <robert.chiras@nxp.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  .../bindings/display/bridge/nwl-dsi.yaml      | 176 ++++++++++++++++++
> >  1 file changed, 176 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml b/Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml
> > new file mode 100644
> > index 000000000000..31119c7885ff
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml
> > @@ -0,0 +1,176 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: https://protect2.fireeye.com/url?k=7c9397fbdbbe3fd5.7c921cb4-87fc4542b5f41502&u=http://devicetree.org/schemas/display/bridge/nwl-dsi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Northwest Logic MIPI-DSI controller on i.MX SoCs
> > +
> > +maintainers:
> > +  - Guido Gúnther <agx@sigxcpu.org>
> > +  - Robert Chiras <robert.chiras@nxp.com>
> > +
> > +description: |
> > +  NWL MIPI-DSI host controller found on i.MX8 platforms. This is a dsi bridge for
> > +  the SOCs NWL MIPI-DSI host controller.
> > +
> > +properties:
> > +  compatible:
> > +    const: fsl,imx8mq-nwl-dsi
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +  clocks:
> > +    items:
> > +      - description: DSI core clock
> > +      - description: RX_ESC clock (used in escape mode)
> > +      - description: TX_ESC clock (used in escape mode)
> > +      - description: PHY_REF clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: core
> > +      - const: rx_esc
> > +      - const: tx_esc
> > +      - const: phy_ref
> > +
> > +  mux-controls:
> > +    description:
> > +      mux controller node to use for operating the input mux
> > +
> > +  phys:
> > +    maxItems: 1
> > +    description:
> > +      A phandle to the phy module representing the DPHY
> > +
> > +  phy-names:
> > +    items:
> > +      - const: dphy
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    items:
> > +      - description: dsi byte reset line
> > +      - description: dsi dpi reset line
> > +      - description: dsi esc reset line
> > +      - description: dsi pclk reset line
> > +
> > +  reset-names:
> > +    items:
> > +      - const: byte
> > +      - const: dpi
> > +      - const: esc
> > +      - const: pclk
> > +
> > +  ports:
> > +    type: object
> > +    description:
> > +      A node containing DSI input & output port nodes with endpoint
> > +      definitions as documented in
> > +      Documentation/devicetree/bindings/graph.txt.
> > +    properties:
> > +      port@0:
> > +        type: object
> > +        description:
> > +          Input port node to receive pixel data from the
> > +          display controller
> > +
> > +      port@1:
> > +        type: object
> > +        description:
> > +          DSI output port node to the panel or the next bridge
> > +          in the chain
> > +
> > +      '#address-cells':
> > +        const: 1
> > +
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    required:
> > +      - '#address-cells'
> > +      - '#size-cells'
> > +      - port@0
> > +      - port@1
> > +
> > +    additionalProperties: false
> > +
> > +patternProperties:
> > +  "^panel@[0-9]+$":
> > +    type: object
> > +
> > +required:
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +  - clock-names
> > +  - clocks
> > +  - compatible
> > +  - interrupts
> > +  - mux-controls
> 
> 
> As I understand mux is not a part of the device, so maybe would be safer
> to make it optional.

I had mux-sel required for imx8mq *only* but Rob suggested to make things
required in general until we add other SoCs:

    https://lore.kernel.org/linux-arm-kernel/CAL_JsqK-5=WMZgNuJDTJ3Dm3YOJNw_9QCrPOOSe7MQzMV26pHw@mail.gmail.com/

Cheers,
 -- Guido

> 
> 
> Regards
> 
> Andrzej
> 
> 
> > +  - phy-names
> > +  - phys
> > +  - ports
> > +  - reg
> > +  - reset-names
> > +  - resets
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +
> > +   mipi_dsi: mipi_dsi@30a00000 {
> > +              #address-cells = <1>;
> > +              #size-cells = <0>;
> > +              compatible = "fsl,imx8mq-nwl-dsi";
> > +              reg = <0x30A00000 0x300>;
> > +              clocks = <&clk 163>, <&clk 244>, <&clk 245>, <&clk 164>;
> > +              clock-names = "core", "rx_esc", "tx_esc", "phy_ref";
> > +              interrupts = <0 34 4>;
> > +              mux-controls = <&mux 0>;
> > +              power-domains = <&pgc_mipi>;
> > +              resets = <&src 0>, <&src 1>, <&src 2>, <&src 3>;
> > +              reset-names = "byte", "dpi", "esc", "pclk";
> > +              phys = <&dphy>;
> > +              phy-names = "dphy";
> > +
> > +              panel@0 {
> > +                      compatible = "rocktech,jh057n00900";
> > +                      reg = <0>;
> > +                      port@0 {
> > +                           panel_in: endpoint {
> > +                                     remote-endpoint = <&mipi_dsi_out>;
> > +                           };
> > +                      };
> > +              };
> > +
> > +              ports {
> > +                    #address-cells = <1>;
> > +                    #size-cells = <0>;
> > +
> > +                    port@0 {
> > +                           reg = <0>;
> > +                           mipi_dsi_in: endpoint {
> > +                                        remote-endpoint = <&lcdif_mipi_dsi>;
> > +                           };
> > +                    };
> > +                    port@1 {
> > +                           reg = <1>;
> > +                           mipi_dsi_out: endpoint {
> > +                                         remote-endpoint = <&panel_in>;
> > +                           };
> > +                    };
> > +              };
> > +      };
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v6 2/2] drm/bridge: Add NWL MIPI DSI host controller support
From: Andrzej Hajda @ 2019-09-23 10:01 UTC (permalink / raw)
  To: Guido Günther, David Airlie, Daniel Vetter, Rob Herring,
	Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, Neil Armstrong, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Lee Jones, dri-devel, devicetree,
	linux-arm-kernel, linux-kernel, Robert Chiras, Sam Ravnborg,
	Arnd Bergmann
In-Reply-To: <c0ac0b203fb65ae7efd1b9b54664b491ca2fb157.1569170717.git.agx@sigxcpu.org>

On 22.09.2019 18:47, Guido Günther wrote:
> This adds initial support for the NWL MIPI DSI Host controller found on
> i.MX8 SoCs.
>
> It adds support for the i.MX8MQ but the same IP can be found on
> e.g. the i.MX8QXP.
>
> It has been tested on the Librem 5 devkit using mxsfb.
>
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Co-developed-by: Robert Chiras <robert.chiras@nxp.com>
> Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
> Tested-by: Robert Chiras <robert.chiras@nxp.com>
> ---
>  drivers/gpu/drm/bridge/Kconfig   |   16 +
>  drivers/gpu/drm/bridge/Makefile  |    3 +
>  drivers/gpu/drm/bridge/nwl-dsi.c | 1180 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/bridge/nwl-dsi.h |  144 ++++
>  4 files changed, 1343 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/nwl-dsi.c
>  create mode 100644 drivers/gpu/drm/bridge/nwl-dsi.h
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 1cc9f502c1f2..13021f9a6107 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -65,6 +65,22 @@ config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
>            to DP++. This is used with the i.MX6 imx-ldb
>            driver. You are likely to say N here.
>  
> +config DRM_NWL_MIPI_DSI
> +	tristate "Northwest Logic MIPI DSI Host controller"
> +	depends on DRM
> +	depends on COMMON_CLK
> +	depends on OF && HAS_IOMEM
> +	select DRM_KMS_HELPER
> +	select DRM_MIPI_DSI
> +	select DRM_PANEL_BRIDGE
> +	select GENERIC_PHY_MIPI_DPHY
> +	select MFD_SYSCON
> +	select MULTIPLEXER
> +	select REGMAP_MMIO
> +	help
> +	  This enables the Northwest Logic MIPI DSI Host controller as
> +	  for example found on NXP's i.MX8 Processors.
> +
>  config DRM_NXP_PTN3460
>  	tristate "NXP PTN3460 DP/LVDS bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 4934fcf5a6f8..c3f3a43e9b8f 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -16,4 +16,7 @@ obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>  obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> +obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>  obj-y += synopsys/
> +
> +header-test-y += nwl-dsi.h
> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> new file mode 100644
> index 000000000000..dea5429a1e17
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -0,0 +1,1180 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * i.MX8 NWL MIPI DSI host driver
> + *
> + * Copyright (C) 2017 NXP
> + * Copyright (C) 2019 Purism SPC
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/irq.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/sys_soc.h>
> +#include <linux/time64.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include <video/mipi_display.h>
> +#include <video/videomode.h>
> +
> +#include "nwl-dsi.h"
> +
> +#define DRV_NAME "nwl-dsi"
> +
> +/* i.MX8 NWL quirks */
> +/* i.MX8MQ errata E11418 */
> +#define E11418_HS_MODE_QUIRK	BIT(0)
> +/* Skip DSI bits in SRC on disable to avoid blank display on enable */
> +#define SRC_RESET_QUIRK		BIT(1)
> +
> +#define NWL_DSI_MIPI_FIFO_TIMEOUT msecs_to_jiffies(500)
> +
> +enum transfer_direction {
> +	DSI_PACKET_SEND,
> +	DSI_PACKET_RECEIVE,
> +};
> +
> +/* Possible platform specific clocks */
> +#define NWL_DSI_CLK_CORE	    "core"
> +#define NWL_DSI_MAX_PLATFORM_CLOCKS 1
> +
> +struct nwl_dsi_plat_clk_config {
> +	const char *id;
> +	struct clk *clk;
> +	bool present;
> +};
> +
> +struct nwl_dsi_transfer {
> +	const struct mipi_dsi_msg *msg;
> +	struct mipi_dsi_packet packet;
> +	struct completion completed;
> +
> +	int status; /* status of transmission */
> +	enum transfer_direction direction;
> +	bool need_bta;
> +	u8 cmd;
> +	u16 rx_word_count;
> +	size_t tx_len; /* in bytes */
> +	size_t rx_len; /* in bytes */
> +};
> +
> +struct nwl_dsi {
> +	struct drm_bridge bridge;
> +	struct mipi_dsi_host dsi_host;
> +	struct drm_bridge *panel_bridge;
> +	struct device *dev;
> +	struct phy *phy;
> +	union phy_configure_opts phy_cfg;
> +	unsigned int quirks;
> +
> +	struct regmap *regmap;
> +	int irq;
> +	struct reset_control *rstc;
> +	struct mux_control *mux;
> +
> +	/* DSI clocks */
> +	struct clk *phy_ref_clk;
> +	struct clk *rx_esc_clk;
> +	struct clk *tx_esc_clk;
> +	/* Platform dependent clocks */
> +	struct nwl_dsi_plat_clk_config clk_config[NWL_DSI_MAX_PLATFORM_CLOCKS];


These clocks are not documented.


> +
> +	/* dsi lanes */
> +	u32 lanes;
> +	enum mipi_dsi_pixel_format format;
> +	struct drm_display_mode mode;
> +	unsigned long dsi_mode_flags;
> +
> +	struct nwl_dsi_transfer *xfer;
> +
> +	const struct nwl_dsi_platform_data *pdata;
> +};
> +
> +/* Platform specific hooks to enable other SoCs like the i.MX8QM */
> +struct nwl_dsi_platform_data {
> +	int (*poweron)(struct nwl_dsi *dsi);
> +	int (*poweroff)(struct nwl_dsi *dsi);
> +	int (*select_input)(struct nwl_dsi *dsi);
> +	int (*deselect_input)(struct nwl_dsi *dsi);
> +	struct nwl_dsi_plat_clk_config clk_config[NWL_DSI_MAX_PLATFORM_CLOCKS];
> +};
> +
> +static const struct regmap_config nwl_dsi_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = NWL_DSI_IRQ_MASK2,
> +	.name = DRV_NAME,
> +};
> +
> +static inline struct nwl_dsi *bridge_to_dsi(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct nwl_dsi, bridge);
> +}
> +
> +


spare empty line


> +static int nwl_dsi_write(struct nwl_dsi *dsi, unsigned int reg, u32 val)
> +{
> +	int ret;
> +
> +	ret = regmap_write(dsi->regmap, reg, val);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(dsi->dev,
> +			      "Failed to write NWL DSI reg 0x%x: %d\n", reg,
> +			      ret);
> +	return ret;
> +}
> +
> +static u32 nwl_dsi_read(struct nwl_dsi *dsi, u32 reg)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(dsi->regmap, reg, &val);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(dsi->dev, "Failed to read NWL DSI reg 0x%x: %d\n",
> +			      reg, ret);
> +
> +	return val;


Error is logged, but not propagated, as a result caller will assume the
read was successful, quite fishy.


> +}
> +
> +static u32 nwl_dsi_get_dpi_pixel_format(enum mipi_dsi_pixel_format format)
> +{
> +	switch (format) {
> +	case MIPI_DSI_FMT_RGB565:
> +		return NWL_DSI_PIXEL_FORMAT_16;
> +	case MIPI_DSI_FMT_RGB666:
> +		return NWL_DSI_PIXEL_FORMAT_18L;
> +	case MIPI_DSI_FMT_RGB666_PACKED:
> +		return NWL_DSI_PIXEL_FORMAT_18;
> +	case MIPI_DSI_FMT_RGB888:
> +		return NWL_DSI_PIXEL_FORMAT_24;
> +	default:
> +		return -EINVAL;


return type should be signed


> +	}
> +}
> +
> +/*
> + * ps2bc - Picoseconds to byte clock cycles
> + */
> +static u32 ps2bc(struct nwl_dsi *dsi, unsigned long long ps)
> +{
> +	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +
> +	return DIV64_U64_ROUND_UP(ps * dsi->mode.clock * bpp,
> +				  dsi->lanes * 8 * NSEC_PER_SEC);
> +}
> +
> +/*
> + * ui2bc - UI time periods to byte clock cycles
> + */
> +static u32 ui2bc(struct nwl_dsi *dsi, unsigned long long ui)
> +{
> +	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +
> +	return DIV64_U64_ROUND_UP(ui * dsi->lanes,
> +				  dsi->mode.clock * 1000 * bpp);
> +}
> +
> +/*
> + * us2bc - micro seconds to lp clock cycles
> + */
> +static u32 us2lp(u32 lp_clk_rate, unsigned long us)
> +{
> +	return DIV_ROUND_UP(us * lp_clk_rate, USEC_PER_SEC);
> +}
> +
> +static int nwl_dsi_config_host(struct nwl_dsi *dsi)
> +{
> +	u32 cycles;
> +	struct phy_configure_opts_mipi_dphy *cfg = &dsi->phy_cfg.mipi_dphy;
> +
> +	if (dsi->lanes < 1 || dsi->lanes > 4)
> +		return -EINVAL;
> +
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "DSI Lanes %d\n", dsi->lanes);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_NUM_LANES, dsi->lanes - 1);
> +
> +	if (dsi->dsi_mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> +		nwl_dsi_write(dsi, NWL_DSI_CFG_NONCONTINUOUS_CLK, 0x01);
> +		nwl_dsi_write(dsi, NWL_DSI_CFG_AUTOINSERT_EOTP, 0x01);
> +	} else {
> +		nwl_dsi_write(dsi, NWL_DSI_CFG_NONCONTINUOUS_CLK, 0x00);
> +		nwl_dsi_write(dsi, NWL_DSI_CFG_AUTOINSERT_EOTP, 0x00);
> +	}
> +
> +	/* values in byte clock cycles */
> +	cycles = ui2bc(dsi, cfg->clk_pre);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_T_PRE, cycles);
> +	cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles);
> +	cycles += ui2bc(dsi, cfg->clk_pre);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_post: 0x%x\n", cycles);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_T_POST, cycles);
> +	cycles = ps2bc(dsi, cfg->hs_exit);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap: 0x%x\n", cycles);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_TX_GAP, cycles);
> +
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_EXTRA_CMDS_AFTER_EOTP, 0x01);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_HTX_TO_COUNT, 0x00);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_LRX_H_TO_COUNT, 0x00);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_BTA_H_TO_COUNT, 0x00);
> +	/* In LP clock cycles */
> +	cycles = us2lp(cfg->lp_clk_rate, cfg->wakeup);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_twakeup: 0x%x\n", cycles);
> +	nwl_dsi_write(dsi, NWL_DSI_CFG_TWAKEUP, cycles);
> +
> +	return 0;
> +}
> +
> +static int nwl_dsi_config_dpi(struct nwl_dsi *dsi)
> +{
> +	u32 color_format, mode;
> +	bool burst_mode;
> +	int hfront_porch, hback_porch, vfront_porch, vback_porch;
> +	int hsync_len, vsync_len;
> +
> +	hfront_porch = dsi->mode.hsync_start - dsi->mode.hdisplay;
> +	hsync_len = dsi->mode.hsync_end - dsi->mode.hsync_start;
> +	hback_porch = dsi->mode.htotal - dsi->mode.hsync_end;
> +
> +	vfront_porch = dsi->mode.vsync_start - dsi->mode.vdisplay;
> +	vsync_len = dsi->mode.vsync_end - dsi->mode.vsync_start;
> +	vback_porch = dsi->mode.vtotal - dsi->mode.vsync_end;
> +
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "hfront_porch = %d\n", hfront_porch);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "hback_porch = %d\n", hback_porch);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "hsync_len = %d\n", hsync_len);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "hdisplay = %d\n", dsi->mode.hdisplay);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "vfront_porch = %d\n", vfront_porch);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "vback_porch = %d\n", vback_porch);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "vsync_len = %d\n", vsync_len);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "vactive = %d\n", dsi->mode.vdisplay);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "clock = %d kHz\n", dsi->mode.clock);
> +
> +	color_format = nwl_dsi_get_dpi_pixel_format(dsi->format);
> +	if (color_format < 0) {


As robot mentioned  it is always false.


> +		DRM_DEV_ERROR(dsi->dev, "Invalid color format 0x%x\n",
> +			      dsi->format);
> +		return color_format;
> +	}
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "pixel fmt = %d\n", dsi->format);
> +
> +	nwl_dsi_write(dsi, NWL_DSI_INTERFACE_COLOR_CODING, NWL_DSI_DPI_24_BIT);
> +	nwl_dsi_write(dsi, NWL_DSI_PIXEL_FORMAT, color_format);
> +	/*
> +	 * Adjusting input polarity based on the video mode results in
> +	 * a black screen so always pick active low:
> +	 */
> +	nwl_dsi_write(dsi, NWL_DSI_VSYNC_POLARITY,
> +		      NWL_DSI_VSYNC_POLARITY_ACTIVE_LOW);
> +	nwl_dsi_write(dsi, NWL_DSI_HSYNC_POLARITY,
> +		      NWL_DSI_HSYNC_POLARITY_ACTIVE_LOW);
> +
> +	burst_mode = (dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_BURST) &&
> +		     !(dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE);
> +
> +	if (burst_mode) {
> +		nwl_dsi_write(dsi, NWL_DSI_VIDEO_MODE, NWL_DSI_VM_BURST_MODE);
> +		nwl_dsi_write(dsi, NWL_DSI_PIXEL_FIFO_SEND_LEVEL, 256);
> +	} else {
> +		mode = ((dsi->dsi_mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) ?
> +				NWL_DSI_VM_BURST_MODE_WITH_SYNC_PULSES :
> +				NWL_DSI_VM_NON_BURST_MODE_WITH_SYNC_EVENTS);
> +		nwl_dsi_write(dsi, NWL_DSI_VIDEO_MODE, mode);
> +		nwl_dsi_write(dsi, NWL_DSI_PIXEL_FIFO_SEND_LEVEL,
> +			      dsi->mode.hdisplay);
> +	}
> +
> +	nwl_dsi_write(dsi, NWL_DSI_HFP, hfront_porch);
> +	nwl_dsi_write(dsi, NWL_DSI_HBP, hback_porch);
> +	nwl_dsi_write(dsi, NWL_DSI_HSA, hsync_len);
> +
> +	nwl_dsi_write(dsi, NWL_DSI_ENABLE_MULT_PKTS, 0x0);
> +	nwl_dsi_write(dsi, NWL_DSI_BLLP_MODE, 0x1);
> +	nwl_dsi_write(dsi, NWL_DSI_USE_NULL_PKT_BLLP, 0x0);
> +	nwl_dsi_write(dsi, NWL_DSI_VC, 0x0);
> +
> +	nwl_dsi_write(dsi, NWL_DSI_PIXEL_PAYLOAD_SIZE, dsi->mode.hdisplay);
> +	nwl_dsi_write(dsi, NWL_DSI_VACTIVE, dsi->mode.vdisplay - 1);
> +	nwl_dsi_write(dsi, NWL_DSI_VBP, vback_porch);
> +	nwl_dsi_write(dsi, NWL_DSI_VFP, vfront_porch);


No checks for results of nwl_dsi_write. You should either add check
after every call, or try "error state" pattern (see for example for
tc358764.c driver).


> +
> +	return 0;
> +}
> +
> +static void nwl_dsi_init_interrupts(struct nwl_dsi *dsi)
> +{
> +	u32 irq_enable;
> +
> +	nwl_dsi_write(dsi, NWL_DSI_IRQ_MASK, 0xffffffff);
> +	nwl_dsi_write(dsi, NWL_DSI_IRQ_MASK2, 0x7);
> +
> +	irq_enable = ~(u32)(NWL_DSI_TX_PKT_DONE_MASK |
> +			    NWL_DSI_RX_PKT_HDR_RCVD_MASK |
> +			    NWL_DSI_TX_FIFO_OVFLW_MASK |
> +			    NWL_DSI_HS_TX_TIMEOUT_MASK);


I wonder if lack of '(u32)' would lead to some bugs/warnings.


> +
> +	nwl_dsi_write(dsi, NWL_DSI_IRQ_MASK, irq_enable);
> +}
> +
> +static int nwl_dsi_host_attach(struct mipi_dsi_host *dsi_host,
> +			       struct mipi_dsi_device *device)
> +{
> +	struct nwl_dsi *dsi = container_of(dsi_host, struct nwl_dsi, dsi_host);
> +	struct device *dev = dsi->dev;
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	DRM_DEV_INFO(dev, "lanes=%u, format=0x%x flags=0x%lx\n", device->lanes,
> +		     device->format, device->mode_flags);
> +
> +	if (device->lanes < 1 || device->lanes > 4)
> +		return -EINVAL;


I guess this sanity check should be rather in mipi_dsi_attach, but since
it is not there let's leave it here.


> +
> +	dsi->lanes = device->lanes;
> +	dsi->format = device->format;
> +	dsi->dsi_mode_flags = device->mode_flags;
> +
> +	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &panel,
> +					  &bridge);
> +	if (ret)
> +		return ret;
> +
> +	if (panel) {
> +		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI);
> +		if (IS_ERR(bridge))
> +			return PTR_ERR(bridge);
> +	}
> +
> +	dsi->panel_bridge = bridge;
> +	drm_bridge_add(&dsi->bridge);
> +
> +	return 0;
> +}
> +
> +static int nwl_dsi_host_detach(struct mipi_dsi_host *dsi_host,
> +			       struct mipi_dsi_device *device)
> +{
> +	struct nwl_dsi *dsi = container_of(dsi_host, struct nwl_dsi, dsi_host);
> +
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0);
> +	drm_bridge_remove(&dsi->bridge);
> +
> +	return 0;
> +}
> +
> +static bool nwl_dsi_read_packet(struct nwl_dsi *dsi, u32 status)
> +{
> +	struct device *dev = dsi->dev;
> +	struct nwl_dsi_transfer *xfer = dsi->xfer;
> +	u8 *payload = xfer->msg->rx_buf;
> +	u32 val;
> +	u16 word_count;
> +	u8 channel;
> +	u8 data_type;
> +
> +	xfer->status = 0;
> +
> +	if (xfer->rx_word_count == 0) {
> +		if (!(status & NWL_DSI_RX_PKT_HDR_RCVD))
> +			return false;
> +		/* Get the RX header and parse it */
> +		val = nwl_dsi_read(dsi, NWL_DSI_RX_PKT_HEADER);
> +		word_count = NWL_DSI_WC(val);
> +		channel = NWL_DSI_RX_VC(val);
> +		data_type = NWL_DSI_RX_DT(val);
> +
> +		if (channel != xfer->msg->channel) {
> +			DRM_DEV_ERROR(dev,
> +				      "[%02X] Channel mismatch (%u != %u)\n",
> +				      xfer->cmd, channel, xfer->msg->channel);
> +			return true;
> +		}
> +
> +		switch (data_type) {
> +		case MIPI_DSI_RX_GENERIC_SHORT_READ_RESPONSE_2BYTE:
> +			/* Fall through */
> +		case MIPI_DSI_RX_DCS_SHORT_READ_RESPONSE_2BYTE:
> +			if (xfer->msg->rx_len > 1) {
> +				/* read second byte */
> +				payload[1] = word_count >> 8;
> +				++xfer->rx_len;
> +			}
> +			/* Fall through */
> +		case MIPI_DSI_RX_GENERIC_SHORT_READ_RESPONSE_1BYTE:
> +			/* Fall through */
> +		case MIPI_DSI_RX_DCS_SHORT_READ_RESPONSE_1BYTE:
> +			if (xfer->msg->rx_len > 0) {
> +				/* read first byte */
> +				payload[0] = word_count & 0xff;
> +				++xfer->rx_len;
> +			}
> +			xfer->status = xfer->rx_len;
> +			return true;
> +		case MIPI_DSI_RX_ACKNOWLEDGE_AND_ERROR_REPORT:
> +			word_count &= 0xff;
> +			DRM_DEV_ERROR(dev, "[%02X] DSI error report: 0x%02x\n",
> +				      xfer->cmd, word_count);
> +			xfer->status = -EPROTO;
> +			return true;
> +		}
> +
> +		if (word_count > xfer->msg->rx_len) {
> +			DRM_DEV_ERROR(
> +				dev,
> +				"[%02X] Receive buffer too small: %zu (< %u)\n",
> +				xfer->cmd, xfer->msg->rx_len, word_count);
> +			return true;
> +		}
> +
> +		xfer->rx_word_count = word_count;
> +	} else {
> +		/* Set word_count from previous header read */
> +		word_count = xfer->rx_word_count;
> +	}
> +
> +	/* If RX payload is not yet received, wait for it */
> +	if (!(status & NWL_DSI_RX_PKT_PAYLOAD_DATA_RCVD))
> +		return false;
> +
> +	/* Read the RX payload */
> +	while (word_count >= 4) {
> +		val = nwl_dsi_read(dsi, NWL_DSI_RX_PAYLOAD);
> +		payload[0] = (val >> 0) & 0xff;
> +		payload[1] = (val >> 8) & 0xff;
> +		payload[2] = (val >> 16) & 0xff;
> +		payload[3] = (val >> 24) & 0xff;
> +		payload += 4;
> +		xfer->rx_len += 4;
> +		word_count -= 4;
> +	}
> +
> +	if (word_count > 0) {
> +		val = nwl_dsi_read(dsi, NWL_DSI_RX_PAYLOAD);
> +		switch (word_count) {
> +		case 3:
> +			payload[2] = (val >> 16) & 0xff;
> +			++xfer->rx_len;
> +			/* Fall through */
> +		case 2:
> +			payload[1] = (val >> 8) & 0xff;
> +			++xfer->rx_len;
> +			/* Fall through */
> +		case 1:
> +			payload[0] = (val >> 0) & 0xff;
> +			++xfer->rx_len;
> +			break;
> +		}
> +	}
> +
> +	xfer->status = xfer->rx_len;
> +
> +	return true;
> +}
> +
> +static void nwl_dsi_finish_transmission(struct nwl_dsi *dsi, u32 status)
> +{
> +	struct nwl_dsi_transfer *xfer = dsi->xfer;
> +	bool end_packet = false;
> +
> +	if (!xfer)
> +		return;
> +
> +	if (xfer->direction == DSI_PACKET_SEND &&
> +	    status & NWL_DSI_TX_PKT_DONE) {
> +		xfer->status = xfer->tx_len;
> +		end_packet = true;
> +	} else if (status & NWL_DSI_DPHY_DIRECTION &&
> +		   ((status & (NWL_DSI_RX_PKT_HDR_RCVD |
> +			       NWL_DSI_RX_PKT_PAYLOAD_DATA_RCVD)))) {
> +		end_packet = nwl_dsi_read_packet(dsi, status);
> +	}
> +
> +	if (end_packet)
> +		complete(&xfer->completed);
> +}
> +
> +static void nwl_dsi_begin_transmission(struct nwl_dsi *dsi)
> +{
> +	struct nwl_dsi_transfer *xfer = dsi->xfer;
> +	struct mipi_dsi_packet *pkt = &xfer->packet;
> +	const u8 *payload;
> +	size_t length;
> +	u16 word_count;
> +	u8 hs_mode;
> +	u32 val;
> +	u32 hs_workaround = 0;
> +
> +	/* Send the payload, if any */
> +	length = pkt->payload_length;
> +	payload = pkt->payload;
> +
> +	while (length >= 4) {
> +		val = *(u32 *)payload;
> +		hs_workaround |= !(val & 0xFFFF00);
> +		nwl_dsi_write(dsi, NWL_DSI_TX_PAYLOAD, val);
> +		payload += 4;
> +		length -= 4;
> +	}
> +	/* Send the rest of the payload */
> +	val = 0;
> +	switch (length) {
> +	case 3:
> +		val |= payload[2] << 16;
> +		/* Fall through */
> +	case 2:
> +		val |= payload[1] << 8;
> +		hs_workaround |= !(val & 0xFFFF00);
> +		/* Fall through */
> +	case 1:
> +		val |= payload[0];
> +		nwl_dsi_write(dsi, NWL_DSI_TX_PAYLOAD, val);
> +		break;
> +	}
> +	xfer->tx_len = pkt->payload_length;
> +
> +	/*
> +	 * Send the header
> +	 * header[0] = Virtual Channel + Data Type
> +	 * header[1] = Word Count LSB (LP) or first param (SP)
> +	 * header[2] = Word Count MSB (LP) or second param (SP)
> +	 */
> +	word_count = pkt->header[1] | (pkt->header[2] << 8);
> +	if (hs_workaround && (dsi->quirks & E11418_HS_MODE_QUIRK)) {
> +		DRM_DEV_DEBUG_DRIVER(dsi->dev,
> +				     "Using hs mode workaround for cmd 0x%x\n",
> +				     xfer->cmd);
> +		hs_mode = 1;
> +	} else {
> +		hs_mode = (xfer->msg->flags & MIPI_DSI_MSG_USE_LPM) ? 0 : 1;
> +	}
> +	val = NWL_DSI_WC(word_count) | NWL_DSI_TX_VC(xfer->msg->channel) |
> +	      NWL_DSI_TX_DT(xfer->msg->type) | NWL_DSI_HS_SEL(hs_mode) |
> +	      NWL_DSI_BTA_TX(xfer->need_bta);
> +	nwl_dsi_write(dsi, NWL_DSI_PKT_CONTROL, val);
> +
> +	/* Send packet command */
> +	nwl_dsi_write(dsi, NWL_DSI_SEND_PACKET, 0x1);
> +}
> +
> +static ssize_t nwl_dsi_host_transfer(struct mipi_dsi_host *dsi_host,
> +				     const struct mipi_dsi_msg *msg)
> +{
> +	struct nwl_dsi *dsi = container_of(dsi_host, struct nwl_dsi, dsi_host);
> +	struct nwl_dsi_transfer xfer;
> +	ssize_t ret = 0;
> +
> +	/* Create packet to be sent */
> +	dsi->xfer = &xfer;
> +	ret = mipi_dsi_create_packet(&xfer.packet, msg);
> +	if (ret < 0) {
> +		dsi->xfer = NULL;
> +		return ret;
> +	}
> +
> +	if ((msg->type & MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM ||
> +	     msg->type & MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM ||
> +	     msg->type & MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM ||
> +	     msg->type & MIPI_DSI_DCS_READ) &&
> +	    msg->rx_len > 0 && msg->rx_buf != NULL)
> +		xfer.direction = DSI_PACKET_RECEIVE;
> +	else
> +		xfer.direction = DSI_PACKET_SEND;
> +
> +	xfer.need_bta = (xfer.direction == DSI_PACKET_RECEIVE);
> +	xfer.need_bta |= (msg->flags & MIPI_DSI_MSG_REQ_ACK) ? 1 : 0;
> +	xfer.msg = msg;
> +	xfer.status = -ETIMEDOUT;
> +	xfer.rx_word_count = 0;
> +	xfer.rx_len = 0;
> +	xfer.cmd = 0x00;
> +	if (msg->tx_len > 0)
> +		xfer.cmd = ((u8 *)(msg->tx_buf))[0];
> +	init_completion(&xfer.completed);
> +
> +	ret = clk_prepare_enable(dsi->rx_esc_clk);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dsi->dev, "Failed to enable rx_esc clk: %zd\n",
> +			      ret);
> +		return ret;
> +	}
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "Enabled rx_esc clk @%lu Hz\n",
> +			     clk_get_rate(dsi->rx_esc_clk));
> +
> +	/* Initiate the DSI packet transmision */
> +	nwl_dsi_begin_transmission(dsi);
> +
> +	if (!wait_for_completion_timeout(&xfer.completed,
> +					 NWL_DSI_MIPI_FIFO_TIMEOUT)) {
> +		DRM_DEV_ERROR(dsi_host->dev, "[%02X] DSI transfer timed out\n",
> +			      xfer.cmd);
> +		ret = -ETIMEDOUT;
> +	} else {
> +		ret = xfer.status;
> +	}
> +
> +	clk_disable_unprepare(dsi->rx_esc_clk);
> +
> +	return ret;
> +}
> +
> +static const struct mipi_dsi_host_ops nwl_dsi_host_ops = {
> +	.attach = nwl_dsi_host_attach,
> +	.detach = nwl_dsi_host_detach,
> +	.transfer = nwl_dsi_host_transfer,
> +};
> +
> +static irqreturn_t nwl_dsi_irq_handler(int irq, void *data)
> +{
> +	u32 irq_status;
> +	struct nwl_dsi *dsi = data;
> +
> +	irq_status = nwl_dsi_read(dsi, NWL_DSI_IRQ_STATUS);
> +
> +	if (irq_status & NWL_DSI_TX_FIFO_OVFLW)
> +		DRM_DEV_ERROR_RATELIMITED(dsi->dev, "tx fifo overflow\n");
> +
> +	if (irq_status & NWL_DSI_HS_TX_TIMEOUT)
> +		DRM_DEV_ERROR_RATELIMITED(dsi->dev, "HS tx timeout\n");
> +
> +	if (irq_status & NWL_DSI_TX_PKT_DONE ||
> +	    irq_status & NWL_DSI_RX_PKT_HDR_RCVD ||
> +	    irq_status & NWL_DSI_RX_PKT_PAYLOAD_DATA_RCVD)
> +		nwl_dsi_finish_transmission(dsi, irq_status);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int nwl_dsi_enable(struct nwl_dsi *dsi)
> +{
> +	struct device *dev = dsi->dev;
> +	union phy_configure_opts *phy_cfg = &dsi->phy_cfg;
> +	int ret;
> +
> +	if (!dsi->lanes) {
> +		DRM_DEV_ERROR(dev, "Need DSI lanes: %d\n", dsi->lanes);
> +		return -EINVAL;
> +	}
> +
> +	ret = phy_init(dsi->phy);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to init DSI phy: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = phy_configure(dsi->phy, phy_cfg);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to configure DSI phy: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(dsi->tx_esc_clk);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dsi->dev, "Failed to enable tx_esc clk: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "Enabled tx_esc clk @%lu Hz\n",
> +			     clk_get_rate(dsi->tx_esc_clk));
> +
> +	ret = nwl_dsi_config_host(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to set up DSI: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = nwl_dsi_config_dpi(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to set up DPI: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = phy_power_on(dsi->phy);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to power on DPHY (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	nwl_dsi_init_interrupts(dsi);
> +
> +	return 0;


no unwinding in case of error


> +}
> +
> +static int nwl_dsi_disable(struct nwl_dsi *dsi)
> +{
> +	struct device *dev = dsi->dev;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "Disabling clocks and phy\n");
> +
> +	phy_power_off(dsi->phy);
> +	phy_exit(dsi->phy);
> +
> +	/* Disabling the clock before the phy breaks enabling dsi again */
> +	clk_disable_unprepare(dsi->tx_esc_clk);
> +
> +	return 0;
> +}
> +
> +static int nwl_dsi_set_platform_clocks(struct nwl_dsi *dsi, bool enable)
> +{
> +	struct device *dev = dsi->dev;
> +	const char *id;
> +	struct clk *clk;
> +	size_t i;
> +	unsigned long rate;
> +	int ret, result = 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "%s platform clocks\n",
> +			     enable ? "enabling" : "disabling");
> +	for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) {
> +		if (!dsi->clk_config[i].present)
> +			continue;
> +		id = dsi->clk_config[i].id;
> +		clk = dsi->clk_config[i].clk;
> +
> +		if (enable) {
> +			ret = clk_prepare_enable(clk);
> +			if (ret < 0) {
> +				DRM_DEV_ERROR(dev,
> +					      "Failed to enable %s clk: %d\n",
> +					      id, ret);
> +				result = result ?: ret;
> +			}
> +			rate = clk_get_rate(clk);
> +			DRM_DEV_DEBUG_DRIVER(dev, "Enabled %s clk @%lu Hz\n",
> +					     id, rate);
> +		} else {
> +			clk_disable_unprepare(clk);
> +			DRM_DEV_DEBUG_DRIVER(dev, "Disabled %s clk\n", id);
> +		}
> +	}
> +
> +	return result;
> +}
> +
> +static int nwl_dsi_plat_enable(struct nwl_dsi *dsi)
> +{
> +	struct device *dev = dsi->dev;
> +	int ret;
> +
> +	if (dsi->pdata->select_input)
> +		dsi->pdata->select_input(dsi);
> +
> +	ret = nwl_dsi_set_platform_clocks(dsi, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dsi->pdata->poweron(dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(dev, "Failed to power on DSI: %d\n", ret);
> +	return ret;
> +}
> +
> +static void nwl_dsi_plat_disable(struct nwl_dsi *dsi)
> +{
> +	dsi->pdata->poweroff(dsi);
> +	nwl_dsi_set_platform_clocks(dsi, false);
> +	if (dsi->pdata->deselect_input)
> +		dsi->pdata->deselect_input(dsi);
> +}
> +
> +static void nwl_dsi_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct nwl_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	nwl_dsi_disable(dsi);
> +	nwl_dsi_plat_disable(dsi);
> +	pm_runtime_put(dsi->dev);
> +}
> +
> +static int nwl_dsi_get_dphy_params(struct nwl_dsi *dsi,
> +				   const struct drm_display_mode *mode,
> +				   union phy_configure_opts *phy_opts)
> +{
> +	unsigned long rate;
> +	int ret;
> +
> +	if (dsi->lanes < 1 || dsi->lanes > 4)
> +		return -EINVAL;
> +
> +	/*
> +	 * So far the DPHY spec minimal timings work for both mixel
> +	 * dphy and nwl dsi host
> +	 */
> +	ret = phy_mipi_dphy_get_default_config(
> +		mode->crtc_clock * 1000,
> +		mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes,
> +		&phy_opts->mipi_dphy);
> +	if (ret < 0)
> +		return ret;
> +
> +	rate = clk_get_rate(dsi->tx_esc_clk);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "LP clk is @%lu Hz\n", rate);
> +	phy_opts->mipi_dphy.lp_clk_rate = rate;
> +
> +	return 0;
> +}
> +
> +static bool nwl_dsi_bridge_mode_fixup(struct drm_bridge *bridge,
> +				      const struct drm_display_mode *mode,
> +				      struct drm_display_mode *adjusted_mode)
> +{
> +	/* At least LCDIF + NWL needs active high sync */
> +	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> +
> +	return true;
> +}
> +
> +static enum drm_mode_status
> +nwl_dsi_bridge_mode_valid(struct drm_bridge *bridge,
> +			  const struct drm_display_mode *mode)
> +{
> +	struct nwl_dsi *dsi = bridge_to_dsi(bridge);
> +	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +
> +	if (mode->clock * bpp > 15000000 * dsi->lanes)
> +		return MODE_CLOCK_HIGH;
> +
> +	if (mode->clock * bpp < 80000 * dsi->lanes)
> +		return MODE_CLOCK_LOW;
> +
> +	return MODE_OK;
> +}
> +
> +static void
> +nwl_dsi_bridge_mode_set(struct drm_bridge *bridge,
> +			const struct drm_display_mode *mode,
> +			const struct drm_display_mode *adjusted_mode)
> +{
> +	struct nwl_dsi *dsi = bridge_to_dsi(bridge);
> +	struct device *dev = dsi->dev;
> +	union phy_configure_opts new_cfg;
> +	unsigned long phy_ref_rate;
> +	int ret;
> +
> +	ret = nwl_dsi_get_dphy_params(dsi, adjusted_mode, &new_cfg);
> +	if (ret < 0)
> +		return;
> +
> +	/*
> +	 * If hs clock is unchanged, we're all good - all parameters are
> +	 * derived from it atm.
> +	 */
> +	if (new_cfg.mipi_dphy.hs_clk_rate == dsi->phy_cfg.mipi_dphy.hs_clk_rate)
> +		return;
> +
> +	phy_ref_rate = clk_get_rate(dsi->phy_ref_clk);
> +	DRM_DEV_DEBUG_DRIVER(dev, "PHY at ref rate: %lu\n", phy_ref_rate);
> +	/* Save the new desired phy config */
> +	memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg));
> +
> +	memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode));
> +	drm_mode_debug_printmodeline(adjusted_mode);
> +}
> +
> +static void nwl_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct nwl_dsi *dsi = bridge_to_dsi(bridge);
> +
> +	pm_runtime_get_sync(dsi->dev);
> +	nwl_dsi_plat_enable(dsi);
> +	nwl_dsi_enable(dsi);
> +}
> +
> +static int nwl_dsi_bridge_attach(struct drm_bridge *bridge)
> +{
> +	struct nwl_dsi *dsi = bridge->driver_private;


Please use container_of here, driver_private shouldn't be used in
embeded bridges.


> +
> +	return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge);
> +}
> +
> +static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = {
> +	.pre_enable = nwl_dsi_bridge_pre_enable,
> +	.disable    = nwl_dsi_bridge_disable,
> +	.mode_fixup = nwl_dsi_bridge_mode_fixup,
> +	.mode_set   = nwl_dsi_bridge_mode_set,
> +	.mode_valid = nwl_dsi_bridge_mode_valid,
> +	.attach	    = nwl_dsi_bridge_attach,
> +};
> +
> +static int nwl_dsi_parse_dt(struct nwl_dsi *dsi)
> +{
> +	struct platform_device *pdev = to_platform_device(dsi->dev);
> +	struct clk *clk;
> +	const char *clk_id;
> +	void __iomem *base;
> +	int i, ret;
> +
> +	dsi->phy = devm_phy_get(dsi->dev, "dphy");
> +	if (IS_ERR(dsi->phy)) {
> +		ret = PTR_ERR(dsi->phy);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dsi->dev, "Could not get PHY: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Platform dependent clocks */
> +	memcpy(dsi->clk_config, dsi->pdata->clk_config,
> +	       sizeof(dsi->pdata->clk_config));
> +
> +	for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) {
> +		if (!dsi->clk_config[i].present)
> +			continue;
> +
> +		clk_id = dsi->clk_config[i].id;
> +		clk = devm_clk_get(dsi->dev, clk_id);
> +		if (IS_ERR(clk)) {
> +			ret = PTR_ERR(clk);
> +			DRM_DEV_ERROR(dsi->dev, "Failed to get %s clock: %d\n",
> +				      clk_id, ret);
> +			return ret;
> +		}
> +		DRM_DEV_DEBUG_DRIVER(dsi->dev, "Setup clk %s (rate: %lu)\n",
> +				     clk_id, clk_get_rate(clk));
> +		dsi->clk_config[i].clk = clk;
> +	}


As I said earlier this is undocumented.


> +
> +	/* DSI clocks */
> +	clk = devm_clk_get(dsi->dev, "phy_ref");
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		DRM_DEV_ERROR(dsi->dev, "Failed to get phy_ref clock: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +	dsi->phy_ref_clk = clk;
> +
> +	clk = devm_clk_get(dsi->dev, "rx_esc");
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		DRM_DEV_ERROR(dsi->dev, "Failed to get rx_esc clock: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +	dsi->rx_esc_clk = clk;
> +
> +	clk = devm_clk_get(dsi->dev, "tx_esc");
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		DRM_DEV_ERROR(dsi->dev, "Failed to get tx_esc clock: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +	dsi->tx_esc_clk = clk;
> +
> +	dsi->mux = devm_mux_control_get(dsi->dev, NULL);
> +	if (IS_ERR(dsi->mux)) {
> +		ret = PTR_ERR(dsi->mux);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dsi->dev, "Failed to get mux: %d\n", ret);
> +		return ret;
> +	}
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	dsi->regmap =
> +		devm_regmap_init_mmio(dsi->dev, base, &nwl_dsi_regmap_config);
> +	if (IS_ERR(dsi->regmap)) {
> +		ret = PTR_ERR(dsi->regmap);
> +		DRM_DEV_ERROR(dsi->dev, "Failed to create NWL DSI regmap: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	dsi->irq = platform_get_irq(pdev, 0);
> +	if (dsi->irq < 0) {
> +		DRM_DEV_ERROR(dsi->dev, "Failed to get device IRQ: %d\n",
> +			      dsi->irq);
> +		return dsi->irq;
> +	}
> +
> +	dsi->rstc = devm_reset_control_array_get(dsi->dev, false, true);
> +	if (IS_ERR(dsi->rstc)) {
> +		DRM_DEV_ERROR(dsi->dev, "Failed to get resets: %ld\n",
> +			      PTR_ERR(dsi->rstc));
> +		return PTR_ERR(dsi->rstc);
> +	}


You should not error log in case of EPROBE_DEFER.


> +
> +	return 0;
> +}
> +
> +static int imx8mq_dsi_select_input(struct nwl_dsi *dsi)
> +{
> +	struct device_node *remote;
> +	u32 use_dcss = 1;
> +	int ret;
> +
> +	remote = of_graph_get_remote_node(dsi->dev->of_node, 0, 0);
> +	if (strcmp(remote->name, "lcdif") == 0)
> +		use_dcss = 0;


Checking name of remote note does not look correct, you should check
only props from your node.

If the ip has separate lines for DCSS and LCDIF you should distinguish
by port number. If they are shared

you can use endpoint number to specify DCSS or LCDIF, in both cases
bindings should be adjusted.


Regards

Andrzej


> +
> +	DRM_DEV_INFO(dsi->dev, "Using %s as input source\n",
> +		     (use_dcss) ? "DCSS" : "LCDIF");
> +
> +	ret = mux_control_try_select(dsi->mux, use_dcss);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(dsi->dev, "Failed to select input: %d\n", ret);
> +
> +	of_node_put(remote);
> +	return ret;
> +}
> +
> +
> +static int imx8mq_dsi_deselect_input(struct nwl_dsi *dsi)
> +{
> +	int ret;
> +
> +	ret = mux_control_deselect(dsi->mux);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(dsi->dev, "Failed to deselect input: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +
> +static int imx8mq_dsi_poweron(struct nwl_dsi *dsi)
> +{
> +	int ret = 0;
> +
> +	/* otherwise the display stays blank */
> +	usleep_range(200, 300);
> +
> +	if (dsi->rstc)
> +		ret = reset_control_deassert(dsi->rstc);
> +
> +	return ret;
> +}
> +
> +static int imx8mq_dsi_poweroff(struct nwl_dsi *dsi)
> +{
> +	int ret = 0;
> +
> +	if (dsi->quirks & SRC_RESET_QUIRK)
> +		return 0;
> +
> +	if (dsi->rstc)
> +		ret = reset_control_assert(dsi->rstc);
> +	return ret;
> +}
> +
> +static const struct drm_bridge_timings nwl_dsi_timings = {
> +	.input_bus_flags = DRM_BUS_FLAG_DE_LOW,
> +};
> +
> +static const struct nwl_dsi_platform_data imx8mq_dev = {
> +	.poweron = &imx8mq_dsi_poweron,
> +	.poweroff = &imx8mq_dsi_poweroff,
> +	.select_input = &imx8mq_dsi_select_input,
> +	.deselect_input = &imx8mq_dsi_deselect_input,
> +	.clk_config = {
> +		{ .id = NWL_DSI_CLK_CORE, .present = true },
> +	},
> +};
> +
> +static const struct of_device_id nwl_dsi_dt_ids[] = {
> +	{ .compatible = "fsl,imx8mq-nwl-dsi", .data = &imx8mq_dev, },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, nwl_dsi_dt_ids);
> +
> +static const struct soc_device_attribute nwl_dsi_quirks_match[] = {
> +	{ .soc_id = "i.MX8MQ", .revision = "2.0",
> +	  .data = (void *)(E11418_HS_MODE_QUIRK | SRC_RESET_QUIRK) },
> +	{ /* sentinel. */ },
> +};
> +
> +static int nwl_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *of_id = of_match_device(nwl_dsi_dt_ids, dev);
> +	const struct nwl_dsi_platform_data *pdata = of_id->data;
> +	const struct soc_device_attribute *attr;
> +	struct nwl_dsi *dsi;
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->dev = dev;
> +	dsi->pdata = pdata;
> +
> +	ret = nwl_dsi_parse_dt(dsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_irq(dev, dsi->irq, nwl_dsi_irq_handler, 0,
> +			       dev_name(dev), dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to request IRQ %d: %d\n", dsi->irq,
> +			      ret);
> +		return ret;
> +	}
> +
> +	dsi->dsi_host.ops = &nwl_dsi_host_ops;
> +	dsi->dsi_host.dev = dev;
> +	ret = mipi_dsi_host_register(&dsi->dsi_host);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret);
> +		return ret;
> +	}
> +
> +	attr = soc_device_match(nwl_dsi_quirks_match);
> +	if (attr)
> +		dsi->quirks = (uintptr_t)attr->data;
> +
> +	dsi->bridge.driver_private = dsi;
> +	dsi->bridge.funcs = &nwl_dsi_bridge_funcs;
> +	dsi->bridge.of_node = dev->of_node;
> +	dsi->bridge.timings = &nwl_dsi_timings;
> +
> +	dev_set_drvdata(dev, dsi);
> +	pm_runtime_enable(dev);
> +	return 0;
> +}
> +
> +static int nwl_dsi_remove(struct platform_device *pdev)
> +{
> +	struct nwl_dsi *dsi = platform_get_drvdata(pdev);
> +
> +	mipi_dsi_host_unregister(&dsi->dsi_host);
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
> +}
> +
> +static struct platform_driver nwl_dsi_driver = {
> +	.probe		= nwl_dsi_probe,
> +	.remove		= nwl_dsi_remove,
> +	.driver		= {
> +		.of_match_table = nwl_dsi_dt_ids,
> +		.name	= DRV_NAME,
> +	},
> +};
> +
> +module_platform_driver(nwl_dsi_driver);
> +
> +MODULE_AUTHOR("NXP Semiconductor");
> +MODULE_AUTHOR("Purism SPC");
> +MODULE_DESCRIPTION("Northwest Logic MIPI-DSI driver");
> +MODULE_LICENSE("GPL"); /* GPLv2 or later */
> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.h b/drivers/gpu/drm/bridge/nwl-dsi.h
> new file mode 100644
> index 000000000000..a247a8a11c7c
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.h
> @@ -0,0 +1,144 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * NWL MIPI DSI host driver
> + *
> + * Copyright (C) 2017 NXP
> + * Copyright (C) 2019 Purism SPC
> + */
> +#ifndef __NWL_DSI_H__
> +#define __NWL_DSI_H__
> +
> +/* DSI HOST registers */
> +#define NWL_DSI_CFG_NUM_LANES			0x0
> +#define NWL_DSI_CFG_NONCONTINUOUS_CLK		0x4
> +#define NWL_DSI_CFG_T_PRE			0x8
> +#define NWL_DSI_CFG_T_POST			0xc
> +#define NWL_DSI_CFG_TX_GAP			0x10
> +#define NWL_DSI_CFG_AUTOINSERT_EOTP		0x14
> +#define NWL_DSI_CFG_EXTRA_CMDS_AFTER_EOTP	0x18
> +#define NWL_DSI_CFG_HTX_TO_COUNT		0x1c
> +#define NWL_DSI_CFG_LRX_H_TO_COUNT		0x20
> +#define NWL_DSI_CFG_BTA_H_TO_COUNT		0x24
> +#define NWL_DSI_CFG_TWAKEUP			0x28
> +#define NWL_DSI_CFG_STATUS_OUT			0x2c
> +#define NWL_DSI_RX_ERROR_STATUS			0x30
> +
> +/* DSI DPI registers */
> +#define NWL_DSI_PIXEL_PAYLOAD_SIZE		0x200
> +#define NWL_DSI_PIXEL_FIFO_SEND_LEVEL		0x204
> +#define NWL_DSI_INTERFACE_COLOR_CODING		0x208
> +#define NWL_DSI_PIXEL_FORMAT			0x20c
> +#define NWL_DSI_VSYNC_POLARITY			0x210
> +#define NWL_DSI_VSYNC_POLARITY_ACTIVE_LOW	0
> +#define NWL_DSI_VSYNC_POLARITY_ACTIVE_HIGH	BIT(1)
> +
> +#define NWL_DSI_HSYNC_POLARITY			0x214
> +#define NWL_DSI_HSYNC_POLARITY_ACTIVE_LOW	0
> +#define NWL_DSI_HSYNC_POLARITY_ACTIVE_HIGH	BIT(1)
> +
> +#define NWL_DSI_VIDEO_MODE			0x218
> +#define NWL_DSI_HFP				0x21c
> +#define NWL_DSI_HBP				0x220
> +#define NWL_DSI_HSA				0x224
> +#define NWL_DSI_ENABLE_MULT_PKTS		0x228
> +#define NWL_DSI_VBP				0x22c
> +#define NWL_DSI_VFP				0x230
> +#define NWL_DSI_BLLP_MODE			0x234
> +#define NWL_DSI_USE_NULL_PKT_BLLP		0x238
> +#define NWL_DSI_VACTIVE				0x23c
> +#define NWL_DSI_VC				0x240
> +
> +/* DSI APB PKT control */
> +#define NWL_DSI_TX_PAYLOAD			0x280
> +#define NWL_DSI_PKT_CONTROL			0x284
> +#define NWL_DSI_SEND_PACKET			0x288
> +#define NWL_DSI_PKT_STATUS			0x28c
> +#define NWL_DSI_PKT_FIFO_WR_LEVEL		0x290
> +#define NWL_DSI_PKT_FIFO_RD_LEVEL		0x294
> +#define NWL_DSI_RX_PAYLOAD			0x298
> +#define NWL_DSI_RX_PKT_HEADER			0x29c
> +
> +/* DSI IRQ handling */
> +#define NWL_DSI_IRQ_STATUS			0x2a0
> +#define NWL_DSI_SM_NOT_IDLE			BIT(0)
> +#define NWL_DSI_TX_PKT_DONE			BIT(1)
> +#define NWL_DSI_DPHY_DIRECTION			BIT(2)
> +#define NWL_DSI_TX_FIFO_OVFLW			BIT(3)
> +#define NWL_DSI_TX_FIFO_UDFLW			BIT(4)
> +#define NWL_DSI_RX_FIFO_OVFLW			BIT(5)
> +#define NWL_DSI_RX_FIFO_UDFLW			BIT(6)
> +#define NWL_DSI_RX_PKT_HDR_RCVD			BIT(7)
> +#define NWL_DSI_RX_PKT_PAYLOAD_DATA_RCVD	BIT(8)
> +#define NWL_DSI_BTA_TIMEOUT			BIT(29)
> +#define NWL_DSI_LP_RX_TIMEOUT			BIT(30)
> +#define NWL_DSI_HS_TX_TIMEOUT			BIT(31)
> +
> +#define NWL_DSI_IRQ_STATUS2			0x2a4
> +#define NWL_DSI_SINGLE_BIT_ECC_ERR		BIT(0)
> +#define NWL_DSI_MULTI_BIT_ECC_ERR		BIT(1)
> +#define NWL_DSI_CRC_ERR				BIT(2)
> +
> +#define NWL_DSI_IRQ_MASK			0x2a8
> +#define NWL_DSI_SM_NOT_IDLE_MASK		BIT(0)
> +#define NWL_DSI_TX_PKT_DONE_MASK		BIT(1)
> +#define NWL_DSI_DPHY_DIRECTION_MASK		BIT(2)
> +#define NWL_DSI_TX_FIFO_OVFLW_MASK		BIT(3)
> +#define NWL_DSI_TX_FIFO_UDFLW_MASK		BIT(4)
> +#define NWL_DSI_RX_FIFO_OVFLW_MASK		BIT(5)
> +#define NWL_DSI_RX_FIFO_UDFLW_MASK		BIT(6)
> +#define NWL_DSI_RX_PKT_HDR_RCVD_MASK		BIT(7)
> +#define NWL_DSI_RX_PKT_PAYLOAD_DATA_RCVD_MASK	BIT(8)
> +#define NWL_DSI_BTA_TIMEOUT_MASK		BIT(29)
> +#define NWL_DSI_LP_RX_TIMEOUT_MASK		BIT(30)
> +#define NWL_DSI_HS_TX_TIMEOUT_MASK		BIT(31)
> +
> +#define NWL_DSI_IRQ_MASK2			0x2ac
> +#define NWL_DSI_SINGLE_BIT_ECC_ERR_MASK		BIT(0)
> +#define NWL_DSI_MULTI_BIT_ECC_ERR_MASK		BIT(1)
> +#define NWL_DSI_CRC_ERR_MASK			BIT(2)
> +
> +/*
> + * PKT_CONTROL format:
> + * [15: 0] - word count
> + * [17:16] - virtual channel
> + * [23:18] - data type
> + * [24]	   - LP or HS select (0 - LP, 1 - HS)
> + * [25]	   - perform BTA after packet is sent
> + * [26]	   - perform BTA only, no packet tx
> + */
> +#define NWL_DSI_WC(x)		FIELD_PREP(GENMASK(15, 0), (x))
> +#define NWL_DSI_TX_VC(x)	FIELD_PREP(GENMASK(17, 16), (x))
> +#define NWL_DSI_TX_DT(x)	FIELD_PREP(GENMASK(23, 18), (x))
> +#define NWL_DSI_HS_SEL(x)	FIELD_PREP(GENMASK(24, 24), (x))
> +#define NWL_DSI_BTA_TX(x)	FIELD_PREP(GENMASK(25, 25), (x))
> +#define NWL_DSI_BTA_NO_TX(x)	FIELD_PREP(GENMASK(26, 26), (x))
> +
> +/*
> + * RX_PKT_HEADER format:
> + * [15: 0] - word count
> + * [21:16] - data type
> + * [23:22] - virtual channel
> + */
> +#define NWL_DSI_RX_DT(x)	FIELD_GET(GENMASK(21, 16), (x))
> +#define NWL_DSI_RX_VC(x)	FIELD_GET(GENMASK(23, 22), (x))
> +
> +/* DSI Video mode */
> +#define NWL_DSI_VM_BURST_MODE_WITH_SYNC_PULSES		0
> +#define NWL_DSI_VM_NON_BURST_MODE_WITH_SYNC_EVENTS	BIT(0)
> +#define NWL_DSI_VM_BURST_MODE				BIT(1)
> +
> +/* * DPI color coding */
> +#define NWL_DSI_DPI_16_BIT_565_PACKED	0
> +#define NWL_DSI_DPI_16_BIT_565_ALIGNED	1
> +#define NWL_DSI_DPI_16_BIT_565_SHIFTED	2
> +#define NWL_DSI_DPI_18_BIT_PACKED	3
> +#define NWL_DSI_DPI_18_BIT_ALIGNED	4
> +#define NWL_DSI_DPI_24_BIT		5
> +
> +/* * DPI Pixel format */
> +#define NWL_DSI_PIXEL_FORMAT_16  0
> +#define NWL_DSI_PIXEL_FORMAT_18  BIT(0)
> +#define NWL_DSI_PIXEL_FORMAT_18L BIT(1)
> +#define NWL_DSI_PIXEL_FORMAT_24  (BIT(0) | BIT(1))
> +
> +#endif /* __NWL_DSI_H__ */



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 0/6] add the DDR clock controller on Meson8 and Meson8b
From: Jerome Brunet @ 2019-09-23 10:06 UTC (permalink / raw)
  To: Martin Blumenstingl, narmstrong, robh+dt, mark.rutland,
	linux-amlogic, devicetree, khilman
  Cc: Martin Blumenstingl, linux-kernel, linux-arm-kernel, linux-clk
In-Reply-To: <20190921151835.770263-1-martin.blumenstingl@googlemail.com>

On Sat 21 Sep 2019 at 17:18, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Meson8 and Meson8b SoCs embed a DDR clock controller in their MMCBUS
> registers. This series:
> - adds support for this DDR clock controller (patches 0 and 1)
> - wires up the DDR PLL as input for two audio clocks (patches 2 and 3)

Have you been able to validate somehow that DDR rate calculated by CCF
is the actual rate that gets to the audio clocks ?

While I understand the interest for completeness, I suspect the having
the DDR clock as an audio parent was just for debugging purpose. IOW,
I'm not sure if adding this parent is useful to an actual audio use
case. As far as audio would be concerned, I think we are better of
without this parent.

> - adds the DDR clock controller to meson8.dtsi and meson8b.dtsi
>

Could you please separate the driver and DT series in the future ? Those
take different paths and are meant for different maintainers.

> Special thanks go out to Alexandre Mergnat for switching the Amlogic
> clock drivers over to parent_hws and parent_data. That made this series
> a lot easier for me!
>
> This series depends on my other series from [0]:
> "provide the XTAL clock via OF on Meson8/8b/8m2"
>
>
> [0] https://patchwork.kernel.org/cover/11155515/
>
>
> Martin Blumenstingl (6):
>   dt-bindings: clock: add the Amlogic Meson8 DDR clock controller
>     binding
>   clk: meson: add a driver for the Meson8/8b/8m2 DDR clock controller
>   clk: meson: meson8b: use of_clk_hw_register to register the clocks
>   clk: meson: meson8b: add the ddr_pll input for the audio clocks
>   ARM: dts: meson8: add the DDR clock controller
>   ARM: dts: meson8b: add the DDR clock controller
>
>  .../clock/amlogic,meson8-ddr-clkc.yaml        |  50 ++++++
>  arch/arm/boot/dts/meson8.dtsi                 |  13 +-
>  arch/arm/boot/dts/meson8b.dtsi                |  13 +-
>  drivers/clk/meson/Makefile                    |   2 +-
>  drivers/clk/meson/meson8-ddr.c                | 153 ++++++++++++++++++
>  drivers/clk/meson/meson8b.c                   |  36 ++---
>  include/dt-bindings/clock/meson8-ddr-clkc.h   |   4 +
>  7 files changed, 245 insertions(+), 26 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,meson8-ddr-clkc.yaml
>  create mode 100644 drivers/clk/meson/meson8-ddr.c
>  create mode 100644 include/dt-bindings/clock/meson8-ddr-clkc.h
>
> -- 
> 2.23.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 03/10] ASoC: wm8994: Add support for setting MCLK clock rate
From: Krzysztof Kozlowski @ 2019-09-23 10:09 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: devicetree, alsa-devel, linux-samsung-soc, ckeepax, b.zolnierkie,
	sbkim73, patches, lgirdwood, robh+dt, broonie, linux-arm-kernel,
	m.szyprowski
In-Reply-To: <7334ce45-f192-4421-aa3d-d142582153ef@samsung.com>

On Mon, Sep 23, 2019 at 11:10:48AM +0200, Sylwester Nawrocki wrote:
> On 9/20/19 15:02, Sylwester Nawrocki wrote:
> > Extend the set_sysclk() handler so we also set frequency of the MCLK1,
> > MCLK2 clocks through clk API when those clocks are specified in DT.
> > 
> > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Sorry, I squashed other patch to this one but forgot to remove above tags, 
> not sure if those still stand as there was rather significant change in 
> the patch. 

It's good. For the record:

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] arm64: use generic free_initrd_mem()
From: Anshuman Khandual @ 2019-09-23 10:11 UTC (permalink / raw)
  To: Mike Rapoport, Laura Abbott
  Cc: Mark Rutland, linux-arch, Catalin Marinas, linux-kernel,
	Mike Rapoport, Will Deacon, linux-arm-kernel
In-Reply-To: <20190916135542.GC5196@rapoport-lnx>



On 09/16/2019 07:25 PM, Mike Rapoport wrote:
> (added linux-arch)
> 
> On Mon, Sep 16, 2019 at 08:23:29AM -0400, Laura Abbott wrote:
>> On 9/16/19 8:21 AM, Mike Rapoport wrote:
>>> From: Mike Rapoport <rppt@linux.ibm.com>
>>>
>>> arm64 calls memblock_free() for the initrd area in its implementation of
>>> free_initrd_mem(), but this call has no actual effect that late in the boot
>>> process. By the time initrd is freed, all the reserved memory is managed by
>>> the page allocator and the memblock.reserved is unused, so there is no
>>> point to update it.
>>>
>>
>> People like to use memblock for keeping track of memory even if it has no
>> actual effect. We made this change explicitly (see 05c58752f9dc ("arm64: To remove
>> initrd reserved area entry from memblock") That said, moving to the generic
>> APIs would be nice. Maybe we can find another place to update the accounting?
> 
> Any other place in arch/arm64 would make it messy because it would have to
> duplicate keepinitrd logic.
> 
> We could put the memblock_free() in the generic free_initrd_mem() with
> something like:
> 
> diff --git a/init/initramfs.c b/init/initramfs.c
> index c47dad0..403c6a0 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -531,6 +531,10 @@ void __weak free_initrd_mem(unsigned long start,
> unsigned long end)
>  {
>         free_reserved_area((void *)start, (void *)end, POISON_FREE_INITMEM,
>                         "initrd");
> +
> +#ifdef CONFIG_ARCH_KEEP_MEMBLOCK
> +       memblock_free(__virt_to_phys(start), end - start);
> +#endif
>  }

This makes sense.

>  
>  #ifdef CONFIG_KEXEC_CORE
> 
> 
> Then powerpc and s390 folks will also be able to track the initrd memory :)

Sure.

> 
>>> Without the memblock_free() call the only difference between arm64 and the
>>> generic versions of free_initrd_mem() is the memory poisoning. Switching
>>> arm64 to the generic version will enable the poisoning.
>>>
>>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
>>> ---
>>>
>>> I've boot tested it on qemu and I've checked that kexec works.
>>>
>>>  arch/arm64/mm/init.c | 8 --------
>>>  1 file changed, 8 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>>> index f3c7952..8ad2934 100644
>>> --- a/arch/arm64/mm/init.c
>>> +++ b/arch/arm64/mm/init.c
>>> @@ -567,14 +567,6 @@ void free_initmem(void)
>>>  	unmap_kernel_range((u64)__init_begin, (u64)(__init_end - __init_begin));
>>>  }
>>> -#ifdef CONFIG_BLK_DEV_INITRD
>>> -void __init free_initrd_mem(unsigned long start, unsigned long end)
>>> -{
>>> -	free_reserved_area((void *)start, (void *)end, 0, "initrd");
>>> -	memblock_free(__virt_to_phys(start), end - start);
>>> -}
>>> -#endif
>>> -
>>>  /*
>>>   * Dump out memory limit information on panic.
>>>   */
>>>
>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: printk() + memory offline deadlock (WAS Re: page_alloc.shuffle=1 + CONFIG_PROVE_LOCKING=y = arm64 hang)
From: Sergey Senozhatsky @ 2019-09-23 10:21 UTC (permalink / raw)
  To: Qian Cai
  Cc: Petr Mladek, Theodore Ts'o, Sergey Senozhatsky, Arnd Bergmann,
	linux-mm, Peter Zijlstra, Catalin Marinas, linux-kernel,
	Steven Rostedt, Sergey Senozhatsky, Greg Kroah-Hartman,
	Waiman Long, Dan Williams, Will Deacon, Thomas Gleixner,
	linux-arm-kernel
In-Reply-To: <1568823006.5576.178.camel@lca.pw>

On (09/18/19 12:10), Qian Cai wrote:
[..]
> > So you have debug objects enabled. Right? This thing does not behave
> > when it comes to printing. debug_objects are slightly problematic.
> 
> Yes, but there is an also a similar splat without the debug_objects. It looks
> like anything try to allocate memory in that path will trigger it anyway.

Appears to be different, yet somehow very familiar.

> [  297.425908] WARNING: possible circular locking dependency detected
> [  297.425908] 5.3.0-next-20190917 #8 Not tainted
> [  297.425909] ------------------------------------------------------
> [  297.425910] test.sh/8653 is trying to acquire lock:
> [  297.425911] ffffffff865a4460 (console_owner){-.-.}, at:
> console_unlock+0x207/0x750
> 
> [  297.425914] but task is already holding lock:
> [  297.425915] ffff88883fff3c58 (&(&zone->lock)->rlock){-.-.}, at:
> __offline_isolated_pages+0x179/0x3e0
> 
> [  297.425919] which lock already depends on the new lock.
> 
> 
> [  297.425920] the existing dependency chain (in reverse order) is:
> 
> [  297.425922] -> #3 (&(&zone->lock)->rlock){-.-.}:
> [  297.425925]        __lock_acquire+0x5b3/0xb40
> [  297.425925]        lock_acquire+0x126/0x280
> [  297.425926]        _raw_spin_lock+0x2f/0x40
> [  297.425927]        rmqueue_bulk.constprop.21+0xb6/0x1160
> [  297.425928]        get_page_from_freelist+0x898/0x22c0
> [  297.425928]        __alloc_pages_nodemask+0x2f3/0x1cd0
> [  297.425929]        alloc_pages_current+0x9c/0x110
> [  297.425930]        allocate_slab+0x4c6/0x19c0
> [  297.425931]        new_slab+0x46/0x70
> [  297.425931]        ___slab_alloc+0x58b/0x960
> [  297.425932]        __slab_alloc+0x43/0x70
> [  297.425933]        __kmalloc+0x3ad/0x4b0
> [  297.425933]        __tty_buffer_request_room+0x100/0x250
> [  297.425934]        tty_insert_flip_string_fixed_flag+0x67/0x110
> [  297.425935]        pty_write+0xa2/0xf0
> [  297.425936]        n_tty_write+0x36b/0x7b0
> [  297.425936]        tty_write+0x284/0x4c0
> [  297.425937]        __vfs_write+0x50/0xa0
> [  297.425938]        vfs_write+0x105/0x290
> [  297.425939]        redirected_tty_write+0x6a/0xc0
> [  297.425939]        do_iter_write+0x248/0x2a0
> [  297.425940]        vfs_writev+0x106/0x1e0
> [  297.425941]        do_writev+0xd4/0x180
> [  297.425941]        __x64_sys_writev+0x45/0x50
> [  297.425942]        do_syscall_64+0xcc/0x76c
> [  297.425943]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> [  297.425944] -> #2 (&(&port->lock)->rlock){-.-.}:
> [  297.425946]        __lock_acquire+0x5b3/0xb40
> [  297.425947]        lock_acquire+0x126/0x280
> [  297.425948]        _raw_spin_lock_irqsave+0x3a/0x50
> [  297.425949]        tty_port_tty_get+0x20/0x60
> [  297.425949]        tty_port_default_wakeup+0xf/0x30
> [  297.425950]        tty_port_tty_wakeup+0x39/0x40
> [  297.425951]        uart_write_wakeup+0x2a/0x40
> [  297.425952]        serial8250_tx_chars+0x22e/0x440
> [  297.425952]        serial8250_handle_irq.part.8+0x14a/0x170
> [  297.425953]        serial8250_default_handle_irq+0x5c/0x90
> [  297.425954]        serial8250_interrupt+0xa6/0x130
> [  297.425955]        __handle_irq_event_percpu+0x78/0x4f0
> [  297.425955]        handle_irq_event_percpu+0x70/0x100
> [  297.425956]        handle_irq_event+0x5a/0x8b
> [  297.425957]        handle_edge_irq+0x117/0x370
> [  297.425958]        do_IRQ+0x9e/0x1e0
> [  297.425958]        ret_from_intr+0x0/0x2a
> [  297.425959]        cpuidle_enter_state+0x156/0x8e0
> [  297.425960]        cpuidle_enter+0x41/0x70
> [  297.425960]        call_cpuidle+0x5e/0x90
> [  297.425961]        do_idle+0x333/0x370
> [  297.425962]        cpu_startup_entry+0x1d/0x1f
> [  297.425962]        start_secondary+0x290/0x330
> [  297.425963]        secondary_startup_64+0xb6/0xc0
> 
> [  297.425964] -> #1 (&port_lock_key){-.-.}:
> [  297.425967]        __lock_acquire+0x5b3/0xb40
> [  297.425967]        lock_acquire+0x126/0x280
> [  297.425968]        _raw_spin_lock_irqsave+0x3a/0x50
> [  297.425969]        serial8250_console_write+0x3e4/0x450
> [  297.425970]        univ8250_console_write+0x4b/0x60
> [  297.425970]        console_unlock+0x501/0x750
> [  297.425971]        vprintk_emit+0x10d/0x340
> [  297.425972]        vprintk_default+0x1f/0x30
> [  297.425972]        vprintk_func+0x44/0xd4
> [  297.425973]        printk+0x9f/0xc5
> [  297.425974]        register_console+0x39c/0x520
> [  297.425975]        univ8250_console_init+0x23/0x2d
> [  297.425975]        console_init+0x338/0x4cd
> [  297.425976]        start_kernel+0x534/0x724
> [  297.425977]        x86_64_start_reservations+0x24/0x26
> [  297.425977]        x86_64_start_kernel+0xf4/0xfb
> [  297.425978]        secondary_startup_64+0xb6/0xc0
> 
> [  297.425979] -> #0 (console_owner){-.-.}:
> [  297.425982]        check_prev_add+0x107/0xea0
> [  297.425982]        validate_chain+0x8fc/0x1200
> [  297.425983]        __lock_acquire+0x5b3/0xb40
> [  297.425984]        lock_acquire+0x126/0x280
> [  297.425984]        console_unlock+0x269/0x750
> [  297.425985]        vprintk_emit+0x10d/0x340
> [  297.425986]        vprintk_default+0x1f/0x30
> [  297.425987]        vprintk_func+0x44/0xd4
> [  297.425987]        printk+0x9f/0xc5
> [  297.425988]        __offline_isolated_pages.cold.52+0x2f/0x30a
> [  297.425989]        offline_isolated_pages_cb+0x17/0x30
> [  297.425990]        walk_system_ram_range+0xda/0x160
> [  297.425990]        __offline_pages+0x79c/0xa10
> [  297.425991]        offline_pages+0x11/0x20
> [  297.425992]        memory_subsys_offline+0x7e/0xc0
> [  297.425992]        device_offline+0xd5/0x110
> [  297.425993]        state_store+0xc6/0xe0
> [  297.425994]        dev_attr_store+0x3f/0x60
> [  297.425995]        sysfs_kf_write+0x89/0xb0
> [  297.425995]        kernfs_fop_write+0x188/0x240
> [  297.425996]        __vfs_write+0x50/0xa0
> [  297.425997]        vfs_write+0x105/0x290
> [  297.425997]        ksys_write+0xc6/0x160
> [  297.425998]        __x64_sys_write+0x43/0x50
> [  297.425999]        do_syscall_64+0xcc/0x76c
> [  297.426000]        entry_SYSCALL_64_after_hwframe+0x49/0xbe

I suppose you run with CONFIG_DEBUG_VM...

So we have

port->lock -> MM -> zone->lock
	// from pty_write()->__tty_buffer_request_room()->kmalloc()

vs

zone->lock -> printk() -> port->lock
	// from __offline_pages()->__offline_isolated_pages()->printk()


A number of debugging options make the kernel less stable.
Sad but true.

	-ss

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: Propagating audio properties along the audio path
From: Marc Gonzalez @ 2019-09-23 10:47 UTC (permalink / raw)
  To: alsa-devel; +Cc: Takashi Iwai, Linux ARM, Jaroslav Kysela
In-Reply-To: <d67b68b9-49c0-6f78-4649-27b3b437a65f@free.fr>

On 20/09/2019 11:50, Marc Gonzalez wrote:

> One more concern popped up: if the audio stream changes mid-capture
> (for example, a different TV program uses different audio settings),
> then I would detect this in the eARC receiver, but it's not clear
> (to me) how to propagate the info to the DSP...
> 
> I'm not even sure when the HW params actually get applied...
> Is it for SNDRV_PCM_IOCTL_PREPARE? SNDRV_PCM_IOCTL_START?

I enabled debug logs in the sound layer:
echo "file sound/* +fpm" > /sys/kernel/debug/dynamic_debug/control

and sprinkled dump_stack() in several driver callbacks.

When I run 'tinycap /tmp/earc.wav -t 10 -r 44100 -b 32'
I see the open/SyS_openat call and the capture ioctl call
which together generate calls to
1) dpcm_fe_dai_open
2) dpcm_fe_dai_hw_params
3) dpcm_fe_dai_prepare
4) dpcm_fe_dai_trigger

But everything looks "synchronous", as in "reaction to user-space commands".
I don't see how "asynchronous" events are dealt with, such as the stream
params changing while a capture is active?

Regards.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 0/3] iommu/io-pgtable-arm: Mali LPAE improvements
From: Robin Murphy @ 2019-09-23 11:04 UTC (permalink / raw)
  To: Tomeu Vizoso, Will Deacon
  Cc: Rob Herring, Neil Armstrong, Joerg Roedel, Steven Price,
	Linux IOMMU,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAAObsKCja0xTaiayNq5TWE7YJ0RPiJbq5OqOdEtpKdCQivR0uw@mail.gmail.com>

Hi Tomeu,

On 23/09/2019 09:17, Tomeu Vizoso wrote:
>>> There is some argument for taking #1 and #2 as 5.4 fixes, though - the
>>> upcoming Mesa 19.2 release will enable T820 support on the userspace side -
>>> so let's pick that discussion up again in a few weeks.
>>
>> Ok, I'll include those two in my fixes pull to Joerg at -rc1.
> 
> Hi Will,
> 
> Looks like this didn't end up happening?

Don't panic, there's still another week until rc1 is even out to base a 
PR on ;)

FWIW I was planning to rebase and resend these with the review tags 
included early next week, once the dust has settled and queues are 
opening again.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ 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