Devicetree
 help / color / mirror / Atom feed
* Re: [PATCHv1 19/19] dt-bindings: power: sbs-battery: Convert to yaml
From: Rob Herring @ 2020-05-28  2:40 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Rob Herring, kernel, Rafael J . Wysocki, devicetree,
	Greg Kroah-Hartman, Sebastian Reichel, linux-pm, linux-kernel
In-Reply-To: <20200513185615.508236-20-sebastian.reichel@collabora.com>

On Wed, 13 May 2020 20:56:15 +0200, Sebastian Reichel wrote:
> Convert sbs-battery bindings to YAML.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../power/supply/sbs,sbs-battery.yaml         | 83 +++++++++++++++++++
>  .../bindings/power/supply/sbs_sbs-battery.txt | 30 -------
>  2 files changed, 83 insertions(+), 30 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/sbs,sbs-battery.yaml
>  delete mode 100644 Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [PATCHv1 05/19] power: supply: sbs-battery: Add TI BQ20Z65 support
From: Rob Herring @ 2020-05-28  2:37 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Rafael J . Wysocki, kernel, devicetree, Greg Kroah-Hartman,
	linux-pm, linux-kernel, Rob Herring, Sebastian Reichel
In-Reply-To: <20200513185615.508236-6-sebastian.reichel@collabora.com>

On Wed, 13 May 2020 20:56:01 +0200, Sebastian Reichel wrote:
> Add support for BQ20Z65 manufacturer data to the sbs-battery
> driver. Implementation has been verified using the public TRM
> available from [0] and tested using a GE Flex 3S2P battery.
> 
> [0] http://www.ti.com/lit/pdf/sluu386
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/power/supply/sbs_sbs-battery.txt     |  1 +
>  drivers/power/supply/sbs-battery.c                | 15 ++++++++++-----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [PATCH v3] dt-bindings: thermal: rcar-gen3-thermal: Convert bindings to json-schema
From: Rob Herring @ 2020-05-28  2:36 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Geert Uytterhoeven, linux-pm, Rob Herring, linux-renesas-soc,
	devicetree
In-Reply-To: <20200513151201.1258162-1-niklas.soderlund+renesas@ragnatech.se>

On Wed, 13 May 2020 17:12:01 +0200, Niklas Söderlund wrote:
> Convert Renesas R-Car Gen3 Thermal bindings documentation to
> json-schema.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> * Changes since v2
> - Use reg = <0xe6198000 0x100> instead of reg = <0 0xe6198000 0 0x100>
>   in examples due to #{address,size}-cells = <1>.
> 
> * Changes since v1
> - Improved on reg and interrupt descriptions with the use of 'items:'
> - Improved the examples inside the yaml file
> - Added compatibility value renesas,r8a77961-thermal for R-Car M3-W+
>   which was merged in the text binding description.
> ---
>  .../bindings/thermal/rcar-gen3-thermal.txt    | 60 -----------
>  .../bindings/thermal/rcar-gen3-thermal.yaml   | 99 +++++++++++++++++++
>  2 files changed, 99 insertions(+), 60 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt
>  create mode 100644 Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.yaml
> 

Applied, thanks!

^ permalink raw reply

* Re: [PATCH v3 7/7] mfd: madera: Move binding over to dtschema
From: Rob Herring @ 2020-05-28  2:35 UTC (permalink / raw)
  To: Charles Keepax
  Cc: robh+dt, patches, linus.walleij, lee.jones, linux-kernel,
	lgirdwood, cw00.choi, devicetree, broonie, myungjoo.ham
In-Reply-To: <20200513095720.8867-7-ckeepax@opensource.cirrus.com>

On Wed, 13 May 2020 10:57:20 +0100, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Changes since v2:
>  - Removed some more redundant descriptions
>  - Updated pinctrl node naming in the example
> 
> Thanks,
> Charles
> 
>  .../devicetree/bindings/mfd/cirrus,madera.yaml     | 311 +++++++++++++++++++++
>  Documentation/devicetree/bindings/mfd/madera.txt   | 114 --------
>  MAINTAINERS                                        |   6 +-
>  3 files changed, 314 insertions(+), 117 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/cirrus,madera.yaml
>  delete mode 100644 Documentation/devicetree/bindings/mfd/madera.txt
> 

Applied, thanks!

^ permalink raw reply

* Re: [PATCH v3 6/7] pinctrl: madera: Move binding over to dtschema
From: Rob Herring @ 2020-05-28  2:35 UTC (permalink / raw)
  To: Charles Keepax
  Cc: robh+dt, cw00.choi, broonie, lgirdwood, devicetree, linus.walleij,
	lee.jones, linux-kernel, patches, myungjoo.ham
In-Reply-To: <20200513095720.8867-6-ckeepax@opensource.cirrus.com>

On Wed, 13 May 2020 10:57:19 +0100, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Changes since v2:
>  - Remove some more redundant descriptions
>  - Force pinctrl node to be called "pin-settings"
>  - Force suffix on individual config nodes to -pins
> 
> Thanks,
> Charles
> 
>  .../bindings/pinctrl/cirrus,madera-pinctrl.txt     |  99 -----------------
>  .../devicetree/bindings/pinctrl/cirrus,madera.yaml | 122 +++++++++++++++++++++
>  2 files changed, 122 insertions(+), 99 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,madera-pinctrl.txt
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,madera.yaml
> 

Applied, thanks!

^ permalink raw reply

* Re: [PATCH v3 5/7] ASoC: madera: Move binding over to dtschema
From: Rob Herring @ 2020-05-28  2:34 UTC (permalink / raw)
  To: Charles Keepax
  Cc: broonie, cw00.choi, devicetree, lee.jones, lgirdwood, patches,
	myungjoo.ham, linux-kernel, robh+dt, linus.walleij
In-Reply-To: <20200513095720.8867-5-ckeepax@opensource.cirrus.com>

On Wed, 13 May 2020 10:57:18 +0100, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> No changes since v2.
> 
> Thanks,
> Charles
> 
>  .../devicetree/bindings/sound/cirrus,madera.yaml   | 113 +++++++++++++++++++++
>  Documentation/devicetree/bindings/sound/madera.txt |  67 ------------
>  2 files changed, 113 insertions(+), 67 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/cirrus,madera.yaml
>  delete mode 100644 Documentation/devicetree/bindings/sound/madera.txt
> 

Applied, thanks!

^ permalink raw reply

* Re: [PATCH 15/15] dt-bindings: usb: dwc2: Fix issues for stm32mp15x SoC
From: Rob Herring @ 2020-05-28  2:34 UTC (permalink / raw)
  To: Benjamin Gaignard
  Cc: alexandre.torgue, linux-arm-kernel, linux-gpio, robh+dt, gregkh,
	mcoquelin.stm32, linux-stm32, devicetree, linux-usb,
	linus.walleij, linux-kernel
In-Reply-To: <20200513145935.22493-16-benjamin.gaignard@st.com>

On Wed, 13 May 2020 16:59:35 +0200, Benjamin Gaignard wrote:
> Correct the compatible list for stm32mp15x SoC.
> Fix the name of the stm32mp15x dedicated supply to be aligned with
> what the driver use.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc2.yaml | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [PATCH 14/15] dt-bindings: pinctrl: stm32: Add missing interrupts property
From: Rob Herring @ 2020-05-28  2:34 UTC (permalink / raw)
  To: Benjamin Gaignard
  Cc: robh+dt, alexandre.torgue, gregkh, mcoquelin.stm32, linux-gpio,
	linux-arm-kernel, linux-kernel, devicetree, linus.walleij,
	linux-usb, linux-stm32
In-Reply-To: <20200513145935.22493-15-benjamin.gaignard@st.com>

On Wed, 13 May 2020 16:59:34 +0200, Benjamin Gaignard wrote:
> Driver use interrupt-parent field so update the bindings to allow it.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [PATCH 2/3] arm64: dts: sparx5: Add hwmon temperature sensor
From: Rob Herring @ 2020-05-28  2:29 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: Guenter Roeck, SoC Team, Jean Delvare,
	Microchip Linux Driver Support, linux-hwmon, devicetree,
	linux-arm-kernel, linux-kernel, Alexandre Belloni
In-Reply-To: <20200513134140.25357-3-lars.povlsen@microchip.com>

