* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs
From: Alexander Stein @ 2024-03-26 7:14 UTC (permalink / raw)
To: Laurent Pinchart
Cc: imx, linux-arm-kernel, Paul Elder, Adam Ford, Conor Dooley,
Fabio Estevam, Krzysztof Kozlowski, Marek Vasut,
Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo,
devicetree, linux-media
In-Reply-To: <20240325204924.GY18799@pendragon.ideasonboard.com>
Hi Laurent,
Am Montag, 25. März 2024, 21:49:24 CET schrieb Laurent Pinchart:
> Hi Alexander,
>
> On Mon, Mar 25, 2024 at 04:52:21PM +0100, Alexander Stein wrote:
> > Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart:
> > > From: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > The ISP supports both CSI and parallel interfaces, where port 0
> > > corresponds to the former and port 1 corresponds to the latter. Since
> > > the i.MX8MP's ISPs are connected by the parallel interface to the CSI
> > > receiver, set them both to port 1.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - Fix clock ordering
> > > - Add #address-cells and #size-cells to ports nodes
> > > ---
> > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++
> > > 1 file changed, 50 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > index bfc5c81a5bd4..1d2670b91b53 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint {
> > > };
> > > };
> > >
> > > + isp_0: isp@32e10000 {
> > > + compatible = "fsl,imx8mp-isp";
> > > + reg = <0x32e10000 0x10000>;
> > > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>,
> > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>,
> > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>;
> > > + clock-names = "isp", "aclk", "hclk";
> > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>;
> > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
> > > + assigned-clock-rates = <500000000>;
> > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>;
> > > + fsl,blk-ctrl = <&media_blk_ctrl 0>;
> > > + status = "disabled";
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@1 {
> > > + reg = <1>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + isp_1: isp@32e20000 {
> > > + compatible = "fsl,imx8mp-isp";
> > > + reg = <0x32e20000 0x10000>;
> > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>,
> > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>,
> > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>;
> > > + clock-names = "isp", "aclk", "hclk";
> > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>;
> > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
> > > + assigned-clock-rates = <500000000>;
> > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>;
> > > + fsl,blk-ctrl = <&media_blk_ctrl 1>;
> > > + status = "disabled";
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@1 {
> > > + reg = <1>;
> > > + };
> > > + };
> > > + };
> > > +
> >
> > The patch itself is okay. But you might not be able to
> > configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before.
> > This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp'
> > power domain. Reparenting is not possible anymore in this case.
>
> Good point.
>
> > Something like
> > ---8<---
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
> > <&clk IMX8MP_CLK_MEDIA_APB>,
> > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
> > <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
> > + <&clk IMX8MP_CLK_MEDIA_ISP>,
> > <&clk IMX8MP_VIDEO_PLL1>;
> > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
> > <&clk IMX8MP_SYS_PLL1_800M>,
> > <&clk IMX8MP_VIDEO_PLL1_OUT>,
> > - <&clk IMX8MP_VIDEO_PLL1_OUT>;
> > + <&clk IMX8MP_VIDEO_PLL1_OUT>,
> > + <&clk IMX8MP_SYS_PLL2_500M>;
> > assigned-clock-rates = <500000000>, <200000000>,
> > <0>, <0>, <1039500000>;
>
> With an assigned clock rate here too then ?
You are right. This posted diff is what I was using for a while now though.
Apparently the clock frequency was still correct.
Best regards,
Alexander
> > #power-domain-cells = <1>;
> > ---8<---
> > is needed.
>
> Sascha, are you OK with this approach ?
>
> > > dewarp: dwe@32e30000 {
> > > compatible = "nxp,imx8mp-dw100";
> > > reg = <0x32e30000 0x10000>;
> > >
> > > base-commit: 4cece764965020c22cff7665b18a012006359095
>
>
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
^ permalink raw reply
* Re: [PATCH 5/5] arm64: dts: Add device tree source for the Au-Zone Maivin Starter Kit
From: Krzysztof Kozlowski @ 2024-03-26 7:12 UTC (permalink / raw)
To: Laurent Pinchart, devicetree, imx, linux-arm-kernel
Cc: Trevor Zaharichuk, Greg Lytle, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
In-Reply-To: <20240325203245.31660-6-laurent.pinchart@ideasonboard.com>
On 25/03/2024 21:32, Laurent Pinchart wrote:
> The Maivin board is an AI vision starter kit sold by Au-Zone
> Technologies, developed in collaboration with Toradex and Vision
> Components. It is based on a Toradex Verdin i.MX8MP SoM.
>
> Add a device tree that covers the base set the peripherals found on the
> board:
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
And drop redundant parts.
arm64: dts: imx8mp-maivin: Add Au-Zone Maivin Starter Kit board
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 4/5] dt-bindings: arm: Add Au-Zone Maivin AI Vision Starter Kit
From: Krzysztof Kozlowski @ 2024-03-26 7:11 UTC (permalink / raw)
To: Laurent Pinchart, devicetree, imx, linux-arm-kernel
Cc: Trevor Zaharichuk, Greg Lytle, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shawn Guo, Li Yang
In-Reply-To: <20240325203245.31660-5-laurent.pinchart@ideasonboard.com>
On 25/03/2024 21:32, Laurent Pinchart wrote:
> The Maivin board is an AI vision starter kit sold by Au-Zone
> Technologies, developed in collaboration with Toradex and Vision
> Components. It is based on a Toradex Verdin i.MX8MP SoM.
>
> Add a corresponding compatible string.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> index 0027201e19f8..d892c4f9fda3 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> @@ -1064,6 +1064,13 @@ properties:
> - toradex,verdin-imx8mp-wifi # Verdin iMX8M Plus Wi-Fi / BT Modules
> - const: fsl,imx8mp
>
> + - description: Au-Zone Technologies i.MX8MP-based boards
> + items:
> + - const: au-zone,maivin-starter-kit # Au-Zone Maivin AI Vision Starter Kit
> + - const: toradex,verdin-imx8mp-nonwifi # Verdin iMX8M Plus Module without Wi-Fi / BT
> + - const: toradex,verdin-imx8mp # Verdin iMX8M Plus Module
I think this should be part of existing "Toradex Boards with Verdin
iMX8M Plus Modules)", just renamed to "boards using Toradex Verdin ...".
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 3/5] dt-bindings: vendor-prefix: Add prefix for Au-Zone Technologies
From: Krzysztof Kozlowski @ 2024-03-26 7:10 UTC (permalink / raw)
To: Laurent Pinchart, devicetree, imx, linux-arm-kernel
Cc: Trevor Zaharichuk, Greg Lytle, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
In-Reply-To: <20240325203245.31660-4-laurent.pinchart@ideasonboard.com>
On 25/03/2024 21:32, Laurent Pinchart wrote:
> Au-Zone Technologies is a company that designs and manufactures products
> for AI vision applications. Add a DT vendor prefix for it.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 2/2] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add gpio child node
From: Krzysztof Kozlowski @ 2024-03-26 7:09 UTC (permalink / raw)
To: Laurent Pinchart, devicetree, linux-rpi-kernel, linux-arm-kernel,
linux-gpio
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Florian Fainelli,
Broadcom internal kernel review list, Ray Jui, Scott Branden,
Linus Walleij, Bartosz Golaszewski, Eric Anholt, Stefan Wahren
In-Reply-To: <20240326004902.17054-3-laurent.pinchart@ideasonboard.com>
On 26/03/2024 01:49, Laurent Pinchart wrote:
> Unlike the other child nodes of the raspberrypi,bcm2835-firmware device,
> the gpio child is documented in a legacy text-based binding in
> gpio/raspberrypi,firmware-gpio.txt. This causes DT validation failures:
> + type: object
> + additionalProperties: false
> +
> + properties:
> + compatible:
> + const: raspberrypi,firmware-gpio
> +
> + gpio-controller: true
> +
> + "#gpio-cells":
> + const: 2
> + description:
> + The first cell is the pin number, and the second cell is used to
> + specify the gpio polarity (GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW).
> +
> + gpio-line-names: true
You could provide here maxItems, if this is known, but it's fine as is
as well.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties
From: Krzysztof Kozlowski @ 2024-03-26 7:06 UTC (permalink / raw)
To: Laurent Pinchart, devicetree, linux-rpi-kernel, linux-arm-kernel,
linux-gpio
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Florian Fainelli,
Broadcom internal kernel review list, Ray Jui, Scott Branden,
Linus Walleij, Bartosz Golaszewski, Eric Anholt, Stefan Wahren
In-Reply-To: <20240326004902.17054-2-laurent.pinchart@ideasonboard.com>
On 26/03/2024 01:49, Laurent Pinchart wrote:
> The raspberrypi,bcm2835-firmware devices requires a dma-ranges property,
> and, as a result, also needs to specify #address-cells and #size-cells.
> Those properties have been added to thebcm2835-rpi.dtsi in commits
> be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware
> bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA
> limitations"), but the DT bindings haven't been updated, resulting in
> validation errors:
>
> arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+'
> from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml#
>
> Fix this by adding the properties to the bindings.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Children do not perform any IO on their own, because everything is
handled by parent. It is really odd to see dma-ranges without ranges.
Referenced commits might be also wrong.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v6 1/2] driver core: Introduce device_link_wait_removal()
From: Nuno Sá @ 2024-03-26 7:05 UTC (permalink / raw)
To: Greg Kroah-Hartman, Herve Codina
Cc: Rafael J. Wysocki, Rob Herring, Frank Rowand, Saravana Kannan,
Lizhi Hou, Max Zhen, Sonal Santan, Stefano Stabellini,
Jonathan Cameron, linux-kernel, devicetree, Allan Nielsen,
Horatiu Vultur, Steen Hegelund, Luca Ceresoli, Nuno Sa,
Thomas Petazzoni, stable
In-Reply-To: <2024032554-tipoff-extrude-a631@gregkh>
On Mon, 2024-03-25 at 19:38 +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 25, 2024 at 04:21:25PM +0100, Herve Codina wrote:
> > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > introduces a workqueue to release the consumer and supplier devices used
> > in the devlink.
> > In the job queued, devices are release and in turn, when all the
> > references to these devices are dropped, the release function of the
> > device itself is called.
> >
> > Nothing is present to provide some synchronisation with this workqueue
> > in order to ensure that all ongoing releasing operations are done and
> > so, some other operations can be started safely.
> >
> > For instance, in the following sequence:
> > 1) of_platform_depopulate()
> > 2) of_overlay_remove()
>
> So this is only an issue for overlays? Why has no one noticed this in
> the years since 80dd33cf72d1 was added? Why is this an issue now
> suddenly?
>
Not sure either... Note this is only an issue if device links are in place. So the
overlay needs to have nodes creating those links. You need to have regulators, pwm,
eth phy (at least these ones I'm aware they create links) to trigger this. We would
have to dig through git to understand when would this be noticeable. But note this is
very straight to trigger.
May also very well be that most people don't really "play" with overlay removal...
For example, I have been dealing with overlays on rpi's for the last 5 years and only
noticed this last year when we had an usecase that involved overlay removal.
- Nuno Sá
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: usb: dwc2: Add support for Sophgo CV18XX/SG200X series SoC
From: Krzysztof Kozlowski @ 2024-03-26 7:01 UTC (permalink / raw)
To: Inochi Amaoto, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Minas Harutyunyan,
Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Chen Wang, Liu Gui, linux-usb, devicetree, linux-kernel,
linux-riscv
In-Reply-To: <IA1PR20MB49530A43A81CF4B809DBC2F8BB352@IA1PR20MB4953.namprd20.prod.outlook.com>
On 26/03/2024 03:37, Inochi Amaoto wrote:
> Add compatible string for the DWC2 IP which is used by
> Sophgo CV18XX/SG2000 series SoC.
>
> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v4 1/1] dt-bindings: net: starfive,jh7110-dwmac: Add StarFive JH8100 support
From: Tan Chun Hau @ 2024-03-26 5:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Emil Renner Berthing, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Simon Horman, Bartosz Golaszewski,
Andrew Halaney, Jisheng Zhang, Uwe Kleine-König,
Russell King
Cc: Ley Foon Tan, Jee Heng Sia, netdev, devicetree, linux-kernel,
linux-stm32, linux-arm-kernel, linux-riscv
In-Reply-To: <20240326052505.197408-1-chunhau.tan@starfivetech.com>
Add StarFive JH8100 dwmac support.
The JH8100 dwmac shares the same driver code as the JH7110 dwmac
and has only one reset signal.
Please refer to below:
JH8100: reset-names = "stmmaceth";
JH7110: reset-names = "stmmaceth", "ahb";
JH7100: reset-names = "ahb";
Example usage of JH8100 in the device tree:
gmac0: ethernet@16030000 {
compatible = "starfive,jh8100-dwmac",
"starfive,jh7110-dwmac",
"snps,dwmac-5.20";
...
};
Signed-off-by: Tan Chun Hau <chunhau.tan@starfivetech.com>
---
.../devicetree/bindings/net/snps,dwmac.yaml | 1 +
.../bindings/net/starfive,jh7110-dwmac.yaml | 54 ++++++++++++++-----
2 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 6b0341a8e0ea..a6d596b7dcf4 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -97,6 +97,7 @@ properties:
- snps,dwxgmac-2.10
- starfive,jh7100-dwmac
- starfive,jh7110-dwmac
+ - starfive,jh8100-dwmac
reg:
minItems: 1
diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
index 0d1962980f57..ce018e9768d2 100644
--- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
@@ -18,6 +18,7 @@ select:
enum:
- starfive,jh7100-dwmac
- starfive,jh7110-dwmac
+ - starfive,jh8100-dwmac
required:
- compatible
@@ -30,6 +31,10 @@ properties:
- items:
- const: starfive,jh7110-dwmac
- const: snps,dwmac-5.20
+ - items:
+ - const: starfive,jh8100-dwmac
+ - const: starfive,jh7110-dwmac
+ - const: snps,dwmac-5.20
reg:
maxItems: 1
@@ -107,20 +112,41 @@ allOf:
contains:
const: starfive,jh7110-dwmac
then:
- properties:
- interrupts:
- minItems: 3
- maxItems: 3
-
- interrupt-names:
- minItems: 3
- maxItems: 3
-
- resets:
- minItems: 2
-
- reset-names:
- minItems: 2
+ if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh8100-dwmac
+ then:
+ properties:
+ interrupts:
+ minItems: 3
+ maxItems: 3
+
+ interrupt-names:
+ minItems: 3
+ maxItems: 3
+
+ resets:
+ maxItems: 1
+
+ reset-names:
+ const: stmmaceth
+ else:
+ properties:
+ interrupts:
+ minItems: 3
+ maxItems: 3
+
+ interrupt-names:
+ minItems: 3
+ maxItems: 3
+
+ resets:
+ minItems: 2
+
+ reset-names:
+ minItems: 2
unevaluatedProperties: false
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC
From: Krzysztof Kozlowski @ 2024-03-26 6:57 UTC (permalink / raw)
To: Inochi Amaoto, Vinod Koul, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen Wang, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Jisheng Zhang, Liu Gui, Jingbao Qiu, dlan, dmaengine, devicetree,
linux-kernel, linux-riscv
In-Reply-To: <IA1PR20MB4953E2937788D9D92C91ABBBBB352@IA1PR20MB4953.namprd20.prod.outlook.com>
On 26/03/2024 02:47, Inochi Amaoto wrote:
> The DMA IP of Sophgo CV18XX/SG200X is based on a DW AXI CORE, with
> an additional channel remap register located in the top system control
> area. The DMA channel is exclusive to each core.
>
> Add the dmamux binding for CV18XX/SG200X series SoC
>
> +
> +allOf:
> + - $ref: dma-router.yaml#
> +
> +properties:
> + compatible:
> + const: sophgo,cv1800-dmamux
> +
> + reg:
> + items:
> + - description: DMA channal remapping register
> + - description: DMA channel interrupt mapping register
> +
> + '#dma-cells':
> + const: 2
> + description:
> + The first cells is device id. The second one is the cpu id.
> +
> + dma-masters:
> + maxItems: 1
> +
> + dma-requests:
> + const: 8
If this is const, why do you need it in the DTS in the first place?
compatible defines it.
> +
> +required:
> + - '#dma-cells'
> + - dma-masters
> +
I don't understand what happened here. Previously you had a child and I
proposed to properly describe it with $ref.
Now, all children are gone. Binding is supposed to be complete. Based on
your cover letter, this is not complete, but why? What is missing and
why it cannot be added?
> +additionalProperties: false
> +
> +examples:
> + - |
> + dma-router {
> + compatible = "sophgo,cv1800-dmamux";
> + #dma-cells = <2>;
> + dma-masters = <&dmac>;
> + dma-requests = <8>;
> + };
> diff --git a/include/dt-bindings/dma/cv1800-dma.h b/include/dt-bindings/dma/cv1800-dma.h
> new file mode 100644
> index 000000000000..3ce9dac25259
> --- /dev/null
> +++ b/include/dt-bindings/dma/cv1800-dma.h
Filename should match bindings filename.
Anyway, the problem is that it is a dead header. I don't see it being
used, so it is not a binding.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 2/2] dt-bindings: clock: rockchip: Add support for clk input / output switch
From: kernel test robot @ 2024-03-26 6:53 UTC (permalink / raw)
To: Sugar Zhang, heiko
Cc: oe-kbuild-all, linux-rockchip, Sugar Zhang, Conor Dooley,
Krzysztof Kozlowski, Michael Turquette, Rob Herring, Stephen Boyd,
devicetree, linux-arm-kernel, linux-clk, linux-kernel
In-Reply-To: <1711340191-69588-2-git-send-email-sugar.zhang@rock-chips.com>
Hi Sugar,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rockchip/for-next]
[also build test WARNING on linus/master v6.9-rc1 next-20240325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sugar-Zhang/clk-rockchip-Add-support-for-clk-input-output-switch/20240325-212211
base: https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
patch link: https://lore.kernel.org/r/1711340191-69588-2-git-send-email-sugar.zhang%40rock-chips.com
patch subject: [PATCH v1 2/2] dt-bindings: clock: rockchip: Add support for clk input / output switch
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240326/202403261442.9P6rk3Wk-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403261442.9P6rk3Wk-lkp@intel.com/
dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/clock/rockchip,clk-out.yaml: title: 'Rockchip Clock Out Control Module Binding' should not be valid under {'pattern': '([Bb]inding| [Ss]chema)'}
hint: Everything is a binding/schema, no need to say it. Describe what hardware the binding is for.
from schema $id: http://devicetree.org/meta-schemas/base.yaml#
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC
From: Krzysztof Kozlowski @ 2024-03-26 6:51 UTC (permalink / raw)
To: Frank Li, Inochi Amaoto
Cc: Vinod Koul, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Chen Wang, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Jisheng Zhang, Liu Gui, Jingbao Qiu, dlan, dmaengine, devicetree,
linux-kernel, linux-riscv
In-Reply-To: <ZgJDCL+aq3ZTE6/1@lizhi-Precision-Tower-5810>
On 26/03/2024 04:37, Frank Li wrote:
>> +properties:
>> + compatible:
>> + const: sophgo,cv1800-dmamux
>> +
>> + reg:
>> + items:
>> + - description: DMA channal remapping register
>> + - description: DMA channel interrupt mapping register
>> +
>
> Look like driver have not use it.
And what does it mean for the bindings?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v5 1/3] dt-bindings: dmaengine: Add dmamux for CV18XX/SG200X series SoC
From: Krzysztof Kozlowski @ 2024-03-26 6:50 UTC (permalink / raw)
To: Frank Li, Inochi Amaoto
Cc: Vinod Koul, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Chen Wang, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Jisheng Zhang, Liu Gui, Jingbao Qiu, dlan, dmaengine, devicetree,
linux-kernel, linux-riscv
In-Reply-To: <ZgJDCL+aq3ZTE6/1@lizhi-Precision-Tower-5810>
On 26/03/2024 04:37, Frank Li wrote:
> On Tue, Mar 26, 2024 at 09:47:03AM +0800, Inochi Amaoto wrote:
>> The DMA IP of Sophgo CV18XX/SG200X is based on a DW AXI CORE, with
>> an additional channel remap register located in the top system control
>> area. The DMA channel is exclusive to each core.
>>
>> Add the dmamux binding for CV18XX/SG200X series SoC
>>
>> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
>> ---
>> .../bindings/dma/sophgo,cv1800-dmamux.yaml | 48 ++++++++++++++++
>> include/dt-bindings/dma/cv1800-dma.h | 55 +++++++++++++++++++
>
> I remember checkpatch.pl require .h have seperate patch.
Why do you insist on that? Bindings header should be with binding.
That's incorrect advice.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 2/4] dt-bindings: interconnect: Add Qualcomm IPQ9574 support
From: Krzysztof Kozlowski @ 2024-03-26 6:49 UTC (permalink / raw)
To: Varadarajan Narayanan, andersson, konrad.dybcio, mturquette,
sboyd, robh, krzysztof.kozlowski+dt, conor+dt, djakov,
quic_anusha, linux-arm-msm, linux-clk, devicetree, linux-kernel,
linux-pm
In-Reply-To: <20240325102036.95484-3-quic_varada@quicinc.com>
On 25/03/2024 11:20, Varadarajan Narayanan wrote:
> Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> interfaces. This will be used by the gcc-ipq9574 driver
> that will for providing interconnect services using the
> icc-clk framework.
>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v2: Rename master slave macros
> Fix license identifier
Both patches should be squashed. Header is parts of bindings and your
previous patch adds the interconnects, doesn't it?
> ---
> .../dt-bindings/interconnect/qcom,ipq9574.h | 62 +++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
>
> diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
> new file mode 100644
> index 000000000000..b7b32aa6bbb1
> --- /dev/null
> +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> +#define INTERCONNECT_QCOM_IPQ9574_H
> +
> +#define IPQ_APPS_ID 9574 /* some unique value */
Why random unique values are bindings? Why this cannot be 0? Please
explain how this is used by DTS and driver.
> +#define IPQ_NSS_ID (IPQ_APPS_ID * 2)
This does not seem right.
Best regards,
Krzysztof
^ permalink raw reply
* Re: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller
From: Krzysztof Kozlowski @ 2024-03-26 6:46 UTC (permalink / raw)
To: Xingyu Wu, Liam Girdwood, Mark Brown, Claudiu Beznea,
Jaroslav Kysela, Takashi Iwai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
alsa-devel@alsa-project.org, linux-sound@vger.kernel.org
In-Reply-To: <NTZPR01MB0956BD6189974378958562D99F35A@NTZPR01MB0956.CHNPR01.prod.partner.outlook.cn>
On 26/03/2024 07:29, Xingyu Wu wrote:
>>
>> On 20/03/2024 10:02, Xingyu Wu wrote:
>>> Add bindings for the Multi-Channel I2S controller of Cadence.
Your email app responds very weird. You bypassed all possible filters
and it is simply not visible that you answer to me. You Reply to
everyone, not to me with Cc to others. There is no standard header whom
do you quote, e.g. "On 26/03/2024 07:29, Xingyu Wu wrote:"
Please use some decent email system, otherwise I will miss all replies
from you.
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - cdns,i2s-mc
>>
>> Why did this appear? Who asked for this? Usually these blocks are not usable on their
>> own.
>
> I wonder if I should keep the original IP compatible. Do I not need it?
As I said, it is not usable on its own, so unless you have other
arguments then no. But my point was that no one asked for this.
>
>>
>>> + - starfive,jh8100-i2s
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + description:
>>> + The interrupt line number for the I2S controller. Add this
>>> + parameter if the I2S controller that you are using does not
>>> + using DMA.
>>
>> That's still wrong. You already got comment on this. Either you have interrupt or not.
>> You do not add interrupts, based on your choice or not of having DMA. Drop the
>> comment.
>
> Do I keep this property and drop this description?
Drop description. Keep property, if your hardware has interrupts.
...
>>
>>> + - compatible
>>> + - reg
>>> + - clocks
>>> + - clock-names
>>> + - resets
>>> +
>>> +oneOf:
>>> + - required:
>>> + - dmas
>>> + - dma-names
>>> + - required:
>>> + - interrupts
>>
>> This won't work. Provide both interrupts and dmas, and then test your DTS.
>
> I provided both properties in the DTS and test by dtbs_check. Then it printed that:
> 'More than one condition true in one of shema: ...'
Exactly. Having both properties is a correct DTS. Interrupts do not
disappear just because you decide to describe DMA. It is OS choice what
to use if both are provided.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: arm64: marvell: add solidrun cn9130 clearfog boards
From: Krzysztof Kozlowski @ 2024-03-26 6:41 UTC (permalink / raw)
To: Josua Mayer, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Yazan Shhady, linux-arm-kernel@lists.infradead.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <6a6e7c4d-d5fd-42ae-bc3d-a3978d65e8e4@solid-run.com>
On 25/03/2024 21:12, Josua Mayer wrote:
> Am 25.03.24 um 20:34 schrieb Krzysztof Kozlowski:
>> On 22/03/2024 11:08, Josua Mayer wrote:
>>> Am 21.03.24 um 22:47 schrieb Josua Mayer:
>>>> Add bindings for SolidRun Clearfog boards, using a new SoM based on
>>>> CN9130 SoC.
>>>> The carrier boards are identical to the older Armada 388 based Clearfog
>>>> boards. For consistency the carrier part of compatible strings are
>>>> copied, including the established "-a1" suffix.
>>>>
>>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>>> ---
>>>> .../devicetree/bindings/arm/marvell/armada-7k-8k.yaml | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
>>>> index 16d2e132d3d1..36bdfd1bedd9 100644
>>>> --- a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
>>>> +++ b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
>>>> @@ -82,4 +82,16 @@ properties:
>>>> - const: marvell,armada-ap807-quad
>>>> - const: marvell,armada-ap807
>>>>
>>>> + - description:
>>>> + SolidRun CN9130 clearfog family single-board computers
>>>> + items:
>>>> + - enum:
>>>> + - solidrun,clearfog-base-a1
>>>> + - solidrun,clearfog-pro-a1
>>>> + - const: solidrun,clearfog-a1
>>>> + - const: solidrun,cn9130-sr-som
>>>> + - const: marvell,cn9130
>>>> + - const: marvell,armada-ap807-quad
>>>> + - const: marvell,armada-ap807
>>>> +
>>>> additionalProperties: true
>>> Before merging I would like some feedback about adding
>>> another product later, to ensure the compatibles above
>>> are adequate? In particular:
>>> - sequence of soc, cp, carrier compatibles
>>> - name of som compatible
>>>
>>> Draft for future bindings:
>>> - description:
>>> SolidRun CN9130 SoM based single-board computers
>>> with 1 external CP on the Carrier.
>>> items:
>>> - enum:
>>> - solidrun,cn9131-solidwan
>>> - const: marvell,cn9131
>>> - const: solidrun,cn9130-sr-som
>> This does not look correct. cn9131 is not compatible with your som.
> This is partially my question.
> I considered changing the som to "cn913x-sr-som".
>
> The SoM itself is always 9130, it contains the base SoC
> with 1x AP and 1x CP in a single chip.
> 9131 and 9132 <happen> on the carrier boards.
No wildcards, but if the SoM name is 9130 then use 9130.
The problem is that you use cn9130 SoC as fallback.
>
>>
>>> - const: marvell,cn9130
>> SoCs are compatible only in some cases, e.g. one is a subset of another
>> like stripped out of modem. Are you sure this is your case?
> This is more complex, CN9131 and CN9132 are not single SoCs.
> A "9132" is instantiated by connecting two southbridge chips
> via a Marvell defined bus, each providing additional IO
> such as network, i2c, gpio.
>
> Note that even the first, "9130", while a single chip, contains two dies:
> An "AP" (Application Processor I assume) with very limited IO (1xsdio, 1xi2c),
> and a "CP" (Communication Processor I assume) with lots of IO.
> This CP as far as I know today is identical to the southbridges
> mentioned above.
OK, but how does it affect compatibility between them? Which parts are
the same? Or how much is shared?
>
>>> - const: marvell,armada-ap807-quad
>>> - const: marvell,armada-ap807
>> Anyway, 6 compatibles is beyond useful amount. What are you expressing
>> here?
> I copied this part from the examples earlier in the file, such as:
> - description: Armada CN9132 SoC with two external CPs
> items:
> - const: marvell,cn9132
> - const: marvell,cn9131
> - const: marvell,cn9130
> - const: marvell,armada-ap807-quad
> - const: marvell,armada-ap807
>> Why is this even armada ap807?
> We noticed ap807 != ap806 (cn913x != 8040),
> because the thermal sensor coefficients converting
> raw values to celsius differed.
That's also not the best example. Might be correct but also looks
over-complicated. The point of board-level compatibles is to identify
machine and its common parts. It has little impact inside of kernel (at
least should be almost no users inside!), but there can be some users,
e.g. firmware or user-space.
This claims that cn9132 is compatible with ap807, so you have exactly
the same base. The same base is not CPU! It's about the S in SoC, so
"System". Could firmware use marvell,armada-ap807 compatible to properly
detect type of system and treat all these boards as ap807?
Best regards,
Krzysztof
^ permalink raw reply
* 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller
From: Xingyu Wu @ 2024-03-26 2:02 UTC (permalink / raw)
To: Pierre-Louis Bossart, Liam Girdwood, Mark Brown, Claudiu Beznea,
Jaroslav Kysela, Takashi Iwai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
alsa-devel@alsa-project.org, linux-sound@vger.kernel.org
In-Reply-To: <1d0399d2-684f-490e-8711-f636e987a0b8@linux.intel.com>
> > diff --git a/sound/soc/cdns/Kconfig b/sound/soc/cdns/Kconfig new file
> > mode 100644 index 000000000000..61ef718ebfe7
> > --- /dev/null
> > +++ b/sound/soc/cdns/Kconfig
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: GPL-2.0-only config SND_SOC_CADENCE_I2S_MC
> > + tristate "Cadence I2S Multi-Channel Controller Device Driver"
> > + depends on HAVE_CLK
>
> indentation is off
Will fix. Thanks.
>
> > + select SND_SOC_GENERIC_DMAENGINE_PCM
> > + help
> > + Say Y or M if you want to add support for I2S driver for the
> > + Cadence Multi-Channel I2S Controller device. The device supports
> > + up to a maximum of 8 channels each for play and record.
>
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Cadence Multi-Channel I2S controller PCM driver
> > + *
> > + * Copyright (c) 2022-2023 StarFive Technology Co., Ltd.
>
> 2024?
Will fix.
>
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/rcupdate.h>
> > +#include <sound/pcm_params.h>
> > +#include "cdns-i2s-mc.h"
> > +
> > +#define PERIOD_BYTES_MIN 4096
> > +#define BUFFER_BYTES_MAX (3 * 2 * 8 * PERIOD_BYTES_MIN)
>
> what are those 3 and 2 and 8 factors? a comment or the use of macros would help.
Will fix.
>
> > +#define PERIODS_MIN 2
> > +
> > +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
> > + struct snd_pcm_runtime *runtime,
> > + unsigned int tx_ptr, bool *period_elapsed,
> > + snd_pcm_format_t format)
> > +{
> > + unsigned int period_pos = tx_ptr % runtime->period_size;
>
> not following what the modulo is for, usually it's modulo the buffer size?
This is to see if the new data is divisible by period_size and to determine whether
it is enough for a period_size in the later loop.
>
> > + const u16 (*p16)[2] = (void *)runtime->dma_area;
> > + const u32 (*p32)[2] = (void *)runtime->dma_area;
> > + u32 data[2];
> > + int i;
> > +
> > + for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {
> > + if (format == SNDRV_PCM_FORMAT_S16_LE) {
> > + data[0] = p16[tx_ptr][0];
> > + data[1] = p16[tx_ptr][1];
> > + } else if (format == SNDRV_PCM_FORMAT_S32_LE) {
> > + data[0] = p32[tx_ptr][0];
> > + data[1] = p32[tx_ptr][1];
> > + }
>
> what about other formats implied by the use of 'else if' ?
I think I just support S16_LE and S32_LE in the snd_soc_dai_driver struct,
and ALSA device will filter out other formats for me, so I didn't add them.
Do I still have to add the other case?
> > +
> > + iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
> > + iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
> > + period_pos++;
> > + if (++tx_ptr >= runtime->buffer_size)
> > + tx_ptr = 0;
> > + }
> > +
> > + *period_elapsed = period_pos >= runtime->period_size;
> > + return tx_ptr;
> > +}
>
> > +static void cdns_i2s_pcm_transfer(struct cdns_i2s_dev *dev, bool
> > +push)
>
> 'push' really means 'tx' so what not use a simpler naming?
Will fix.
>
> > +{
> > + struct snd_pcm_substream *substream;
> > + bool active, period_elapsed;
> > +
> > + rcu_read_lock();
> > + if (push)
> > + substream = rcu_dereference(dev->tx_substream);
> > + else
> > + substream = rcu_dereference(dev->rx_substream);
> > +
> > + active = substream && snd_pcm_running(substream);
> > + if (active) {
> > + unsigned int ptr;
> > + unsigned int new_ptr;
> > +
> > + if (push) {
> > + ptr = READ_ONCE(dev->tx_ptr);
> > + new_ptr = dev->tx_fn(dev, substream->runtime, ptr,
> > + &period_elapsed, dev->format);
> > + cmpxchg(&dev->tx_ptr, ptr, new_ptr);
> > + } else {
> > + ptr = READ_ONCE(dev->rx_ptr);
> > + new_ptr = dev->rx_fn(dev, substream->runtime, ptr,
> > + &period_elapsed, dev->format);
> > + cmpxchg(&dev->rx_ptr, ptr, new_ptr);
> > + }
> > +
> > + if (period_elapsed)
> > + snd_pcm_period_elapsed(substream);
> > + }
> > + rcu_read_unlock();
> > +}
> > +
> > +void cdns_i2s_pcm_push_tx(struct cdns_i2s_dev *dev) {
> > + cdns_i2s_pcm_transfer(dev, true);
> > +}
> > +
> > +void cdns_i2s_pcm_pop_rx(struct cdns_i2s_dev *dev) {
> > + cdns_i2s_pcm_transfer(dev, false);
> > +}
> > +
> > +static int cdns_i2s_pcm_open(struct snd_soc_component *component,
> > + struct snd_pcm_substream *substream) {
> > + struct snd_pcm_runtime *runtime = substream->runtime;
> > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> > + struct cdns_i2s_dev *dev =
> > +snd_soc_dai_get_drvdata(snd_soc_rtd_to_cpu(rtd, 0));
> > +
> > + snd_soc_set_runtime_hwparams(substream, &cdns_i2s_pcm_hardware);
> > + snd_pcm_hw_constraint_integer(runtime,
> SNDRV_PCM_HW_PARAM_PERIODS);
> > + runtime->private_data = dev;
> > +
> > + return 0;
> > +}
> > +
> > +static int cdns_i2s_pcm_close(struct snd_soc_component *component,
> > + struct snd_pcm_substream *substream) {
> > + synchronize_rcu();
> > + return 0;
>
> runtime->private_data = NULL?
Will add.
>
> > +}
> > +
> > +static int cdns_i2s_pcm_hw_params(struct snd_soc_component *component,
> > + struct snd_pcm_substream *substream,
> > + struct snd_pcm_hw_params *hw_params) {
> > + struct snd_pcm_runtime *runtime = substream->runtime;
> > + struct cdns_i2s_dev *dev = runtime->private_data;
> > +
> > + dev->format = params_format(hw_params);
>
> don't you need to test if the formats are supported?
Will add the test.
>
> > + dev->tx_fn = cdns_i2s_pcm_tx;
> > + dev->rx_fn = cdns_i2s_pcm_rx;
> > +
> > + return 0;
> > +}
>
> > +static int cdns_i2s_start(struct cdns_i2s_dev *i2s,
> > + struct snd_pcm_substream *substream) {
> > + struct snd_pcm_runtime *runtime = substream->runtime;
> > + unsigned char max_ch = i2s->max_channels;
> > + unsigned char i2s_ch;
> > + int i;
> > +
> > + /* Each channel is stereo */
> > + i2s_ch = runtime->channels / 2;
> > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > + if ((i2s_ch + i2s->tx_using_channels) > max_ch) {
> > + dev_err(i2s->dev,
> > + "Max %d channels: using %d for TX, do not support %d for RX\n",
> > + max_ch, i2s->tx_using_channels, i2s_ch);
> > + return -ENOMEM;
>
> ENOMEM is for memory allocation, that looks more like EINVAL?
Will fix.
>
> > + }
> > +
> > + i2s->rx_using_channels = i2s_ch;
> > + /* Enable channels from 0 to 'max_ch' as tx */
> > + for (i = 0; i < i2s_ch; i++)
> > + cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
> > + CDNS_I2S_TC_RECEIVER);
> > +
> > + } else {
> > + if ((i2s_ch + i2s->rx_using_channels) > max_ch) {
> > + dev_err(i2s->dev,
> > + "Max %d channels: using %d for RX, do not support %d for TX\n",
> > + max_ch, i2s->rx_using_channels, i2s_ch);
> > + return -ENOMEM;
> > + }
> > +
> > + i2s->tx_using_channels = i2s_ch;
> > + /* Enable channels from 'max_ch' to 0 as rx */
> > + for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
> > + if (i < 0)
> > + return -EINVAL;
>
> that is a test you can probably factor out of the loop before doing anything that's
> inconsistent.
OK, I will move it out of the loop. Thanks.
>
> > +
> > + cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
> > + CDNS_I2S_TC_TRANSMITTER);
> > + }
> > + }
> > + cdns_i2s_enable_clock(i2s, substream->stream);
> > +
> > + if (i2s->irq >= 0)
> > + cdns_i2s_set_all_irq_mask(i2s, false);
> > +
> > + cdns_i2s_clear_int(i2s);
> > +
> > + return 0;
> > +}
> > +
> > +static int cdns_i2s_stop(struct cdns_i2s_dev *i2s,
> > + struct snd_pcm_substream *substream) {
> > + unsigned char i2s_ch;
> > + int i;
> > +
> > + cdns_i2s_clear_int(i2s);
> > +
> > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > + i2s_ch = i2s->rx_using_channels;
> > + for (i = 0; i < i2s_ch; i++)
> > + cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
> > +
> > + i2s->rx_using_channels = 0;
> > + } else {
> > + unsigned char max_ch = i2s->max_channels;
> > +
> > + i2s_ch = i2s->tx_using_channels;
> > + for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
> > + if (i < 0)
> > + return -EINVAL;
>
> same here, first test if the channel maps are valid, then do the loop?
Will fix.
>
> > +
> > + cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
> > + }
> > +
> > + i2s->tx_using_channels = 0;
> > + }
> > +
> > + if (i2s->irq >= 0 && !i2s->tx_using_channels && !i2s->rx_using_channels)
> > + cdns_i2s_set_all_irq_mask(i2s, true);
> > +
> > + return 0;
> > +}
>
> > +static int cdns_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
> > + unsigned int fmt)
> > +{
> > + struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(cpu_dai);
> > + int ret = 0;
> > +
> > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > + case SND_SOC_DAIFMT_CBM_CFM:
> > + i2s->tx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
> > + i2s->rx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
> > + cdns_i2s_set_ms_mode(i2s);
> > + break;
> > + case SND_SOC_DAIFMT_CBS_CFS:
> > + i2s->tx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
> > + i2s->rx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
> > + cdns_i2s_set_ms_mode(i2s);
> > + break;
> > + case SND_SOC_DAIFMT_CBM_CFS:
> > + case SND_SOC_DAIFMT_CBS_CFM:
>
> that's the old stuff, use CBP/CBC macros please.
Will fix.
>
> > + ret = -EINVAL;
> > + break;
> > + default:
> > + dev_dbg(i2s->dev, "Invalid master/slave format\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + return ret;
> > +}
>
> > +#ifdef CONFIG_PM
>
> Do we need this or just rely on __unused?
I think both are OK.
>
> > +static int cdns_i2s_runtime_suspend(struct device *dev) {
> > + struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(i2s->clks[1].clk); /* ICG clock */
> > + return 0;
> > +}
> > +
> > +static int cdns_i2s_runtime_resume(struct device *dev) {
> > + struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
> > +
> > + return clk_prepare_enable(i2s->clks[1].clk); /* ICG clock */ }
> > +#endif
>
> > +static int cdns_i2s_probe(struct platform_device *pdev) {
> > + struct cdns_i2s_dev *i2s;
> > + struct resource *res;
> > + int ret;
> > +
> > + i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> > + if (!i2s) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > + platform_set_drvdata(pdev, i2s);
> > +
> > + i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > + if (IS_ERR(i2s->base)) {
> > + ret = PTR_ERR(i2s->base);
> > + goto err;
> > + }
> > +
> > + i2s->dev = &pdev->dev;
> > + i2s->phybase = res->start;
> > +
> > + ret = cdns_i2s_init(i2s);
> > + if (ret)
> > + goto err;
> > +
> > + i2s->irq = platform_get_irq(pdev, 0);
> > + if (i2s->irq >= 0) {
> > + ret = devm_request_irq(&pdev->dev, i2s->irq, cdns_i2s_irq_handler,
> > + 0, pdev->name, i2s);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "request_irq failed\n");
> > + goto err;
> > + }
> > + }
> > +
> > + ret = devm_snd_soc_register_component(&pdev->dev,
> > + &cdns_i2s_component,
> > + &cdns_i2s_dai, 1);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "couldn't register component\n");
> > + goto err;
> > + }
> > +
> > + if (i2s->irq >= 0)
> > + ret = cdns_i2s_pcm_register(pdev);
> > + else
> > + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> > +
> > + if (ret) {
> > + dev_err(&pdev->dev, "could not register pcm: %d\n", ret);
> > + goto err;
> > + }
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + if (pm_runtime_enabled(&pdev->dev))
> > + cdns_i2s_runtime_suspend(&pdev->dev);
>
> that sequence looks suspicious.... Why would you suspend immediately during the
> probe? You're probably missing all the autosuspend stuff?
Since I have enabled clocks before, and the device is in the suspend state after
pm_runtime_enable(), I need to disable clocks in cdns_i2s_runtime_suspend()
to match the suspend state.
>
> > +
> > + dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
> > + i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
> > +
> > + return 0;
> > +
> > +err:
> > + return ret;
> > +}
> > +
> > +static int cdns_i2s_remove(struct platform_device *pdev) {
> > + pm_runtime_disable(&pdev->dev);
> > + if (!pm_runtime_status_suspended(&pdev->dev))
> > + cdns_i2s_runtime_suspend(&pdev->dev);
>
> ... and this one too. Once you've disabled pm_runtime, checking the status is
> irrelevant...
I think the clocks need to be always enabled after probe if disable pm_runtime,
and should be disabled when remove. This will do that.
>
> > +
> > + return 0;
> > +}
> > +
Best regards,
Xingyu Wu
^ permalink raw reply
* Re: [PATCH] ASoC: dt-bindings: wm8974: Convert to dtschema
From: Kartik Agarwala @ 2024-03-26 6:34 UTC (permalink / raw)
To: Krzysztof Kozlowski, lgirdwood, broonie, robh,
krzysztof.kozlowski+dt, conor+dt
Cc: patches, linux-sound, devicetree, linux-kernel,
javier.carrasco.cruz
In-Reply-To: <02fc496c-caac-4434-8e18-229074733b1c@linaro.org>
On 3/26/24 1:00 AM, Krzysztof Kozlowski wrote:
> On 25/03/2024 19:19, Kartik Agarwala wrote:
>> Convert WM8974 audio CODEC bindings from text to dtschema.
>>
>> Signed-off-by: Kartik Agarwala <agarwala.kartik@gmail.com>
>
> Thank you for your patch. There are no DTS users of this binding, so
> while such conversions are useful, they have significantly smaller
> impact. In the future, please consider converting bindings from active
> platforms (arm64 defconfig, arm multi_v7). This would have significantly
> bigger impact.
>
> See also:
> https://lore.kernel.org/all/6552bcb8-e046-4882-91da-1094fff3d239@linaro.org/
Noted, thanks for letting me know.
Regards,
Kartik Agarwala
^ permalink raw reply
* Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality
From: Krzysztof Kozlowski @ 2024-03-26 6:32 UTC (permalink / raw)
To: Andreas Kemnade
Cc: lee, robh+dt, krzysztof.kozlowski+dt, conor+dt, mazziesaccount,
devicetree, linux-kernel, imx
In-Reply-To: <20240325212127.7583c845@aktux>
On 25/03/2024 21:21, Andreas Kemnade wrote:
> On Mon, 25 Mar 2024 13:13:13 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>
>> On 24/03/2024 21:12, Andreas Kemnade wrote:
>>> struct regmap_irq_chip_data *irq_data;
>>> @@ -542,7 +560,18 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
>>> ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells,
>>> NULL, 0, regmap_irq_get_domain(irq_data));
>>> if (ret)
>>> - dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>>> + return dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>>> +
>>> + if (of_device_is_system_power_controller(i2c->dev.of_node)) {
>>> + if (!pm_power_off) {
>>> + bd71828_dev = i2c;
>>> + pm_power_off = bd71828_power_off;
>>> + ret = devm_add_action_or_reset(&i2c->dev,
>>> + bd71828_remove_poweroff,
>>> + NULL);
>>> + } else
>>> + dev_warn(&i2c->dev, "Poweroff callback already assigned\n");
>>
>> Missing {}
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. Some
>> warnings can be ignored, but the code here looks like it needs a fix.
>> Feel free to get in touch if the warning is not clear.
>>
> No, it does not complain about the {}. I was a bit unsure whether it is
> required or not, but I was sure that checkpatch.pl does catch such things.
> Yes, documentation clearly says that braces are required in those cases.
"CHECK: braces {} should be used on all arms of this statement"
I will update my template-response to use --strict.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v4 4/7] dt-bindings: iio: accel: adxl345: Add spi-3wire
From: Krzysztof Kozlowski @ 2024-03-26 6:30 UTC (permalink / raw)
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <CAFXKEHaEVwiAW9co0+=kZ5w5a8eWg3QL0dmg38bvrmLdnBEA7w@mail.gmail.com>
On 25/03/2024 23:09, Lothar Rubusch wrote:
>>
>>
>>>
>>>> the tags. The upstream maintainer will do that for tags received on the
>>>> version they apply.
>>>>
>>>
>>> I'm pretty sure we will still see further iterations. So, I apply the
>>> tags in the next version, already scheduled. Ok?
>>>
>>>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>>>
>>>
>>> Going over the books I feel it does not make sense to still mention
>>> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
>>> does it? Your link mentiones "However if the patch has changed
>>
>> I don't understand. When did you receive the tag? v3, right? So what do
>> you mean by v1 and v2?
>>
>
> V1: The first version of the 3wire patch. I have split the single
> patch upon some feedback (yours?!) - V2... So, my current
> interpretation is, that every feedback I need to mention as
> Reviewed-by tag, no?
What? Feedback is not review. It's clearly explained in submitting
patches. Please read it.
Best regards,
Krzysztof
^ permalink raw reply
* 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller
From: Xingyu Wu @ 2024-03-26 6:29 UTC (permalink / raw)
To: Krzysztof Kozlowski, Liam Girdwood, Mark Brown, Claudiu Beznea,
Jaroslav Kysela, Takashi Iwai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
alsa-devel@alsa-project.org, linux-sound@vger.kernel.org
In-Reply-To: <9d9efb8a-0b3c-4e7a-8673-07cd3b1f5f87@linaro.org>
>
> On 20/03/2024 10:02, Xingyu Wu wrote:
> > Add bindings for the Multi-Channel I2S controller of Cadence.
> >
> > The Multi-Channel I2S (I2S-MC) implements a function of the 8-channel
> > I2S bus interfasce. Each channel can become receiver or transmitter.
> > Four I2S instances are used on the StarFive
> > JH8100 SoC. One instance of them is limited to 2 channels, two
> > instance are limited to 4 channels, and the other one can use most 8
> > channels. Add a unique property about 'starfive,i2s-max-channels' to
> > distinguish each instance.
> >
> > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > ---
> > .../bindings/sound/cdns,i2s-mc.yaml | 110 ++++++++++++++++++
> > 1 file changed, 110 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > new file mode 100644
> > index 000000000000..0a1b0122a246
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cadence multi-channel I2S controller
> > +
> > +description:
> > + The Cadence I2S Controller implements a function of the
> > +multi-channel
> > + (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
> > +
> > +maintainers:
> > + - Xingyu Wu <xingyu.wu@starfivetech.com>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - cdns,i2s-mc
>
> Why did this appear? Who asked for this? Usually these blocks are not usable on their
> own.
I wonder if I should keep the original IP compatible. Do I not need it?
>
> > + - starfive,jh8100-i2s
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + description:
> > + The interrupt line number for the I2S controller. Add this
> > + parameter if the I2S controller that you are using does not
> > + using DMA.
>
> That's still wrong. You already got comment on this. Either you have interrupt or not.
> You do not add interrupts, based on your choice or not of having DMA. Drop the
> comment.
Do I keep this property and drop this description?
>
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: Bit clock
> > + - description: Main ICG clock
> > + - description: Inner master clock
> > +
> > + clock-names:
> > + items:
> > + - const: bclk
> > + - const: icg
> > + - const: mclk_inner
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + dmas:
> > + items:
> > + - description: TX DMA Channel
> > + - description: RX DMA Channel
> > + minItems: 1
> > +
> > + dma-names:
> > + items:
> > + - const: tx
> > + - const: rx
> > + minItems: 1
> > +
> > + "#sound-dai-cells":
> > + const: 0
> > +
> > +allOf:
> > + - $ref: dai-common.yaml#
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: starfive,jh8100-i2s
> > + then:
> > + properties:
> > + starfive,i2s-max-channels:
> > + description:
> > + Number of I2S max stereo channels supported on the StarFive
> > + JH8100 SoC.
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> Properties must be defined in top-level. You can disallow the property for other
> variants, but I claim you won't have here other variants.
>
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/e
> xample-schema.yaml#L212
>
Will fix.
>
> > + enum: [2, 4, 8]
> > + required:
> > + - starfive,i2s-max-channels
> > +
> > +required:
>
> required goes after properties.
Will fix.
>
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - resets
> > +
> > +oneOf:
> > + - required:
> > + - dmas
> > + - dma-names
> > + - required:
> > + - interrupts
>
> This won't work. Provide both interrupts and dmas, and then test your DTS.
I provided both properties in the DTS and test by dtbs_check. Then it printed that:
'More than one condition true in one of shema: ...'
>
> > +
> > +unevaluatedProperties: false
> > +
>
> Best regards,
> Krzysztof
Best regards,
Xingyu Wu
^ permalink raw reply
* Re: [PATCH v2 0/3] QCM2290 LMH
From: Krzysztof Kozlowski @ 2024-03-26 6:29 UTC (permalink / raw)
To: Nícolas F. R. A. Prado
Cc: Konrad Dybcio, Bjorn Andersson, Rafael J. Wysocki, Daniel Lezcano,
Zhang Rui, Lukasz Luba, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thara Gopinath, Amit Kucheria, Marijn Suijten,
linux-arm-msm, linux-pm, devicetree, linux-kernel,
Dmitry Baryshkov, stable, Loic Poulain
In-Reply-To: <8e0cc005-0b3a-4475-bfe4-82ec46d918a5@notapiano>
On 26/03/2024 00:01, Nícolas F. R. A. Prado wrote:
> On Mon, Mar 25, 2024 at 08:59:55PM +0100, Krzysztof Kozlowski wrote:
>> On 20/03/2024 20:08, Nícolas F. R. A. Prado wrote:
>>>> Loic Poulain (1):
>>>> arm64: dts: qcom: qcm2290: Add LMH node
>>>>
>>>> Documentation/devicetree/bindings/thermal/qcom-lmh.yaml | 12 ++++++++----
>>>> arch/arm64/boot/dts/qcom/qcm2290.dtsi | 14 +++++++++++++-
>>>> drivers/thermal/qcom/lmh.c | 3 +++
>>>> 3 files changed, 24 insertions(+), 5 deletions(-)
>>>
>>> Hi,
>>>
>>> I've started tracking the results of 'make dtbs_check' on linux-next, and I've
>>> noticed that on today's next, next-20240320, there's a new warning coming from
>>> this. The reason is that the DT change has landed, but the binding has not,
>>> since it goes through a separate tree. I thought the binding was supposed to
>>> always land before the driver and DT that make use of it, but looking through
>>
>> There is no such rule. Of course new binding should be documented in
>> earlier or the same kernel release cycle as users get in, but it's not a
>> requirement.
>
> So, after giving the documentation a second look, I found this:
>
> "For new platforms, or additions to existing ones, make dtbs_check should not
> add any new warnings."
>
> Source: https://www.kernel.org/doc/html/latest/process/maintainer-soc.html#validating-devicetree-files
It's just "should"...
>
> What is not clear there is what the reference point is: is it on linux-next?
> Mainline release?
Does it matter? There was never a new warning introduced by this
patchset. The patchset itself is correct. No new warnings.
>
> As Konrad pointed out it's tricky (and maybe not worth it) to guarantee this for
> linux-next. But for mainline release it seems feasible (and IMO the target, as
> after that stability guarantees should apply).
I don't believe in such guarantees. Different maintainers apply patches
differently, especially bindings, so this is beyond our control. Often
also beyond SoC maintainer control.
>
>>
>>
>>> the dt-binding documentation pages I couldn't find anything confirming or
>>> denying that.
>>>
>>> I expect this to happen again in the future, which is why I'm reaching out to
>>> understand better how to deal with this kind of situation.
>>
>> Deal as what to do? Are you asking in terms of maintenance of some
>> subsystem or sending some patches? In this particular case here, I don't
>> think there is anything on your side to deal with.
>
> I'm asking what's the most helpful way to you the maintainers for me to report
> these failures in the future.
The most effective way is LKP-like or Rob's-bot-like automated replies
to original email threads, by testing the original patchset on
linux-next. But Rob's bot is actually doing it, just on different base.
Other reports, like for cases when only parts of patch is applied, could
be also useful but I am afraid you will generate way too much of them.
Binding is supposed to go via subsystem, DTS via SoC, so basically 90%
of patchsets might have some sort of delays resulting in dtbs_check
false positive warnings.
For my SoC I check my trees, mainline and next, and keep adding list of
exceptions for expected issues. What's useful for Qualcomm? Konrad,
Bjorn, any thoughts?
Have in mind that expected warnings can be for entire cycle when dealing
with technical debt, because DTS goes N+1.
>
> Rob has already automated running dtbs_check for patches coming into the mailing
> list. And I have set up KernelCI to run dtbs_check on linux-next in order to
> catch any issues that might slip through, or happen during integration of the
> trees, etc.
>
> Now, if we agree that dtbs_check regressions on linux-next are acceptable, at
> least ones like this, where the issue is just synchronization between
Yes and no. True regressions are not acceptable. Expected intermediate
regressions as a result of patchset being applying, but not yet fully
applied, are OK. Expected regressions for intra-cycle-work are also OK.
> maintainers, then I can simply not report them in the future. But we should
> have some point where dtbs_check should not regress, and mainline release seems
> the reasonable choice, because if we don't then dtbs_check warnings would just
> keep growing forever.
I invite therefore to my session:
https://eoss24.sched.com/event/1aBEf?iframe=no
We'll see if they keep growing :)
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 3/4] clk: qcom: add IPQ9574 interconnect clocks support
From: Varadarajan Narayanan @ 2024-03-26 6:03 UTC (permalink / raw)
To: Konrad Dybcio
Cc: andersson, mturquette, sboyd, robh, krzysztof.kozlowski+dt,
conor+dt, djakov, quic_anusha, linux-arm-msm, linux-clk,
devicetree, linux-kernel, linux-pm
In-Reply-To: <fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org>
On Mon, Mar 25, 2024 at 08:42:21PM +0100, Konrad Dybcio wrote:
> On 25.03.2024 11:20 AM, Varadarajan Narayanan wrote:
> > Unlike MSM platforms that manage NoC related clocks and scaling
> > from RPM, IPQ SoCs dont involve RPM in managing NoC related
> > clocks and there is no NoC scaling.
> >
> > However, there is a requirement to enable some NoC interface
> > clocks for accessing the peripheral controllers present on
> > these NoCs. Though exposing these as normal clocks would work,
> > having a minimalistic interconnect driver to handle these clocks
> > would make it consistent with other Qualcomm platforms resulting
> > in common code paths. This is similar to msm8996-cbf's usage of
> > icc-clk framework.
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
>
> [...]
>
> >
> > +
> > +static struct icc_clk_data *icc_ipq9574;
> > +
>
> What does this help achieve?
Had it as a place holder in case if the provider pointer is needed
for any debug. Will remove it.
> > +static int noc_clks[] = {
>
> We could probably use indexed identifiers here to avoid confusion:
> [ICC_BINDING_NAME] = CLK_BINDING_NAME
ok.
> > static int gcc_ipq9574_probe(struct platform_device *pdev)
> > {
> > - return qcom_cc_probe(pdev, &gcc_ipq9574_desc);
> > + int ret = qcom_cc_probe(pdev, &gcc_ipq9574_desc);
> > + struct icc_provider *provider;
> > + struct icc_clk_data *icd;
> > + int i;
> > +
> > + if (ret)
>
> I'd personally prefer if you left ret uninitialized and assigned it
> above the if-statement.
ok
> > + return dev_err_probe(&pdev->dev, ret, "%s failed\n", __func__);
>
> Please avoid the use of __func__ throughout your change and write
> a more useful error message.
>
> > +
> > + icd = devm_kmalloc(&pdev->dev, ARRAY_SIZE(noc_clks) * sizeof(*icd),
> > + GFP_KERNEL);
>
> devm_kcalloc
ok
> > +
> > + if (IS_ERR_OR_NULL(icd))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(icd),
> > + "%s malloc failed\n", __func__);
>
> ditto
ok
> > +
> > + icc_ipq9574 = icd;
> > +
> > + for (i = 0; i < ARRAY_SIZE(noc_clks); i++, icd++) {
> > + icd->clk = gcc_ipq9574_clks[noc_clks[i]]->hw.clk;
> > + if (IS_ERR_OR_NULL(icd->clk)) {
> > + dev_err(&pdev->dev, "%s: %d clock not found\n",
> > + __func__, noc_clks[i]);
> > + return -ENOENT;
>
> return dev_err_probe
ok
> > + }
> > + icd->name = clk_hw_get_name(&gcc_ipq9574_clks[noc_clks[i]]->hw);
> > + }
> > +
> > + provider = icc_clk_register(&pdev->dev, IPQ_APPS_ID,
> > + ARRAY_SIZE(noc_clks), icc_ipq9574);
> > + if (IS_ERR_OR_NULL(provider))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(provider),
> > + "%s: icc_clk_register failed\n", __func__);
>
> ditto
ok
> On a second thought, since I'm assuming you're going to expand this to other
> IPQ SoCs, it might be useful to factor this out into drivers/clk/qcom/common.c
Will move it.
Thanks
Varada
^ permalink raw reply
* RE: [PATCH v3 1/1] dt-bindings: net: starfive,jh7110-dwmac: Add StarFive JH8100 support
From: ChunHau Tan @ 2024-03-26 5:24 UTC (permalink / raw)
To: Rob Herring
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Emil Renner Berthing, Krzysztof Kozlowski, Conor Dooley,
Maxime Coquelin, Alexandre Torgue, Simon Horman,
Bartosz Golaszewski, Andrew Halaney, Jisheng Zhang,
Uwe Kleine-König, Russell King, Leyfoon Tan, JeeHeng Sia,
netdev@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-riscv@lists.infradead.org
In-Reply-To: <20240325162245.GA4167001-robh@kernel.org>
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, 26 March, 2024 12:23 AM
> To: ChunHau Tan <chunhau.tan@starfivetech.com>
> Cc: David S . Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Emil Renner Berthing <kernel@esmil.dk>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> Alexandre Torgue <alexandre.torgue@foss.st.com>; Simon Horman
> <horms@kernel.org>; Bartosz Golaszewski <bartosz.golaszewski@linaro.org>;
> Andrew Halaney <ahalaney@redhat.com>; Jisheng Zhang <jszhang@kernel.org>;
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Russell King
> <rmk+kernel@armlinux.org.uk>; Leyfoon Tan <leyfoon.tan@starfivetech.com>;
> JeeHeng Sia <jeeheng.sia@starfivetech.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v3 1/1] dt-bindings: net: starfive,jh7110-dwmac: Add
> StarFive JH8100 support
>
> On Mon, Mar 25, 2024 at 01:51:31AM -0700, Tan Chun Hau wrote:
> > Add StarFive JH8100 dwmac support.
> > The JH8100 dwmac shares the same driver code as the JH7110 dwmac and
> > has only one reset signal.
> >
> > Please refer to below:
> >
> > JH8100: reset-names = "stmmaceth";
> > JH7110: reset-names = "stmmaceth", "ahb";
>
> It's debatable whether JH8100 is compatible with JH7110 if the 2nd reset was
> not optional. I guess if the Linux driver treated it that way, we can get away with
> it. It would simplify the conditionals in the schema if the t also treated the
> 2nd entry as optional on JH7110 as well.
Yes, Linux driver and snps,dwmac.yaml treated it that way.
please refer to
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/snps%2Cdwmac.yaml#L143
>
> > JH7100: reset-names = "ahb";
> >
> > Example usage of JH8100 in the device tree:
> >
> > gmac0: ethernet@16030000 {
> > compatible = "starfive,jh8100-dwmac",
> > "starfive,jh7110-dwmac",
> > "snps,dwmac-5.20";
> > ...
> > };
> >
> > Signed-off-by: Tan Chun Hau <chunhau.tan@starfivetech.com>
> > ---
> > .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
> > .../bindings/net/starfive,jh7110-dwmac.yaml | 82 +++++++++++++------
> > 2 files changed, 58 insertions(+), 25 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > index 6b0341a8e0ea..a6d596b7dcf4 100644
> > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > @@ -97,6 +97,7 @@ properties:
> > - snps,dwxgmac-2.10
> > - starfive,jh7100-dwmac
> > - starfive,jh7110-dwmac
> > + - starfive,jh8100-dwmac
> >
> > reg:
> > minItems: 1
> > diff --git
> > a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> > b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> > index 0d1962980f57..da3cc984fec9 100644
> > --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> > @@ -18,6 +18,7 @@ select:
> > enum:
> > - starfive,jh7100-dwmac
> > - starfive,jh7110-dwmac
> > + - starfive,jh8100-dwmac
> > required:
> > - compatible
> >
> > @@ -30,6 +31,10 @@ properties:
> > - items:
> > - const: starfive,jh7110-dwmac
> > - const: snps,dwmac-5.20
> > + - items:
> > + - const: starfive,jh8100-dwmac
> > + - const: starfive,jh7110-dwmac
> > + - const: snps,dwmac-5.20
> >
> > reg:
> > maxItems: 1
> > @@ -83,29 +88,13 @@ allOf:
> > - if:
> > properties:
> > compatible:
> > - contains:
> > - const: starfive,jh7100-dwmac
> > - then:
> > - properties:
> > - interrupts:
> > - minItems: 2
> > - maxItems: 2
> > -
> > - interrupt-names:
> > - minItems: 2
> > - maxItems: 2
> > -
> > - resets:
> > - maxItems: 1
> > -
> > - reset-names:
> > - const: ahb
> > -
> > - - if:
> > - properties:
> > - compatible:
> > - contains:
> > - const: starfive,jh7110-dwmac
> > + allOf:
> > + - contains:
> > + enum:
> > + - starfive,jh8100-dwmac
> > + - contains:
> > + enum:
> > + - starfive,jh7110-dwmac
>
> There's no need for the 2nd entry. You just need to check
>
> I would something like this structure:
>
> - if:
> properties:
> compatible:
> contains:
> const: starfive,jh7100-dwmac
>
> then:
>
> if:
> properties:
> compatible:
> contains:
> const: starfive,jh8100-dwmac
> then:
> ...
> else:
> ...
>
Okay, thank you for the feedback.
> > then:
> > properties:
> > interrupts:
> > @@ -117,10 +106,53 @@ allOf:
> > maxItems: 3
> >
> > resets:
> > - minItems: 2
> > + maxItems: 1
> >
> > reset-names:
> > - minItems: 2
> > + const: stmmaceth
> > +
> > + else:
>
> I don't think you need the else. Just do another 'if' entry.
>
> > + if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: starfive,jh7100-dwmac
> > + then:
> > + properties:
> > + interrupts:
> > + minItems: 2
> > + maxItems: 2
> > +
> > + interrupt-names:
> > + minItems: 2
> > + maxItems: 2
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + reset-names:
> > + const: ahb
> > +
> > + if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: starfive,jh7110-dwmac
> > + then:
> > + properties:
> > + interrupts:
> > + minItems: 3
> > + maxItems: 3
> > +
> > + interrupt-names:
> > + minItems: 3
> > + maxItems: 3
> > +
> > + resets:
> > + minItems: 2
> > +
> > + reset-names:
> > + minItems: 2
> >
> > unevaluatedProperties: false
> >
> > --
> > 2.25.1
> >
^ permalink raw reply
* Re: [PATCH v1 1/4] dt-bindings: input: Add Himax HX83102J touchscreen
From: Allen Lin @ 2024-03-26 5:46 UTC (permalink / raw)
To: Conor Dooley
Cc: Rob Herring, dmitry.torokhov, krzysztof.kozlowski+dt, conor+dt,
jikos, benjamin.tissoires, linux-input, devicetree, linux-kernel
In-Reply-To: <20240322-rectified-udder-fef9102f58da@spud>
Conor Dooley <conor@kernel.org> 於 2024年3月23日 週六 上午2:34寫道：
>
> On Fri, Mar 22, 2024 at 01:30:09PM -0500, Rob Herring wrote:
> > On Fri, Mar 22, 2024 at 05:54:08PM +0000, Conor Dooley wrote:
> > > On Fri, Mar 22, 2024 at 04:56:03PM +0800, Allen_Lin wrote:
> > > > Add the HX83102j touchscreen device tree bindings documents.
> > > > HX83102j is a Himax TDDI touchscreen controller.
> > > > It's power sequence should be bound with a lcm driver, thus it
> > > > needs to be a panel follower. Others are the same as normal SPI
> > > > touchscreen controller.
> > > >
> > > > Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> > >
> > > note to self/Krzysztof/Rob:
> > > There was a previous attempt at this kind of device. This version looks
> > > better but might be incomplete given there's a bunch more properties in
> > > that patchset:
> > > https://lore.kernel.org/all/20231017091900.801989-1-tylor_yang@himax.corp-partner.google.com/
> >
> > Those don't look like properties we want coming back.
>
> Oh, I don't want most of them coming back either. There are some
> supplies in there though that I think we would like to come back, no?
> Maybe this particular device doesn't have any supplies, but that doesn't
> really seem credible.
We will use Firmware-name in Device Tree.
For power supply settings, because there may be other device using
same regulator.
We plan to define it as an optional property for user to control in
next release.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox