Re: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5

Re: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5

[Krzysztof Kozlowski]

On Sun, Feb 19, 2017 at 08:11:26PM +0100, Leandro Santiago wrote:
> Thank you for the tips. I have just added linux-samsung-soc@vger.kernel.org
> to cc, even though get_mantainer.pl did not suggested such mailing list.

The scripts suggests it:
linux-samsung-soc@vger.kernel.org (moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES)

It also suggests other mailing lists...

Maybe you are not using mainline kernel but in that case patch needs
testing on mainline by someone else.

Please send a proper v2 patch (git format-patch -v2) with proper title
(I do not see any change below).

I do not see also any change in the node order.

Please address all of the comments. If you do not agree with some of
them we can discuss but ignoring a comment is a no-go.


> About the interrupts property, on the original chromeos-3.8, it was defined
> as <2 0>, I just replaced the second zero by it by IRQ_TYPE_NONE as it
> evaluates to zero anyways and is compatible with the existing trackpad
> node. I have fixed it by setting it to IRQ_TYPE_EDGE_FALLING as you
> suggested. I have no access to the platform data though, so could not check
> if it's really correct or not. But still works fine on my chromebook.

The platform data is in Linux kernel:
http://lxr.free-electrons.com/source/drivers/platform/chrome/chromeos_laptop.c#L116

Best regards,
Krzysztof

Re: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5

[Javier Martinez Canillas]

Hello Leandro,

On 02/19/2017 04:11 PM, Leandro Santiago wrote:
> Thank you for the tips. I have just added linux-samsung-soc@vger.kernel.org
> to cc, even though get_mantainer.pl did not suggested such mailing list.
> 
> About the interrupts property, on the original chromeos-3.8, it was defined
> as <2 0>, I just replaced the second zero by it by IRQ_TYPE_NONE as it
> evaluates to zero anyways and is compatible with the existing trackpad
> node. I have fixed it by setting it to IRQ_TYPE_EDGE_FALLING as you
> suggested. I have no access to the platform data though, so could not check
> if it's really correct or not. But still works fine on my chromebook.
> 
> Here is the updated version of the patch:
> 
> From 458b5b891a98e6163321396514b9d9227ae50e69 Mon Sep 17 00:00:00 2001
> From: Leandro Santiago <leandrosansilva@gmail.com>
> Date: Sun, 19 Feb 2017 16:50:18 +0100
> Subject: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5
> 
> The Samsung Chromebook uses an Atmel maxTouch as trackpad.
> 
> The keymap was based on Exynos5250-based Google Spring and most of
> the other values were based on chromeos-3.8 branch from the ChromeOS
> kernel.
> 
> Signed-off-by: Leandro Santiago <leandrosansilva@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5250-snow-common.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5250-snow-common.dtsi
> b/arch/arm/boot/dts/exynos5250-snow-common.dtsi
> index 8f3a80430748..183ff57c6455 100644
> --- a/arch/arm/boot/dts/exynos5250-snow-common.dtsi
> +++ b/arch/arm/boot/dts/exynos5250-snow-common.dtsi
> @@ -434,6 +434,20 @@
>                 interrupt-parent = <&gpx1>;
>                 wakeup-source;
>         };
> +
> +       trackpad@4b {
> +               compatible = "atmel,atmel_mxt_tp";
> +               reg = <0x4b>;
> +               interrupt-parent = <&gpx1>;
> +               interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
> +               linux,gpio-keymap = <KEY_RESERVED
> +                                    KEY_RESERVED
> +                                    KEY_RESERVED
> +                                    KEY_RESERVED
> +                                    KEY_RESERVED
> +                                    BTN_LEFT>;
> +               wakeup-source;
> +       };
>  };
> 

I noticed that the downstream ChromiumOS tree does the same, but I don't
think that's correct to add both trackpad devices under the same I2C bus.

DT is a hardware description and so it should describe what the device
truly has and not add all the possible combinations and make the driver
fail or succeed if according to what's found on a particular board.

So this is a DT hack and instead all the possible variations should be
properly described in different DT with (possibly) common DTS snippets.

I thought that all Snow variations used the Cypress cyapa chip but now
recall a conversation on #linux-exynos about some Snow rev5 variations
using the same Atmel chip used in the Exynos5420/5800 Peach Chromebooks.

That definitely isn't the case for all Snow rev5 though since some have
the Cypress chip like in previous revisions, according to other users.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5

[Krzysztof Kozlowski]