On Wed, May 13, 2020 at 03:41:39PM +0200, Lars Povlsen wrote:
> This adds a hwmon temperature node sensor to the Sparx5 SoC.
> 
> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  arch/arm64/boot/dts/microchip/sparx5.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> index f09a49c41ce19..b5f2d088af30e 100644
> --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
> +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> @@ -233,5 +233,11 @@ i2c1: i2c@600103000 {
>  			clock-frequency = <100000>;
>  			clocks = <&ahb_clk>;
>  		};
> +
> +		tmon0: tmon@610508110 {
> +			compatible = "microchip,sparx5-temp";
> +			reg = <0x6 0x10508110 0xc>;

These nodes are all very odd with a couple of registers spread out at 
randomish addresses. DT nodes should roughly correlate to h/w blocks, 
not sets of registers for a driver like this seems to be.

Please make the dts files one patch. Reviewing a node at a time is not 
all that effective.

Rob

^ permalink raw reply

* Re: [PATCH 3/5] dt-bindings: reset: ocelot: Add documentation for 'microchip,reset-switch-core' property
From: Rob Herring @ 2020-05-28  2:25 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: Sebastian Reichel, SoC Team, Alexandre Belloni,
	Microchip Linux Driver Support, linux-pm, devicetree,
	linux-kernel, linux-arm-kernel
In-Reply-To: <20200513130842.24847-4-lars.povlsen@microchip.com>

On Wed, May 13, 2020 at 03:08:40PM +0200, Lars Povlsen wrote:
> This documents the 'microchip,reset-switch-core' property in the
> ocelot-reset driver.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  .../devicetree/bindings/power/reset/ocelot-reset.txt        | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt b/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
> index 4d530d8154848..20fff03753ad2 100644
> --- a/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
> +++ b/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
> @@ -9,9 +9,15 @@ microchip Sparx5 armv8 SoC's.
>  Required Properties:
>   - compatible: "mscc,ocelot-chip-reset" or "microchip,sparx5-chip-reset"
> 
> +Optional properties:
> +- microchip,reset-switch-core : Perform a switch core reset at the
> +  time of driver load. This is may be used to initialize the switch
> +  core to a known state (before other drivers are loaded).

How do you know when other drivers are loaded? This could be a module 
perhaps. Doesn't seem like something that belongs in DT.

Can this behavior be implied with "microchip,sparx5-chip-reset"?

Rob

^ permalink raw reply

* Re: [PATCH 10/14] dt-bindings: clock: sparx5: Add Sparx5 SoC DPLL clock
From: Rob Herring @ 2020-05-28  2:18 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: SoC Team, Arnd Bergmann, Stephen Boyd, Linus Walleij,
	Steen Hegelund, Microchip Linux Driver Support, Olof Johansson,
	Michael Turquette, devicetree, linux-clk, linux-gpio,
	linux-arm-kernel, linux-kernel, Alexandre Belloni
In-Reply-To: <20200513125532.24585-11-lars.povlsen@microchip.com>

On Wed, May 13, 2020 at 02:55:28PM +0200, Lars Povlsen wrote:
> This add the DT bindings documentation for the Sparx5 SoC DPLL clock
> 
> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  .../bindings/clock/microchip,sparx5-dpll.yaml | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/microchip,sparx5-dpll.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/microchip,sparx5-dpll.yaml b/Documentation/devicetree/bindings/clock/microchip,sparx5-dpll.yaml
> new file mode 100644
> index 0000000000000..594007d8fc59a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/microchip,sparx5-dpll.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/microchip,sparx5-dpll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip Sparx5 DPLL Clock
> +
> +maintainers:
> +  - Lars Povlsen <lars.povlsen@microchip.com>
> +
> +description: |
> +  The Sparx5 DPLL clock controller generates and supplies clock to
> +  various peripherals within the SoC.
> +
> +  This binding uses common clock bindings
> +  [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +properties:
> +  compatible:
> +    const: microchip,sparx5-dpll
> +
> +  reg:
> +    items:
> +      - description: dpll registers

For a single entry, just:

maxItems: 1

> +
> +  '#clock-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  # Clock provider for eMMC:
> +  - |
> +    clks: clks@61110000c {

clock-controller@1110000c {

> +         compatible = "microchip,sparx5-dpll";
> +         #clock-cells = <1>;
> +         reg = <0x1110000c 0x24>;

Looks like this is a sub-block in some other h/w block. What's the 
parent device? That should be described and this should be part of it 
either as a single node or a child node. Without a complete view of what 
this block has I can't provide any guidance.

Rob

^ permalink raw reply

* Re: [PATCH v12 5/7] clk: Ingenic: Add CGU driver for X1830.
From: Zhou Yanjie @ 2020-05-28  2:14 UTC (permalink / raw)
  To: Stephen Boyd, linux-clk
  Cc: linux-kernel, devicetree, mturquette, robh+dt, dongsheng.qiu,
	aric.pzqi, rick.tyliu, yanfei.li, sernia.zhou, zhenwenjin, paul
In-Reply-To: <159062842562.69627.2356351510003565560@swboyd.mtv.corp.google.com>

Hi Stephen,

在 2020/5/28 上午9:13, Stephen Boyd 写道:
> Quoting Zhou Yanjie (2020-05-27 10:56:33)
>> diff --git a/drivers/clk/ingenic/x1830-cgu.c b/drivers/clk/ingenic/x1830-cgu.c
>> new file mode 100644
>> index 000000000000..29a637f4a2cc
>> --- /dev/null
>> +++ b/drivers/clk/ingenic/x1830-cgu.c
>> @@ -0,0 +1,443 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * X1830 SoC CGU driver
>> + * Copyright (c) 2019 \u5468\u7430\u6770 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
> Add linux/io.h here.


Sure.


>> +#include <linux/of.h>
>> +
>> +#include <dt-bindings/clock/x1830-cgu.h>
>> +
>> +#include "cgu.h"
> [...]
>> +               return;
>> +       }
>> +
>> +       ingenic_cgu_register_syscore_ops(cgu);
>> +}
>> +CLK_OF_DECLARE_DRIVER(x1830_cgu, "ingenic,x1830-cgu", x1830_cgu_init);
> Same question about why this is DECLARE_DRIVER.


CGU has some children devices, this is useful for probing children 
devices in the case where the device node is compatible with 
"simple-mfd" (see commit 03d570e1a4dc for a reference).

Thanks and best regards!



^ permalink raw reply

* Re: [PATCH v12 7/7] clk: X1000: Add FIXDIV for SSI clock of X1000.
From: Zhou Yanjie @ 2020-05-28  2:13 UTC (permalink / raw)
  To: Stephen Boyd, linux-clk
  Cc: linux-kernel, devicetree, mturquette, robh+dt, dongsheng.qiu,
	aric.pzqi, rick.tyliu, yanfei.li, sernia.zhou, zhenwenjin, paul
In-Reply-To: <159062837338.69627.14365746093599072888@swboyd.mtv.corp.google.com>

Hi Stephen,

在 2020/5/28 上午9:12, Stephen Boyd 写道:
> Quoting Zhou Yanjie (2020-05-27 10:56:35)
>> @@ -40,8 +43,47 @@
>>   #define OPCR_SPENDN0           BIT(7)
>>   #define OPCR_SPENDN1           BIT(6)
>>   
>> +/* bits within the USBPCR register */
>> +#define USBPCR_SIDDQ           BIT(21)
>> +#define USBPCR_OTG_DISABLE     BIT(20)
>> +
>>   static struct ingenic_cgu *cgu;
>>   
>> +static int x1000_usb_phy_enable(struct clk_hw *hw)
>> +{
>> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
>> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
>> +
>> +       writel(readl(reg_opcr) | OPCR_SPENDN0, reg_opcr);
> Please include linux/io.h for writel/readl.


Sure, I'll add it.


>> +       writel(readl(reg_usbpcr) & ~USBPCR_OTG_DISABLE & ~USBPCR_SIDDQ, reg_usbpcr);
>> +       return 0;
>> +}
>> +
>> +static void x1000_usb_phy_disable(struct clk_hw *hw)
>> +{
>> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
>> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
>> +
>> +       writel(readl(reg_opcr) & ~OPCR_SPENDN0, reg_opcr);
>> +       writel(readl(reg_usbpcr) | USBPCR_OTG_DISABLE | USBPCR_SIDDQ, reg_usbpcr);
>> +}
>> +
>> +static int x1000_usb_phy_is_enabled(struct clk_hw *hw)
>> +{
>> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
>> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
>> +
>> +       return (readl(reg_opcr) & OPCR_SPENDN0) &&
>> +               !(readl(reg_usbpcr) & USBPCR_SIDDQ) &&
>> +               !(readl(reg_usbpcr) & USBPCR_OTG_DISABLE);
>> +}
>> +
>> +static const struct clk_ops x1000_otg_phy_ops = {
>> +       .enable         = x1000_usb_phy_enable,
>> +       .disable        = x1000_usb_phy_disable,
>> +       .is_enabled     = x1000_usb_phy_is_enabled,
>> +};
>> +
>>   static const s8 pll_od_encoding[8] = {
>>          0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
>>   };
>> @@ -277,4 +377,4 @@ static void __init x1000_cgu_init(struct device_node *np)
>>   
>>          ingenic_cgu_register_syscore_ops(cgu);
>>   }
>> -CLK_OF_DECLARE(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);
>> +CLK_OF_DECLARE_DRIVER(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);
> Why does this change to DECLARE_DRIVER? Can you please add a comment
> here in the code indicating what other driver is probing this compatible
> string?


Yes, CGU has some children devices, this is useful for probing children 
devices in the case where the device node is compatible with 
"simple-mfd" (see commit 03d570e1a4dc for a reference).

Thanks and best regards!



^ permalink raw reply

* Re: [PATCH 05/14] dt-bindings: arm: sparx5: Add documentation for Microchip Sparx5 SoC
From: Rob Herring @ 2020-05-28  2:11 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: SoC Team, Arnd Bergmann, Stephen Boyd, Linus Walleij,
	Steen Hegelund, Microchip Linux Driver Support, Olof Johansson,
	Michael Turquette, devicetree, linux-clk, linux-gpio,
	linux-arm-kernel, linux-kernel, Alexandre Belloni
In-Reply-To: <20200513125532.24585-6-lars.povlsen@microchip.com>

On Wed, May 13, 2020 at 02:55:23PM +0200, Lars Povlsen wrote:
> This adds the main Sparx5 SoC DT documentation file, with information
> abut the supported board types.
> 
> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  .../bindings/arm/microchip,sparx5.yaml        | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/microchip,sparx5.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/microchip,sparx5.yaml b/Documentation/devicetree/bindings/arm/microchip,sparx5.yaml
> new file mode 100644
> index 0000000000000..83b36d1217988
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/microchip,sparx5.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/microchip,sparx5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip Sparx5 Boards Device Tree Bindings
> +
> +maintainers:
> +  - Lars Povlsen <lars.povlsen@microchip.com>
> +
> +description: |+
> +   The Microchip Sparx5 SoC is a ARMv8-based used in a family of
> +   gigabit TSN-capable gigabit switches.
> +
> +   The SparX-5 Ethernet switch family provides a rich set of switching
> +   features such as advanced TCAM-based VLAN and QoS processing
> +   enabling delivery of differentiated services, and security through
> +   TCAM-based frame processing using versatile content aware processor
> +   (VCAP)
> +
> +properties:
> +  $nodename:
> +    const: '/'
> +  compatible:
> +    oneOf:
> +      - description: The Sparx5 pcb125 board is a modular board,
> +          which has both spi-nor and eMMC storage. The modular design
> +          allows for connection of different network ports.
> +        items:
> +          - const: microchip,sparx5-pcb125
> +          - const: microchip,sparx5
> +
> +      - description: The Sparx5 pcb134 is a pizzabox form factor
> +          gigabit switch with 20 SFP ports. It features spi-nor and
> +          either spi-nand or eMMC storage (mount option).
> +        items:
> +          - const: microchip,sparx5-pcb134
> +          - const: microchip,sparx5
> +
> +      - description: The Sparx5 pcb135 is a pizzabox form factor
> +          gigabit switch with 48+4 Cu ports. It features spi-nor and
> +          either spi-nand or eMMC storage (mount option).
> +        items:
> +          - const: microchip,sparx5-pcb135
> +          - const: microchip,sparx5
> +
> +  axi@600000000:
> +    type: object
> +    description: the root node in the Sparx5 platforms must contain
> +      an axi bus child node. They are always at physical address
> +      0x600000000 in all the Sparx5 variants.
> +    properties:
> +      compatible:
> +        items:
> +          - const: simple-bus
> +      reg:
> +        maxItems: 1

simple-bus doesn't have 'reg'. If there's bus registers, then it's not 
simple.

> +
> +    required:
> +      - compatible
> +      - reg
> +
> +patternProperties:
> +  "^syscon@[0-9a-f]+$":

This should be under a bus node.

> +    description: All Sparx5 boards must provide a system controller,
> +      typically under the axi bus node. It contain reset registers and
> +      other system control.
> +    type: object
> +    properties:
> +      compatible:
> +        items:
> +          - const: microchip,sparx5-cpu-syscon
> +          - const: syscon

This probably should be in its own document. If really this simple, 
there's already syscon.yaml you can add to. 

> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
> +required:
> +  - compatible
> +  - axi@600000000
> +  - syscon@600000000
> +
> +...
> --
> 2.26.2

^ permalink raw reply

* Re: [PATCHv1 2/2] dt-bindings: power: supply: gpio-charger: convert to yaml
From: Rob Herring @ 2020-05-28  2:06 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Reichel, linux-pm, devicetree, linux-kernel, kernel
In-Reply-To: <20200513115601.360642-2-sebastian.reichel@collabora.com>

On Wed, May 13, 2020 at 01:56:01PM +0200, Sebastian Reichel wrote:
> Convert the gpio-charger bindings from text format to
> new YAML based representation.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/power/supply/gpio-charger.txt    | 38 ----------
>  .../bindings/power/supply/gpio-charger.yaml   | 75 +++++++++++++++++++
>  2 files changed, 75 insertions(+), 38 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/power/supply/gpio-charger.txt
>  create mode 100644 Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> deleted file mode 100644
> index dbfd29029f69..000000000000
> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -gpio-charger
> -
> -Required properties :
> - - compatible : "gpio-charger"
> - - charger-type : power supply type, one of
> -     unknown
> -     battery
> -     ups
> -     mains
> -     usb-sdp (USB standard downstream port)
> -     usb-dcp (USB dedicated charging port)
> -     usb-cdp (USB charging downstream port)
> -     usb-aca (USB accessory charger adapter)
> -
> -Optional properties:
> - - gpios : GPIO indicating the charger presence.
> -   See GPIO binding in bindings/gpio/gpio.txt .
> - - charge-status-gpios: GPIO indicating whether a battery is charging.
> - - charge-current-limit-gpios: Output GPIOs specifiers for limiting the charge current
> - - charge-current-limit-mapping: List of touples with current in uA and a GPIO bitmap (in this order).
> -                                The GPIOs are encoded in the same order as specified in charge-current-limit-gpios.
> -				The touples must be provided in descending order of the current limit.
> -
> -Example:
> -
> -	usb_charger: charger {
> -		compatible = "gpio-charger";
> -		charger-type = "usb-sdp";
> -		gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
> -		charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
> -
> -		charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>, <&gpioA 12 GPIO_ACTIVE_HIGH>;
> -		charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
> -	};
> -
> -	battery {
> -		power-supplies = <&usb_charger>;
> -	};
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> new file mode 100644
> index 000000000000..14fb3e54f861
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/gpio-charger.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: simple battery chargers only communicating through GPIOs
> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description: |

Can drop '|' if formatting is not important.

> +  This binding is for all chargers, which are working more
> +  or less autonomously, only providing some status GPIOs
> +  and possibly some GPIOs for limited control over the
> +  charging process.
> +
> +properties:
> +  compatible:
> +    const: gpio-charger
> +
> +  charger-type:
> +    oneOf:
> +      - const: unknown
> +      - const: battery
> +      - const: ups
> +      - const: mains
> +      - const: usb-sdp                   # USB standard downstream port
> +      - const: usb-dcp                   # USB dedicated charging port
> +      - const: usb-cdp                   # USB charging downstream port
> +      - const: usb-aca                   # USB accessory charger adapter

Use enum rather than oneOf+const

Should have a description too.

> +
> +  gpios:
> +    maxItems: 1
> +    description: GPIO indicating the charger presence
> +
> +  charge-status-gpios:
> +    maxItems: 1
> +    description: GPIO indicating the charging status
> +
> +  charge-current-limit-gpios:
> +    minItems: 1
> +    maxItems: 32
> +    description: GPIOs used for current limiting
> +
> +  charge-current-limit-mapping:
> +    description: List of touples with current in uA and a GPIO bitmap (in
> +      this order). The GPIOs are encoded in the same order as specified in
> +      charge-current-limit-gpios. The touples must be provided in descending
> +      order of the current limit.
> +    $ref: "/meta-schemas/cell.yaml#array"

That's a first... A meta-schema is what checks this document. Not what 
you want. Should be like this:

$ref: /schemas/types.yaml#/definitions/uint32-matrix
items:
  items:
    - description: Current limit in uA
    - description: Encoded GPIO setting...

I guess there's not any more constraints we can add here.

> +
> +required:
> +  - compatible

blank line

At least 1 of the gpio properties is required, right? Can be expressed 
with required entries under a oneOf or anyOf.

> +additionalProperties: false
> +
> +dependencies:
> +  charge-current-limit-gpios: [ charge-current-limit-mapping ]
> +  charge-current-limit-mapping: [ charge-current-limit-gpios ]
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    charger {
> +      compatible = "gpio-charger";
> +      charger-type = "usb-sdp";
> +
> +      gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
> +      charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
> +
> +      charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>,
> +                                   <&gpioA 12 GPIO_ACTIVE_HIGH>;
> +      charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
> +    };
> -- 
> 2.26.2
> 

^ permalink raw reply

* Re: [PATCH V6 4/5] clk: qcom: Add ipq6018 apss clock controller
From: Stephen Boyd @ 2020-05-28  1:59 UTC (permalink / raw)
  To: Sivaprakash Murugesan, agross, bjorn.andersson, devicetree,
	linux-arm-msm, linux-clk, linux-kernel, mturquette, robh+dt
  Cc: Sivaprakash Murugesan
In-Reply-To: <1590582292-13314-5-git-send-email-sivaprak@codeaurora.org>

Quoting Sivaprakash Murugesan (2020-05-27 05:24:51)
> diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
> new file mode 100644
> index 0000000..004f7e1
> --- /dev/null
> +++ b/drivers/clk/qcom/apss-ipq6018.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/module.h>
> +
> +#include <dt-bindings/clock/qcom,apss-ipq.h>
> +
> +#include "common.h"
> +#include "clk-regmap.h"
> +#include "clk-branch.h"
> +#include "clk-alpha-pll.h"
> +#include "clk-regmap-mux.h"
> +
> +enum {
> +       P_XO,
> +       P_APSS_PLL_EARLY,
> +};
> +
> +static const struct clk_parent_data parents_apcs_alias0_clk_src[] = {
> +       { .fw_name = "xo" },
> +       { .fw_name = "pll" },

This pll clk is not described in the binding. Please add it there.

> +};
> +
> +static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
> +       { P_XO, 0 },
> +       { P_APSS_PLL_EARLY, 5 },
> +};
> +

