| Message ID | 20240715-cros-backlight-dt-probe-v1-1-0b5afe64c94b@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] arm64: dts: mediatek: mt8195-cherry: Remove keyboard-backlight node | expand |
On Tue, Jul 16, 2024 at 12:13 AM Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote: > > Commit 970c3a6b7aa3 ("mfd: cros_ec: Register keyboard backlight > subdevice") introduced support for detecting keyboard backlight > fuctionality through communication with the ChromeOS EC. This means that > the DT node is no longer used. Remove the unneeded node. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > Different CrosEC FW versions could potentially not support discovering > the keyboard backlight functionality, but I've tested both a recent > > tomato_v2.0.23149-099cd3e539 tomato_15699.72.0 2024-01-03 > > and an old > > tomato_v2.0.10686-234e646fd8 tomato_14268.0.0 2021-10-07 > > version on mt8195-cherry-tomato and on both relying only on the > discoverability works. I've tested on both tomato-r2 and tomato-r3. I > have not tested on dojo, however, as I don't have access to it. I grabbed one from our warehouse and this works as explained. So, Tested-by: Chen-Yu Tsai <wenst@chromium.org> Note that there's actually one SKU family for Dojo that doesn't have the keyboard backlight, which I haven't tested yet. It also needs a new device tree to describe the slightly different keyboard layout. > My motivation to remove the node is because the DT kselftest expects DT > nodes that can match to a driver to be probed, and with the "breaking" > commit, the DT node goes unprobed which results in a failure: > > not ok 225 /soc/spi@1100a000/ec@0/keyboard-backlight > > I can also solve this in a different way, by adding this driver to the > ignore list of the test. But this solution seemed better as the DT > isn't meant to describe devices that can be discovered at run time > anyway. I think this makes sense. Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > --- > arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi > index fe5400e17b0f..20dfa18c9dda 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi > @@ -1228,10 +1228,6 @@ cros_ec: ec@0 { > spi-max-frequency = <3000000>; > wakeup-source; > > - keyboard-backlight { > - compatible = "google,cros-kbd-led-backlight"; > - }; > - > i2c_tunnel: i2c-tunnel { > compatible = "google,cros-ec-i2c-tunnel"; > google,remote-bus = <0>; > > --- > base-commit: 91e3b24eb7d297d9d99030800ed96944b8652eaf > change-id: 20240715-cros-backlight-dt-probe-7754a832ad60 > > Best regards, > -- > Nícolas F. R. A. Prado <nfraprado@collabora.com> > >
Il 15/07/24 18:09, Nícolas F. R. A. Prado ha scritto: > Commit 970c3a6b7aa3 ("mfd: cros_ec: Register keyboard backlight > subdevice") introduced support for detecting keyboard backlight > fuctionality through communication with the ChromeOS EC. This means that > the DT node is no longer used. Remove the unneeded node. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > Different CrosEC FW versions could potentially not support discovering > the keyboard backlight functionality, but I've tested both a recent > > tomato_v2.0.23149-099cd3e539 tomato_15699.72.0 2024-01-03 > > and an old > > tomato_v2.0.10686-234e646fd8 tomato_14268.0.0 2021-10-07 > > version on mt8195-cherry-tomato and on both relying only on the > discoverability works. I've tested on both tomato-r2 and tomato-r3. I > have not tested on dojo, however, as I don't have access to it. > Dojo will work anyway because those machines do share the same base FW... but anyway, I'm not sure that this is the right thing to do. The commit that you mentioned says that it is meant to make that "work on machines without specific ACPI or OF support for the keyboard backlight", but not that the intention is to stop using either ACPI nor DT nodes for that. The DT kselftest is relatively young, and I suspect that anyway this is not the only affected device, so the justification is only barely valid. Don't misunderstand me, I'm not saying that I'm not okay with this, but I'd like to have more opinions about this. If we choose to go this way, ideally we should remove this from all of the upstream Chromebook devicetrees (not only MediaTek, clearly!) so that would require a bit more effort to test here and there. Any opinion from anyone? Cheers, Angelo > My motivation to remove the node is because the DT kselftest expects DT > nodes that can match to a driver to be probed, and with the "breaking" > commit, the DT node goes unprobed which results in a failure: > > not ok 225 /soc/spi@1100a000/ec@0/keyboard-backlight > > I can also solve this in a different way, by adding this driver to the > ignore list of the test. But this solution seemed better as the DT > isn't meant to describe devices that can be discovered at run time > anyway. > --- > arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi > index fe5400e17b0f..20dfa18c9dda 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi > @@ -1228,10 +1228,6 @@ cros_ec: ec@0 { > spi-max-frequency = <3000000>; > wakeup-source; > > - keyboard-backlight { > - compatible = "google,cros-kbd-led-backlight"; > - }; > - > i2c_tunnel: i2c-tunnel { > compatible = "google,cros-ec-i2c-tunnel"; > google,remote-bus = <0>; > > --- > base-commit: 91e3b24eb7d297d9d99030800ed96944b8652eaf > change-id: 20240715-cros-backlight-dt-probe-7754a832ad60 > > Best regards,
On Tue, Jul 16, 2024 at 11:24:44AM +0200, AngeloGioacchino Del Regno wrote: > Il 15/07/24 18:09, Nícolas F. R. A. Prado ha scritto: > > Commit 970c3a6b7aa3 ("mfd: cros_ec: Register keyboard backlight > > subdevice") introduced support for detecting keyboard backlight > > fuctionality through communication with the ChromeOS EC. This means that > > the DT node is no longer used. Remove the unneeded node. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Different CrosEC FW versions could potentially not support discovering > > the keyboard backlight functionality, but I've tested both a recent > > > > tomato_v2.0.23149-099cd3e539 tomato_15699.72.0 2024-01-03 > > > > and an old > > > > tomato_v2.0.10686-234e646fd8 tomato_14268.0.0 2021-10-07 > > > > version on mt8195-cherry-tomato and on both relying only on the > > discoverability works. I've tested on both tomato-r2 and tomato-r3. I > > have not tested on dojo, however, as I don't have access to it. > > > > Dojo will work anyway because those machines do share the same base FW... but > anyway, I'm not sure that this is the right thing to do. > > The commit that you mentioned says that it is meant to make that "work on machines > without specific ACPI or OF support for the keyboard backlight", but not that the > intention is to stop using either ACPI nor DT nodes for that. Yes, because as I understand it not every EC might support this protocol. So that commit just added an additional way to probe the keyboard backlight. So we don't need to stop using the DT to probe it. But in practice we have already stopped, as long as the EC supports the protocol (which from my testing is always for these platforms), since that is tried first. Meaning the DT node is now useless. The only point in keeping the DT node would be to use it as a fallback in case the discovery with the EC fails or breaks. But I have never seen a DT node be there just as fallback, so it doesn't feel right to me either. > > The DT kselftest is relatively young, and I suspect that anyway this is not the > only affected device, so the justification is only barely valid. I didn't include the failing test as part of the commit message proper as I don't think it should justify this change. I added it just to clarify my motivation. The test showed me that something unexpected was happening. After looking into it I thought that a DT node that is no longer used to probe has no point in staying around, so that's the justification that I added to the commit message. > > Don't misunderstand me, I'm not saying that I'm not okay with this, but I'd like to > have more opinions about this. > > If we choose to go this way, ideally we should remove this from all of the upstream > Chromebook devicetrees (not only MediaTek, clearly!) so that would require a bit > more effort to test here and there. Note that the cherry DT is the only DT upstream with the google,cros-kbd-led-backlight compatible. So it's really only tomato and dojo that need to be tested. Thanks, Nícolas
Il 16/07/24 14:16, Nícolas F. R. A. Prado ha scritto: > On Tue, Jul 16, 2024 at 11:24:44AM +0200, AngeloGioacchino Del Regno wrote: >> Il 15/07/24 18:09, Nícolas F. R. A. Prado ha scritto: >>> Commit 970c3a6b7aa3 ("mfd: cros_ec: Register keyboard backlight >>> subdevice") introduced support for detecting keyboard backlight >>> fuctionality through communication with the ChromeOS EC. This means that >>> the DT node is no longer used. Remove the unneeded node. >>> >>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> >>> --- >>> Different CrosEC FW versions could potentially not support discovering >>> the keyboard backlight functionality, but I've tested both a recent >>> >>> tomato_v2.0.23149-099cd3e539 tomato_15699.72.0 2024-01-03 >>> >>> and an old >>> >>> tomato_v2.0.10686-234e646fd8 tomato_14268.0.0 2021-10-07 >>> >>> version on mt8195-cherry-tomato and on both relying only on the >>> discoverability works. I've tested on both tomato-r2 and tomato-r3. I >>> have not tested on dojo, however, as I don't have access to it. >>> >> >> Dojo will work anyway because those machines do share the same base FW... but >> anyway, I'm not sure that this is the right thing to do. >> >> The commit that you mentioned says that it is meant to make that "work on machines >> without specific ACPI or OF support for the keyboard backlight", but not that the >> intention is to stop using either ACPI nor DT nodes for that. > > Yes, because as I understand it not every EC might support this protocol. So > that commit just added an additional way to probe the keyboard backlight. > > So we don't need to stop using the DT to probe it. But in practice we have > already stopped, as long as the EC supports the protocol (which from my testing > is always for these platforms), since that is tried first. Meaning the DT node > is now useless. > > The only point in keeping the DT node would be to use it as a fallback in case > the discovery with the EC fails or breaks. But I have never seen a DT node be > there just as fallback, so it doesn't feel right to me either. > >> >> The DT kselftest is relatively young, and I suspect that anyway this is not the >> only affected device, so the justification is only barely valid. > > I didn't include the failing test as part of the commit message proper as I > don't think it should justify this change. I added it just to clarify my > motivation. The test showed me that something unexpected was happening. After > looking into it I thought that a DT node that is no longer used to probe has no > point in staying around, so that's the justification that I added to the commit > message. > >> >> Don't misunderstand me, I'm not saying that I'm not okay with this, but I'd like to >> have more opinions about this. >> >> If we choose to go this way, ideally we should remove this from all of the upstream >> Chromebook devicetrees (not only MediaTek, clearly!) so that would require a bit >> more effort to test here and there. > > Note that the cherry DT is the only DT upstream with the > google,cros-kbd-led-backlight compatible. So it's really only tomato and dojo > that need to be tested. > Perfect. Let's remove it then, possibly with fire ;-) Can you please send the patch without the RFC tag? Also add my R-b, so that I'll remember that I've already checked that patch for when I'll be able to pick it. Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cheers!
diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi index fe5400e17b0f..20dfa18c9dda 100644 --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi @@ -1228,10 +1228,6 @@ cros_ec: ec@0 { spi-max-frequency = <3000000>; wakeup-source; - keyboard-backlight { - compatible = "google,cros-kbd-led-backlight"; - }; - i2c_tunnel: i2c-tunnel { compatible = "google,cros-ec-i2c-tunnel"; google,remote-bus = <0>;
Commit 970c3a6b7aa3 ("mfd: cros_ec: Register keyboard backlight subdevice") introduced support for detecting keyboard backlight fuctionality through communication with the ChromeOS EC. This means that the DT node is no longer used. Remove the unneeded node. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Different CrosEC FW versions could potentially not support discovering the keyboard backlight functionality, but I've tested both a recent tomato_v2.0.23149-099cd3e539 tomato_15699.72.0 2024-01-03 and an old tomato_v2.0.10686-234e646fd8 tomato_14268.0.0 2021-10-07 version on mt8195-cherry-tomato and on both relying only on the discoverability works. I've tested on both tomato-r2 and tomato-r3. I have not tested on dojo, however, as I don't have access to it. My motivation to remove the node is because the DT kselftest expects DT nodes that can match to a driver to be probed, and with the "breaking" commit, the DT node goes unprobed which results in a failure: not ok 225 /soc/spi@1100a000/ec@0/keyboard-backlight I can also solve this in a different way, by adding this driver to the ignore list of the test. But this solution seemed better as the DT isn't meant to describe devices that can be discovered at run time anyway. --- arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 4 ---- 1 file changed, 4 deletions(-) --- base-commit: 91e3b24eb7d297d9d99030800ed96944b8652eaf change-id: 20240715-cros-backlight-dt-probe-7754a832ad60 Best regards,