| Message ID | 20231213110009.v1.2.I274b2d2255eb539cc9d251c9d65a385cc4014c79@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 13/12/2023 19:00, Mark Hasemeyer wrote: > The cros_ec driver currently assumes that cros-ec-spi compatible device > nodes are a wakeup-source even though the wakeup-source property is not > defined. > > Add the wakeup-source property to all cros-ec-spi compatible device > nodes to match expected behavior. > > Signed-off-by: Mark Hasemeyer <markhas@chromium.org> > --- > I did not get any other patches in the set, so no clue what's there... but for this patch: please split per subarch. At least Samsung bits. Best regards, Krzysztof
On Wed, Dec 13, 2023 at 11:00:20AM -0700, Mark Hasemeyer wrote: > The cros_ec driver currently assumes that cros-ec-spi compatible device > nodes are a wakeup-source even though the wakeup-source property is not > defined. If a device knows it is wakeup capable, why do you need a property too? I haven't looked closely enough, but it smells like after patch 6, these properties would be required for wakeup? That would be an ABI break. Rob
Il 13/12/23 19:00, Mark Hasemeyer ha scritto: > The cros_ec driver currently assumes that cros-ec-spi compatible device > nodes are a wakeup-source even though the wakeup-source property is not > defined. > > Add the wakeup-source property to all cros-ec-spi compatible device > nodes to match expected behavior. > > Signed-off-by: Mark Hasemeyer <markhas@chromium.org> I received only patch [2/6] - please send the entire series to the relevant maintainers, as otherwise it's difficult to understand what's going on. As for this patch alone: 1. arch/arm stuff goes to a different commit 2. I would prefer if you split per-arch and per-SoC. Regards, Angelo
On 12/14/23 11:55, AngeloGioacchino Del Regno wrote: > Il 13/12/23 19:00, Mark Hasemeyer ha scritto: >> The cros_ec driver currently assumes that cros-ec-spi compatible device >> nodes are a wakeup-source even though the wakeup-source property is not >> defined. >> >> Add the wakeup-source property to all cros-ec-spi compatible device >> nodes to match expected behavior. >> >> Signed-off-by: Mark Hasemeyer <markhas@chromium.org> > > I received only patch [2/6] - please send the entire series to the relevant > maintainers, as otherwise it's difficult to understand what's going on. > > As for this patch alone: > 1. arch/arm stuff goes to a different commit > 2. I would prefer if you split per-arch and per-SoC. +1, otherwise *somebody* will get merge conflicts that - even if trivial - take additional time to resolve :( Konrad
> If a device knows it is wakeup capable, why do you need a property too? I'm referencing: https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt "Nodes that describe devices which has wakeup capability must contain an "wakeup-source" boolean property." Currently the driver assumes the device is wake capable without parsing the device tree, which is an incorrect assumption as wake capability should not be enabled on some cros_ec systems. > I haven't looked closely enough, but it smells like after patch 6, these > properties would be required for wakeup? That would be an ABI break. Agreed. In this case, the driver is a ChromeOS related driver and DTS is built from source for each OS update. For more context, I will make sure to CC you (and everyone else) and include a cover letter in the next series version.
On Thu, Dec 14, 2023 at 3:04 PM Mark Hasemeyer <markhas@chromium.org> wrote: > > > If a device knows it is wakeup capable, why do you need a property too? > > I'm referencing: > https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt > "Nodes that describe devices which has wakeup capability must contain > an "wakeup-source" boolean property." That's probably too strongly worded because wakeup capable devices existed (and still exist) before this binding was created. Powerpc for example doesn't use it. > Currently the driver assumes the device is wake capable without > parsing the device tree, which is an incorrect assumption as wake > capability should not be enabled on some cros_ec systems. > > > I haven't looked closely enough, but it smells like after patch 6, these > > properties would be required for wakeup? That would be an ABI break. > > Agreed. In this case, the driver is a ChromeOS related driver and DTS > is built from source for each OS update. > For more context, I will make sure to CC you (and everyone else) and > include a cover letter in the next series version. Please explain in the patches with an ABI break why it doesn't matter. Rob
diff --git a/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi b/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi index a2ee371802004..8125c1b3e8d79 100644 --- a/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi +++ b/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi @@ -338,6 +338,7 @@ cros_ec: cros-ec@0 { interrupt-parent = <&gpio>; interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>; reg = <0>; + wakeup-source; google,cros-ec-spi-msg-delay = <2000>; diff --git a/arch/arm/boot/dts/nvidia/tegra124-venice2.dts b/arch/arm/boot/dts/nvidia/tegra124-venice2.dts index 3924ee385dee0..df98dc2a67b85 100644 --- a/arch/arm/boot/dts/nvidia/tegra124-venice2.dts +++ b/arch/arm/boot/dts/nvidia/tegra124-venice2.dts @@ -857,6 +857,7 @@ cros_ec: cros-ec@0 { interrupt-parent = <&gpio>; interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>; reg = <0>; + wakeup-source; google,cros-ec-spi-msg-delay = <2000>; diff --git a/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi b/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi index 092316be67f74..1554fe36e60fe 100644 --- a/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi +++ b/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi @@ -112,6 +112,7 @@ cros_ec: ec@0 { pinctrl-names = "default"; pinctrl-0 = <&ec_int>; spi-max-frequency = <3000000>; + wakeup-source; i2c_tunnel: i2c-tunnel { compatible = "google,cros-ec-i2c-tunnel"; diff --git a/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts b/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts index 4e757b6e28e1c..3759742d38cac 100644 --- a/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts +++ b/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts @@ -967,6 +967,7 @@ cros_ec: cros-ec@0 { reg = <0>; spi-max-frequency = <3125000>; google,has-vbc-nvram; + wakeup-source; controller-data { samsung,spi-feedback-delay = <1>; diff --git a/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts b/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts index f91bc4ae008e4..9bbbdce9103a6 100644 --- a/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts +++ b/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts @@ -949,6 +949,7 @@ cros_ec: cros-ec@0 { reg = <0>; spi-max-frequency = <3125000>; google,has-vbc-nvram; + wakeup-source; controller-data { samsung,spi-feedback-delay = <1>; diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi index 4dd21dd317026..f0395da659a86 100644 --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi @@ -1168,6 +1168,7 @@ cros_ec: ec@0 { interrupt-parent = <&pio>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; google,cros-ec-spi-msg-delay = <500>; + wakeup-source; i2c_tunnel: i2c-tunnel0 { compatible = "google,cros-ec-i2c-tunnel"; diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi index 44647d462e20b..359859f23b1fd 100644 --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi @@ -1013,6 +1013,7 @@ cros_ec: cros-ec@0 { interrupts = <151 IRQ_TYPE_LEVEL_LOW>; pinctrl-names = "default"; pinctrl-0 = <&ec_ap_int_odl>; + wakeup-source; i2c_tunnel: i2c-tunnel { compatible = "google,cros-ec-i2c-tunnel"; diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi index 5f62dc83013f0..74c534d475cb0 100644 --- a/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi @@ -1918,6 +1918,7 @@ cros_ec: ec@0 { interrupts = <13 IRQ_TYPE_LEVEL_LOW>; pinctrl-names = "default"; pinctrl-0 = <&ec_ap_int>; + wakeup-source; i2c_tunnel: i2c-tunnel { compatible = "google,cros-ec-i2c-tunnel"; diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi index a29da53d17894..4594287d60926 100644 --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi @@ -1454,6 +1454,7 @@ cros_ec: ec@0 { spi-max-frequency = <3000000>; pinctrl-names = "default"; pinctrl-0 = <&cros_ec_int>; + wakeup-source; #address-cells = <1>; #size-cells = <0>; diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi index 37a3e9de90ff7..a5ace1b02c3d2 100644 --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi @@ -1034,6 +1034,7 @@ cros_ec: ec@0 { pinctrl-names = "default"; pinctrl-0 = <&cros_ec_int>; spi-max-frequency = <3000000>; + wakeup-source; keyboard-backlight { compatible = "google,cros-kbd-led-backlight"; diff --git a/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts b/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts index bbc2e9bef08da..14d58859bb55c 100644 --- a/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts +++ b/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts @@ -762,6 +762,7 @@ ec: cros-ec@0 { interrupt-parent = <&gpio>; interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>; reg = <0>; + wakeup-source; google,cros-ec-spi-msg-delay = <2000>; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi index 5a33e16a8b677..e6a2ed0463997 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi @@ -650,6 +650,7 @@ cros_ec: ec@0 { pinctrl-names = "default"; pinctrl-0 = <&ap_ec_int_l>; spi-max-frequency = <3000000>; + wakeup-source; cros_ec_pwm: pwm { compatible = "google,cros-ec-pwm"; diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi index 9ea6636125ad9..2ba4ea60cb147 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi @@ -548,6 +548,7 @@ cros_ec: ec@0 { pinctrl-names = "default"; pinctrl-0 = <&ap_ec_int_l>; spi-max-frequency = <3000000>; + wakeup-source; cros_ec_pwm: pwm { compatible = "google,cros-ec-pwm"; diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi index ebae545c587c4..fbfac7534d3c6 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi @@ -19,6 +19,7 @@ cros_ec: ec@0 { pinctrl-names = "default"; pinctrl-0 = <&ap_ec_int_l>; spi-max-frequency = <3000000>; + wakeup-source; cros_ec_pwm: pwm { compatible = "google,cros-ec-pwm"; diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi index f86e7acdfd99f..d8eb45662c931 100644 --- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi @@ -838,6 +838,7 @@ cros_ec: ec@0 { pinctrl-names = "default"; pinctrl-0 = <&ec_ap_int_l>; spi-max-frequency = <3000000>; + wakeup-source; cros_ec_pwm: pwm { compatible = "google,cros-ec-pwm"; diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi index c9bf1d5c3a426..69a0b34f0615b 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi @@ -602,6 +602,7 @@ cros_ec: ec@0 { pinctrl-names = "default"; pinctrl-0 = <&ec_ap_int_l>; spi-max-frequency = <3000000>; + wakeup-source; i2c_tunnel: i2c-tunnel { compatible = "google,cros-ec-i2c-tunnel";
The cros_ec driver currently assumes that cros-ec-spi compatible device nodes are a wakeup-source even though the wakeup-source property is not defined. Add the wakeup-source property to all cros-ec-spi compatible device nodes to match expected behavior. Signed-off-by: Mark Hasemeyer <markhas@chromium.org> --- arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi | 1 + arch/arm/boot/dts/nvidia/tegra124-venice2.dts | 1 + arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi | 1 + arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts | 1 + arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts | 1 + arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 + arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 + arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi | 1 + arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 1 + arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 1 + arch/arm64/boot/dts/nvidia/tegra132-norrin.dts | 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 + arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 + arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 1 + arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 1 + arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 1 + 16 files changed, 16 insertions(+)