^ permalink raw reply

* Re: [PATCH V6 5/5] arm64: dts: ipq6018: Add support for apss pll
From: Stephen Boyd @ 2020-05-28  1:56 UTC (permalink / raw)
  To: Sivaprakash Murugesan, agross, bjorn.andersson, devicetree,
	linux-arm-msm, linux-clk, linux-kernel, mturquette, robh+dt
  Cc: Sivaprakash Murugesan
In-Reply-To: <1590582292-13314-6-git-send-email-sivaprak@codeaurora.org>

Quoting Sivaprakash Murugesan (2020-05-27 05:24:52)
> Enable apss pll support.
> 
> Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
> ---
> [V6]
>  * split the mailbox driver from this patch
>  arch/arm64/boot/dts/qcom/ipq6018.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> index 1aa8d85..3956e44 100644
> --- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> @@ -300,6 +300,14 @@
>                         #mbox-cells = <1>;
>                 };
>  
> +               apsspll: clock@b116000 {
> +                       compatible = "qcom,ipq6018-a53pll";
> +                       reg = <0x0b116000 0x40>;
> +                       #clock-cells = <0>;
> +                       clocks = <&xo>;
> +                       clock-names = "xo";
> +               };
> +

I'd expect to see this inside an soc node. Also this doesn't go via clk
tree so don't send it with the clk patches.