On Mon, Feb 20, 2017 at 5:44 PM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
> Hello Leandro,
>
> On 02/19/2017 04:11 PM, Leandro Santiago wrote:
>> Thank you for the tips. I have just added linux-samsung-soc@vger.kernel.org
>> to cc, even though get_mantainer.pl did not suggested such mailing list.
>>
>> About the interrupts property, on the original chromeos-3.8, it was defined
>> as <2 0>, I just replaced the second zero by it by IRQ_TYPE_NONE as it
>> evaluates to zero anyways and is compatible with the existing trackpad
>> node. I have fixed it by setting it to IRQ_TYPE_EDGE_FALLING as you
>> suggested. I have no access to the platform data though, so could not check
>> if it's really correct or not. But still works fine on my chromebook.
>>
>> Here is the updated version of the patch:
>>
>> From 458b5b891a98e6163321396514b9d9227ae50e69 Mon Sep 17 00:00:00 2001
>> From: Leandro Santiago <leandrosansilva@gmail.com>
>> Date: Sun, 19 Feb 2017 16:50:18 +0100
>> Subject: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5
>>
>> The Samsung Chromebook uses an Atmel maxTouch as trackpad.
>>
>> The keymap was based on Exynos5250-based Google Spring and most of
>> the other values were based on chromeos-3.8 branch from the ChromeOS
>> kernel.
>>
>> Signed-off-by: Leandro Santiago <leandrosansilva@gmail.com>
>> ---
>>  arch/arm/boot/dts/exynos5250-snow-common.dtsi | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos5250-snow-common.dtsi
>> b/arch/arm/boot/dts/exynos5250-snow-common.dtsi
>> index 8f3a80430748..183ff57c6455 100644
>> --- a/arch/arm/boot/dts/exynos5250-snow-common.dtsi
>> +++ b/arch/arm/boot/dts/exynos5250-snow-common.dtsi
>> @@ -434,6 +434,20 @@
>>                 interrupt-parent = <&gpx1>;
>>                 wakeup-source;
>>         };
>> +
>> +       trackpad@4b {
>> +               compatible = "atmel,atmel_mxt_tp";
>> +               reg = <0x4b>;
>> +               interrupt-parent = <&gpx1>;
>> +               interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
>> +               linux,gpio-keymap = <KEY_RESERVED
>> +                                    KEY_RESERVED
>> +                                    KEY_RESERVED
>> +                                    KEY_RESERVED
>> +                                    KEY_RESERVED
>> +                                    BTN_LEFT>;
>> +               wakeup-source;
>> +       };
>>  };
>>
>
> I noticed that the downstream ChromiumOS tree does the same, but I don't
> think that's correct to add both trackpad devices under the same I2C bus.
>
> DT is a hardware description and so it should describe what the device
> truly has and not add all the possible combinations and make the driver
> fail or succeed if according to what's found on a particular board.
>
> So this is a DT hack and instead all the possible variations should be
> properly described in different DT with (possibly) common DTS snippets.
>
> I thought that all Snow variations used the Cypress cyapa chip but now
> recall a conversation on #linux-exynos about some Snow rev5 variations
> using the same Atmel chip used in the Exynos5420/5800 Peach Chromebooks.
>
> That definitely isn't the case for all Snow rev5 though since some have
> the Cypress chip like in previous revisions, according to other users.

Oh, wait, I thought that these two devices actually exist on all Snows
on this I2C bus. They have different addresses so it seemed possible
(although having many trackpads looks weird). You think this is only
for specific revisions of Snows?

Best regards,
Krzysztof

Re: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5

[Javier Martinez Canillas]

Hello Krzysztof,

On 02/20/2017 01:33 PM, Krzysztof Kozlowski wrote:

[snip]

>>> +
>>> +       trackpad@4b {
>>> +               compatible = "atmel,atmel_mxt_tp";
>>> +               reg = <0x4b>;
>>> +               interrupt-parent = <&gpx1>;
>>> +               interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
>>> +               linux,gpio-keymap = <KEY_RESERVED
>>> +                                    KEY_RESERVED
>>> +                                    KEY_RESERVED
>>> +                                    KEY_RESERVED
>>> +                                    KEY_RESERVED
>>> +                                    BTN_LEFT>;
>>> +               wakeup-source;
>>> +       };
>>>  };
>>>
>>
>> I noticed that the downstream ChromiumOS tree does the same, but I don't
>> think that's correct to add both trackpad devices under the same I2C bus.
>>
>> DT is a hardware description and so it should describe what the device
>> truly has and not add all the possible combinations and make the driver
>> fail or succeed if according to what's found on a particular board.
>>
>> So this is a DT hack and instead all the possible variations should be
>> properly described in different DT with (possibly) common DTS snippets.
>>
>> I thought that all Snow variations used the Cypress cyapa chip but now
>> recall a conversation on #linux-exynos about some Snow rev5 variations
>> using the same Atmel chip used in the Exynos5420/5800 Peach Chromebooks.
>>
>> That definitely isn't the case for all Snow rev5 though since some have
>> the Cypress chip like in previous revisions, according to other users.
> 
> Oh, wait, I thought that these two devices actually exist on all Snows
> on this I2C bus. They have different addresses so it seemed possible

