Re: [PATCH RFC] arm64: dts: mediatek: mt8195-cherry: Remove keyboard-backlight node

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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!