>                 timer {
>                         compatible = "arm,armv8-timer";
>                         interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,

^ permalink raw reply

* Re: [PATCHv1 1/2] power: supply: gpio-charger: add charge-current-limit feature
From: Rob Herring @ 2020-05-28  1:56 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Reichel, linux-pm, devicetree, linux-kernel, kernel
In-Reply-To: <20200513115601.360642-1-sebastian.reichel@collabora.com>

On Wed, May 13, 2020 at 01:56:00PM +0200, Sebastian Reichel wrote:
> Add new charge-current-limit feature to gpio-charger. This also
> makes the online status GPIO optional, since hardware might only
> expose the charge-current-limit feature and there is no good reason
> to have it mandatory now that different GPIOs are supported.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/power/supply/gpio-charger.txt    |  11 +-
>  drivers/power/supply/gpio-charger.c           | 176 ++++++++++++++++--
>  2 files changed, 174 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> index 0fb33b2c62a6..dbfd29029f69 100644
> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.txt
> @@ -2,8 +2,6 @@ gpio-charger
>  
>  Required properties :
>   - compatible : "gpio-charger"
> - - gpios : GPIO indicating the charger presence.
> -   See GPIO binding in bindings/gpio/gpio.txt .
>   - charger-type : power supply type, one of
>       unknown
>       battery
> @@ -15,7 +13,13 @@ Required properties :
>       usb-aca (USB accessory charger adapter)
>  
>  Optional properties:
> + - gpios : GPIO indicating the charger presence.
> +   See GPIO binding in bindings/gpio/gpio.txt .
>   - charge-status-gpios: GPIO indicating whether a battery is charging.
> + - charge-current-limit-gpios: Output GPIOs specifiers for limiting the charge current
> + - charge-current-limit-mapping: List of touples with current in uA and a GPIO bitmap (in this order).
> +                                The GPIOs are encoded in the same order as specified in charge-current-limit-gpios.

TBC, index 0 GPIO is bit 0?

> +				The touples must be provided in descending order of the current limit.

tuples

Wrap at 80 columns.

>  
>  Example:
>  
> @@ -24,6 +28,9 @@ Example:
>  		charger-type = "usb-sdp";
>  		gpios = <&gpd 28 GPIO_ACTIVE_LOW>;
>  		charge-status-gpios = <&gpc 27 GPIO_ACTIVE_LOW>;
> +
> +		charge-current-limit-gpios = <&gpioA 11 GPIO_ACTIVE_HIGH>, <&gpioA 12 GPIO_ACTIVE_HIGH>;
> +		charge-current-limit-mapping = <2500000 0x00>, <700000 0x01>, <0 0x02>;
>  	};
>  
>  	battery {
> diff --git a/drivers/power/supply/gpio-charger.c b/drivers/power/supply/gpio-charger.c
> index 1b959c7f8b0e..4a5eac7cc36c 100644
> --- a/drivers/power/supply/gpio-charger.c
> +++ b/drivers/power/supply/gpio-charger.c
> @@ -18,7 +18,13 @@
>  
>  #include <linux/power/gpio-charger.h>
>  
> +struct gpio_mapping {
> +	u32 limit_ua;
> +	u32 gpiodata;
> +} __packed;
> +
>  struct gpio_charger {
> +	struct device *dev;
>  	unsigned int irq;
>  	unsigned int charge_status_irq;
>  	bool wakeup_enabled;
> @@ -27,6 +33,11 @@ struct gpio_charger {
>  	struct power_supply_desc charger_desc;
>  	struct gpio_desc *gpiod;
>  	struct gpio_desc *charge_status;
> +
> +	struct gpio_descs *current_limit_gpios;
> +	struct gpio_mapping *current_limit_map;
> +	u32 current_limit_map_size;
> +	u32 charge_current_limit;
>  };
>  
>  static irqreturn_t gpio_charger_irq(int irq, void *devid)
> @@ -43,6 +54,35 @@ static inline struct gpio_charger *psy_to_gpio_charger(struct power_supply *psy)
>  	return power_supply_get_drvdata(psy);
>  }
>  
> +static int set_charge_current_limit(struct gpio_charger *gpio_charger, int val)
> +{
> +	struct gpio_mapping mapping;
> +	int ndescs = gpio_charger->current_limit_gpios->ndescs;
> +	struct gpio_desc **gpios = gpio_charger->current_limit_gpios->desc;
> +	int i;
> +
> +	if (!gpio_charger->current_limit_map_size)
> +		return -EINVAL;
> +
> +	for (i = 0; i < gpio_charger->current_limit_map_size; i++) {
> +		if (gpio_charger->current_limit_map[i].limit_ua <= val)
> +			break;
> +	}
> +	mapping = gpio_charger->current_limit_map[i];
> +
> +	for (i = 0; i < ndescs; i++) {
> +		bool val = (mapping.gpiodata >> i) & 1;
> +		gpiod_set_value_cansleep(gpios[ndescs-i-1], val);
> +	}
> +
> +	gpio_charger->charge_current_limit = mapping.limit_ua;
> +
> +	dev_dbg(gpio_charger->dev, "set charge current limit to %d (requested: %d)\n",
> +		gpio_charger->charge_current_limit, val);
> +
> +	return 0;
> +}
> +
>  static int gpio_charger_get_property(struct power_supply *psy,
>  		enum power_supply_property psp, union power_supply_propval *val)
>  {
> @@ -58,6 +98,9 @@ static int gpio_charger_get_property(struct power_supply *psy,
>  		else
>  			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
>  		break;
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		val->intval = gpio_charger->charge_current_limit;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -65,6 +108,34 @@ static int gpio_charger_get_property(struct power_supply *psy,
>  	return 0;
>  }
>  
> +static int gpio_charger_set_property(struct power_supply *psy,
> +	enum power_supply_property psp, const union power_supply_propval *val)
> +{
> +	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		return set_charge_current_limit(gpio_charger, val->intval);
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpio_charger_property_is_writeable(struct power_supply *psy,
> +					      enum power_supply_property psp)
> +{
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		return 1;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static enum power_supply_type gpio_charger_get_type(struct device *dev)
>  {
>  	const char *chargetype;
> @@ -112,9 +183,70 @@ static int gpio_charger_get_irq(struct device *dev, void *dev_id,
>  	return irq;
>  }
>  
> +static int init_charge_current_limit(struct device *dev,
> +				    struct gpio_charger *gpio_charger)
> +{
> +	int i, len;
> +	u32 cur_limit = U32_MAX;
> +
> +	gpio_charger->current_limit_gpios = devm_gpiod_get_array_optional(dev,
> +		"charge-current-limit", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpio_charger->current_limit_gpios)) {
> +		dev_err(dev, "error getting current-limit GPIOs\n");
> +		return PTR_ERR(gpio_charger->current_limit_gpios);
> +	}
> +
> +	if (!gpio_charger->current_limit_gpios)
> +		return 0;
> +
> +	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
> +		NULL, 0);
> +	if (len < 0)
> +		return len;
> +
> +	if (len % 2) {
> +		dev_err(dev, "invalid charge-current-limit-mapping length\n");
> +		return -EINVAL;
> +	}
> +
> +	gpio_charger->current_limit_map = devm_kmalloc_array(dev,
> +		len / 2, sizeof(*gpio_charger->current_limit_map), GFP_KERNEL);
> +	if (!gpio_charger->current_limit_map)
> +		return -ENOMEM;
> +
> +	gpio_charger->current_limit_map_size = len / 2;
> +
> +	len = device_property_read_u32_array(dev, "charge-current-limit-mapping",
> +		(u32*) gpio_charger->current_limit_map, len);
> +	if (len < 0)
> +		return len;
> +
> +	for (i=0; i < gpio_charger->current_limit_map_size; i++) {
> +		if (gpio_charger->current_limit_map[i].limit_ua > cur_limit) {
> +			dev_err(dev, "invalid charge-current-limit-mapping\n");
> +			return -EINVAL;
> +		}
> +
> +		cur_limit = gpio_charger->current_limit_map[i].limit_ua;
> +	}
> +
> +	/* default to smallest current limitation for safety reasons */
> +	len = gpio_charger->current_limit_map_size - 1;
> +	set_charge_current_limit(gpio_charger,
> +		gpio_charger->current_limit_map[len].limit_ua);
> +
> +	return 0;
> +}
> +
> +/*
> + * The entries will be overwritten by driver's probe routine depending
> + * on the available features. This list ensures, that the array is big
> + * enough for all optional features.
> + */
>  static enum power_supply_property gpio_charger_properties[] = {
>  	POWER_SUPPLY_PROP_ONLINE,
> -	POWER_SUPPLY_PROP_STATUS /* Must always be last in the array. */
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>  };
>  
>  static int gpio_charger_probe(struct platform_device *pdev)
> @@ -128,6 +260,7 @@ static int gpio_charger_probe(struct platform_device *pdev)
>  	int charge_status_irq;
>  	unsigned long flags;
>  	int ret;
> +	int num_props = 0;
>  
>  	if (!pdata && !dev->of_node) {
>  		dev_err(dev, "No platform data\n");
> @@ -137,18 +270,19 @@ static int gpio_charger_probe(struct platform_device *pdev)
>  	gpio_charger = devm_kzalloc(dev, sizeof(*gpio_charger), GFP_KERNEL);
>  	if (!gpio_charger)
>  		return -ENOMEM;
> +	gpio_charger->dev = dev;
>  
>  	/*
>  	 * This will fetch a GPIO descriptor from device tree, ACPI or
>  	 * boardfile descriptor tables. It's good to try this first.
>  	 */
> -	gpio_charger->gpiod = devm_gpiod_get(dev, NULL, GPIOD_IN);
> +	gpio_charger->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
>  
>  	/*
>  	 * If this fails and we're not using device tree, try the
>  	 * legacy platform data method.
>  	 */
> -	if (IS_ERR(gpio_charger->gpiod) && !dev->of_node) {
> +	if (!gpio_charger->gpiod && !dev->of_node) {
>  		/* Non-DT: use legacy GPIO numbers */
>  		if (!gpio_is_valid(pdata->gpio)) {
>  			dev_err(dev, "Invalid gpio pin in pdata\n");
> @@ -173,18 +307,38 @@ static int gpio_charger_probe(struct platform_device *pdev)
>  		return PTR_ERR(gpio_charger->gpiod);
>  	}
>  
> +	if (gpio_charger->gpiod &&
> +	    num_props < ARRAY_SIZE(gpio_charger_properties)) {
> +		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_ONLINE;
> +		num_props++;
> +	}
> +
>  	charge_status = devm_gpiod_get_optional(dev, "charge-status", GPIOD_IN);
> -	gpio_charger->charge_status = charge_status;
> -	if (IS_ERR(gpio_charger->charge_status))
> -		return PTR_ERR(gpio_charger->charge_status);
> +	if (IS_ERR(charge_status))
> +		return PTR_ERR(charge_status);
> +	if (charge_status && num_props < ARRAY_SIZE(gpio_charger_properties)) {
> +		gpio_charger->charge_status = charge_status;
> +		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_STATUS;
> +		num_props++;
> +	}
> +
> +	ret = init_charge_current_limit(dev, gpio_charger);
> +	if (ret < 0)
> +		return ret;
> +	if (gpio_charger->current_limit_map &&
> +	    num_props < ARRAY_SIZE(gpio_charger_properties)) {
> +		gpio_charger_properties[num_props] =
> +			POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
> +		num_props++;
> +	}
>  
>  	charger_desc = &gpio_charger->charger_desc;
>  	charger_desc->properties = gpio_charger_properties;
> -	charger_desc->num_properties = ARRAY_SIZE(gpio_charger_properties);
> -	/* Remove POWER_SUPPLY_PROP_STATUS from the supported properties. */
> -	if (!gpio_charger->charge_status)
> -		charger_desc->num_properties -= 1;
> +	charger_desc->num_properties = num_props;
>  	charger_desc->get_property = gpio_charger_get_property;
> +	charger_desc->set_property = gpio_charger_set_property;
> +	charger_desc->property_is_writeable =
> +					gpio_charger_property_is_writeable;
>  
>  	psy_cfg.of_node = dev->of_node;
>  	psy_cfg.drv_data = gpio_charger;
> @@ -269,6 +423,6 @@ static struct platform_driver gpio_charger_driver = {
>  module_platform_driver(gpio_charger_driver);
>  
>  MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> -MODULE_DESCRIPTION("Driver for chargers which report their online status through a GPIO");
> +MODULE_DESCRIPTION("Driver for chargers only communicating via GPIO(s)");
>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS("platform:gpio-charger");
> -- 
> 2.26.2
> 

^ permalink raw reply

* Re: [PATCH v8 1/3] dt-bindings: phy: Add DT bindings for Xilinx ZynqMP PSGTR PHY
From: Laurent Pinchart @ 2020-05-28  1:55 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Michal Simek, devicetree, Rob Herring, Anurag Kumar Vulisha,
	linux-kernel, Vinod Koul
In-Reply-To: <20200526183201.GA134956@bogus>

On Tue, May 26, 2020 at 12:32:01PM -0600, Rob Herring wrote:
> On Wed, 13 May 2020 20:22:37 +0300, Laurent Pinchart wrote:
> > From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > 
> > Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> > Processing System Gigabit Transceiver which provides PHY capabilities to
> > USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.
> > 
> > Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v7:
> > 
> > - Switch to GPL-2.0-only OR BSD-2-Clause
> > 
> > Changes since v6:
> > 
> > - Fixed specification of compatible-dependent xlnx,tx-termination-fix
> >   property
> > - Dropped status property from example
> > - Use 4 spaces to indent example
> > 
> > Changes since v5:
> > 
> > - Document clocks and clock-names properties
> > - Document resets and reset-names properties
> > - Replace subnodes with an additional entry in the PHY cells
> > - Drop lane frequency PHY cell, replaced by reference clock phandle
> > - Convert bindings to YAML
> > - Reword the subject line
> > - Drop Rob's R-b as the bindings have significantly changed
> > - Drop resets and reset-names properties
> > ---
> >  .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 105 ++++++++++++++++++
> >  include/dt-bindings/phy/phy.h                 |   1 +
> >  2 files changed, 106 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thank you Rob.

Kishon, now that the bindings have been acked, could you please take the
series in your tree (which I assume to tbe
https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/) ?
Is it too late for v5.8 ?

-- 
Regards,

Laurent Pinchart

^ permalink raw reply

* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
From: Laurent Pinchart @ 2020-05-28  1:52 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Daniel Vetter, David Airlie, Rob Clark, linux-arm-msm,
	Bjorn Andersson, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
	Jernej Skrabec, Rob Herring, devicetree, linux-kernel, dri-devel
In-Reply-To: <20200513100533.42996-4-vkoul@kernel.org>

Hi Vinod,

Thank you for the patch.

On Wed, May 13, 2020 at 03:35:33PM +0530, Vinod Koul wrote:
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
> 
> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig  |   13 +
>  drivers/gpu/drm/bridge/Makefile |    1 +
>  drivers/gpu/drm/bridge/lt9611.c | 1113 +++++++++++++++++++++++++++++++
>  3 files changed, 1127 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/lt9611.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index aaed2347ace9..5cac15ce2027 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -38,6 +38,19 @@ config DRM_DISPLAY_CONNECTOR
>  	  on ARM-based platforms. Saying Y here when this driver is not needed
>  	  will not cause any issue.
>  
> +config DRM_LONTIUM_LT9611
> +	tristate "Lontium LT9611 DSI/HDMI bridge"
> +	select SND_SOC_HDMI_CODEC if SND_SOC
> +	depends on OF
> +	select DRM_PANEL_BRIDGE
> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C
> +	help
> +	  Driver for Lontium LT9611 DSI to HDMI bridge
> +	  chip driver that converts dual DSI and I2S to
> +	  HDMI signals
> +	  Please say Y if you have such hardware.
> +
>  config DRM_LVDS_CODEC
>  	tristate "Transparent LVDS encoders and decoders support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 6fb062b5b0f0..d2a696e8ca5d 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>  obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
> +obj-$(CONFIG_DRM_LONTIUM_LT9611) += lt9611.o
>  obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lt9611.c b/drivers/gpu/drm/bridge/lt9611.c
> new file mode 100644
> index 000000000000..e6e7ce43980d
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c
> @@ -0,0 +1,1113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2019-2020. Linaro Limited.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_print.h>
> +
> +#define EDID_SEG_SIZE 256
> +
> +#define LT9611_4LANES	0
> +
> +struct lt9611 {
> +	struct device *dev;
> +	struct drm_bridge bridge;
> +	struct drm_connector connector;
> +
> +	struct regmap *regmap;
> +
> +	struct device_node *dsi0_node;
> +	struct device_node *dsi1_node;
> +	struct mipi_dsi_device *dsi0;
> +	struct mipi_dsi_device *dsi1;
> +
> +	bool ac_mode;
> +
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *enable_gpio;
> +
> +	bool power_on;
> +	bool sleep;
> +
> +	struct regulator_bulk_data supplies[2];
> +
> +	struct i2c_client *client;
> +
> +	enum drm_connector_status status;
> +
> +	u8 edid_buf[EDID_SEG_SIZE];
> +	u32 vic;
> +};
> +
> +#define LT9611_PAGE_CONTROL	0xff
> +
> +static const struct regmap_range_cfg lt9611_ranges[] = {
> +	{
> +		.name = "register_range",
> +		.range_min =  0,
> +		.range_max = 0x85ff,
> +		.selector_reg = LT9611_PAGE_CONTROL,
> +		.selector_mask = 0xff,
> +		.selector_shift = 0,
> +		.window_start = 0,
> +		.window_len = 0x100,
> +	},
> +};
> +
> +static const struct regmap_config lt9611_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xffff,
> +	.ranges = lt9611_ranges,
> +	.num_ranges = ARRAY_SIZE(lt9611_ranges),
> +};
> +
> +struct lt9611_mode {
> +	u16 hdisplay;
> +	u16 vdisplay;
> +	u8 fps;
> +	u8 lanes;
> +	u8 intfs;
> +};
> +
> +static struct lt9611_mode lt9611_modes[] = {
> +	{ 3840, 2160, 30, 4, 2 }, /* 3840x2160 24bit 30Hz 4Lane 2ports */
> +	{ 1920, 1080, 60, 4, 1 }, /* 1080P 24bit 60Hz 4lane 1port */
> +	{ 1920, 1080, 30, 3, 1 }, /* 1080P 24bit 30Hz 3lane 1port */
> +	{ 1920, 1080, 24, 3, 1 },
> +	{ 720, 480, 60, 4, 1 },
> +	{ 720, 576, 50, 2, 1 },
> +	{ 640, 480, 60, 2, 1 },
> +};
> +
> +static struct lt9611 *bridge_to_lt9611(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct lt9611, bridge);
> +}
> +
> +static struct lt9611 *connector_to_lt9611(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct lt9611, connector);
> +}
> +
> +static int lt9611_mipi_input_analog(struct lt9611 *lt9611)
> +{
> +	struct reg_sequence reg_cfg[] = {
> +		{ 0x8106, 0x40 }, /*port A rx current*/
> +		{ 0x810a, 0xfe }, /*port A ldo voltage set*/
> +		{ 0x810b, 0xbf }, /*enable port A lprx*/
> +		{ 0x8111, 0x40 }, /*port B rx current*/
> +		{ 0x8115, 0xfe }, /*port B ldo voltage set*/
> +		{ 0x8116, 0xbf }, /*enable port B lprx*/
> +
> +		{ 0x811c, 0x03 }, /*PortA clk lane no-LP mode*/
> +		{ 0x8120, 0x03 }, /*PortB clk lane with-LP mode*/
> +	};
> +
> +	regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> +	return 0;
> +}
> +
> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> +				     const struct drm_display_mode *mode)
> +{
> +	regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> +	if (mode->hdisplay == 3840)
> +		regmap_write(lt9611->regmap, 0x830a, 0x03);
> +	else
> +		regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> +	regmap_write(lt9611->regmap, 0x824f, 0x80);
> +	regmap_write(lt9611->regmap, 0x8250, 0x10);
> +	regmap_write(lt9611->regmap, 0x8302, 0x0a);
> +	regmap_write(lt9611->regmap, 0x8306, 0x0a);
> +
> +	return 0;
> +}
> +
> +static void lt9611_mipi_video_setup(struct lt9611 *lt9611,
> +				    const struct drm_display_mode *mode)
> +{
> +	u32 h_total, h_act, hpw, hfp, hss;
> +	u32 v_total, v_act, vpw, vfp, vss;
> +
> +	h_total = mode->htotal;
> +	v_total = mode->vtotal;
> +
> +	h_act = mode->hdisplay;
> +	hpw = mode->hsync_end - mode->hsync_start;
> +	hfp = mode->hsync_start - mode->hdisplay;
> +	hss = (mode->hsync_end - mode->hsync_start) +
> +	      (mode->htotal - mode->hsync_end);
> +
> +	v_act = mode->vdisplay;
> +	vpw = mode->vsync_end - mode->vsync_start;
> +	vfp = mode->vsync_start - mode->vdisplay;
> +	vss = (mode->vsync_end - mode->vsync_start) +
> +	      (mode->vtotal - mode->vsync_end);
> +
> +	regmap_write(lt9611->regmap, 0x830d, (u8)(v_total / 256));
> +	regmap_write(lt9611->regmap, 0x830e, (u8)(v_total % 256));
> +
> +	regmap_write(lt9611->regmap, 0x830f, (u8)(v_act / 256));
> +	regmap_write(lt9611->regmap, 0x8310, (u8)(v_act % 256));
> +
> +	regmap_write(lt9611->regmap, 0x8311, (u8)(h_total / 256));
> +	regmap_write(lt9611->regmap, 0x8312, (u8)(h_total % 256));
> +
> +	regmap_write(lt9611->regmap, 0x8313, (u8)(h_act / 256));
> +	regmap_write(lt9611->regmap, 0x8314, (u8)(h_act % 256));
> +
> +	regmap_write(lt9611->regmap, 0x8315, (u8)(vpw % 256));
> +	regmap_write(lt9611->regmap, 0x8316, (u8)(hpw % 256));
> +
> +	regmap_write(lt9611->regmap, 0x8317, (u8)(vfp % 256));
> +
> +	regmap_write(lt9611->regmap, 0x8318, (u8)(vss % 256));
> +
> +	regmap_write(lt9611->regmap, 0x8319, (u8)(hfp % 256));
> +
> +	regmap_write(lt9611->regmap, 0x831a, (u8)(hss / 256));
> +	regmap_write(lt9611->regmap, 0x831b, (u8)(hss % 256));
> +}
> +
> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> +			    const struct drm_display_mode *mode)
> +{
> +	struct reg_sequence reg_cfg[] = {
> +		{ 0x830b, 0x01 },
> +		{ 0x830c, 0x10 },
> +		{ 0x8348, 0x00 },
> +		{ 0x8349, 0x81 },
> +
> +		/* stage 1 */
> +		{ 0x8321, 0x4a },
> +		{ 0x8324, 0x71 },
> +		{ 0x8325, 0x30 },
> +		{ 0x832a, 0x01 },
> +
> +		/* stage 2 */
> +		{ 0x834a, 0x40 },
> +		{ 0x831d, 0x10 },
> +
> +		/* MK limit */
> +		{ 0x832d, 0x38 },
> +		{ 0x8331, 0x08 },
> +	};
> +
> +	regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> +	switch (mode->hdisplay) {
> +	case 640:
> +		regmap_write(lt9611->regmap, 0x8326, 0x14);
> +		break;
> +	case 1920:
> +		regmap_write(lt9611->regmap, 0x8326, 0x37);
> +		break;
> +	case 3840:
> +		regmap_write(lt9611->regmap, 0x830b, 0x03);
> +		regmap_write(lt9611->regmap, 0x830c, 0xd0);
> +		regmap_write(lt9611->regmap, 0x8348, 0x03);
> +		regmap_write(lt9611->regmap, 0x8349, 0xe0);
> +		regmap_write(lt9611->regmap, 0x8324, 0x72);
> +		regmap_write(lt9611->regmap, 0x8325, 0x00);
> +		regmap_write(lt9611->regmap, 0x832a, 0x01);
> +		regmap_write(lt9611->regmap, 0x834a, 0x10);
> +		regmap_write(lt9611->regmap, 0x831d, 0x10);
> +		regmap_write(lt9611->regmap, 0x8326, 0x37);
> +		break;
> +	}
> +
> +	/* pcr rst */
> +	regmap_write(lt9611->regmap, 0x8011, 0x5a);
> +	regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> +	return 0;
> +}
> +
> +static int lt9611_pll_setup(struct lt9611 *lt9611,
> +			    const struct drm_display_mode *mode)
> +{
> +	unsigned int pclk = mode->clock;
> +	struct reg_sequence reg_cfg[] = {
> +		/* txpll init */
> +		{ 0x8123, 0x40 },
> +		{ 0x8124, 0x64 },
> +		{ 0x8125, 0x80 },
> +		{ 0x8126, 0x55 },
> +		{ 0x812c, 0x37 },
> +		{ 0x812f, 0x01 },
> +		{ 0x8126, 0x55 },
> +		{ 0x8127, 0x66 },
> +		{ 0x8128, 0x88 },
> +	};
> +
> +	regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> +	if (pclk > 150000)
> +		regmap_write(lt9611->regmap, 0x812d, 0x88);
> +	else if (pclk > 70000)
> +		regmap_write(lt9611->regmap, 0x812d, 0x99);
> +	else
> +		regmap_write(lt9611->regmap, 0x812d, 0xaa);
> +
> +	regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> +	regmap_write(lt9611->regmap, 0x82e4, pclk >> 9);  /* pclk[15:8]  */
> +	regmap_write(lt9611->regmap, 0x82e5, pclk >> 1);  /* pclk[7:0]   */
> +
> +	regmap_write(lt9611->regmap, 0x82de, 0x20);
> +	regmap_write(lt9611->regmap, 0x82de, 0xe0);
> +
> +	regmap_write(lt9611->regmap, 0x8016, 0xf1);
> +	regmap_write(lt9611->regmap, 0x8016, 0xf3);
> +
> +	return 0;
> +}
> +
> +static int lt9611_video_check(struct lt9611 *lt9611)
> +{
> +	u32 v_total, v_act, h_act_a, h_act_b, h_total_sysclk;
> +	unsigned int temp;
> +	int ret;
> +
> +	/* top module video check */
> +
> +	/* v_act */
> +	ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> +	if (ret)
> +		goto end;
> +
> +	v_act = temp << 8;
> +	ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> +	if (ret)
> +		goto end;
> +	v_act = v_act + temp;
> +
> +	/* v_total */
> +	ret = regmap_read(lt9611->regmap, 0x826c, &temp);
> +	if (ret)
> +		goto end;
> +	v_total = temp << 8;
> +	ret = regmap_read(lt9611->regmap, 0x826d, &temp);
> +	if (ret)
> +		goto end;
> +	v_total = v_total + temp;
> +
> +	/* h_total_sysclk */
> +	ret = regmap_read(lt9611->regmap, 0x8286, &temp);
> +	if (ret)
> +		goto end;
> +	h_total_sysclk = temp << 8;
> +	ret = regmap_read(lt9611->regmap, 0x8287, &temp);
> +	if (ret)
> +		goto end;
> +	h_total_sysclk = h_total_sysclk + temp;
> +
> +	/* h_act_a */
> +	ret = regmap_read(lt9611->regmap, 0x8382, &temp);
> +	if (ret)
> +		goto end;
> +	h_act_a = temp << 8;
> +	ret = regmap_read(lt9611->regmap, 0x8383, &temp);
> +	if (ret)
> +		goto end;
> +	h_act_a = (h_act_a + temp) / 3;
> +
> +	/* h_act_b */
> +	ret = regmap_read(lt9611->regmap, 0x8386, &temp);
> +	if (ret)
> +		goto end;
> +	h_act_b = temp << 8;
> +	ret = regmap_read(lt9611->regmap, 0x8387, &temp);
> +	if (ret)
> +		goto end;
> +	h_act_b = (h_act_b + temp) / 3;
> +
> +	dev_info(lt9611->dev,
> +		 "video check: h_act_a=%d, h_act_b=%d, v_act=%d, v_total=%d, h_total_sysclk=%d\n",
> +		 h_act_a, h_act_b, v_act, v_total, h_total_sysclk);
> +
> +	return 0;
> +
> +end:
> +	dev_err(lt9611->dev, "read video check error\n");
> +	return ret;
> +}
> +
> +static void lt9611_hdmi_tx_digital(struct lt9611 *lt9611)
> +{
> +	regmap_write(lt9611->regmap, 0x8443, 0x46 - lt9611->vic);
> +	regmap_write(lt9611->regmap, 0x8447, lt9611->vic);
> +	regmap_write(lt9611->regmap, 0x843d, 0x0a); /* UD1 infoframe */
> +
> +	regmap_write(lt9611->regmap, 0x82d6, 0x8c);
> +	regmap_write(lt9611->regmap, 0x82d7, 0x04);
> +}
> +
> +static void lt9611_hdmi_tx_phy(struct lt9611 *lt9611)
> +{
> +	struct reg_sequence reg_cfg[] = {
> +		{ 0x8130, 0x6a },
> +		{ 0x8131, 0x44 }, /* HDMI DC mode */
> +		{ 0x8132, 0x4a },
> +		{ 0x8133, 0x0b },
> +		{ 0x8134, 0x00 },
> +		{ 0x8135, 0x00 },
> +		{ 0x8136, 0x00 },
> +		{ 0x8137, 0x44 },
> +		{ 0x813f, 0x0f },
> +		{ 0x8140, 0xa0 },
> +		{ 0x8141, 0xa0 },
> +		{ 0x8142, 0xa0 },
> +		{ 0x8143, 0xa0 },
> +		{ 0x8144, 0x0a },
> +	};
> +
> +	/* HDMI AC mode */
> +	if (lt9611->ac_mode)
> +		reg_cfg[2].def = 0x73;
> +
> +	regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +}
> +
> +static irqreturn_t lt9611_irq_thread_handler(int irq, void *dev_id)
> +{
> +	struct lt9611 *lt9611 = dev_id;
> +	unsigned int irq_flag0 = 0;
> +	unsigned int irq_flag3 = 0;
> +
> +	regmap_read(lt9611->regmap, 0x820f, &irq_flag3);
> +	regmap_read(lt9611->regmap, 0x820c, &irq_flag0);
> +
> +	pr_debug("%s() irq_flag0: %#x irq_flag3: %#x\n",
> +		 __func__, irq_flag0, irq_flag3);
> +
> +	 /* hpd changed low */
> +	if (irq_flag3 & 0x80) {
> +		dev_info(lt9611->dev, "hdmi cable disconnected\n");
> +
> +		regmap_write(lt9611->regmap, 0x8207, 0xbf);
> +		regmap_write(lt9611->regmap, 0x8207, 0x3f);
> +	}
> +	 /* hpd changed high */
> +	if (irq_flag3 & 0x40) {
> +		dev_info(lt9611->dev, "hdmi cable connected\n");
> +
> +		regmap_write(lt9611->regmap, 0x8207, 0x7f);
> +		regmap_write(lt9611->regmap, 0x8207, 0x3f);
> +	}
> +
> +	if (irq_flag3 & 0xc0)
> +		drm_kms_helper_hotplug_event(lt9611->bridge.dev);
> +
> +	/* video input changed */
> +	if (irq_flag0 & 0x01) {
> +		dev_info(lt9611->dev, "video input changed\n");
> +		regmap_write(lt9611->regmap, 0x829e, 0xff);
> +		regmap_write(lt9611->regmap, 0x829e, 0xf7);
> +		regmap_write(lt9611->regmap, 0x8204, 0xff);
> +		regmap_write(lt9611->regmap, 0x8204, 0xfe);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void lt9611_enable_hpd_interrupts(struct lt9611 *lt9611)
> +{
> +	unsigned int val;
> +
> +	dev_dbg(lt9611->dev, "enabling hpd interrupts\n");
> +
> +	regmap_read(lt9611->regmap, 0x8203, &val);
> +
> +	val &= ~0xc0;
> +	regmap_write(lt9611->regmap, 0x8203, val);
> +	regmap_write(lt9611->regmap, 0x8207, 0xff); //clear
> +	regmap_write(lt9611->regmap, 0x8207, 0x3f);
> +}
> +
> +static void lt9611_sleep_setup(struct lt9611 *lt9611)
> +{
> +	struct reg_sequence sleep_setup[] = {
> +		{ 0x8024, 0x76 },
> +		{ 0x8023, 0x01 },
> +		{ 0x8157, 0x03 }, //set addr pin as output
> +		{ 0x8149, 0x0b },
> +		{ 0x8151, 0x30 }, //disable IRQ
> +		{ 0x8102, 0x48 }, //MIPI Rx power down
> +		{ 0x8123, 0x80 },
> +		{ 0x8130, 0x00 },
> +		{ 0x8100, 0x01 }, //bandgap power down
> +		{ 0x8101, 0x00 }, //system clk power down
> +	};
> +
> +	dev_dbg(lt9611->dev, "sleep\n");
> +
> +	regmap_multi_reg_write(lt9611->regmap,
> +			       sleep_setup, ARRAY_SIZE(sleep_setup));
> +	lt9611->sleep = true;
> +}
> +
> +static int lt9611_power_on(struct lt9611 *lt9611)
> +{
> +	int ret;
> +	const struct reg_sequence seq[] = {
> +		/* LT9611_System_Init */
> +		{ 0x8101, 0x18 }, /* sel xtal clock */
> +
> +		/* timer for frequency meter */
> +		{ 0x821b, 0x69 }, /*timer 2*/
> +		{ 0x821c, 0x78 },
> +		{ 0x82cb, 0x69 }, /*timer 1 */
> +		{ 0x82cc, 0x78 },
> +
> +		/* irq init */
> +		{ 0x8251, 0x01 },
> +		{ 0x8258, 0x0a }, /* hpd irq */
> +		{ 0x8259, 0x80 }, /* hpd debounce width */
> +		{ 0x829e, 0xf7 }, /* video check irq */
> +
> +		/* power consumption for work */
> +		{ 0x8004, 0xf0 },
> +		{ 0x8006, 0xf0 },
> +		{ 0x800a, 0x80 },
> +		{ 0x800b, 0x40 },
> +		{ 0x800d, 0xef },
> +		{ 0x8011, 0xfa },
> +	};
> +
> +	if (lt9611->power_on)
> +		return 0;
> +
> +	dev_dbg(lt9611->dev, "power on\n");
> +
> +	ret = regmap_multi_reg_write(lt9611->regmap, seq, ARRAY_SIZE(seq));
> +	if (!ret)
> +		lt9611->power_on = true;
> +
> +	return ret;
> +}
> +
> +static int lt9611_power_off(struct lt9611 *lt9611)
> +{
> +	int ret;
> +
> +	dev_dbg(lt9611->dev, "power off\n");
> +
> +	ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
> +	if (!ret)
> +		lt9611->power_on = false;
> +
> +	return ret;
> +}
> +
> +static void lt9611_reset(struct lt9611 *lt9611)
> +{
> +	gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
> +	msleep(20);
> +	gpiod_set_value_cansleep(lt9611->reset_gpio, 0);
> +	msleep(20);
> +	gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
> +	msleep(100);
> +}
> +
> +static void lt9611_assert_5v(struct lt9611 *lt9611)
> +{
> +	if (!lt9611->enable_gpio)
> +		return;
> +
> +	gpiod_set_value_cansleep(lt9611->enable_gpio, 1);
> +	msleep(20);
> +}
> +
> +static int lt9611_regulator_init(struct lt9611 *lt9611)
> +{
> +	int ret;
> +
> +	lt9611->supplies[0].supply = "vdd";
> +	lt9611->supplies[1].supply = "vcc";
> +	ret = devm_regulator_bulk_get(lt9611->dev, 2, lt9611->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	return regulator_set_load(lt9611->supplies[0].consumer, 300000);
> +}
> +
> +static int lt9611_regulator_enable(struct lt9611 *lt9611)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(lt9611->supplies[0].consumer);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(1000, 10000);
> +
> +	ret = regulator_enable(lt9611->supplies[1].consumer);
> +	if (ret < 0) {
> +		regulator_disable(lt9611->supplies[0].consumer);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct lt9611_mode *lt9611_find_mode(const struct drm_display_mode *mode)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(lt9611_modes); i++) {
> +		if (lt9611_modes[i].hdisplay == mode->hdisplay &&
> +		    lt9611_modes[i].vdisplay == mode->vdisplay &&
> +		    lt9611_modes[i].fps == drm_mode_vrefresh(mode)) {
> +			return &lt9611_modes[i];
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +/* connector funcs */
> +static enum drm_connector_status
> +lt9611_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct lt9611 *lt9611 = connector_to_lt9611(connector);
> +	unsigned int reg_val = 0;
> +	int connected = 0;
> +
> +	regmap_read(lt9611->regmap, 0x825e, &reg_val);
> +	connected  = (reg_val & BIT(2));
> +	dev_dbg(lt9611->dev, "connected = %x\n", connected);
> +
> +	lt9611->status = connected ?  connector_status_connected :
> +				connector_status_disconnected;
> +
> +	return lt9611->status;
> +}
> +
> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> +	unsigned int temp;
> +	int ret = 0;
> +	int i, j;
> +
> +	memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
> +
> +	regmap_write(lt9611->regmap, 0x8503, 0xc9);
> +
> +	/* 0xA0 is EDID device address */
> +	regmap_write(lt9611->regmap, 0x8504, 0xa0);
> +	/* 0x00 is EDID offset address */
> +	regmap_write(lt9611->regmap, 0x8505, 0x00);
> +	/* length for read */
> +	regmap_write(lt9611->regmap, 0x8506, 0x20);
> +	regmap_write(lt9611->regmap, 0x8514, 0x7f);
> +
> +	for (i = 0 ; i < 8 ; i++) {
> +		/* offset address */
> +		regmap_write(lt9611->regmap, 0x8505, i * 32);
> +		regmap_write(lt9611->regmap, 0x8507, 0x36);
> +		regmap_write(lt9611->regmap, 0x8507, 0x31);
> +		regmap_write(lt9611->regmap, 0x8507, 0x37);
> +		usleep_range(5000, 10000);
> +
> +		regmap_read(lt9611->regmap, 0x8540, &temp);
> +
> +		if (temp & 0x02) {  /*KEY_DDC_ACCS_DONE=1*/
> +			for (j = 0; j < 32; j++) {
> +				regmap_read(lt9611->regmap, 0x8583, &temp);
> +				lt9611->edid_buf[i * 32 + j] = temp;
> +			}
> +		} else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
> +			dev_err(lt9611->dev, "read edid failed: no ack\n");
> +			ret = -EIO;
> +			goto end;
> +		} else {
> +			dev_err(lt9611->dev,
> +				"read edid failed: access not done\n");
> +			ret = -EIO;
> +			goto end;
> +		}
> +	}
> +
> +	dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
> +		lt9611->edid_buf[255]);
> +
> +end:
> +	regmap_write(lt9611->regmap, 0x8507, 0x1f);
> +	return ret;
> +}
> +
> +/* TODO: add support for more extension blocks */
> +static int
> +lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct lt9611 *lt9611 = data;
> +	int ret;
> +
> +	dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
> +		block, (int)len);
> +
> +	if (len > 128)
> +		return -EINVAL;
> +
> +	/* support up to 1 extension block */
> +	if (block > 1)
> +		return -EINVAL;
> +
> +	if (block == 0) {
> +		/* always read 2 edid blocks once */
> +		ret = lt9611_read_edid(lt9611);
> +		if (ret) {
> +			dev_err(lt9611->dev, "edid read failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (block % 2 == 0)
> +		memcpy(buf, lt9611->edid_buf, len);
> +	else
> +		memcpy(buf, lt9611->edid_buf + 128, len);
> +
> +	return 0;
> +}
> +
> +static int lt9611_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct lt9611 *lt9611 = connector_to_lt9611(connector);
> +	unsigned int count;
> +	struct edid *edid;
> +
> +	dev_dbg(lt9611->dev, "get modes\n");
> +
> +	lt9611_power_on(lt9611);
> +	edid = drm_do_get_edid(connector, lt9611_get_edid_block, lt9611);
> +	drm_connector_update_edid_property(connector, edid);
> +	count = drm_add_edid_modes(connector, edid);
> +	kfree(edid);
> +
> +	return count;
> +}
> +
> +static enum drm_mode_status
> +lt9611_connector_mode_valid(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
> +
> +	return lt9611_mode ? MODE_OK : MODE_BAD;
> +}
> +
> +/* bridge funcs */
> +static void lt9611_bridge_enable(struct drm_bridge *bridge)
> +{
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> +	dev_dbg(lt9611->dev, "bridge enable\n");
> +
> +	if (lt9611_power_on(lt9611)) {
> +		dev_err(lt9611->dev, "power on failed\n");
> +		return;
> +	}
> +
> +	dev_dbg(lt9611->dev, "video on\n");
> +
> +	lt9611_mipi_input_analog(lt9611);
> +	lt9611_hdmi_tx_digital(lt9611);
> +	lt9611_hdmi_tx_phy(lt9611);
> +
> +	msleep(500);
> +
> +	lt9611_video_check(lt9611);
> +
> +	/* Enable HDMI output */
> +	regmap_write(lt9611->regmap, 0x8130, 0xea);
> +}
> +
> +static void lt9611_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +	int ret;
> +
> +	dev_dbg(lt9611->dev, "bridge disable\n");
> +
> +	/* Disable HDMI output */
> +	ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
> +	if (ret) {
> +		dev_err(lt9611->dev, "video on failed\n");
> +		return;
> +	}
> +
> +	if (lt9611_power_off(lt9611)) {
> +		dev_err(lt9611->dev, "power on failed\n");
> +		return;
> +	}
> +}
> +
> +static struct
> +drm_connector_helper_funcs lt9611_bridge_connector_helper_funcs = {
> +	.get_modes = lt9611_connector_get_modes,
> +	.mode_valid = lt9611_connector_mode_valid,
> +};
> +
> +static const struct drm_connector_funcs lt9611_bridge_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = lt9611_connector_detect,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611,
> +						 struct device_node *dsi_node)
> +{
> +	const struct mipi_dsi_device_info info = { "lt9611", 0, NULL };
> +	struct mipi_dsi_device *dsi;
> +	struct mipi_dsi_host *host;
> +	int ret;
> +
> +	host = of_find_mipi_dsi_host_by_node(dsi_node);
> +	if (!host) {
> +		dev_err(lt9611->dev, "failed to find dsi host\n");
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	dsi = mipi_dsi_device_register_full(host, &info);
> +	if (IS_ERR(dsi)) {
> +		dev_err(lt9611->dev, "failed to create dsi device\n");
> +		return dsi;
> +	}
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +			  MIPI_DSI_MODE_VIDEO_HSE;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		dev_err(lt9611->dev, "failed to attach dsi to host\n");
> +		mipi_dsi_device_unregister(dsi);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return dsi;
> +}
> +
> +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> +				enum drm_bridge_attach_flags flags)
> +{
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +	int ret;
> +
> +	dev_dbg(lt9611->dev, "bridge attach\n");


Connector creation in bridge drivers is deprecated. Please at least add
support for the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, to make connector
creation optional. Ideally the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case
should not be implemented at all. This will require the display
controller driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Which display
controller(s) do you use this driver with ?

> +
> +	ret = drm_connector_init(bridge->dev, &lt9611->connector,
> +				 &lt9611_bridge_connector_funcs,
> +				 DRM_MODE_CONNECTOR_HDMIA);
> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +
> +	drm_connector_helper_add(&lt9611->connector,
> +				 &lt9611_bridge_connector_helper_funcs);
> +	drm_connector_attach_encoder(&lt9611->connector, bridge->encoder);
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");
> +		return -ENODEV;
> +	}
> +
> +	/* Attach primary DSI */
> +	lt9611->dsi0 = lt9611_attach_dsi(lt9611, lt9611->dsi0_node);
> +	if (IS_ERR(lt9611->dsi0))
> +		return PTR_ERR(lt9611->dsi0);
> +
> +	/* Attach secondary DSI, if specified */
> +	if (lt9611->dsi1_node) {
> +		lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> +		if (IS_ERR(lt9611->dsi1)) {
> +			ret = PTR_ERR(lt9611->dsi1);
> +			goto err_unregister_dsi0;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_unregister_dsi0:
> +	mipi_dsi_device_unregister(lt9611->dsi0);
> +
> +	return ret;
> +}
> +
> +static void lt9611_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> +	if (lt9611->dsi1) {
> +		mipi_dsi_detach(lt9611->dsi1);
> +		mipi_dsi_device_unregister(lt9611->dsi1);
> +	}
> +
> +	mipi_dsi_detach(lt9611->dsi0);
> +	mipi_dsi_device_unregister(lt9611->dsi0);
> +}
> +
> +static enum drm_mode_status
> +lt9611_bridge_mode_valid(struct drm_bridge *bridge,
> +			 const struct drm_display_mode *mode)
> +{
> +	struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> +	if (lt9611_mode->intfs > 1 && !lt9611->dsi1)
> +		return MODE_PANEL;
> +	else
> +		return MODE_OK;
> +}
> +
> +static void lt9611_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> +	dev_dbg(lt9611->dev, "bridge pre_enable\n");
> +
> +	if (!lt9611->sleep)
> +		return;
> +
> +	lt9611_reset(lt9611);
> +	regmap_write(lt9611->regmap, 0x80ee, 0x01);
> +
> +	lt9611->sleep = false;
> +}
> +
> +static void lt9611_bridge_post_disable(struct drm_bridge *bridge)
> +{
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> +	dev_dbg(lt9611->dev, "bridge post_disable\n");
> +
> +	lt9611_sleep_setup(lt9611);
> +}
> +
> +static void lt9611_bridge_mode_set(struct drm_bridge *bridge,
> +				   const struct drm_display_mode *mode,
> +				   const struct drm_display_mode *adj_mode)
> +{
> +	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +	struct hdmi_avi_infoframe avi_frame;
> +	int ret;
> +
> +	dev_dbg(lt9611->dev, "bridge mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
> +		adj_mode->hdisplay, adj_mode->vdisplay,
> +		adj_mode->vrefresh, adj_mode->clock);
> +
> +	lt9611_bridge_pre_enable(bridge);
> +
> +	lt9611_mipi_input_digital(lt9611, mode);
> +	lt9611_pll_setup(lt9611, mode);
> +	lt9611_mipi_video_setup(lt9611, mode);
> +	lt9611_pcr_setup(lt9611, mode);
> +
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame,
> +						       &lt9611->connector,
> +						       mode);
> +	if (!ret)
> +		lt9611->vic = avi_frame.video_code;
> +}
> +
> +static const struct drm_bridge_funcs lt9611_bridge_funcs = {
> +	.attach = lt9611_bridge_attach,
> +	.detach = lt9611_bridge_detach,
> +	.mode_valid = lt9611_bridge_mode_valid,
> +	.enable = lt9611_bridge_enable,
> +	.disable = lt9611_bridge_disable,
> +	.post_disable = lt9611_bridge_post_disable,
> +	.mode_set = lt9611_bridge_mode_set,
> +};
> +
> +static int lt9611_parse_dt(struct device *dev,
> +			   struct lt9611 *lt9611)
> +{
> +	lt9611->dsi0_node = of_graph_get_remote_node(dev->of_node, 1, -1);
> +	if (!lt9611->dsi0_node) {
> +		DRM_DEV_ERROR(dev, "failed to get remote node for primary dsi\n");
> +		return -ENODEV;
> +	}
> +
> +	lt9611->dsi1_node = of_graph_get_remote_node(dev->of_node, 2, -1);
> +
> +	lt9611->ac_mode = of_property_read_bool(dev->of_node, "lt,ac-mode");
> +	dev_dbg(lt9611->dev, "ac_mode=%d\n", lt9611->ac_mode);
> +
> +	return 0;
> +}
> +
> +static int lt9611_gpio_init(struct lt9611 *lt9611)
> +{
> +	struct device *dev = lt9611->dev;
> +
> +	lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(lt9611->reset_gpio)) {
> +		dev_err(dev, "failed to acquire reset gpio\n");
> +		return PTR_ERR(lt9611->reset_gpio);
> +	}
> +
> +	lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> +						      GPIOD_OUT_LOW);
> +	if (IS_ERR(lt9611->enable_gpio)) {
> +		dev_err(dev, "failed to acquire enable gpio\n");
> +		return PTR_ERR(lt9611->enable_gpio);
> +	}
> +
> +	return 0;
> +}
> +
> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> +	unsigned int rev;
> +	int ret;
> +
> +	regmap_write(lt9611->regmap, 0x80ee, 0x01);
> +	ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> +	if (ret)
> +		dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
> +	dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> +	return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct lt9611 *lt9611;
> +	struct device *dev = &client->dev;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(dev, "device doesn't support I2C\n");
> +		return -ENODEV;
> +	}
> +
> +	lt9611 = devm_kzalloc(dev, sizeof(*lt9611), GFP_KERNEL);
> +	if (!lt9611)
> +		return -ENOMEM;
> +
> +	lt9611->dev = &client->dev;
> +	lt9611->client = client;
> +	lt9611->sleep = false;
> +
> +	lt9611->regmap = devm_regmap_init_i2c(client, &lt9611_regmap_config);
> +	if (IS_ERR(lt9611->regmap)) {
> +		DRM_ERROR("regmap i2c init failed\n");
> +		return PTR_ERR(lt9611->regmap);
> +	}
> +
> +	ret = lt9611_parse_dt(&client->dev, lt9611);
> +	if (ret) {
> +		dev_err(dev, "failed to parse device tree\n");
> +		return ret;
> +	}
> +
> +	ret = lt9611_gpio_init(lt9611);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = lt9611_regulator_init(lt9611);
> +	if (ret < 0)
> +		return ret;
> +
> +	lt9611_assert_5v(lt9611);
> +
> +	ret = lt9611_regulator_enable(lt9611);
> +	if (ret)
> +		return ret;
> +
> +	lt9611_reset(lt9611);
> +
> +	ret = lt9611_read_device_rev(lt9611);
> +	if (ret) {
> +		dev_err(dev, "failed to read chip rev\n");
> +		goto err_disable_regulators;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					lt9611_irq_thread_handler,
> +					IRQF_ONESHOT, "lt9611", lt9611);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		goto err_disable_regulators;
> +	}
> +
> +	i2c_set_clientdata(client, lt9611);
> +
> +	lt9611->bridge.funcs = &lt9611_bridge_funcs;
> +	lt9611->bridge.of_node = client->dev.of_node;
> +
> +	drm_bridge_add(&lt9611->bridge);
> +
> +	lt9611_enable_hpd_interrupts(lt9611);
> +
> +	return 0;
> +
> +err_disable_regulators:
> +	regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> +	of_node_put(lt9611->dsi0_node);
> +	of_node_put(lt9611->dsi1_node);
> +
> +	return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> +	struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> +	disable_irq(client->irq);
> +	drm_bridge_remove(&lt9611->bridge);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> +	of_node_put(lt9611->dsi0_node);
> +	of_node_put(lt9611->dsi1_node);
> +
> +	return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> +	{ "lontium,lt9611", 0},
> +	{}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> +	{.compatible = "lontium,lt9611"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, lt9611_match_table);
> +
> +static struct i2c_driver lt9611_driver = {
> +	.driver = {
> +		.name = "lt9611",
> +		.of_match_table = lt9611_match_table,
> +	},
> +	.probe = lt9611_probe,
> +	.remove = lt9611_remove,
> +	.id_table = lt9611_id,
> +};
> +module_i2c_driver(lt9611_driver);
> +
> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