No, they don't. As mentioned, most Snows revs use the Cypress trackpad and
the Spring (another Exynos5250 Chromebook) uses the Atmel trackpad instead.

Also the Exynos5420/5800 Chromebooks use the Atmel trackpad too, but turns
out that some Snow revisions use the Atmel trackpad as well.

> (although having many trackpads looks weird). You think this is only
> for specific revisions of Snows?
>

Yes, all Chromebooks have only one trackpad and it varies between revisions.

The reason the downstream ChromiumOS tree have both I2C devices in the same
I2C bus was to have a common DTSI file to be used for all Exynos5250 boards.

This is the downstream commit in the ChromiumOS tree that added both devices
to the common DTSI file and you can see the intention in the commit [0]:

"Since we'd now like both defined for exynos5250-snow, move the definition
up to the cros5250-common device tree file.  This allows snow to work with
_either_ the cyapa or atmel touch pad dynamically."

So in the downstream tree both cyapa and atmel_mxt_ts drivers are probed and
only one succeeds. Obviously that's not the correct approach for doing this.

> Best regards,
> Krzysztof
> 

[0]: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0838b43f82908a3f67fea67303ebba1bebfc497a

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5

[Krzysztof Kozlowski]

On Mon, Feb 20, 2017 at 04:11:17PM -0300, Javier Martinez Canillas wrote:
> > (although having many trackpads looks weird). You think this is only
> > for specific revisions of Snows?
> >
> 
> Yes, all Chromebooks have only one trackpad and it varies between revisions.
> 
> The reason the downstream ChromiumOS tree have both I2C devices in the same
> I2C bus was to have a common DTSI file to be used for all Exynos5250 boards.
> 
> This is the downstream commit in the ChromiumOS tree that added both devices
> to the common DTSI file and you can see the intention in the commit [0]:
> 
> "Since we'd now like both defined for exynos5250-snow, move the definition
> up to the cros5250-common device tree file.  This allows snow to work with
> _either_ the cyapa or atmel touch pad dynamically."
> 
> So in the downstream tree both cyapa and atmel_mxt_ts drivers are probed and
> only one succeeds. Obviously that's not the correct approach for doing this.

Indeed, this does not look right. Also meaningless probing of invalid I2C
devices usually takes some time... The trackpad should be added to
DTS for given revision of Snow.

Best regards,
Krzysztof

Re: [PATCH] ARM: dts: Add trackpad to exynos5250-snow-rev5

[Krzysztof Kozlowski]

On Mon, Feb 20, 2017 at 08:13:21PM +0100, Leandro Santiago wrote:
> I am sending the v2 of the patch, with the following issues addressed:
> - properties ordering by importance
> - nodes ordering by address
> - fixed commit message
> - fixed interrupts property with edge failing interruption.

You make reading the patches especially difficult. Let me make it clear:
1. The previous one was v2 so this is the same?
2. Changelog goes after --- separator.
3. git format-patch -v2 (or whatever number)
4. scripts/get_maintainer.pl v2-....
5. git send-email --to ..... v2-...

Instead you attached v2 to some old conversation. Also your patch seems
to miss the mailing lists even though you CC-ed one of them here.
Probably you need to subscribe it first.

Missing the mailing list means:
1. It is not public.
2. It is not present on Patchwork which I use to apply the patches:
https://patchwork.kernel.org/project/linux-samsung-soc/list/

The patch's subject is still wrong - no changes here.

Beside the formal stuff, Javier's points remain valid. This should be
added to proper revision.

Best regards,
Krzysztof

> 
> Javier, I really I don't know exactly about the revision of my chromebook,
> but with the chromeos kernel it boots using the revision 5 dtb.
> 
> From a59ffe148a03e9b66918c30a553d564db32b885f Mon Sep 17 00:00:00 2001
> From: Leandro Santiago <leandrosansilva@gmail.com>
> Date: Sun, 19 Feb 2017 16:50:18 +0100
> Subject: [PATCH v2] ARM: dts: Add atmel maxtouch trackpad to
>  exynos5250-snow-rev5