[v1,2/6] arm: arm64: dts: Enable cros-ec-spi as wake source
Commit Message
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(+)
Comments
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
@@ -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>;
@@ -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>;
@@ -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";
@@ -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>;
@@ -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>;
@@ -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";
@@ -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";
@@ -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";
@@ -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>;
@@ -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";
@@ -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>;
@@ -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";
@@ -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";
@@ -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";
@@ -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";
@@ -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";