^ permalink raw reply

* Re: [PATCH 2/3] dt-bindings: display: bridge: Add documentation for LT9611
From: Laurent Pinchart @ 2020-05-28  1:48 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Daniel Vetter, David Airlie, Rob Clark, linux-arm-msm,
	Bjorn Andersson, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
	Jernej Skrabec, Rob Herring, devicetree, linux-kernel, dri-devel
In-Reply-To: <20200513100533.42996-3-vkoul@kernel.org>

Hi Vinod,

Thank you for the patch.

On Wed, May 13, 2020 at 03:35:32PM +0530, Vinod Koul wrote:
> Lontium LT9611 is a DSI to HDMI bridge which supports 2 DSI ports
> and I2S port as input and one HDMI port as output
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../display/bridge/lontium,lt9611.yaml        | 178 ++++++++++++++++++
>  1 file changed, 178 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lontium,lt9611.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt9611.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt9611.yaml
> new file mode 100644
> index 000000000000..77ee8cc35cd8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt9611.yaml
> @@ -0,0 +1,178 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/lontium,lt9611.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Lontium LT9611 2 Port MIPI to HDMI Bridge
> +
> +maintainers:
> +  - Vinod Koul <vkoul@kernel.org>
> +
> +description: |
> +  The LT9611 is a bridge device which converts DSI to HDMI
> +
> +properties:
> +  compatible:
> +    enum:
> +      - lontium,lt9611
> +
> +  reg:
> +    maxItems: 1
> +    description: base I2C address of the device.
> +
> +  "#sound-dai-cells":
> +    const: 1
> +
> +  interrupts:
> +    maxItems: 1
> +    description: interrupt line for the chip

I think you could drop the descriptions for the reg and interrupt
properties, they don't add much.

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO connected to active high RESET pin.
> +
> +  vdd-supply:
> +    description: Regulator for 1.8V MIPI phy power.
> +
> +  vcc-supply:
> +    description: Regulator for 3.3V IO power.
> +
> +  ports:
> +    type: object
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      port@0:
> +        type: object
> +        additionalProperties: false
> +
> +        description: |
> +          HDMI port for HDMI output

The usual practice is to have the input ports first, followed by the
output ports. Is there a reason not to follow that rule ?

> +
> +        properties:
> +          reg:
> +            const: 0
> +
> +        patternProperties:
> +          endpoint:

If you want to use patternProperties, this should be

          "^endpoint@[0-9]+$":

(including the quotes). Same below.

> +            type: object
> +            additionalProperties: false
> +
> +            properties:
> +              remote-endpoint: true

How about

              remote-endpoint:
                $ref: /schemas/types.yaml#/definitions/phandle

and the same below ?

You also need a reg property if multiple endpoints are present.

> +
> +        required:
> +          - reg
> +
> +      port@1:
> +        type: object
> +        additionalProperties: false
> +
> +        description: |
> +          MIPI port-1 for MIPI input
> +
> +        properties:
> +          reg:
> +            const: 1
> +
> +        patternProperties:
> +          endpoint:
> +            type: object
> +            additionalProperties: false
> +
> +            properties:
> +              remote-endpoint: true
> +
> +        required:
> +          - reg
> +
> +      port@2:
> +        type: object
> +        additionalProperties: false
> +
> +        description: |
> +          MIPI port-2 for MIPI input

A description of how the two MIPI inputs differ would be useful. In
particular, are both mandatory, or is it valid to connect only one of
the two ? If using a single input is supported, can it be either, or
does it have to be the first one ? When using both inputs, what should
be connected to them ?

> +
> +        properties:
> +          reg:
> +            const: 2
> +
> +        patternProperties:
> +          endpoint:
> +            type: object
> +            additionalProperties: false
> +
> +            properties:
> +              remote-endpoint: true
> +
> +        required:
> +          - reg
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - vdd-supply
> +  - vcc-supply
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c10 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      lt9611_codec: hdmi-bridge@3b {

Please drop unused labels.

> +        compatible = "lontium,lt9611";
> +        reg = <0x3b>;
> +
> +        reset-gpios = <&tlmm 128 GPIO_ACTIVE_HIGH>;
> +        interrupts-extended = <&tlmm 84 IRQ_TYPE_EDGE_FALLING>;
> +
> +        vdd-supply = <&lt9611_1v8>;
> +        vcc-supply = <&lt9611_3v3>;
> +
> +        ports {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          port@0 {
> +            reg = <0>;
> +            lt9611_out: endpoint {
> +              remote-endpoint = <&hdmi_con>;
> +            };
> +          };
> +
> +          port@1 {
> +            reg = <1>;
> +            lt9611_a: endpoint {
> +              remote-endpoint = <&dsi0_out>;
> +            };
> +          };
> +
> +          port@2 {
> +            reg = <2>;
> +            lt9611_b: endpoint {
> +              remote-endpoint = <&dsi1_out>;
> +            };
> +          };
> +        };
> +      };
> +    };

It's customary to end YAML schema files with ... on a separate line.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply

* [RFC v3 2/3] arm64: boot: dts: qcom: sm8150: Enable dynamic TX FIFO resize logic
From: Wesley Cheng @ 2020-05-28  1:46 UTC (permalink / raw)
  To: robh+dt, bjorn.andersson, balbi, gregkh, agross
  Cc: linux-kernel, linux-arm-msm, devicetree, linux-usb, Wesley Cheng
In-Reply-To: <1590630363-3934-1-git-send-email-wcheng@codeaurora.org>

Enable the flexible TX FIFO resize logic on SM8150.  Using a larger TX FIFO
SZ can help account for situations when system latency is greater than the
USB bus transmission latency.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index a36512d..c285233 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -708,6 +708,7 @@
 				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
 				snps,dis_u2_susphy_quirk;
 				snps,dis_enblslpm_quirk;
+				tx-fifo-resize;
 				phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
 				phy-names = "usb2-phy", "usb3-phy";
 			};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related

* [RFC v3 1/3] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
From: Wesley Cheng @ 2020-05-28  1:46 UTC (permalink / raw)
  To: robh+dt, bjorn.andersson, balbi, gregkh, agross
  Cc: linux-kernel, linux-arm-msm, devicetree, linux-usb, Wesley Cheng
In-Reply-To: <1590630363-3934-1-git-send-email-wcheng@codeaurora.org>

Some devices have USB compositions which may require multiple endpoints
that support EP bursting.  HW defined TX FIFO sizes may not always be
sufficient for these compositions.  By utilizing flexible TX FIFO
allocation, this allows for endpoints to request the required FIFO depth to
achieve higher bandwidth.  With some higher bMaxBurst configurations, using
a larger TX FIFO size results in better TX throughput.

Ensure that one TX FIFO is reserved for every IN endpoint.  This allows for
the FIFO logic to prevent running out of FIFO space.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 drivers/usb/dwc3/core.c   |   2 +
 drivers/usb/dwc3/core.h   |   8 ++++
 drivers/usb/dwc3/ep0.c    |  37 ++++++++++++++-
 drivers/usb/dwc3/gadget.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index edc1715..cca5554 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1304,6 +1304,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				&tx_thr_num_pkt_prd);
 	device_property_read_u8(dev, "snps,tx-max-burst-prd",
 				&tx_max_burst_prd);
+	dwc->needs_fifo_resize = device_property_read_bool(dev,
+				"tx-fifo-resize");
 
 	dwc->disable_scramble_quirk = device_property_read_bool(dev,
 				"snps,disable_scramble_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4c171a8..ce0bf28 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -675,6 +675,7 @@ struct dwc3_event_buffer {
  *		isochronous START TRANSFER command failure workaround
  * @start_cmd_status: the status of testing START TRANSFER command with
  *		combo_num = 'b00
+ * @fifo_depth: allocated TXFIFO depth
  */
 struct dwc3_ep {
 	struct usb_ep		endpoint;
@@ -727,6 +728,7 @@ struct dwc3_ep {
 	/* For isochronous START TRANSFER workaround only */
 	u8			combo_num;
 	int			start_cmd_status;
+	int			fifo_depth;
 };
 
 enum dwc3_phy {
@@ -1004,6 +1006,7 @@ struct dwc3_scratchpad_array {
  * 	1	- utmi_l1_suspend_n
  * @is_fpga: true when we are using the FPGA board
  * @pending_events: true when we have pending IRQs to be handled
+ * @needs_fifo_resize: not all users might want fifo resizing, flag it
  * @pullups_connected: true when Run/Stop bit is set
  * @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
  * @three_stage_setup: set if we perform a three phase setup
@@ -1044,6 +1047,8 @@ struct dwc3_scratchpad_array {
  * @dis_metastability_quirk: set to disable metastability quirk.
  * @imod_interval: set the interrupt moderation interval in 250ns
  *                 increments or 0 to disable.
+ * @last_fifo_depth: total TXFIFO depth of all enabled USB IN/INT endpoints
+ * @num_ep_resized: the number of TX FIFOs that have already been resized
  */
 struct dwc3 {
 	struct work_struct	drd_work;
@@ -1204,6 +1209,7 @@ struct dwc3 {
 	unsigned		is_utmi_l1_suspend:1;
 	unsigned		is_fpga:1;
 	unsigned		pending_events:1;
+	unsigned		needs_fifo_resize:1;
 	unsigned		pullups_connected:1;
 	unsigned		setup_packet_pending:1;
 	unsigned		three_stage_setup:1;
@@ -1236,6 +1242,8 @@ struct dwc3 {
 	unsigned		dis_metastability_quirk:1;
 
 	u16			imod_interval;
+	int			last_fifo_depth;
+	int			num_ep_resized;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 6dee4da..76db9b5 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -601,8 +601,9 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
 {
 	enum usb_device_state state = dwc->gadget.state;
 	u32 cfg;
-	int ret;
+	int ret, num, size;
 	u32 reg;
+	struct dwc3_ep *dep;
 
 	cfg = le16_to_cpu(ctrl->wValue);
 
@@ -611,6 +612,40 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
 		return -EINVAL;
 
 	case USB_STATE_ADDRESS:
+		/*
+		 * If tx-fifo-resize flag is not set for the controller, then
+		 * do not clear existing allocated TXFIFO since we do not
+		 * allocate it again in dwc3_gadget_resize_tx_fifos
+		 */
+		if (dwc->needs_fifo_resize) {
+			/* Read ep0IN related TXFIFO size */
+			dep = dwc->eps[1];
+			size = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0));
+			if (dwc3_is_usb31(dwc))
+				dep->fifo_depth = DWC31_GTXFIFOSIZ_TXFDEP(size);
+			else
+				dep->fifo_depth = DWC3_GTXFIFOSIZ_TXFDEP(size);
+
+			dwc->last_fifo_depth = dep->fifo_depth;
+			/* Clear existing TXFIFO for all IN eps except ep0 */
+			for (num = 3; num < min_t(int, dwc->num_eps,
+				DWC3_ENDPOINTS_NUM); num += 2) {
+				dep = dwc->eps[num];
+				/* Don't change TXFRAMNUM on usb31 version */
+				size = dwc3_is_usb31(dwc) ?
+					dwc3_readl(dwc->regs,
+						   DWC3_GTXFIFOSIZ(num >> 1)) &
+						   DWC31_GTXFIFOSIZ_TXFRAMNUM :
+						   0;
+
+				dwc3_writel(dwc->regs,
+					    DWC3_GTXFIFOSIZ(num >> 1),
+					    size);
+				dep->fifo_depth = 0;
+			}
+			dwc->num_ep_resized = 0;
+		}
+
 		ret = dwc3_ep0_delegate_req(dwc, ctrl);
 		/* if the cfg matches and the cfg is non zero */
 		if (cfg && (!ret || (ret == USB_GADGET_DELAYED_STATUS))) {
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 00746c2..9b09528 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -540,6 +540,117 @@ static int dwc3_gadget_start_config(struct dwc3_ep *dep)
 	return 0;
 }
 
+/*
+ * dwc3_gadget_resize_tx_fifos - reallocate fifo spaces for current use-case
+ * @dwc: pointer to our context structure
+ *
+ * This function will a best effort FIFO allocation in order
+ * to improve FIFO usage and throughput, while still allowing
+ * us to enable as many endpoints as possible.
+ *
+ * Keep in mind that this operation will be highly dependent
+ * on the configured size for RAM1 - which contains TxFifo -,
+ * the amount of endpoints enabled on coreConsultant tool, and
+ * the width of the Master Bus.
+ *
+ * In general, FIFO depths are represented with the following equation:
+ *
+ * fifo_size = mult * ((max_packet + mdwidth)/mdwidth + 1) + 1
+ *
+ * Conversions can be done to the equation to derive the number of packets that
+ * will fit to a particular FIFO size value.
+ */
+static int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc, struct dwc3_ep *dep)
+{
+	int ram1_depth, mdwidth, fifo_0_start, tmp, num_in_ep;
+	int min_depth, remaining, fifo_size, mult = 1, fifo, max_packet = 1024;
+
+	if (!dwc->needs_fifo_resize)
+		return 0;
+
+	/* resize IN endpoints except ep0 */
+	if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1)
+		return 0;
+
+	/* Don't resize already resized IN endpoint */
+	if (dep->fifo_depth)
+		return 0;
+
+	ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7);
+	mdwidth = DWC3_MDWIDTH(dwc->hwparams.hwparams0);
+	/* MDWIDTH is represented in bits, we need it in bytes */
+	mdwidth >>= 3;
+
+	if (((dep->endpoint.maxburst > 1) &&
+			usb_endpoint_xfer_bulk(dep->endpoint.desc))
+			|| usb_endpoint_xfer_isoc(dep->endpoint.desc))
+		mult = 3;
+
+	if ((dep->endpoint.maxburst > 6) &&
+			usb_endpoint_xfer_bulk(dep->endpoint.desc)
+			&& dwc3_is_usb31(dwc))
+		mult = 6;
+
+	/* FIFO size for a single buffer */
+	fifo = (max_packet + mdwidth)/mdwidth;
+	fifo++;
+
+	/* Calculate the number of remaining EPs w/o any FIFO */
+	num_in_ep = dwc->num_eps/2;
+	num_in_ep -= dwc->num_ep_resized;
+	/* Ignore EP0 IN */
+	num_in_ep--;
+
+	/* Reserve at least one FIFO for the number of IN EPs */
+	min_depth = num_in_ep * (fifo+1);
+	remaining = ram1_depth - min_depth - dwc->last_fifo_depth;
+
+	/* We've already reserved 1 FIFO per EP, so check what we can fit in
+	 * addition to it.  If there is not enough remaining space, allocate
+	 * all the remaining space to the EP.
+	 */
+	fifo_size = (mult-1) * fifo;
+	if (remaining < fifo_size) {
+		if (remaining > 0)
+			fifo_size = remaining;
+		else
+			fifo_size = 0;
+	}
+
+	fifo_size += fifo;
+	fifo_size++;
+	dep->fifo_depth = fifo_size;
+
+	/* Check if TXFIFOs start at non-zero addr */
+	tmp = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0));
+	fifo_0_start = DWC3_GTXFIFOSIZ_TXFSTADDR(tmp);
+
+	fifo_size |= (fifo_0_start + (dwc->last_fifo_depth << 16));
+	if (dwc3_is_usb31(dwc))
+		dwc->last_fifo_depth += DWC31_GTXFIFOSIZ_TXFDEP(fifo_size);
+	else
+		dwc->last_fifo_depth += DWC3_GTXFIFOSIZ_TXFDEP(fifo_size);
+
+	/* Check fifo size allocation doesn't exceed available RAM size. */
+	if (dwc->last_fifo_depth >= ram1_depth) {
+		dev_err(dwc->dev, "Fifosize(%d) > RAM size(%d) %s depth:%d\n",
+				(dwc->last_fifo_depth * mdwidth), ram1_depth,
+				dep->endpoint.name, fifo_size);
+		if (dwc3_is_usb31(dwc))
+			fifo_size = DWC31_GTXFIFOSIZ_TXFDEP(fifo_size);
+		else
+			fifo_size = DWC3_GTXFIFOSIZ_TXFDEP(fifo_size);
+		dwc->last_fifo_depth -= fifo_size;
+		dep->fifo_depth = 0;
+		WARN_ON(1);
+		return -ENOMEM;
+	}
+
+	dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1), fifo_size);
+	dwc->num_ep_resized++;
+	return 0;
+}
+
 static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 {
 	const struct usb_ss_ep_comp_descriptor *comp_desc;
@@ -620,6 +731,10 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	int			ret;
 
 	if (!(dep->flags & DWC3_EP_ENABLED)) {
+		ret = dwc3_gadget_resize_tx_fifos(dwc, dep);
+		if (ret)
+			return ret;
+
 		ret = dwc3_gadget_start_config(dep);
 		if (ret)
 			return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related

* [RFC v3 0/3] Re-introduce TX FIFO resize for larger EP bursting
From: Wesley Cheng @ 2020-05-28  1:46 UTC (permalink / raw)
  To: robh+dt, bjorn.andersson, balbi, gregkh, agross
  Cc: linux-kernel, linux-arm-msm, devicetree, linux-usb, Wesley Cheng

Changes in V3:
 - Removed "Reviewed-by" tags
 - Renamed series back to RFC
 - Modified logic to ensure that fifo_size is reset if we pass the minimum
   threshold.  Tested with binding multiple FDs requesting 6 FIFOs.

Changes in V2:
 - Modified TXFIFO resizing logic to ensure that each EP is reserved a
   FIFO.
 - Removed dev_dbg() prints and fixed typos from patches
 - Added some more description on the dt-bindings commit message

Currently, there is no functionality to allow for resizing the TXFIFOs, and
relying on the HW default setting for the TXFIFO depth.  In most cases, the
HW default is probably sufficient, but for USB compositions that contain
multiple functions that require EP bursting, the default settings
might not be enough.  Also to note, the current SW will assign an EP to a
function driver w/o checking to see if the TXFIFO size for that particular
EP is large enough. (this is a problem if there are multiple HW defined
values for the TXFIFO size)

It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3
is required for an EP that supports bursting.  Otherwise, there may be
frequent occurences of bursts ending.  For high bandwidth functions,
such as data tethering (protocols that support data aggregation), mass
storage, and media transfer protocol (over FFS), the bMaxBurst value can be
large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB
throughput. (which can be associated to system access latency, etc...)  It
allows for a more consistent burst of traffic, w/o any interruptions, as
data is readily available in the FIFO.

With testing done using the mass storage function driver, the results show
that with a larger TXFIFO depth, the bandwidth increased significantly.

Test Parameters:
 - Platform: Qualcomm SM8150
 - bMaxBurst = 6
 - USB req size = 256kB
 - Num of USB reqs = 16
 - USB Speed = Super-Speed
 - Function Driver: Mass Storage (w/ ramdisk)
 - Test Application: CrystalDiskMark

Results:

TXFIFO Depth = 3 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     | 
Read      |9 loops    | 193.60
	  |           | 195.86
          |           | 184.77
          |           | 193.60
-------------------------------------------

TXFIFO Depth = 6 max packets

Test Case | Data Size | AVG tput (in MB/s)
-------------------------------------------
Sequential|1 GB x     | 
Read      |9 loops    | 287.35
	  |           | 304.94
          |           | 289.64
          |           | 293.61
-------------------------------------------

Wesley Cheng (3):
  usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
  arm64: boot: dts: qcom: sm8150: Enable dynamic TX FIFO resize logic
  dt-bindings: usb: dwc3: Add entry for tx-fifo-resize

 Documentation/devicetree/bindings/usb/dwc3.txt |   2 +-
 arch/arm64/boot/dts/qcom/sm8150.dtsi           |   1 +
 drivers/usb/dwc3/core.c                        |   2 +
 drivers/usb/dwc3/core.h                        |   8 ++
 drivers/usb/dwc3/ep0.c                         |  37 +++++++-
 drivers/usb/dwc3/gadget.c                      | 115 +++++++++++++++++++++++++
 6 files changed, 163 insertions(+), 2 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply

* [RFC v3 3/3] dt-bindings: usb: dwc3: Add entry for tx-fifo-resize
From: Wesley Cheng @ 2020-05-28  1:46 UTC (permalink / raw)
  To: robh+dt, bjorn.andersson, balbi, gregkh, agross
  Cc: linux-kernel, linux-arm-msm, devicetree, linux-usb, Wesley Cheng
In-Reply-To: <1590630363-3934-1-git-send-email-wcheng@codeaurora.org>

Re-introduce the comment for the tx-fifo-resize setting for the DWC3
controller.  This allows for vendors to control if they require the TX FIFO
resizing logic on their HW, as the default FIFO size configurations may
already be sufficient.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 9946ff9..489f5da 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -105,7 +105,7 @@ Optional properties:
 			1-16 (DWC_usb31 programming guide section 1.2.3) to
 			enable periodic ESS TX threshold.
 
- - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
+ - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
 			register, undefined length INCR burst type enable and INCRx type.
 			When just one value, which means INCRX burst mode enabled. When
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related


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