[PATCH v2 2/2] hp-wmi: Fix detection for dock and tablet mode
From: Carlo Caione <ca...@endlessm.com>
The current driver code is not checking for the error values returned by
'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the
returned values down to 'input_report_switch()'. This error code is
being translated to '1' in the input subsystem, reporting the wrong
status.
The biggest problem caused by this issue is that several laptops are
wrongly reported by the driver as docked, preventing them to be put to
sleep using the LID (and in most cases they are not even dockable).
With this patch we create the report switches only if we are able to
read the dock and tablet mode status correctly from ACPI.
Signed-off-by: Carlo Caione <ca...@endlessm.com>
---
drivers/platform/x86/hp-wmi.c | 40 +++-
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 8c7773a..95fab81 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context)
switch (event_id) {
case HPWMI_DOCK_EVENT:
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
break;
case HPWMI_PARK_HDD:
@@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void)
{
acpi_status status;
int err;
+ int val;
hp_wmi_input_dev = input_allocate_device();
if (!hp_wmi_input_dev)
@@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void)
hp_wmi_input_dev->id.bustype = BUS_HOST;
__set_bit(EV_SW, hp_wmi_input_dev->evbit);
- __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
- __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+
+ /* Dock */
+ val = hp_wmi_dock_state();
+ if (!(val < 0)) {
+ __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
+ }
+
+ /* Tablet mode */
+ val = hp_wmi_tablet_state();
+ if (!(val < 0)) {
+ __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
+ }
err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL);
if (err)
goto err_free_dev;
/* Set initial hardware state */
- input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later())
@@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device)
* changed.
*/
if (hp_wmi_input_dev) {
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
}
--
2.9.3
[PATCH v2 2/2] hp-wmi: Fix detection for dock and tablet mode
From: Carlo Caione
The current driver code is not checking for the error values returned by
'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the
returned values down to 'input_report_switch()'. This error code is
being translated to '1' in the input subsystem, reporting the wrong
status.
The biggest problem caused by this issue is that several laptops are
wrongly reported by the driver as docked, preventing them to be put to
sleep using the LID (and in most cases they are not even dockable).
With this patch we create the report switches only if we are able to
read the dock and tablet mode status correctly from ACPI.
Signed-off-by: Carlo Caione
---
drivers/platform/x86/hp-wmi.c | 40 +++-
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 8c7773a..95fab81 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context)
switch (event_id) {
case HPWMI_DOCK_EVENT:
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
break;
case HPWMI_PARK_HDD:
@@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void)
{
acpi_status status;
int err;
+ int val;
hp_wmi_input_dev = input_allocate_device();
if (!hp_wmi_input_dev)
@@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void)
hp_wmi_input_dev->id.bustype = BUS_HOST;
__set_bit(EV_SW, hp_wmi_input_dev->evbit);
- __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
- __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+
+ /* Dock */
+ val = hp_wmi_dock_state();
+ if (!(val < 0)) {
+ __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
+ }
+
+ /* Tablet mode */
+ val = hp_wmi_tablet_state();
+ if (!(val < 0)) {
+ __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
+ }
err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL);
if (err)
goto err_free_dev;
/* Set initial hardware state */
- input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later())
@@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device)
* changed.
*/
if (hp_wmi_input_dev) {
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
}
--
2.9.3
Re: [PATCH 1/2] hp-wmi: Fix error value for hp_wmi_tablet_state
On Wed, Apr 19, 2017 at 6:21 PM, Andy Shevchenko <andy.shevche...@gmail.com> wrote: > On Sun, Apr 9, 2017 at 4:56 PM, Carlo Caione <ca...@caione.org> wrote: >> From: Carlo Caione <ca...@endlessm.com> >> >> hp_wmi_tablet_state() fails to return the correct error code when >> hp_wmi_perform_query() returns the HP WMI query specific error code >> that is a positive value. > >> int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, , >>sizeof(state), sizeof(state)); >> if (ret) >> - return ret; >> + return -EINVAL; > > Shouldn't be something like > > if (ret) > return ret < 0 ? ret : -EINVAL; > > Looking into the code it looks like it may return all possible values: > 0, negative, positive. When the HP WMI query returns a positive value something went wrong. hp_wmi_perform_query() returns 0 on success. -- Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH 1/2] hp-wmi: Fix error value for hp_wmi_tablet_state
On Wed, Apr 19, 2017 at 6:21 PM, Andy Shevchenko wrote: > On Sun, Apr 9, 2017 at 4:56 PM, Carlo Caione wrote: >> From: Carlo Caione >> >> hp_wmi_tablet_state() fails to return the correct error code when >> hp_wmi_perform_query() returns the HP WMI query specific error code >> that is a positive value. > >> int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, , >>sizeof(state), sizeof(state)); >> if (ret) >> - return ret; >> + return -EINVAL; > > Shouldn't be something like > > if (ret) > return ret < 0 ? ret : -EINVAL; > > Looking into the code it looks like it may return all possible values: > 0, negative, positive. When the HP WMI query returns a positive value something went wrong. hp_wmi_perform_query() returns 0 on success. -- Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart <dvh...@infradead.org> wrote:
> On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote:
>> From: Carlo Caione <ca...@endlessm.com>
/cut
>> @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void)
>> {
>> acpi_status status;
>> int err;
>> + int val;
>>
>> hp_wmi_input_dev = input_allocate_device();
>> if (!hp_wmi_input_dev)
>> @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void)
>> hp_wmi_input_dev->id.bustype = BUS_HOST;
>>
>> __set_bit(EV_SW, hp_wmi_input_dev->evbit);
>> - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
>> - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
>> +
>> + /* Dock */
>> + val = hp_wmi_dock_state();
>> + if (!(val < 0)) {
>> + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
>> + input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
>> + }
>
> In general, these are fine and can go in. I did want to get your opinion on
> one
> thought though.
>
> This adds some complexity to deal with what appears to be an unknown failure
> mode (the query fails, we don't know why, so we don't set the bit on the input
> dev for that feature). Since we don't know why it fails, can we be confident
> it
> will always fail?
That's not exactly true, at least for the firmware I have on the
laptop I'm working on.
For this hardware (can we assume for all the HP models?) when the WMI
calls returns the value of 0x04, that means that the query
(HPWMI_HARDWARE_QUERY in this case) is not implemented at all in the
SSDT.
In general reading the disassembled AML code when the WMI query fails
and returns a positive value this can be:
- 0x04: Query ID is unknown / not implemented but valid
- 0x02: Wrong signature
- 0x05: Wrong / invalid query number (?)
The problem here is that: (1) this is my personal interpretation of
the AML code obtained by disassembling the SSDT and (2) we cannot be
sure that this is the same on all the HP firmwares around.
For sure in general in all the cases I extracted from the SSDT table
on this hardware if the call failed the first time all the chances are
that it is going to fail also in the future.
In [1] is the SSDT table, the WMI method is WMAA if you want to check
my interpretation.
> Could it succeed at init here, but then fail later and leave
> us in the same situation we are in now?
I think that this is really unlikely
> If so, have you considered just returning 0 on error and using a WARN_ONCE
> print
> statement to report the error? This would simplify a lot of this logic that
> you're adding in here to handle something we could just report and ignore.
Yes, I thought to report just 0 but in that case we are advertising to
userspace fake capabilities for the hardware, like dockability or
laptop mode that in most cases are not even implemented on the
hardware (like on this laptop).
> That being said, your version avoids the input_report_switch() in the event
> of a
> failure at init. In practice, I don't know if this is worth the added
> complexity.
>
> Your thoughts?
AFAICT we can fail in hp_wmi_perform_query (as written in the comment
to the function):
1) with -EINVAL if the query was not successful or the output buffer
size exceeds buffersize. In this case I don't see how the next calls
could be successful.
2) with a positive error code returned from the WMI method. Given my
interpretation of this positive code reported before I don't see why
we should fail only on init and not on all the subsequent calls
So I'm still convinced that my implementation is correct and that
probably adding complexity on top is not really worth it. But of
course this is your call as maintainer :)
Thanks,
[1]
https://paste.fedoraproject.org/paste/bBnqUlazz1tAjKsJKq7NHl5M1UNdIGYhyRLivL9gydE=
--
Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
On Thu, Apr 13, 2017 at 8:21 PM, Darren Hart wrote:
> On Sun, Apr 09, 2017 at 03:56:08PM +0200, Carlo Caione wrote:
>> From: Carlo Caione
/cut
>> @@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void)
>> {
>> acpi_status status;
>> int err;
>> + int val;
>>
>> hp_wmi_input_dev = input_allocate_device();
>> if (!hp_wmi_input_dev)
>> @@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void)
>> hp_wmi_input_dev->id.bustype = BUS_HOST;
>>
>> __set_bit(EV_SW, hp_wmi_input_dev->evbit);
>> - __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
>> - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
>> +
>> + /* Dock */
>> + val = hp_wmi_dock_state();
>> + if (!(val < 0)) {
>> + __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
>> + input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
>> + }
>
> In general, these are fine and can go in. I did want to get your opinion on
> one
> thought though.
>
> This adds some complexity to deal with what appears to be an unknown failure
> mode (the query fails, we don't know why, so we don't set the bit on the input
> dev for that feature). Since we don't know why it fails, can we be confident
> it
> will always fail?
That's not exactly true, at least for the firmware I have on the
laptop I'm working on.
For this hardware (can we assume for all the HP models?) when the WMI
calls returns the value of 0x04, that means that the query
(HPWMI_HARDWARE_QUERY in this case) is not implemented at all in the
SSDT.
In general reading the disassembled AML code when the WMI query fails
and returns a positive value this can be:
- 0x04: Query ID is unknown / not implemented but valid
- 0x02: Wrong signature
- 0x05: Wrong / invalid query number (?)
The problem here is that: (1) this is my personal interpretation of
the AML code obtained by disassembling the SSDT and (2) we cannot be
sure that this is the same on all the HP firmwares around.
For sure in general in all the cases I extracted from the SSDT table
on this hardware if the call failed the first time all the chances are
that it is going to fail also in the future.
In [1] is the SSDT table, the WMI method is WMAA if you want to check
my interpretation.
> Could it succeed at init here, but then fail later and leave
> us in the same situation we are in now?
I think that this is really unlikely
> If so, have you considered just returning 0 on error and using a WARN_ONCE
> print
> statement to report the error? This would simplify a lot of this logic that
> you're adding in here to handle something we could just report and ignore.
Yes, I thought to report just 0 but in that case we are advertising to
userspace fake capabilities for the hardware, like dockability or
laptop mode that in most cases are not even implemented on the
hardware (like on this laptop).
> That being said, your version avoids the input_report_switch() in the event
> of a
> failure at init. In practice, I don't know if this is worth the added
> complexity.
>
> Your thoughts?
AFAICT we can fail in hp_wmi_perform_query (as written in the comment
to the function):
1) with -EINVAL if the query was not successful or the output buffer
size exceeds buffersize. In this case I don't see how the next calls
could be successful.
2) with a positive error code returned from the WMI method. Given my
interpretation of this positive code reported before I don't see why
we should fail only on init and not on all the subsequent calls
So I'm still convinced that my implementation is correct and that
probably adding complexity on top is not really worth it. But of
course this is your call as maintainer :)
Thanks,
[1]
https://paste.fedoraproject.org/paste/bBnqUlazz1tAjKsJKq7NHl5M1UNdIGYhyRLivL9gydE=
--
Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH 0/2] hp-wmi: Fix dock status and tablet mode reporting
On Sun, Apr 9, 2017 at 3:56 PM, Carlo Caione <ca...@caione.org> wrote: > From: Carlo Caione <ca...@endlessm.com> > > Several HP laptops cannot be put to sleep using the LID since systemd > complains > that the system is docked even though the laptop is not even dockable (see > [1]). > > This is due to a bug in hp-wmi where the driver is failing to check for errors > before creating the input switches. > > [1]: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1574120 > > Carlo Caione (2): > hp-wmi: Fix error value for hp_wmi_tablet_state > hp-wmi: Fix detection for dock and tablet mode Gentle ping. -- Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH 0/2] hp-wmi: Fix dock status and tablet mode reporting
On Sun, Apr 9, 2017 at 3:56 PM, Carlo Caione wrote: > From: Carlo Caione > > Several HP laptops cannot be put to sleep using the LID since systemd > complains > that the system is docked even though the laptop is not even dockable (see > [1]). > > This is due to a bug in hp-wmi where the driver is failing to check for errors > before creating the input switches. > > [1]: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1574120 > > Carlo Caione (2): > hp-wmi: Fix error value for hp_wmi_tablet_state > hp-wmi: Fix detection for dock and tablet mode Gentle ping. -- Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH v3] HID: asus: support backlight on USB keyboards
On Wed, Apr 12, 2017 at 10:12 PM, Jiri Kosina <ji...@kernel.org> wrote:
> On Wed, 12 Apr 2017, Carlo Caione wrote:
>
>> >> + ret = devm_led_classdev_register(>dev,
>> >> >kbd_backlight->cdev);
>> >> + if (ret < 0) {
>> >> + /* No need to have this still around */
>> >> + devm_kfree(>dev, drvdata->kbd_backlight);
>> >> + cancel_work_sync(>kbd_backlight->work);
>> >
>> > Small nitpick, you don't need to call cancel_work_sync() here, nobody
>> > could have called a worker. But OTOH, it doesn't hurt.
>> >
>> > Reviewed-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>
>>
>> Thanks Benjamin. Who should pick this up?
>
> I am going to.
>
> I'll drop the cancel_work_sync() call while doing it, ok?
Sure, go for it.
Thank you,
--
Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH v3] HID: asus: support backlight on USB keyboards
On Wed, Apr 12, 2017 at 10:12 PM, Jiri Kosina wrote:
> On Wed, 12 Apr 2017, Carlo Caione wrote:
>
>> >> + ret = devm_led_classdev_register(>dev,
>> >> >kbd_backlight->cdev);
>> >> + if (ret < 0) {
>> >> + /* No need to have this still around */
>> >> + devm_kfree(>dev, drvdata->kbd_backlight);
>> >> + cancel_work_sync(>kbd_backlight->work);
>> >
>> > Small nitpick, you don't need to call cancel_work_sync() here, nobody
>> > could have called a worker. But OTOH, it doesn't hurt.
>> >
>> > Reviewed-by: Benjamin Tissoires
>>
>> Thanks Benjamin. Who should pick this up?
>
> I am going to.
>
> I'll drop the cancel_work_sync() call while doing it, ok?
Sure, go for it.
Thank you,
--
Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH v3] HID: asus: support backlight on USB keyboards
On Mon, Apr 10, 2017 at 8:39 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> On Apr 06 2017 or thereabouts, Carlo Caione wrote:
>> + ret = devm_led_classdev_register(>dev,
>> >kbd_backlight->cdev);
>> + if (ret < 0) {
>> + /* No need to have this still around */
>> + devm_kfree(>dev, drvdata->kbd_backlight);
>> + cancel_work_sync(>kbd_backlight->work);
>
> Small nitpick, you don't need to call cancel_work_sync() here, nobody
> could have called a worker. But OTOH, it doesn't hurt.
>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>
Thanks Benjamin. Who should pick this up?
--
Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH v3] HID: asus: support backlight on USB keyboards
On Mon, Apr 10, 2017 at 8:39 AM, Benjamin Tissoires
wrote:
> On Apr 06 2017 or thereabouts, Carlo Caione wrote:
>> + ret = devm_led_classdev_register(>dev,
>> >kbd_backlight->cdev);
>> + if (ret < 0) {
>> + /* No need to have this still around */
>> + devm_kfree(>dev, drvdata->kbd_backlight);
>> + cancel_work_sync(>kbd_backlight->work);
>
> Small nitpick, you don't need to call cancel_work_sync() here, nobody
> could have called a worker. But OTOH, it doesn't hurt.
>
> Reviewed-by: Benjamin Tissoires
Thanks Benjamin. Who should pick this up?
--
Carlo Caione | +39.340.80.30.096 | Endless
[PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
From: Carlo Caione <ca...@endlessm.com>
The current driver code is not checking for the error values returned by
'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the
returned values down to 'input_report_switch()'. This error code is
being translated to '1' in the input subsystem, reporting the wrong
status.
The biggest problem caused by this issue is that several laptops are
wrongly reported by the driver as docked, preventing them to be put to
sleep using the LID (and in most cases they are not even dockable).
With this patch we create the report switches only if we are able to
read the dock and tablet mode status correctly from ACPI.
Signed-off-by: Carlo Caione <ca...@endlessm.com>
---
drivers/platform/x86/hp-wmi.c | 40 +++-
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 13ba65c..2b721fd 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context)
switch (event_id) {
case HPWMI_DOCK_EVENT:
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
break;
case HPWMI_PARK_HDD:
@@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void)
{
acpi_status status;
int err;
+ int val;
hp_wmi_input_dev = input_allocate_device();
if (!hp_wmi_input_dev)
@@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void)
hp_wmi_input_dev->id.bustype = BUS_HOST;
__set_bit(EV_SW, hp_wmi_input_dev->evbit);
- __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
- __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+
+ /* Dock */
+ val = hp_wmi_dock_state();
+ if (!(val < 0)) {
+ __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
+ }
+
+ /* Tablet mode */
+ val = hp_wmi_tablet_state();
+ if (!(val < 0)) {
+ __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
+ }
err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL);
if (err)
goto err_free_dev;
/* Set initial hardware state */
- input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later())
@@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device)
* changed.
*/
if (hp_wmi_input_dev) {
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
}
--
2.9.3
[PATCH 0/2] hp-wmi: Fix dock status and tablet mode reporting
From: Carlo Caione <ca...@endlessm.com> Several HP laptops cannot be put to sleep using the LID since systemd complains that the system is docked even though the laptop is not even dockable (see [1]). This is due to a bug in hp-wmi where the driver is failing to check for errors before creating the input switches. [1]: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1574120 Carlo Caione (2): hp-wmi: Fix error value for hp_wmi_tablet_state hp-wmi: Fix detection for dock and tablet mode drivers/platform/x86/hp-wmi.c | 42 -- 1 file changed, 28 insertions(+), 14 deletions(-) -- 2.9.3
[PATCH 2/2] hp-wmi: Fix detection for dock and tablet mode
From: Carlo Caione
The current driver code is not checking for the error values returned by
'hp_wmi_dock_state()' and 'hp_wmi_tablet_state()' before passing the
returned values down to 'input_report_switch()'. This error code is
being translated to '1' in the input subsystem, reporting the wrong
status.
The biggest problem caused by this issue is that several laptops are
wrongly reported by the driver as docked, preventing them to be put to
sleep using the LID (and in most cases they are not even dockable).
With this patch we create the report switches only if we are able to
read the dock and tablet mode status correctly from ACPI.
Signed-off-by: Carlo Caione
---
drivers/platform/x86/hp-wmi.c | 40 +++-
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 13ba65c..2b721fd 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -572,10 +572,12 @@ static void hp_wmi_notify(u32 value, void *context)
switch (event_id) {
case HPWMI_DOCK_EVENT:
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
break;
case HPWMI_PARK_HDD:
@@ -644,6 +646,7 @@ static int __init hp_wmi_input_setup(void)
{
acpi_status status;
int err;
+ int val;
hp_wmi_input_dev = input_allocate_device();
if (!hp_wmi_input_dev)
@@ -654,17 +657,26 @@ static int __init hp_wmi_input_setup(void)
hp_wmi_input_dev->id.bustype = BUS_HOST;
__set_bit(EV_SW, hp_wmi_input_dev->evbit);
- __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
- __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+
+ /* Dock */
+ val = hp_wmi_dock_state();
+ if (!(val < 0)) {
+ __set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
+ }
+
+ /* Tablet mode */
+ val = hp_wmi_tablet_state();
+ if (!(val < 0)) {
+ __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
+ }
err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL);
if (err)
goto err_free_dev;
/* Set initial hardware state */
- input_report_switch(hp_wmi_input_dev, SW_DOCK, hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
if (!hp_wmi_bios_2009_later() && hp_wmi_bios_2008_later())
@@ -950,10 +962,12 @@ static int hp_wmi_resume_handler(struct device *device)
* changed.
*/
if (hp_wmi_input_dev) {
- input_report_switch(hp_wmi_input_dev, SW_DOCK,
- hp_wmi_dock_state());
- input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
- hp_wmi_tablet_state());
+ if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_DOCK,
+ hp_wmi_dock_state());
+ if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
+ input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
+ hp_wmi_tablet_state());
input_sync(hp_wmi_input_dev);
}
--
2.9.3
[PATCH 0/2] hp-wmi: Fix dock status and tablet mode reporting
From: Carlo Caione Several HP laptops cannot be put to sleep using the LID since systemd complains that the system is docked even though the laptop is not even dockable (see [1]). This is due to a bug in hp-wmi where the driver is failing to check for errors before creating the input switches. [1]: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1574120 Carlo Caione (2): hp-wmi: Fix error value for hp_wmi_tablet_state hp-wmi: Fix detection for dock and tablet mode drivers/platform/x86/hp-wmi.c | 42 -- 1 file changed, 28 insertions(+), 14 deletions(-) -- 2.9.3
[PATCH 1/2] hp-wmi: Fix error value for hp_wmi_tablet_state
From: Carlo Caione <ca...@endlessm.com> hp_wmi_tablet_state() fails to return the correct error code when hp_wmi_perform_query() returns the HP WMI query specific error code that is a positive value. Signed-off-by: Carlo Caione <ca...@endlessm.com> --- drivers/platform/x86/hp-wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 96ffda4..13ba65c 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -290,7 +290,7 @@ static int hp_wmi_tablet_state(void) int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, , sizeof(state), sizeof(state)); if (ret) - return ret; + return -EINVAL; return (state & 0x4) ? 1 : 0; } -- 2.9.3
[PATCH 1/2] hp-wmi: Fix error value for hp_wmi_tablet_state
From: Carlo Caione hp_wmi_tablet_state() fails to return the correct error code when hp_wmi_perform_query() returns the HP WMI query specific error code that is a positive value. Signed-off-by: Carlo Caione --- drivers/platform/x86/hp-wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 96ffda4..13ba65c 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -290,7 +290,7 @@ static int hp_wmi_tablet_state(void) int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, , sizeof(state), sizeof(state)); if (ret) - return ret; + return -EINVAL; return (state & 0x4) ? 1 : 0; } -- 2.9.3
[PATCH v3] HID: asus: support backlight on USB keyboards
From: Carlo Caione <ca...@endlessm.com>
The latest USB keyboards shipped on several ASUS laptop models
(including ROG laptop models such as GL702VMK) have the keyboards
backlight controlled by the keyboard firmware.
The firmware implements at least 3 different commands:
- Init command (to use when the system starts)
- Configuration command (to get keyboard status/information)
- Backlight level control (to change the level of the keyboard light)
With this patch we create the usual 'asus::kbd_backlight' led class
entry to control the keyboard backlight.
Signed-off-by: Carlo Caione <ca...@endlessm.com>
---
Changelog:
v2:
- Code refactoring
- s/sysfs/led class
- No error messages on kzalloc/kmemdup failing
- Fixed race condition when removing device
- Used devm_* when possible
- Added LEDS_CLASS dependency
v3:
- s/asus_kbd_get_report/asus_kbd_set_report
- Added cancel_work_sync()
- Return error from asus_kbd_register_leds()
- Do not leak drvdata->kbd_backlight on devm_led_classdev_register()
failure
- Coding style
---
drivers/hid/Kconfig| 1 +
drivers/hid/hid-asus.c | 184 -
2 files changed, 184 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index eb1846a..dec5f3e 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -136,6 +136,7 @@ config HID_APPLEIR
config HID_ASUS
tristate "Asus"
+ depends on LEDS_CLASS
---help---
Support for Asus notebook built-in keyboard and touchpad via i2c, and
the Asus Republic of Gamers laptop keyboard special keys.
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bacba97..e78c7de 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -40,8 +40,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define FEATURE_KBD_REPORT_ID 0x5a
#define INPUT_REPORT_SIZE 28
+#define FEATURE_KBD_REPORT_SIZE 16
+
+#define SUPPORT_KBD_BACKLIGHT BIT(0)
#define MAX_CONTACTS 5
@@ -64,6 +68,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_SKIP_INPUT_MAPPING BIT(2)
#define QUIRK_IS_MULTITOUCHBIT(3)
#define QUIRK_NO_CONSUMER_USAGES BIT(4)
+#define QUIRK_USE_KBD_BACKLIGHTBIT(5)
#define I2C_KEYBOARD_QUIRKS(QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -74,9 +79,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
+struct asus_kbd_leds {
+ struct led_classdev cdev;
+ struct hid_device *hdev;
+ struct work_struct work;
+ unsigned int brightness;
+ bool removed;
+};
+
struct asus_drvdata {
unsigned long quirks;
struct input_dev *input;
+ struct asus_kbd_leds *kbd_backlight;
+ bool enable_backlight;
};
static void asus_report_contact_down(struct input_dev *input,
@@ -171,6 +186,149 @@ static int asus_raw_event(struct hid_device *hdev,
return 0;
}
+static int asus_kbd_set_report(struct hid_device *hdev, u8 *buf, size_t
buf_size)
+{
+ unsigned char *dmabuf;
+ int ret;
+
+ dmabuf = kmemdup(buf, buf_size, GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+buf_size, HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+ kfree(dmabuf);
+
+ return ret;
+}
+
+static int asus_kbd_init(struct hid_device *hdev)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
+0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ int ret;
+
+ ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+ if (ret < 0)
+ hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+
+ return ret;
+}
+
+static int asus_kbd_get_functions(struct hid_device *hdev,
+ unsigned char *kbd_func)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x20, 0x31, 0x00, 0x08 };
+ u8 *readbuf;
+ int ret;
+
+ ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to send configuration command:
%d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+HID_REQ_GET_REPORT);
+ if (ret < 0) {
+ hid_err(hdev, "Asus faile
[PATCH v3] HID: asus: support backlight on USB keyboards
From: Carlo Caione
The latest USB keyboards shipped on several ASUS laptop models
(including ROG laptop models such as GL702VMK) have the keyboards
backlight controlled by the keyboard firmware.
The firmware implements at least 3 different commands:
- Init command (to use when the system starts)
- Configuration command (to get keyboard status/information)
- Backlight level control (to change the level of the keyboard light)
With this patch we create the usual 'asus::kbd_backlight' led class
entry to control the keyboard backlight.
Signed-off-by: Carlo Caione
---
Changelog:
v2:
- Code refactoring
- s/sysfs/led class
- No error messages on kzalloc/kmemdup failing
- Fixed race condition when removing device
- Used devm_* when possible
- Added LEDS_CLASS dependency
v3:
- s/asus_kbd_get_report/asus_kbd_set_report
- Added cancel_work_sync()
- Return error from asus_kbd_register_leds()
- Do not leak drvdata->kbd_backlight on devm_led_classdev_register()
failure
- Coding style
---
drivers/hid/Kconfig| 1 +
drivers/hid/hid-asus.c | 184 -
2 files changed, 184 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index eb1846a..dec5f3e 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -136,6 +136,7 @@ config HID_APPLEIR
config HID_ASUS
tristate "Asus"
+ depends on LEDS_CLASS
---help---
Support for Asus notebook built-in keyboard and touchpad via i2c, and
the Asus Republic of Gamers laptop keyboard special keys.
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bacba97..e78c7de 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -40,8 +40,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define FEATURE_KBD_REPORT_ID 0x5a
#define INPUT_REPORT_SIZE 28
+#define FEATURE_KBD_REPORT_SIZE 16
+
+#define SUPPORT_KBD_BACKLIGHT BIT(0)
#define MAX_CONTACTS 5
@@ -64,6 +68,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_SKIP_INPUT_MAPPING BIT(2)
#define QUIRK_IS_MULTITOUCHBIT(3)
#define QUIRK_NO_CONSUMER_USAGES BIT(4)
+#define QUIRK_USE_KBD_BACKLIGHTBIT(5)
#define I2C_KEYBOARD_QUIRKS(QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -74,9 +79,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
+struct asus_kbd_leds {
+ struct led_classdev cdev;
+ struct hid_device *hdev;
+ struct work_struct work;
+ unsigned int brightness;
+ bool removed;
+};
+
struct asus_drvdata {
unsigned long quirks;
struct input_dev *input;
+ struct asus_kbd_leds *kbd_backlight;
+ bool enable_backlight;
};
static void asus_report_contact_down(struct input_dev *input,
@@ -171,6 +186,149 @@ static int asus_raw_event(struct hid_device *hdev,
return 0;
}
+static int asus_kbd_set_report(struct hid_device *hdev, u8 *buf, size_t
buf_size)
+{
+ unsigned char *dmabuf;
+ int ret;
+
+ dmabuf = kmemdup(buf, buf_size, GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+buf_size, HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+ kfree(dmabuf);
+
+ return ret;
+}
+
+static int asus_kbd_init(struct hid_device *hdev)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
+0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ int ret;
+
+ ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+ if (ret < 0)
+ hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+
+ return ret;
+}
+
+static int asus_kbd_get_functions(struct hid_device *hdev,
+ unsigned char *kbd_func)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x20, 0x31, 0x00, 0x08 };
+ u8 *readbuf;
+ int ret;
+
+ ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to send configuration command:
%d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+HID_REQ_GET_REPORT);
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to request functions: %d\n", ret);
+ kfree(readbuf);
Re: [PATCH v2] HID: asus: support backlight on USB keyboards
On Thu, Apr 6, 2017 at 11:11 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> Hi Carlo,
Hi Benjamin,
[cut]
>> +static int asus_kbd_get_report(struct hid_device *hdev, u8 *buf, size_t
>> buf_size)
>
> Nitpick: should be asus_kbd_set_report()
right :)
[cut]
>> +static void asus_kbd_backlight_work(struct work_struct *work)
>> +{
>> + struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds,
>> work);
>> + u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
>> + int ret;
>> +
>
> You should probably protect leds->removed by a mutex here to avoid
> having it set to false right after the test. (or not, see asus_remove())
Yeah, makes sense.
[cut]
>> + /* Initialize keyboard */
>> + if (asus_kbd_init(hdev) < 0)
>> + return -ENODEV;
>
> Don't hide the returned error code please. I know it'll be dropped in
> the end, but better not hiding it with an other one. (same for the next
> 2)
ok for the next one but when checking the bit field in kbd_func I
think we still want to return -ENODEV.
>> struct input_dev *input = hi->input;
>> @@ -178,7 +326,6 @@ static int asus_input_configured(struct hid_device
>> *hdev, struct hid_input *hi)
>>
>> if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
>> int ret;
>> -
>
> Leftover from the previous patch, but please don't remove this empty
> line, it's the coding style policy.
yeah, this slipped through
>> input_set_abs_params(input, ABS_MT_POSITION_X, 0, MAX_X, 0, 0);
>> input_set_abs_params(input, ABS_MT_POSITION_Y, 0, MAX_Y, 0, 0);
>> input_set_abs_params(input, ABS_TOOL_WIDTH, 0,
>> MAX_TOUCH_MAJOR, 0, 0);
>> @@ -198,6 +345,10 @@ static int asus_input_configured(struct hid_device
>> *hdev, struct hid_input *hi)
>>
>> drvdata->input = input;
>>
>> + if (drvdata->enable_backlight)
>> + if (asus_kbd_register_leds(hdev))
>
> could be changed into:
> if (drvdata->enable_backlight && asus_kbd_register_leds(hdev))
OK
[cut]
>> +static void asus_remove(struct hid_device *hdev)
>> +{
>> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> +
>> + if (drvdata->kbd_backlight)
>> + drvdata->kbd_backlight->removed = true;
>
> Add a cancel_work_sync() here too to terminate currently working
> workers. Bonus point, you don't need the mutex if you call
> cancel_work_sync().
I'll do.
Thanks for the quick review.
--
Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH v2] HID: asus: support backlight on USB keyboards
On Thu, Apr 6, 2017 at 11:11 AM, Benjamin Tissoires
wrote:
> Hi Carlo,
Hi Benjamin,
[cut]
>> +static int asus_kbd_get_report(struct hid_device *hdev, u8 *buf, size_t
>> buf_size)
>
> Nitpick: should be asus_kbd_set_report()
right :)
[cut]
>> +static void asus_kbd_backlight_work(struct work_struct *work)
>> +{
>> + struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds,
>> work);
>> + u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
>> + int ret;
>> +
>
> You should probably protect leds->removed by a mutex here to avoid
> having it set to false right after the test. (or not, see asus_remove())
Yeah, makes sense.
[cut]
>> + /* Initialize keyboard */
>> + if (asus_kbd_init(hdev) < 0)
>> + return -ENODEV;
>
> Don't hide the returned error code please. I know it'll be dropped in
> the end, but better not hiding it with an other one. (same for the next
> 2)
ok for the next one but when checking the bit field in kbd_func I
think we still want to return -ENODEV.
>> struct input_dev *input = hi->input;
>> @@ -178,7 +326,6 @@ static int asus_input_configured(struct hid_device
>> *hdev, struct hid_input *hi)
>>
>> if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
>> int ret;
>> -
>
> Leftover from the previous patch, but please don't remove this empty
> line, it's the coding style policy.
yeah, this slipped through
>> input_set_abs_params(input, ABS_MT_POSITION_X, 0, MAX_X, 0, 0);
>> input_set_abs_params(input, ABS_MT_POSITION_Y, 0, MAX_Y, 0, 0);
>> input_set_abs_params(input, ABS_TOOL_WIDTH, 0,
>> MAX_TOUCH_MAJOR, 0, 0);
>> @@ -198,6 +345,10 @@ static int asus_input_configured(struct hid_device
>> *hdev, struct hid_input *hi)
>>
>> drvdata->input = input;
>>
>> + if (drvdata->enable_backlight)
>> + if (asus_kbd_register_leds(hdev))
>
> could be changed into:
> if (drvdata->enable_backlight && asus_kbd_register_leds(hdev))
OK
[cut]
>> +static void asus_remove(struct hid_device *hdev)
>> +{
>> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> +
>> + if (drvdata->kbd_backlight)
>> + drvdata->kbd_backlight->removed = true;
>
> Add a cancel_work_sync() here too to terminate currently working
> workers. Bonus point, you don't need the mutex if you call
> cancel_work_sync().
I'll do.
Thanks for the quick review.
--
Carlo Caione | +39.340.80.30.096 | Endless
[PATCH v2] HID: asus: support backlight on USB keyboards
From: Carlo Caione <ca...@endlessm.com>
The latest USB keyboards shipped on several ASUS laptop models
(including ROG laptop models such as GL702VMK) have the keyboards
backlight controlled by the keyboard firmware.
The firmware implements at least 3 different commands:
- Init command (to use when the system starts)
- Configuration command (to get keyboard status/information)
- Backlight level control (to change the level of the keyboard light)
With this patch we create the usual 'asus::kbd_backlight' led class
entry to control the keyboard backlight.
Signed-off-by: Carlo Caione <ca...@endlessm.com>
---
Changelog:
v2:
- Code refactoring
- s/sysfs/led class
- No error messages on kzalloc/kmemdup failing
- Fixed race condition when removing device
- Used devm_* when possible
- Added LEDS_CLASS dependency
---
drivers/hid/Kconfig| 1 +
drivers/hid/hid-asus.c | 174 -
2 files changed, 173 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index eb1846a..dec5f3e 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -136,6 +136,7 @@ config HID_APPLEIR
config HID_ASUS
tristate "Asus"
+ depends on LEDS_CLASS
---help---
Support for Asus notebook built-in keyboard and touchpad via i2c, and
the Asus Republic of Gamers laptop keyboard special keys.
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bacba97..845e68a 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -40,8 +40,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define FEATURE_KBD_REPORT_ID 0x5a
#define INPUT_REPORT_SIZE 28
+#define FEATURE_KBD_REPORT_SIZE 16
+
+#define SUPPORT_KBD_BACKLIGHT BIT(0)
#define MAX_CONTACTS 5
@@ -64,6 +68,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_SKIP_INPUT_MAPPING BIT(2)
#define QUIRK_IS_MULTITOUCHBIT(3)
#define QUIRK_NO_CONSUMER_USAGES BIT(4)
+#define QUIRK_USE_KBD_BACKLIGHTBIT(5)
#define I2C_KEYBOARD_QUIRKS(QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -74,9 +79,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
+struct asus_kbd_leds {
+ struct led_classdev cdev;
+ struct hid_device *hdev;
+ struct work_struct work;
+ unsigned int brightness;
+ bool removed;
+};
+
struct asus_drvdata {
unsigned long quirks;
struct input_dev *input;
+ struct asus_kbd_leds *kbd_backlight;
+ bool enable_backlight;
};
static void asus_report_contact_down(struct input_dev *input,
@@ -171,6 +186,139 @@ static int asus_raw_event(struct hid_device *hdev,
return 0;
}
+static int asus_kbd_get_report(struct hid_device *hdev, u8 *buf, size_t
buf_size)
+{
+ unsigned char *dmabuf;
+ int ret;
+
+ dmabuf = kmemdup(buf, buf_size, GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+buf_size, HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+ kfree(dmabuf);
+
+ return ret;
+}
+
+static int asus_kbd_init(struct hid_device *hdev)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
+0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ int ret;
+
+ ret = asus_kbd_get_report(hdev, buf, sizeof(buf));
+ if (ret < 0)
+ hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+
+ return ret;
+}
+
+static int asus_kbd_get_functions(struct hid_device *hdev,
+ unsigned char *kbd_func)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x20, 0x31, 0x00, 0x08 };
+ u8 *readbuf;
+ int ret;
+
+ ret = asus_kbd_get_report(hdev, buf, sizeof(buf));
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to send configuration command:
%d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+HID_REQ_GET_REPORT);
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to request functions: %d\n", ret);
+ kfree(readbuf);
+ return ret;
+ }
+
+ *kbd_func = readbuf[6];
+
+ kfree(readbuf);
+ return ret;
+}
+
+static void asus_kbd_backlight_set
[PATCH v2] HID: asus: support backlight on USB keyboards
From: Carlo Caione
The latest USB keyboards shipped on several ASUS laptop models
(including ROG laptop models such as GL702VMK) have the keyboards
backlight controlled by the keyboard firmware.
The firmware implements at least 3 different commands:
- Init command (to use when the system starts)
- Configuration command (to get keyboard status/information)
- Backlight level control (to change the level of the keyboard light)
With this patch we create the usual 'asus::kbd_backlight' led class
entry to control the keyboard backlight.
Signed-off-by: Carlo Caione
---
Changelog:
v2:
- Code refactoring
- s/sysfs/led class
- No error messages on kzalloc/kmemdup failing
- Fixed race condition when removing device
- Used devm_* when possible
- Added LEDS_CLASS dependency
---
drivers/hid/Kconfig| 1 +
drivers/hid/hid-asus.c | 174 -
2 files changed, 173 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index eb1846a..dec5f3e 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -136,6 +136,7 @@ config HID_APPLEIR
config HID_ASUS
tristate "Asus"
+ depends on LEDS_CLASS
---help---
Support for Asus notebook built-in keyboard and touchpad via i2c, and
the Asus Republic of Gamers laptop keyboard special keys.
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bacba97..845e68a 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -40,8 +40,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define FEATURE_KBD_REPORT_ID 0x5a
#define INPUT_REPORT_SIZE 28
+#define FEATURE_KBD_REPORT_SIZE 16
+
+#define SUPPORT_KBD_BACKLIGHT BIT(0)
#define MAX_CONTACTS 5
@@ -64,6 +68,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_SKIP_INPUT_MAPPING BIT(2)
#define QUIRK_IS_MULTITOUCHBIT(3)
#define QUIRK_NO_CONSUMER_USAGES BIT(4)
+#define QUIRK_USE_KBD_BACKLIGHTBIT(5)
#define I2C_KEYBOARD_QUIRKS(QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -74,9 +79,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
+struct asus_kbd_leds {
+ struct led_classdev cdev;
+ struct hid_device *hdev;
+ struct work_struct work;
+ unsigned int brightness;
+ bool removed;
+};
+
struct asus_drvdata {
unsigned long quirks;
struct input_dev *input;
+ struct asus_kbd_leds *kbd_backlight;
+ bool enable_backlight;
};
static void asus_report_contact_down(struct input_dev *input,
@@ -171,6 +186,139 @@ static int asus_raw_event(struct hid_device *hdev,
return 0;
}
+static int asus_kbd_get_report(struct hid_device *hdev, u8 *buf, size_t
buf_size)
+{
+ unsigned char *dmabuf;
+ int ret;
+
+ dmabuf = kmemdup(buf, buf_size, GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+buf_size, HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+ kfree(dmabuf);
+
+ return ret;
+}
+
+static int asus_kbd_init(struct hid_device *hdev)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
+0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ int ret;
+
+ ret = asus_kbd_get_report(hdev, buf, sizeof(buf));
+ if (ret < 0)
+ hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+
+ return ret;
+}
+
+static int asus_kbd_get_functions(struct hid_device *hdev,
+ unsigned char *kbd_func)
+{
+ u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x20, 0x31, 0x00, 0x08 };
+ u8 *readbuf;
+ int ret;
+
+ ret = asus_kbd_get_report(hdev, buf, sizeof(buf));
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to send configuration command:
%d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+HID_REQ_GET_REPORT);
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to request functions: %d\n", ret);
+ kfree(readbuf);
+ return ret;
+ }
+
+ *kbd_func = readbuf[6];
+
+ kfree(readbuf);
+ return ret;
+}
+
+static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
+
Re: [PATCH] HID: asus: support backlight on USB keyboards
On Wed, Apr 5, 2017 at 11:41 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> Hi Carlo,
[cut]
>> With this patch we create the usual 'asus::kbd_backlight' sysfs entry
>
> Please change 'sysfs' with 'led class' or this will mislead people into
> understanding that you are adding a custom sysfs entry.
Agree
[cut]
>> +static int asus_init_kbd(struct hid_device *hdev)
>> +{
>> + int ret;
>> + const unsigned char buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55,
>> + 0x53, 0x20, 0x54, 0x65, 0x63, 0x68, 0x2e,
>> + 0x49, 0x6e, 0x63, 0x2e, 0x00 };
>
> Do you have any hints in what this magical blob is?
Not for the init and the level control commands. I have some hints
about the configuration command.
>> + unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
>> +
>> + if (!dmabuf) {
>> + ret = -ENOMEM;
>> + hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
>
> No need to have an error if kzalloc fails. There will already one to be
> shout by kzalloc. Please remove all of those errors after
> kzalloc/kmemdup.
>> + return ret;
>
> just return -ENOMEM.
Ok
[cut]
>> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
>
> That's a lot of kzalloc/kmemdup/kfree. I wonder if you couldn't just
> prepare some buffers in asus_register_kbd_leds() and just reuse them.
I'll try this.
[cut]
>> +static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>> +enum led_brightness brightness)
>> +{
>> + struct asus_kbd_leds *led = container_of(led_cdev, struct
>> asus_kbd_leds,
>> + cdev);
>> + led->brightness = brightness;
>> + schedule_work(>work);
>
> If a worker is already happening, aren't we losing the last brightness
> setting?
Uhm, I don't see how. The brightness setting is used in
asus_kbd_backlight_work() and this is scheduled only here.
Am I missing anything?
> When unplugging the device, you should also make sure nobody queued an
> event and that you are not allowing anybody to schedule a new work (by
> unregistering the led class interface first or adding a guard.
Good point.
[cut]
>> + drvdata->kbd_backlight = kzalloc(sizeof(struct asus_kbd_leds),
>> + GFP_KERNEL);
>
> Unless I am mistaken, I do not see the matching kfree.
> Also note that the rest of the driver uses devres (devm_* functions) so
> for data that's allocated and which needs to stay during the entire life
> of the device, please use the devm API.
Yeah, I'll do. Thanks for noticing this.
[cut]
>> + ret = led_classdev_register(>dev, >kbd_backlight->cdev);
>
> I do not see the matching led_classdev_unregister() call too. Note that
> there is also a devm_led_classdev_register() which might help you.
Ouch.
[cut]
>> + if (drvdata->enable_backlight) {
>> + if (asus_init_kbd(hdev))
>> + return 0;
>
> Returning success here (and in the other statements below) seems wrong.
The rationale here is that if anything goes wrong with backlight
initialization it's just ok to continue, not big deal.
We have already printed the error messages so the user is already
aware, but failing here because the keyboard light is broken seems
unnecessary.
>> +
>> + ret = asus_get_kbd_functions(hdev, _func);
>> + if (ret)
>> + return 0;
>
> I do not fully understand why you need to poll the keyboard for the
> functionality if you have a quirk for it. If it's mandatory to have
> functional backlight that's OK, but otherwise it does seem like waiting
> time.
The problem here is that not all the ASUS keyboard with that PID:VID
have the leds. So for such keyboard we would create a useless (and
probably confusing) sysfs entry for a non-existent backlight.
Otherwise we could do the opposite if you agree: delete the QUIRK and
just using this test to decide whether to create the led class or not.
>> +
>> + if (kbd_func & SUPPORT_BKD_BACKLIGHT)
>> + asus_register_kbd_leds(hdev);
>
> Don't you need to check for the return value here?
As written before, I guess if we fail to register the leds it's just
ok to continue (given that we already printed the error message).
Cheers,
--
Carlo Caione | +39.340.80.30.096 | Endless
Re: [PATCH] HID: asus: support backlight on USB keyboards
On Wed, Apr 5, 2017 at 11:41 AM, Benjamin Tissoires
wrote:
> Hi Carlo,
[cut]
>> With this patch we create the usual 'asus::kbd_backlight' sysfs entry
>
> Please change 'sysfs' with 'led class' or this will mislead people into
> understanding that you are adding a custom sysfs entry.
Agree
[cut]
>> +static int asus_init_kbd(struct hid_device *hdev)
>> +{
>> + int ret;
>> + const unsigned char buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55,
>> + 0x53, 0x20, 0x54, 0x65, 0x63, 0x68, 0x2e,
>> + 0x49, 0x6e, 0x63, 0x2e, 0x00 };
>
> Do you have any hints in what this magical blob is?
Not for the init and the level control commands. I have some hints
about the configuration command.
>> + unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
>> +
>> + if (!dmabuf) {
>> + ret = -ENOMEM;
>> + hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
>
> No need to have an error if kzalloc fails. There will already one to be
> shout by kzalloc. Please remove all of those errors after
> kzalloc/kmemdup.
>> + return ret;
>
> just return -ENOMEM.
Ok
[cut]
>> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
>
> That's a lot of kzalloc/kmemdup/kfree. I wonder if you couldn't just
> prepare some buffers in asus_register_kbd_leds() and just reuse them.
I'll try this.
[cut]
>> +static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>> +enum led_brightness brightness)
>> +{
>> + struct asus_kbd_leds *led = container_of(led_cdev, struct
>> asus_kbd_leds,
>> + cdev);
>> + led->brightness = brightness;
>> + schedule_work(>work);
>
> If a worker is already happening, aren't we losing the last brightness
> setting?
Uhm, I don't see how. The brightness setting is used in
asus_kbd_backlight_work() and this is scheduled only here.
Am I missing anything?
> When unplugging the device, you should also make sure nobody queued an
> event and that you are not allowing anybody to schedule a new work (by
> unregistering the led class interface first or adding a guard.
Good point.
[cut]
>> + drvdata->kbd_backlight = kzalloc(sizeof(struct asus_kbd_leds),
>> + GFP_KERNEL);
>
> Unless I am mistaken, I do not see the matching kfree.
> Also note that the rest of the driver uses devres (devm_* functions) so
> for data that's allocated and which needs to stay during the entire life
> of the device, please use the devm API.
Yeah, I'll do. Thanks for noticing this.
[cut]
>> + ret = led_classdev_register(>dev, >kbd_backlight->cdev);
>
> I do not see the matching led_classdev_unregister() call too. Note that
> there is also a devm_led_classdev_register() which might help you.
Ouch.
[cut]
>> + if (drvdata->enable_backlight) {
>> + if (asus_init_kbd(hdev))
>> + return 0;
>
> Returning success here (and in the other statements below) seems wrong.
The rationale here is that if anything goes wrong with backlight
initialization it's just ok to continue, not big deal.
We have already printed the error messages so the user is already
aware, but failing here because the keyboard light is broken seems
unnecessary.
>> +
>> + ret = asus_get_kbd_functions(hdev, _func);
>> + if (ret)
>> + return 0;
>
> I do not fully understand why you need to poll the keyboard for the
> functionality if you have a quirk for it. If it's mandatory to have
> functional backlight that's OK, but otherwise it does seem like waiting
> time.
The problem here is that not all the ASUS keyboard with that PID:VID
have the leds. So for such keyboard we would create a useless (and
probably confusing) sysfs entry for a non-existent backlight.
Otherwise we could do the opposite if you agree: delete the QUIRK and
just using this test to decide whether to create the led class or not.
>> +
>> + if (kbd_func & SUPPORT_BKD_BACKLIGHT)
>> + asus_register_kbd_leds(hdev);
>
> Don't you need to check for the return value here?
As written before, I guess if we fail to register the leds it's just
ok to continue (given that we already printed the error message).
Cheers,
--
Carlo Caione | +39.340.80.30.096 | Endless
[PATCH] HID: asus: support backlight on USB keyboards
From: Carlo Caione <ca...@endlessm.com>
The latest USB keyboards shipped on several ASUS laptop models
(including ROG laptop models such as GL702VMK) have the keyboards
backlight controlled by the keyboard firmware.
The firmware implements at least 3 different commands:
- Init command (to use when the system starts)
- Configuration command (to get keyboard status/information)
- Backlight level control (to change the level of the keyboard light)
With this patch we create the usual 'asus::kbd_backlight' sysfs entry
to control the keyboard backlight.
Signed-off-by: Carlo Caione <ca...@endlessm.com>
---
drivers/hid/hid-asus.c | 194 -
1 file changed, 191 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bacba97..e40ff72 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -40,8 +40,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define FEATURE_KBD_REPORT_ID 0x5a
#define INPUT_REPORT_SIZE 28
+#define FEATURE_KBD_REPORT_SIZE 16
+
+#define SUPPORT_BKD_BACKLIGHT BIT(0)
#define MAX_CONTACTS 5
@@ -64,6 +68,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_SKIP_INPUT_MAPPING BIT(2)
#define QUIRK_IS_MULTITOUCHBIT(3)
#define QUIRK_NO_CONSUMER_USAGES BIT(4)
+#define QUIRK_USE_KBD_BACKLIGHTBIT(5)
#define I2C_KEYBOARD_QUIRKS(QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -74,9 +79,18 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
+struct asus_kbd_leds {
+ struct led_classdev cdev;
+ struct hid_device *hdev;
+ struct work_struct work;
+ unsigned int brightness;
+};
+
struct asus_drvdata {
unsigned long quirks;
struct input_dev *input;
+ struct asus_kbd_leds *kbd_backlight;
+ bool enable_backlight;
};
static void asus_report_contact_down(struct input_dev *input,
@@ -171,14 +185,166 @@ static int asus_raw_event(struct hid_device *hdev,
return 0;
}
+static int asus_init_kbd(struct hid_device *hdev)
+{
+ int ret;
+ const unsigned char buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55,
+ 0x53, 0x20, 0x54, 0x65, 0x63, 0x68, 0x2e,
+ 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
+
+ if (!dmabuf) {
+ ret = -ENOMEM;
+ hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+sizeof(buf), HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+
+ kfree(dmabuf);
+
+ if (ret != sizeof(buf)) {
+ hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int asus_get_kbd_functions(struct hid_device *hdev,
+ unsigned char *kbd_func)
+{
+ int ret;
+ const unsigned char buf[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x20, 0x31,
+ 0x00, 0x08 };
+ unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
+ unsigned char *readbuf;
+
+ if (!dmabuf) {
+ ret = -ENOMEM;
+ hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+sizeof(buf), HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+
+ kfree(dmabuf);
+
+ if (ret != sizeof(buf)) {
+ hid_err(hdev, "Asus failed to send configuration command:
%d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf) {
+ ret = -ENOMEM;
+ hid_err(hdev, "Asus failed to alloc readbuf: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+HID_REQ_GET_REPORT);
+ if (ret != FEATURE_KBD_REPORT_SIZE) {
+ hid_err(hdev, "Asus failed to request functions: %d\n", ret);
+ kfree(readbuf);
+ return ret;
+ }
+
+ *kbd_func = readbuf[6];
+
+ kfree(readbuf);
+ return 0;
+}
+
+static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
+
[PATCH] HID: asus: support backlight on USB keyboards
From: Carlo Caione
The latest USB keyboards shipped on several ASUS laptop models
(including ROG laptop models such as GL702VMK) have the keyboards
backlight controlled by the keyboard firmware.
The firmware implements at least 3 different commands:
- Init command (to use when the system starts)
- Configuration command (to get keyboard status/information)
- Backlight level control (to change the level of the keyboard light)
With this patch we create the usual 'asus::kbd_backlight' sysfs entry
to control the keyboard backlight.
Signed-off-by: Carlo Caione
---
drivers/hid/hid-asus.c | 194 -
1 file changed, 191 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index bacba97..e40ff72 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -40,8 +40,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define FEATURE_KBD_REPORT_ID 0x5a
#define INPUT_REPORT_SIZE 28
+#define FEATURE_KBD_REPORT_SIZE 16
+
+#define SUPPORT_BKD_BACKLIGHT BIT(0)
#define MAX_CONTACTS 5
@@ -64,6 +68,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_SKIP_INPUT_MAPPING BIT(2)
#define QUIRK_IS_MULTITOUCHBIT(3)
#define QUIRK_NO_CONSUMER_USAGES BIT(4)
+#define QUIRK_USE_KBD_BACKLIGHTBIT(5)
#define I2C_KEYBOARD_QUIRKS(QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -74,9 +79,18 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define TRKID_SGN ((TRKID_MAX + 1) >> 1)
+struct asus_kbd_leds {
+ struct led_classdev cdev;
+ struct hid_device *hdev;
+ struct work_struct work;
+ unsigned int brightness;
+};
+
struct asus_drvdata {
unsigned long quirks;
struct input_dev *input;
+ struct asus_kbd_leds *kbd_backlight;
+ bool enable_backlight;
};
static void asus_report_contact_down(struct input_dev *input,
@@ -171,14 +185,166 @@ static int asus_raw_event(struct hid_device *hdev,
return 0;
}
+static int asus_init_kbd(struct hid_device *hdev)
+{
+ int ret;
+ const unsigned char buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55,
+ 0x53, 0x20, 0x54, 0x65, 0x63, 0x68, 0x2e,
+ 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
+
+ if (!dmabuf) {
+ ret = -ENOMEM;
+ hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+sizeof(buf), HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+
+ kfree(dmabuf);
+
+ if (ret != sizeof(buf)) {
+ hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int asus_get_kbd_functions(struct hid_device *hdev,
+ unsigned char *kbd_func)
+{
+ int ret;
+ const unsigned char buf[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x20, 0x31,
+ 0x00, 0x08 };
+ unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
+ unsigned char *readbuf;
+
+ if (!dmabuf) {
+ ret = -ENOMEM;
+ hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
+sizeof(buf), HID_FEATURE_REPORT,
+HID_REQ_SET_REPORT);
+
+ kfree(dmabuf);
+
+ if (ret != sizeof(buf)) {
+ hid_err(hdev, "Asus failed to send configuration command:
%d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf) {
+ ret = -ENOMEM;
+ hid_err(hdev, "Asus failed to alloc readbuf: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+HID_REQ_GET_REPORT);
+ if (ret != FEATURE_KBD_REPORT_SIZE) {
+ hid_err(hdev, "Asus failed to request functions: %d\n", ret);
+ kfree(readbuf);
+ return ret;
+ }
+
+ *kbd_func = readbuf[6];
+
+ kfree(readbuf);
+ return 0;
+}
+
+static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
+ enum led_brightnes
Re: [PATCH 2/3] ARM64: dts: amlogic: Add basic support for Amlogic S905X
On Mon, Sep 12, 2016 at 11:43 PM, Andreas Färber <afaer...@suse.de> wrote: [cut] > I'm not arguing over the file name, where it perfectly makes sense to > have a meson-gxl- prefix (already discussed), just about the compatible > string where we don't have "amlogic,meson-gxl-s905x-p231" either because > it is completely unnecessary and does _not_ add any value. > > Not that we're checking this string anywhere anyway... If you want to > check for the GXL family you have to use "amlogic,meson-gxl"; if you > want to check for the specific SoC you use "amlogic,s905x". Simple. We > never match partial strings, so there is no sense in a hardcoded prefix > that is duplicating information already available. Ok, then. Fine with me. Neil, do you want to resend my patch or you can take care of the fixes for the whole patchset? Thanks, -- Carlo Caione
Re: [PATCH 2/3] ARM64: dts: amlogic: Add basic support for Amlogic S905X
On Mon, Sep 12, 2016 at 11:43 PM, Andreas Färber wrote: [cut] > I'm not arguing over the file name, where it perfectly makes sense to > have a meson-gxl- prefix (already discussed), just about the compatible > string where we don't have "amlogic,meson-gxl-s905x-p231" either because > it is completely unnecessary and does _not_ add any value. > > Not that we're checking this string anywhere anyway... If you want to > check for the GXL family you have to use "amlogic,meson-gxl"; if you > want to check for the specific SoC you use "amlogic,s905x". Simple. We > never match partial strings, so there is no sense in a hardcoded prefix > that is duplicating information already available. Ok, then. Fine with me. Neil, do you want to resend my patch or you can take care of the fixes for the whole patchset? Thanks, -- Carlo Caione
Re: [PATCH 2/3] ARM64: dts: amlogic: Add basic support for Amlogic S905X
On Mon, Sep 12, 2016 at 6:28 PM, Andreas Färber <afaer...@suse.de> wrote:
>> +Boards with the Amlogic Meson GXL SoC shall have the following properties:
>> + Required root node property:
>> +compatible: "amlogic,meson-gxl-s905x", "amlogic,meson-gxl";
>
> Can we please use "amlogic,s905x", "amlogic,meson-gxl"? No need to
> complicate the name. Also affects .dtsi and .dts below.
gxl != s905x.
AFAWK to the GXL family belong several different SoCs, like S905X,
S905D, etc... (see patch 3/3)
This is why we use meson-gxl-s905x, meson-gxl-s905d, etc...
We could s/meson-gxl-s905x/meson-s905x/ and
s/meson-gxl-s905d/meson-s905d/ but I honestly prefer this way because
we can clearly see which family the SoC belongs to (the Amlogic naming
convention is already messy enough).
I mean, yes it's longer, but it's for the sake of documentation IMO.
[cut]
>> + compatible = "amlogic,p212", "amlogic,meson-gxl-s905x",
>> "amlogic,meson-gxl";
>> + model = "Amlogic Meson GXL (S905X) P212 Development Board";
>
> Is that its official name? No objection, just wondering whether we need
> both GXL and S905X in the name, and then again the SoC name at all if
> the P212 is unique. Haven't compared the previous Pxxx ones.
P212 is the official name for the S905X Amlogic dev board. It follows
the name we used for the other P20x boards.
[cut]
>> +
>> +/* This UART is brought out to the DB9 connector */
>> +_AO {
>> + status = "okay";
>> +};
>> +
>
> Trailing white line - please watch out for that, git-am will complain.
Right.
[cut]
>> +#include "meson-gxl.dtsi"
>> +
>> +/ {
>> + compatible = "amlogic,meson-gxl", "amlogic,meson-gxl-s905x";
>
> This needs to be reversed.
Agree.
Cheers,
--
Carlo Caione
Re: [PATCH 2/3] ARM64: dts: amlogic: Add basic support for Amlogic S905X
On Mon, Sep 12, 2016 at 6:28 PM, Andreas Färber wrote:
>> +Boards with the Amlogic Meson GXL SoC shall have the following properties:
>> + Required root node property:
>> +compatible: "amlogic,meson-gxl-s905x", "amlogic,meson-gxl";
>
> Can we please use "amlogic,s905x", "amlogic,meson-gxl"? No need to
> complicate the name. Also affects .dtsi and .dts below.
gxl != s905x.
AFAWK to the GXL family belong several different SoCs, like S905X,
S905D, etc... (see patch 3/3)
This is why we use meson-gxl-s905x, meson-gxl-s905d, etc...
We could s/meson-gxl-s905x/meson-s905x/ and
s/meson-gxl-s905d/meson-s905d/ but I honestly prefer this way because
we can clearly see which family the SoC belongs to (the Amlogic naming
convention is already messy enough).
I mean, yes it's longer, but it's for the sake of documentation IMO.
[cut]
>> + compatible = "amlogic,p212", "amlogic,meson-gxl-s905x",
>> "amlogic,meson-gxl";
>> + model = "Amlogic Meson GXL (S905X) P212 Development Board";
>
> Is that its official name? No objection, just wondering whether we need
> both GXL and S905X in the name, and then again the SoC name at all if
> the P212 is unique. Haven't compared the previous Pxxx ones.
P212 is the official name for the S905X Amlogic dev board. It follows
the name we used for the other P20x boards.
[cut]
>> +
>> +/* This UART is brought out to the DB9 connector */
>> +_AO {
>> + status = "okay";
>> +};
>> +
>
> Trailing white line - please watch out for that, git-am will complain.
Right.
[cut]
>> +#include "meson-gxl.dtsi"
>> +
>> +/ {
>> + compatible = "amlogic,meson-gxl", "amlogic,meson-gxl-s905x";
>
> This needs to be reversed.
Agree.
Cheers,
--
Carlo Caione
Re: [RFC PATCH 0/3] ARm64: amlogic: Introduce common GX family dtsi
On 29/08/16 20:38, Andreas Färber wrote:
> Am 29.08.2016 um 10:01 schrieb Carlo Caione:
> > On Mon, Aug 29, 2016 at 9:56 AM, Neil Armstrong <narmstr...@baylibre.com>
> > wrote:
> >> The new Amlogic GLX SoCs (S905X and S905D) are part of the Meson GX family
> >> so
> >> they share some basic characteritics that can be described in a common GX
> >> dtsi file used by the Meson GXBB and Meson GXL dtsi.
> >>
> >> This patchset introduces the common dtsi and switches the GLX and GXBB to
> >> use
> >> the common dtsi, the GXBB dtsi is reformated to handle this situation.
> >>
> >> This patchset depends on Carlo Caione "ARM64: dts: amlogic: Add basic
> >> support for Amlogic S905X" [1]
> >>
> >> [1]
> >> http://lkml.kernel.org/r/1472382113-10754-1-git-send-email-ca...@caione.org
> >>
> >> Neil Armstrong (3):
> >> ARM64: dts: amlogic: Add Meson GX Family common dtsi
> >> ARM64: dts: amlogic: Switch Meson GXL dtsi to use common GX dtsi
> >> ARM64: dts: amlogic: Switch Meson GXBB dtsi to use common GX dtsi
> >
> > FWIW
> > Acked-by: Carlo Caione <ca...@endlessm.com>
>
> Adding an unused .dtsi duplicating GXBB makes me uneasy.
S905x (GXL) is different from S905 (GXBB), it's unused now but in the
future we can expect something different in the two DTSI.
> Any chance we can simplify this to at most two steps?
> 1) Move code from gxbb to gx (1/3 + 3/3)
> 2) Add gxl using gx ("Add basic support for Amlogic S905X" + 2/3)
fine by me.
> Alternatively:
> 1) "Add basic support for Amlogic S905X"
> 2) Factor out common bits (1/3 + 2/3 + 3/3)
how is this different from this patchset?
> As for bike-shedding, is there a GX family as well or could we drop
> -common? .dtsi is always something common - compare Exynos or i.MX.
> Since there are meson8b and meson8 I was anticipating that after gxbb
> would come gx, not gxl.
AFAIK we have:
- GXBB
- GXL
- GXM
- GXTVBB
> Do you know what the L in GXL is for? Should we consider renaming gxbb
> to gxb, and then also insert -s905 as suggested by Kevin, for symmetry?
Yes, that make sense.
Cheers,
--
Carlo Caione
Re: [RFC PATCH 0/3] ARm64: amlogic: Introduce common GX family dtsi
On 29/08/16 20:38, Andreas Färber wrote:
> Am 29.08.2016 um 10:01 schrieb Carlo Caione:
> > On Mon, Aug 29, 2016 at 9:56 AM, Neil Armstrong
> > wrote:
> >> The new Amlogic GLX SoCs (S905X and S905D) are part of the Meson GX family
> >> so
> >> they share some basic characteritics that can be described in a common GX
> >> dtsi file used by the Meson GXBB and Meson GXL dtsi.
> >>
> >> This patchset introduces the common dtsi and switches the GLX and GXBB to
> >> use
> >> the common dtsi, the GXBB dtsi is reformated to handle this situation.
> >>
> >> This patchset depends on Carlo Caione "ARM64: dts: amlogic: Add basic
> >> support for Amlogic S905X" [1]
> >>
> >> [1]
> >> http://lkml.kernel.org/r/1472382113-10754-1-git-send-email-ca...@caione.org
> >>
> >> Neil Armstrong (3):
> >> ARM64: dts: amlogic: Add Meson GX Family common dtsi
> >> ARM64: dts: amlogic: Switch Meson GXL dtsi to use common GX dtsi
> >> ARM64: dts: amlogic: Switch Meson GXBB dtsi to use common GX dtsi
> >
> > FWIW
> > Acked-by: Carlo Caione
>
> Adding an unused .dtsi duplicating GXBB makes me uneasy.
S905x (GXL) is different from S905 (GXBB), it's unused now but in the
future we can expect something different in the two DTSI.
> Any chance we can simplify this to at most two steps?
> 1) Move code from gxbb to gx (1/3 + 3/3)
> 2) Add gxl using gx ("Add basic support for Amlogic S905X" + 2/3)
fine by me.
> Alternatively:
> 1) "Add basic support for Amlogic S905X"
> 2) Factor out common bits (1/3 + 2/3 + 3/3)
how is this different from this patchset?
> As for bike-shedding, is there a GX family as well or could we drop
> -common? .dtsi is always something common - compare Exynos or i.MX.
> Since there are meson8b and meson8 I was anticipating that after gxbb
> would come gx, not gxl.
AFAIK we have:
- GXBB
- GXL
- GXM
- GXTVBB
> Do you know what the L in GXL is for? Should we consider renaming gxbb
> to gxb, and then also insert -s905 as suggested by Kevin, for symmetry?
Yes, that make sense.
Cheers,
--
Carlo Caione
Re: [RFC PATCH 0/3] ARm64: amlogic: Introduce common GX family dtsi
On Mon, Aug 29, 2016 at 9:56 AM, Neil Armstrong <narmstr...@baylibre.com> wrote: > The new Amlogic GLX SoCs (S905X and S905D) are part of the Meson GX family so > they share some basic characteritics that can be described in a common GX > dtsi file used by the Meson GXBB and Meson GXL dtsi. > > This patchset introduces the common dtsi and switches the GLX and GXBB to use > the common dtsi, the GXBB dtsi is reformated to handle this situation. > > This patchset depends on Carlo Caione "ARM64: dts: amlogic: Add basic support > for Amlogic S905X" [1] > > [1] > http://lkml.kernel.org/r/1472382113-10754-1-git-send-email-ca...@caione.org > > Neil Armstrong (3): > ARM64: dts: amlogic: Add Meson GX Family common dtsi > ARM64: dts: amlogic: Switch Meson GXL dtsi to use common GX dtsi > ARM64: dts: amlogic: Switch Meson GXBB dtsi to use common GX dtsi FWIW Acked-by: Carlo Caione <ca...@endlessm.com> -- Carlo Caione
Re: [RFC PATCH 0/3] ARm64: amlogic: Introduce common GX family dtsi
On Mon, Aug 29, 2016 at 9:56 AM, Neil Armstrong wrote: > The new Amlogic GLX SoCs (S905X and S905D) are part of the Meson GX family so > they share some basic characteritics that can be described in a common GX > dtsi file used by the Meson GXBB and Meson GXL dtsi. > > This patchset introduces the common dtsi and switches the GLX and GXBB to use > the common dtsi, the GXBB dtsi is reformated to handle this situation. > > This patchset depends on Carlo Caione "ARM64: dts: amlogic: Add basic support > for Amlogic S905X" [1] > > [1] > http://lkml.kernel.org/r/1472382113-10754-1-git-send-email-ca...@caione.org > > Neil Armstrong (3): > ARM64: dts: amlogic: Add Meson GX Family common dtsi > ARM64: dts: amlogic: Switch Meson GXL dtsi to use common GX dtsi > ARM64: dts: amlogic: Switch Meson GXBB dtsi to use common GX dtsi FWIW Acked-by: Carlo Caione -- Carlo Caione
Re: [PATCH v3 1/5] ARM: dts: meson: minix-neo-x8: define PMIC as power controller
On 25/06/16 18:15, Stefan Agner wrote: > The PMIC driver used to register itself as poweroff controller by > default, hence assuming that this device is using the PMIC as > system power controller. > > Signed-off-by: Stefan Agner <ste...@agner.ch> > Reviewed-by: Marcel Ziswiler <marcel.ziswi...@toradex.com> Acked-by: Carlo Caione <ca...@endlessm.com> -- Carlo Caione
Re: [PATCH v3 1/5] ARM: dts: meson: minix-neo-x8: define PMIC as power controller
On 25/06/16 18:15, Stefan Agner wrote: > The PMIC driver used to register itself as poweroff controller by > default, hence assuming that this device is using the PMIC as > system power controller. > > Signed-off-by: Stefan Agner > Reviewed-by: Marcel Ziswiler Acked-by: Carlo Caione -- Carlo Caione
Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger
On 06/06/16 18:56, Marc Zyngier wrote: > The ARM architected timer specification mandates that the interrupt > associated with each timer is level triggered (which corresponds to > the "counter >= comparator" condition). > > A number of DTs are being remarkably creative, declaring the interrupt > to be edge triggered. A quick look at the TRM for the corresponding ARM > CPUs clearly shows that this is wrong, and I've corrected those. > For non-ARM designs (and in the absence of a publicly available TRM), > I've made them active low as well, which can't be completely wrong > as the GIC cannot disinguish between level low and level high. > > The respective maintainers are of course welcome to prove me wrong. > > While I was at it, I took the liberty to fix a couple of related issue, > such as some spurious affinity bits on ThunderX, and their complete > absence on ls1043a (both of which seem to be related to copy-pasting > from other DTs). > > Signed-off-by: Marc Zyngier <marc.zyng...@arm.com> For meson-gxbb.dtsi: Acked-by: Carlo Caione <ca...@endlessm.com> -- Carlo Caione
Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger
On 06/06/16 18:56, Marc Zyngier wrote: > The ARM architected timer specification mandates that the interrupt > associated with each timer is level triggered (which corresponds to > the "counter >= comparator" condition). > > A number of DTs are being remarkably creative, declaring the interrupt > to be edge triggered. A quick look at the TRM for the corresponding ARM > CPUs clearly shows that this is wrong, and I've corrected those. > For non-ARM designs (and in the absence of a publicly available TRM), > I've made them active low as well, which can't be completely wrong > as the GIC cannot disinguish between level low and level high. > > The respective maintainers are of course welcome to prove me wrong. > > While I was at it, I took the liberty to fix a couple of related issue, > such as some spurious affinity bits on ThunderX, and their complete > absence on ls1043a (both of which seem to be related to copy-pasting > from other DTs). > > Signed-off-by: Marc Zyngier For meson-gxbb.dtsi: Acked-by: Carlo Caione -- Carlo Caione
Re: [PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver
On 30/05/16 15:29, Neil Armstrong wrote: > Add watchdog specific driver for Amlogic Meson GXBB SoC. > > Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> > + > +#define GXBB_WDT_CTRL1_REG 0x4 ... > +#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26) ... > +#define GXBB_WDT_CTRL_IRQ_EN BIT(23) ... > +#define GXBB_WDT_CTRL_XTAL_SEL (0) ... > +#define GXBB_WDT_CTRL_CLK81_SEL BIT(19) ... > +#define GXBB_WDT_CTRL1_GPIO_PULSEBIT(17) ... > +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0 BIT(16) > +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1 (0) > +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT(BIT(16) - 1) nit: all these defines are not used at all in the driver. You can remove them Thanks! -- Carlo Caione
Re: [PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver
On 30/05/16 15:29, Neil Armstrong wrote: > Add watchdog specific driver for Amlogic Meson GXBB SoC. > > Signed-off-by: Neil Armstrong > + > +#define GXBB_WDT_CTRL1_REG 0x4 ... > +#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26) ... > +#define GXBB_WDT_CTRL_IRQ_EN BIT(23) ... > +#define GXBB_WDT_CTRL_XTAL_SEL (0) ... > +#define GXBB_WDT_CTRL_CLK81_SEL BIT(19) ... > +#define GXBB_WDT_CTRL1_GPIO_PULSEBIT(17) ... > +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0 BIT(16) > +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1 (0) > +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT(BIT(16) - 1) nit: all these defines are not used at all in the driver. You can remove them Thanks! -- Carlo Caione
Re: [PATCH 0/3] watchdog: Add Amlogic Meson GXBB Watchdog Timer driver
On 30/05/16 15:29, Neil Armstrong wrote: > Adds support for the Amlogic Meson GXBB SoC Watchdog Timer. > It differs from the meson6/meson8b HW, so need for a separate driver. > The HW provides a divider capable of having a 1ms timebase thus simplifying > the counter update. > The restart call is not provided even if the HW is capable of triggering a > system reset immediately because of the PSCI firmare having such > functionnality. > The watchdog is not expected to be running at boot time since there is a > separate > system-level watchdog running from the SCPI co-processor, but this case would > be > handle in a next driver update. > > Changes since RFC version at > http://lkml.kernel.org/r/1464249112-13658-1-git-send-email-narmstr...@baylibre.com > : > - Remove status callback, will re-introduce it later to managed the > already-running use case > - Fix registers defines indentation > - Fix space between operators > - Make all callbacks static functions > - Fix bindings with missing clocks attribute > - Do not stop/start watchdog on a timeout setup > - Fix probe device configuration > > Neil Armstrong (3): > watchdog: Add Meson GXBB Watchdog Driver > dt-bindings: watchdog: Add Meson GXBB Watchdog bindings > ARM64: dts: amlogic: meson-gxbb: Add watchdog node I'm missing [PATCH 2/3] -- Carlo Caione
Re: [PATCH 0/3] watchdog: Add Amlogic Meson GXBB Watchdog Timer driver
On 30/05/16 15:29, Neil Armstrong wrote: > Adds support for the Amlogic Meson GXBB SoC Watchdog Timer. > It differs from the meson6/meson8b HW, so need for a separate driver. > The HW provides a divider capable of having a 1ms timebase thus simplifying > the counter update. > The restart call is not provided even if the HW is capable of triggering a > system reset immediately because of the PSCI firmare having such > functionnality. > The watchdog is not expected to be running at boot time since there is a > separate > system-level watchdog running from the SCPI co-processor, but this case would > be > handle in a next driver update. > > Changes since RFC version at > http://lkml.kernel.org/r/1464249112-13658-1-git-send-email-narmstr...@baylibre.com > : > - Remove status callback, will re-introduce it later to managed the > already-running use case > - Fix registers defines indentation > - Fix space between operators > - Make all callbacks static functions > - Fix bindings with missing clocks attribute > - Do not stop/start watchdog on a timeout setup > - Fix probe device configuration > > Neil Armstrong (3): > watchdog: Add Meson GXBB Watchdog Driver > dt-bindings: watchdog: Add Meson GXBB Watchdog bindings > ARM64: dts: amlogic: meson-gxbb: Add watchdog node I'm missing [PATCH 2/3] -- Carlo Caione
Re: [RFC PATCH 2/3] dt-bindings: watchdog: Add Meson GXBB Watchdog bindings
On 26/05/16 09:51, Neil Armstrong wrote: > Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> > --- > .../devicetree/bindings/watchdog/meson-gxbb-wdt.txt | 13 > + > 1 file changed, 13 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > > diff --git a/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > b/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > new file mode 100644 > index 000..d06c0a0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > @@ -0,0 +1,13 @@ > +Meson GXBB SoCs Watchdog timer > + > +Required properties: > + > +- compatible : should be "amlogic,meson-gxbb-wdt" > +- reg : Specifies base physical address and size of the registers. 'clocks' is also required IIRC. Thanks, -- Carlo Caione
Re: [RFC PATCH 2/3] dt-bindings: watchdog: Add Meson GXBB Watchdog bindings
On 26/05/16 09:51, Neil Armstrong wrote: > Signed-off-by: Neil Armstrong > --- > .../devicetree/bindings/watchdog/meson-gxbb-wdt.txt | 13 > + > 1 file changed, 13 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > > diff --git a/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > b/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > new file mode 100644 > index 000..d06c0a0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/meson-gxbb-wdt.txt > @@ -0,0 +1,13 @@ > +Meson GXBB SoCs Watchdog timer > + > +Required properties: > + > +- compatible : should be "amlogic,meson-gxbb-wdt" > +- reg : Specifies base physical address and size of the registers. 'clocks' is also required IIRC. Thanks, -- Carlo Caione
Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver
On 26/05/16 09:51, Neil Armstrong wrote:
> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> +
[...]
> +#define DEFAULT_TIMEOUT 10 /* seconds */
> +
> +#define GXBB_WDT_CTRL_REG0x0
> +#define GXBB_WDT_CTRL1_REG 0x4
> +#define GXBB_WDT_TCNT_REG0x8
> +#define GXBB_WDT_RSET_REG0xc
> +
> +#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26)
> +
> +#define GXBB_WDT_CTRL_CLKDIV_EN BIT(25)
> +#define GXBB_WDT_CTRL_CLK_EN BIT(24)
> +#define GXBB_WDT_CTRL_IRQ_EN BIT(23)
> +#define GXBB_WDT_CTRL_EE_RESET BIT(21)
> +#define GXBB_WDT_CTRL_XTAL_SEL (0)
> +#define GXBB_WDT_CTRL_CLK81_SEL BIT(19)
> +#define GXBB_WDT_CTRL_EN BIT(18)
> +#define GXBB_WDT_CTRL_DIV_MASK (BIT(18)-1)
> +
> +#define GXBB_WDT_CTRL1_GPIO_PULSEBIT(17)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0 BIT(16)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1 (0)
> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT(BIT(16)-1)
> +
> +#define GXBB_WDT_TCNT_SETUP_MASK (BIT(16)-1)
> +#define GXBB_WDT_TCNT_CNT_SHIFT 16
Indentation
[...]
> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +unsigned int timeout)
> +{
> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> + if (watchdog_active(wdt_dev))
> + meson_gxbb_wdt_stop(wdt_dev);
> +
> + meson_gxbb_wdt_ping(wdt_dev);
> +
> + writel(timeout*1000, data->reg_base + GXBB_WDT_TCNT_REG);
nit: spaces around "*"
[...]
> + data->clk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(data->clk))
> + return PTR_ERR(data->clk);
> +
> + clk_prepare_enable(data->clk);
Do we need to merge the clock controller driver before this?
Cheers,
--
Carlo Caione
Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver
On 26/05/16 09:51, Neil Armstrong wrote:
> Add watchdog specific driver for Amlogic Meson GXBB SoC.
>
> Signed-off-by: Neil Armstrong
> +
[...]
> +#define DEFAULT_TIMEOUT 10 /* seconds */
> +
> +#define GXBB_WDT_CTRL_REG0x0
> +#define GXBB_WDT_CTRL1_REG 0x4
> +#define GXBB_WDT_TCNT_REG0x8
> +#define GXBB_WDT_RSET_REG0xc
> +
> +#define GXBB_WDT_CTRL_EE_RESET_NOW BIT(26)
> +
> +#define GXBB_WDT_CTRL_CLKDIV_EN BIT(25)
> +#define GXBB_WDT_CTRL_CLK_EN BIT(24)
> +#define GXBB_WDT_CTRL_IRQ_EN BIT(23)
> +#define GXBB_WDT_CTRL_EE_RESET BIT(21)
> +#define GXBB_WDT_CTRL_XTAL_SEL (0)
> +#define GXBB_WDT_CTRL_CLK81_SEL BIT(19)
> +#define GXBB_WDT_CTRL_EN BIT(18)
> +#define GXBB_WDT_CTRL_DIV_MASK (BIT(18)-1)
> +
> +#define GXBB_WDT_CTRL1_GPIO_PULSEBIT(17)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_0 BIT(16)
> +#define GXBB_WDT_CTRL1_GPIO_POL_RESET_1 (0)
> +#define GXBB_WDT_CTRL1_GPIO_PULSE_CNT(BIT(16)-1)
> +
> +#define GXBB_WDT_TCNT_SETUP_MASK (BIT(16)-1)
> +#define GXBB_WDT_TCNT_CNT_SHIFT 16
Indentation
[...]
> +int meson_gxbb_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +unsigned int timeout)
> +{
> + struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> +
> + if (watchdog_active(wdt_dev))
> + meson_gxbb_wdt_stop(wdt_dev);
> +
> + meson_gxbb_wdt_ping(wdt_dev);
> +
> + writel(timeout*1000, data->reg_base + GXBB_WDT_TCNT_REG);
nit: spaces around "*"
[...]
> + data->clk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(data->clk))
> + return PTR_ERR(data->clk);
> +
> + clk_prepare_enable(data->clk);
Do we need to merge the clock controller driver before this?
Cheers,
--
Carlo Caione
Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller
On 20/05/16 14:20, Neil Armstrong wrote: > On 05/20/2016 12:04 PM, Carlo Caione wrote: > > On 20/05/16 11:10, Neil Armstrong wrote: > >> On 05/20/2016 11:04 AM, Carlo Caione wrote: > >>> On 20/05/16 10:27, Neil Armstrong wrote: > >>>> This patch adds the platform driver for the Amlogic Meson GXBB Reset > >>>> Controller. > >>>> > >>>> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> > >>>> --- > >>>> drivers/reset/Kconfig| 6 ++ > >>>> drivers/reset/Makefile | 1 + > >>>> drivers/reset/reset-meson-gxbb.c | 129 > >>>> +++ > >>>> 3 files changed, 136 insertions(+) > >>>> create mode 100644 drivers/reset/reset-meson-gxbb.c > >>> > >>> Do we really need to be that specific (-gxbb)? This driver looks generic > >>> and simple enough to be used for several Amlogic families. You are > >>> already differentiating between them with the include file defining the > >>> reset indexes for the SoC. > >> > >> This is a good question, do the S805 have similar registers ? Same count > >> and width ? > >> I no, it should need a rework to add a data structure per-SoC. > > > > According to the datasheet on S805 we have 7 registers with 16 reset > > bits per register. > > It will fit, I made the change to be generic. Do you know if the meson8 and > previous meson6 has these registers ? No idea. No datasheet for those. Cheers, -- Carlo Caione
Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller
On 20/05/16 14:20, Neil Armstrong wrote: > On 05/20/2016 12:04 PM, Carlo Caione wrote: > > On 20/05/16 11:10, Neil Armstrong wrote: > >> On 05/20/2016 11:04 AM, Carlo Caione wrote: > >>> On 20/05/16 10:27, Neil Armstrong wrote: > >>>> This patch adds the platform driver for the Amlogic Meson GXBB Reset > >>>> Controller. > >>>> > >>>> Signed-off-by: Neil Armstrong > >>>> --- > >>>> drivers/reset/Kconfig| 6 ++ > >>>> drivers/reset/Makefile | 1 + > >>>> drivers/reset/reset-meson-gxbb.c | 129 > >>>> +++ > >>>> 3 files changed, 136 insertions(+) > >>>> create mode 100644 drivers/reset/reset-meson-gxbb.c > >>> > >>> Do we really need to be that specific (-gxbb)? This driver looks generic > >>> and simple enough to be used for several Amlogic families. You are > >>> already differentiating between them with the include file defining the > >>> reset indexes for the SoC. > >> > >> This is a good question, do the S805 have similar registers ? Same count > >> and width ? > >> I no, it should need a rework to add a data structure per-SoC. > > > > According to the datasheet on S805 we have 7 registers with 16 reset > > bits per register. > > It will fit, I made the change to be generic. Do you know if the meson8 and > previous meson6 has these registers ? No idea. No datasheet for those. Cheers, -- Carlo Caione
Re: [PATCH 3/3] ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms
On 20/05/16 14:19, Neil Armstrong wrote: [...] > >>> Missing #include ? > >>> > >>> Thanks, > >>> > >> > >> Shouldn't we wait until the reset bindings are actually used in the dtsi ? > >> I'm quite sure kevin will post it with the Ethernet nodes. > > > > Why? The header file is related to the reset controller. Better add it > > now with this patchset. > > > > Cheers, > > > > OK, I will add it in gxbb and meson8b dtsi files. Why also meson8b DTSI? -- Carlo Caione
Re: [PATCH 3/3] ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms
On 20/05/16 14:19, Neil Armstrong wrote: [...] > >>> Missing #include ? > >>> > >>> Thanks, > >>> > >> > >> Shouldn't we wait until the reset bindings are actually used in the dtsi ? > >> I'm quite sure kevin will post it with the Ethernet nodes. > > > > Why? The header file is related to the reset controller. Better add it > > now with this patchset. > > > > Cheers, > > > > OK, I will add it in gxbb and meson8b dtsi files. Why also meson8b DTSI? -- Carlo Caione
Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller
On 20/05/16 11:10, Neil Armstrong wrote:
> On 05/20/2016 11:04 AM, Carlo Caione wrote:
> > On 20/05/16 10:27, Neil Armstrong wrote:
> >> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> >> Controller.
> >>
> >> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> >> ---
> >> drivers/reset/Kconfig| 6 ++
> >> drivers/reset/Makefile | 1 +
> >> drivers/reset/reset-meson-gxbb.c | 129
> >> +++
> >> 3 files changed, 136 insertions(+)
> >> create mode 100644 drivers/reset/reset-meson-gxbb.c
> >
> > Do we really need to be that specific (-gxbb)? This driver looks generic
> > and simple enough to be used for several Amlogic families. You are
> > already differentiating between them with the include file defining the
> > reset indexes for the SoC.
>
> This is a good question, do the S805 have similar registers ? Same count and
> width ?
> I no, it should need a rework to add a data structure per-SoC.
According to the datasheet on S805 we have 7 registers with 16 reset
bits per register.
[...]
> >
> > [...]
> >> +static struct platform_driver meson_gxbb_reset_driver = {
> >> + .probe = meson_gxbb_reset_probe,
> >> + .remove = meson_gxbb_reset_remove,
> >> + .driver = {
> >> + .name = "meson_gxbb_reset",
> >> + .of_match_table = meson_gxbb_reset_dt_ids,
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(meson_gxbb_reset_driver);
> >
> > No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
>
> What is the MODULE_LICENCE format for dual licensing ?
Dual BSD/GPL ?
Cheers,
--
Carlo Caione
Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller
On 20/05/16 11:10, Neil Armstrong wrote:
> On 05/20/2016 11:04 AM, Carlo Caione wrote:
> > On 20/05/16 10:27, Neil Armstrong wrote:
> >> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> >> Controller.
> >>
> >> Signed-off-by: Neil Armstrong
> >> ---
> >> drivers/reset/Kconfig| 6 ++
> >> drivers/reset/Makefile | 1 +
> >> drivers/reset/reset-meson-gxbb.c | 129
> >> +++
> >> 3 files changed, 136 insertions(+)
> >> create mode 100644 drivers/reset/reset-meson-gxbb.c
> >
> > Do we really need to be that specific (-gxbb)? This driver looks generic
> > and simple enough to be used for several Amlogic families. You are
> > already differentiating between them with the include file defining the
> > reset indexes for the SoC.
>
> This is a good question, do the S805 have similar registers ? Same count and
> width ?
> I no, it should need a rework to add a data structure per-SoC.
According to the datasheet on S805 we have 7 registers with 16 reset
bits per register.
[...]
> >
> > [...]
> >> +static struct platform_driver meson_gxbb_reset_driver = {
> >> + .probe = meson_gxbb_reset_probe,
> >> + .remove = meson_gxbb_reset_remove,
> >> + .driver = {
> >> + .name = "meson_gxbb_reset",
> >> + .of_match_table = meson_gxbb_reset_dt_ids,
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(meson_gxbb_reset_driver);
> >
> > No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
>
> What is the MODULE_LICENCE format for dual licensing ?
Dual BSD/GPL ?
Cheers,
--
Carlo Caione
Re: [PATCH 3/3] ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms
On 20/05/16 10:53, Neil Armstrong wrote:
> On 05/20/2016 10:47 AM, Carlo Caione wrote:
> > On 20/05/16 10:27, Neil Armstrong wrote:
> >> Update DTSI file to add the reset controller node.
> >>
> >> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> >> ---
> >> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 6 ++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> index 832815d..2463e04 100644
> >> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> @@ -129,6 +129,12 @@
> >>#size-cells = <2>;
> >>ranges = <0x0 0x0 0x0 0xc110 0x0 0x10>;
> >>
> >> + reset: reset-controller@4404 {
> >> + compatible = "amlogic,meson-gxbb-reset";
> >> + reg = <0x0 0x04404 0x0 0x20>;
> >> + #reset-cells = <1>;
> >> + };
> >> +
> >>uart_A: serial@84c0 {
> >>compatible = "amlogic,meson-uart";
> >>reg = <0x0 0x084c0 0x0 0x14>;
> >
> > Missing #include ?
> >
> > Thanks,
> >
>
> Shouldn't we wait until the reset bindings are actually used in the dtsi ?
> I'm quite sure kevin will post it with the Ethernet nodes.
Why? The header file is related to the reset controller. Better add it
now with this patchset.
Cheers,
--
Carlo Caione
Re: [PATCH 3/3] ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms
On 20/05/16 10:53, Neil Armstrong wrote:
> On 05/20/2016 10:47 AM, Carlo Caione wrote:
> > On 20/05/16 10:27, Neil Armstrong wrote:
> >> Update DTSI file to add the reset controller node.
> >>
> >> Signed-off-by: Neil Armstrong
> >> ---
> >> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 6 ++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> index 832815d..2463e04 100644
> >> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> >> @@ -129,6 +129,12 @@
> >>#size-cells = <2>;
> >>ranges = <0x0 0x0 0x0 0xc110 0x0 0x10>;
> >>
> >> + reset: reset-controller@4404 {
> >> + compatible = "amlogic,meson-gxbb-reset";
> >> + reg = <0x0 0x04404 0x0 0x20>;
> >> + #reset-cells = <1>;
> >> + };
> >> +
> >>uart_A: serial@84c0 {
> >>compatible = "amlogic,meson-uart";
> >>reg = <0x0 0x084c0 0x0 0x14>;
> >
> > Missing #include ?
> >
> > Thanks,
> >
>
> Shouldn't we wait until the reset bindings are actually used in the dtsi ?
> I'm quite sure kevin will post it with the Ethernet nodes.
Why? The header file is related to the reset controller. Better add it
now with this patchset.
Cheers,
--
Carlo Caione
Re: [PATCH 2/3] dt-bindings: reset: Add bindings for the Meson GXBB Reset Controller
On 20/05/16 10:51, Neil Armstrong wrote: > On 05/20/2016 10:46 AM, Carlo Caione wrote: > > On 20/05/16 10:27, Neil Armstrong wrote: > >> Add DT bindings for the Meson GXBB SoC Reset Controller documentation and > >> the > >> associated include file. > > > > [...] > > > >> +#define RESET_I2C_MASTER_2142 > >> +#define RESET_I2C_MASTER_1143 > >> +/*144-159 */ > >> +/*RESET5 */ > >> +/*160-191 */ > >> +/*RESET6 */ > >> +#define RESET_PERIPHS_GENERAL 192 > >> +#define RESET_PERIPHS_SPICC 193 > >> +#define RESET_PERIPHS_SMART_CARD 194 > >> +#define RESET_PERIPHS_SAR_ADC 195 > >> +#define RESET_PERIPHS_I2C_MASTER_0196 > >> +#define RESET_SANA197 > >> +/*198 */ > >> +#define RESET_PERIPHS_STREAM_INTERFACE199 > >> +#define RESET_PERIPHS_SDIO200 > >> +#define RESET_PERIPHS_UART_0 201 > >> +#define RESET_PERIPHS_UART_1_2202 > >> +#define RESET_PERIPHS_ASYNC_0 203 > >> +#define RESET_PERIPHS_ASYNC_1 204 > >> +#define RESET_PERIPHS_SPI_0 205 > >> +#define RESET_PERIPHS_SDHC206 > >> +#define RESET_UART_SLIP 207 > >> +/*208-223 */ > >> +/*RESET7 */ > >> +#define RESET_USB_DDR_0 224 > >> +#define RESET_USB_DDR_1 225 > >> +#define RESET_USB_DDR_2 226 > >> +#define RESET_USB_DDR_3 227 > >> +/*228 */ > >> +#define RESET_DEVICE_MMC_ARB 229 > >> +/*230 */ > >> +#define RESET_VID_LOCK231 > >> +#define RESET_A9_DMC_PIPEL232 > >> +/*233-255 */ > >> + > >> +#endif > > > > Indentation looks really messy. Can you just shift the numbers to the > > right so that they are on the same column? > > > Hi Carlo, > > The patch format makes it very messy, not as in the original format : > https://github.com/superna/linux/blob/6163f8742454bb7ff962956b4e286d110ec0fb79/include/dt-bindings/reset/amlogic%2Cmeson-gxbb-reset.h https://raw.githubusercontent.com/superna/linux/6163f8742454bb7ff962956b4e286d110ec0fb79/include/dt-bindings/reset/amlogic%2Cmeson-gxbb-reset.h yes, but 192 -> 200 are not nicely indented to me :) -- Carlo Caione
Re: [PATCH 2/3] dt-bindings: reset: Add bindings for the Meson GXBB Reset Controller
On 20/05/16 10:51, Neil Armstrong wrote: > On 05/20/2016 10:46 AM, Carlo Caione wrote: > > On 20/05/16 10:27, Neil Armstrong wrote: > >> Add DT bindings for the Meson GXBB SoC Reset Controller documentation and > >> the > >> associated include file. > > > > [...] > > > >> +#define RESET_I2C_MASTER_2142 > >> +#define RESET_I2C_MASTER_1143 > >> +/*144-159 */ > >> +/*RESET5 */ > >> +/*160-191 */ > >> +/*RESET6 */ > >> +#define RESET_PERIPHS_GENERAL 192 > >> +#define RESET_PERIPHS_SPICC 193 > >> +#define RESET_PERIPHS_SMART_CARD 194 > >> +#define RESET_PERIPHS_SAR_ADC 195 > >> +#define RESET_PERIPHS_I2C_MASTER_0196 > >> +#define RESET_SANA197 > >> +/*198 */ > >> +#define RESET_PERIPHS_STREAM_INTERFACE199 > >> +#define RESET_PERIPHS_SDIO200 > >> +#define RESET_PERIPHS_UART_0 201 > >> +#define RESET_PERIPHS_UART_1_2202 > >> +#define RESET_PERIPHS_ASYNC_0 203 > >> +#define RESET_PERIPHS_ASYNC_1 204 > >> +#define RESET_PERIPHS_SPI_0 205 > >> +#define RESET_PERIPHS_SDHC206 > >> +#define RESET_UART_SLIP 207 > >> +/*208-223 */ > >> +/*RESET7 */ > >> +#define RESET_USB_DDR_0 224 > >> +#define RESET_USB_DDR_1 225 > >> +#define RESET_USB_DDR_2 226 > >> +#define RESET_USB_DDR_3 227 > >> +/*228 */ > >> +#define RESET_DEVICE_MMC_ARB 229 > >> +/*230 */ > >> +#define RESET_VID_LOCK231 > >> +#define RESET_A9_DMC_PIPEL232 > >> +/*233-255 */ > >> + > >> +#endif > > > > Indentation looks really messy. Can you just shift the numbers to the > > right so that they are on the same column? > > > Hi Carlo, > > The patch format makes it very messy, not as in the original format : > https://github.com/superna/linux/blob/6163f8742454bb7ff962956b4e286d110ec0fb79/include/dt-bindings/reset/amlogic%2Cmeson-gxbb-reset.h https://raw.githubusercontent.com/superna/linux/6163f8742454bb7ff962956b4e286d110ec0fb79/include/dt-bindings/reset/amlogic%2Cmeson-gxbb-reset.h yes, but 192 -> 200 are not nicely indented to me :) -- Carlo Caione
Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller
On 20/05/16 10:27, Neil Armstrong wrote:
> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> Controller.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
> drivers/reset/Kconfig| 6 ++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-meson-gxbb.c | 129
> +++
> 3 files changed, 136 insertions(+)
> create mode 100644 drivers/reset/reset-meson-gxbb.c
Do we really need to be that specific (-gxbb)? This driver looks generic
and simple enough to be used for several Amlogic families. You are
already differentiating between them with the include file defining the
reset indexes for the SoC.
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..4ac5c4d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>
> If unsure, say no.
>
> +config MESON_GXBB_RESET
> + tristate "Amlogic Meson GXBB Reset Driver"
> + depends on (ARCH_MESON && RESET_CONTROLLER)
> + help
> + Build the Amlogic Meson GxBB reset driver.
> +
> source "drivers/reset/sti/Kconfig"
> source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a1fc8ed..5ff83a1 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
> obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
> obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o
obj-$(CONFIG_ARCH_MESON) ?
[...]
> +static const struct reset_control_ops meson_gxbb_reset_ops = {
> + .reset = meson_gxbb_reset_reset,
> +};
nit: unfortunate name :)
Any reason to not have also .assert / .deassert?
[...]
> +static struct platform_driver meson_gxbb_reset_driver = {
> + .probe = meson_gxbb_reset_probe,
> + .remove = meson_gxbb_reset_remove,
> + .driver = {
> + .name = "meson_gxbb_reset",
> + .of_match_table = meson_gxbb_reset_dt_ids,
> + },
> +};
> +
> +module_platform_driver(meson_gxbb_reset_driver);
No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
--
Carlo Caione
Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller
On 20/05/16 10:27, Neil Armstrong wrote:
> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> Controller.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/reset/Kconfig| 6 ++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-meson-gxbb.c | 129
> +++
> 3 files changed, 136 insertions(+)
> create mode 100644 drivers/reset/reset-meson-gxbb.c
Do we really need to be that specific (-gxbb)? This driver looks generic
and simple enough to be used for several Amlogic families. You are
already differentiating between them with the include file defining the
reset indexes for the SoC.
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..4ac5c4d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>
> If unsure, say no.
>
> +config MESON_GXBB_RESET
> + tristate "Amlogic Meson GXBB Reset Driver"
> + depends on (ARCH_MESON && RESET_CONTROLLER)
> + help
> + Build the Amlogic Meson GxBB reset driver.
> +
> source "drivers/reset/sti/Kconfig"
> source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a1fc8ed..5ff83a1 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
> obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
> obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
> obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o
obj-$(CONFIG_ARCH_MESON) ?
[...]
> +static const struct reset_control_ops meson_gxbb_reset_ops = {
> + .reset = meson_gxbb_reset_reset,
> +};
nit: unfortunate name :)
Any reason to not have also .assert / .deassert?
[...]
> +static struct platform_driver meson_gxbb_reset_driver = {
> + .probe = meson_gxbb_reset_probe,
> + .remove = meson_gxbb_reset_remove,
> + .driver = {
> + .name = "meson_gxbb_reset",
> + .of_match_table = meson_gxbb_reset_dt_ids,
> + },
> +};
> +
> +module_platform_driver(meson_gxbb_reset_driver);
No MODULE_AUTHOR, MODULE_LICENSE, etc... ?
--
Carlo Caione
Re: [PATCH 3/3] ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms
On 20/05/16 10:27, Neil Armstrong wrote:
> Update DTSI file to add the reset controller node.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index 832815d..2463e04 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -129,6 +129,12 @@
> #size-cells = <2>;
> ranges = <0x0 0x0 0x0 0xc110 0x0 0x10>;
>
> + reset: reset-controller@4404 {
> + compatible = "amlogic,meson-gxbb-reset";
> + reg = <0x0 0x04404 0x0 0x20>;
> + #reset-cells = <1>;
> + };
> +
> uart_A: serial@84c0 {
> compatible = "amlogic,meson-uart";
> reg = <0x0 0x084c0 0x0 0x14>;
Missing #include ?
Thanks,
--
Carlo Caione
Re: [PATCH 3/3] ARM64: dts: amlogic: Enable Reset Controller on GXBB-based platforms
On 20/05/16 10:27, Neil Armstrong wrote:
> Update DTSI file to add the reset controller node.
>
> Signed-off-by: Neil Armstrong
> ---
> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index 832815d..2463e04 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -129,6 +129,12 @@
> #size-cells = <2>;
> ranges = <0x0 0x0 0x0 0xc110 0x0 0x10>;
>
> + reset: reset-controller@4404 {
> + compatible = "amlogic,meson-gxbb-reset";
> + reg = <0x0 0x04404 0x0 0x20>;
> + #reset-cells = <1>;
> + };
> +
> uart_A: serial@84c0 {
> compatible = "amlogic,meson-uart";
> reg = <0x0 0x084c0 0x0 0x14>;
Missing #include ?
Thanks,
--
Carlo Caione
Re: [PATCH 2/3] dt-bindings: reset: Add bindings for the Meson GXBB Reset Controller
On 20/05/16 10:27, Neil Armstrong wrote: > Add DT bindings for the Meson GXBB SoC Reset Controller documentation and the > associated include file. [...] > +#define RESET_I2C_MASTER_2 142 > +#define RESET_I2C_MASTER_1 143 > +/* 144-159 */ > +/* RESET5 */ > +/* 160-191 */ > +/* RESET6 */ > +#define RESET_PERIPHS_GENERAL192 > +#define RESET_PERIPHS_SPICC 193 > +#define RESET_PERIPHS_SMART_CARD 194 > +#define RESET_PERIPHS_SAR_ADC195 > +#define RESET_PERIPHS_I2C_MASTER_0 196 > +#define RESET_SANA 197 > +/* 198 */ > +#define RESET_PERIPHS_STREAM_INTERFACE 199 > +#define RESET_PERIPHS_SDIO 200 > +#define RESET_PERIPHS_UART_0 201 > +#define RESET_PERIPHS_UART_1_2 202 > +#define RESET_PERIPHS_ASYNC_0203 > +#define RESET_PERIPHS_ASYNC_1204 > +#define RESET_PERIPHS_SPI_0 205 > +#define RESET_PERIPHS_SDHC 206 > +#define RESET_UART_SLIP 207 > +/* 208-223 */ > +/* RESET7 */ > +#define RESET_USB_DDR_0 224 > +#define RESET_USB_DDR_1 225 > +#define RESET_USB_DDR_2 226 > +#define RESET_USB_DDR_3 227 > +/* 228 */ > +#define RESET_DEVICE_MMC_ARB 229 > +/* 230 */ > +#define RESET_VID_LOCK 231 > +#define RESET_A9_DMC_PIPEL 232 > +/* 233-255 */ > + > +#endif Indentation looks really messy. Can you just shift the numbers to the right so that they are on the same column? -- Carlo Caione
Re: [PATCH 2/3] dt-bindings: reset: Add bindings for the Meson GXBB Reset Controller
On 20/05/16 10:27, Neil Armstrong wrote: > Add DT bindings for the Meson GXBB SoC Reset Controller documentation and the > associated include file. [...] > +#define RESET_I2C_MASTER_2 142 > +#define RESET_I2C_MASTER_1 143 > +/* 144-159 */ > +/* RESET5 */ > +/* 160-191 */ > +/* RESET6 */ > +#define RESET_PERIPHS_GENERAL192 > +#define RESET_PERIPHS_SPICC 193 > +#define RESET_PERIPHS_SMART_CARD 194 > +#define RESET_PERIPHS_SAR_ADC195 > +#define RESET_PERIPHS_I2C_MASTER_0 196 > +#define RESET_SANA 197 > +/* 198 */ > +#define RESET_PERIPHS_STREAM_INTERFACE 199 > +#define RESET_PERIPHS_SDIO 200 > +#define RESET_PERIPHS_UART_0 201 > +#define RESET_PERIPHS_UART_1_2 202 > +#define RESET_PERIPHS_ASYNC_0203 > +#define RESET_PERIPHS_ASYNC_1204 > +#define RESET_PERIPHS_SPI_0 205 > +#define RESET_PERIPHS_SDHC 206 > +#define RESET_UART_SLIP 207 > +/* 208-223 */ > +/* RESET7 */ > +#define RESET_USB_DDR_0 224 > +#define RESET_USB_DDR_1 225 > +#define RESET_USB_DDR_2 226 > +#define RESET_USB_DDR_3 227 > +/* 228 */ > +#define RESET_DEVICE_MMC_ARB 229 > +/* 230 */ > +#define RESET_VID_LOCK 231 > +#define RESET_A9_DMC_PIPEL 232 > +/* 233-255 */ > + > +#endif Indentation looks really messy. Can you just shift the numbers to the right so that they are on the same column? -- Carlo Caione
Re: [PATCH] MAINTAINERS: ARM/Amlogic: add co-maintainer, misc. updates
On Fri, May 6, 2016 at 3:59 PM, Kevin Hilman <khil...@baylibre.com> wrote: > Add myself as co-maintainer, update mailing list entry and add a couple > more directories. > > Signed-off-by: Kevin Hilman <khil...@baylibre.com> Acked-by: Carlo Caione <ca...@caione.org> Thank you! -- Carlo Caione
Re: [PATCH] MAINTAINERS: ARM/Amlogic: add co-maintainer, misc. updates
On Fri, May 6, 2016 at 3:59 PM, Kevin Hilman wrote: > Add myself as co-maintainer, update mailing list entry and add a couple > more directories. > > Signed-off-by: Kevin Hilman Acked-by: Carlo Caione Thank you! -- Carlo Caione
Re: [PATCH v2 0/6] ARM64: meson: GXBaby (S905) and Vega S95 enablement
On Tue, Mar 22, 2016 at 9:29 PM, Andreas Färber <afaer...@suse.de> wrote: > Am 21.03.2016 um 23:36 schrieb Kevin Hilman: >> On Tue, Mar 1, 2016 at 6:34 PM, Andreas Färber <afaer...@suse.de> wrote: >> >>> Note: On the Vega S95 I need to change TEXT_OFFSET as follows, >>> in order to avoid the vendor U-Boot overwriting itself (fwiu); >>> for the Mini Mx that's reportedly not necessary. >> >> FYI, the Amlogic P200 dev board also needs this hack with the factory u-boot. > > I have meanwhile found that > > mkimage -A arm64 -O linux -T kernel -C none -a 0x108 -e 0x108 \ > -n linux-next -d arch/arm64/boot/Image ../uImage > > and then using bootm instead of booti works even without the above hack > on the Vega S95. Not a satisfactory solution yet, but better than > patching the kernel in a distro-incompatible way. I wonder if we can add this kind of information in Documentation/arm/meson/. Probably it could be handy until we have a proper u-boot porting. -- Carlo Caione
Re: [PATCH v2 0/6] ARM64: meson: GXBaby (S905) and Vega S95 enablement
On Tue, Mar 22, 2016 at 9:29 PM, Andreas Färber wrote: > Am 21.03.2016 um 23:36 schrieb Kevin Hilman: >> On Tue, Mar 1, 2016 at 6:34 PM, Andreas Färber wrote: >> >>> Note: On the Vega S95 I need to change TEXT_OFFSET as follows, >>> in order to avoid the vendor U-Boot overwriting itself (fwiu); >>> for the Mini Mx that's reportedly not necessary. >> >> FYI, the Amlogic P200 dev board also needs this hack with the factory u-boot. > > I have meanwhile found that > > mkimage -A arm64 -O linux -T kernel -C none -a 0x108 -e 0x108 \ > -n linux-next -d arch/arm64/boot/Image ../uImage > > and then using bootm instead of booti works even without the above hack > on the Vega S95. Not a satisfactory solution yet, but better than > patching the kernel in a distro-incompatible way. I wonder if we can add this kind of information in Documentation/arm/meson/. Probably it could be handy until we have a proper u-boot porting. -- Carlo Caione
Re: [PATCH] ARM64: dts: amlogic: Clean up Vega S95 /memory nodes
On Thu, Mar 17, 2016 at 12:10 AM, Andreas Färber <afaer...@suse.de> wrote:
> Resolve the following warnings from new dtc by adding the unit address:
>
> DTC arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dtb
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property,
> but no unit name
> DTC arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dtb
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property,
> but no unit name
> DTC arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dtb
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property,
> but no unit name
>
> Fixes: cc733bc90636 ("ARM64: dts: amlogic: Add Tronsmart Vega S95 configs")
> Signed-off-by: Andreas Färber <afaer...@suse.de>
> ---
> arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts | 2 +-
> arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts | 2 +-
> arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> index 399aff9e7975..62fb4968d680 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> @@ -48,7 +48,7 @@
> compatible = "tronsmart,vega-s95-meta", "tronsmart,vega-s95",
> "amlogic,meson-gxbb";
> model = "Tronsmart Vega S95 Meta";
>
> - memory {
> + memory@0 {
> device_type = "memory";
> reg = <0x0 0x0 0x0 0x8000>;
> };
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> index ac5a241b5ec2..9a9663abdf5c 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> @@ -48,7 +48,7 @@
> compatible = "tronsmart,vega-s95-pro", "tronsmart,vega-s95",
> "amlogic,meson-gxbb";
> model = "Tronsmart Vega S95 Pro";
>
> - memory {
> + memory@0 {
> device_type = "memory";
> reg = <0x0 0x0 0x0 0x4000>;
> };
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> index fff7bfa2aa39..2fe167b2609d 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> @@ -48,7 +48,7 @@
> compatible = "tronsmart,vega-s95-telos", "tronsmart,vega-s95",
> "amlogic,meson-gxbb";
> model = "Tronsmart Vega S95 Telos";
>
> - memory {
> + memory@0 {
> device_type = "memory";
> reg = <0x0 0x0 0x0 0x8000>;
> };
Acked-by: Carlo Caione <ca...@endlessm.com>
Thanks,
--
Carlo Caione
Re: [PATCH] ARM64: dts: amlogic: Clean up Vega S95 /memory nodes
On Thu, Mar 17, 2016 at 12:10 AM, Andreas Färber wrote:
> Resolve the following warnings from new dtc by adding the unit address:
>
> DTC arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dtb
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property,
> but no unit name
> DTC arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dtb
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property,
> but no unit name
> DTC arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dtb
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property,
> but no unit name
>
> Fixes: cc733bc90636 ("ARM64: dts: amlogic: Add Tronsmart Vega S95 configs")
> Signed-off-by: Andreas Färber
> ---
> arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts | 2 +-
> arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts | 2 +-
> arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> index 399aff9e7975..62fb4968d680 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-meta.dts
> @@ -48,7 +48,7 @@
> compatible = "tronsmart,vega-s95-meta", "tronsmart,vega-s95",
> "amlogic,meson-gxbb";
> model = "Tronsmart Vega S95 Meta";
>
> - memory {
> + memory@0 {
> device_type = "memory";
> reg = <0x0 0x0 0x0 0x8000>;
> };
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> index ac5a241b5ec2..9a9663abdf5c 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-pro.dts
> @@ -48,7 +48,7 @@
> compatible = "tronsmart,vega-s95-pro", "tronsmart,vega-s95",
> "amlogic,meson-gxbb";
> model = "Tronsmart Vega S95 Pro";
>
> - memory {
> + memory@0 {
> device_type = "memory";
> reg = <0x0 0x0 0x0 0x4000>;
> };
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> index fff7bfa2aa39..2fe167b2609d 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95-telos.dts
> @@ -48,7 +48,7 @@
> compatible = "tronsmart,vega-s95-telos", "tronsmart,vega-s95",
> "amlogic,meson-gxbb";
> model = "Tronsmart Vega S95 Telos";
>
> - memory {
> + memory@0 {
> device_type = "memory";
> reg = <0x0 0x0 0x0 0x8000>;
> };
Acked-by: Carlo Caione
Thanks,
--
Carlo Caione
Re: [PATCH v2 0/6] ARM64: meson: GXBaby (S905) and Vega S95 enablement
On Wed, Mar 2, 2016 at 3:34 AM, Andreas Färber <afaer...@suse.de> wrote: > Hello, > > This series adds initial support for the Amlogic S905 based > Tronsmart Vega S95 Pro, Meta and Telos TV boxes. Thank you for working on this. If there will be no more comments in the few next days I'll pick the whole series up. Cheers, -- Carlo Caione
Re: [PATCH v2 0/6] ARM64: meson: GXBaby (S905) and Vega S95 enablement
On Wed, Mar 2, 2016 at 3:34 AM, Andreas Färber wrote: > Hello, > > This series adds initial support for the Amlogic S905 based > Tronsmart Vega S95 Pro, Meta and Telos TV boxes. Thank you for working on this. If there will be no more comments in the few next days I'll pick the whole series up. Cheers, -- Carlo Caione
Re: [PATCH v3] tty: serial: meson: Implement earlycon support
On Wed, Mar 2, 2016 at 3:49 AM, Andreas Färber <afaer...@suse.de> wrote: > Split off the bulk of the existing meson_serial_console_write() > implementation into meson_serial_port_write() for implementing > meson_serial_early_console_write(). > > Use "meson" as the earlycon driver name, courtesy of Nicolas. > > Signed-off-by: Nicolas Saenz Julienne <nicolassae...@gmail.com> > Signed-off-by: Andreas Färber <afaer...@suse.de> Acked-by: Carlo Caione <ca...@endlessm.com> Thanks, -- Carlo Caione
Re: [PATCH v3] tty: serial: meson: Implement earlycon support
On Wed, Mar 2, 2016 at 3:49 AM, Andreas Färber wrote: > Split off the bulk of the existing meson_serial_console_write() > implementation into meson_serial_port_write() for implementing > meson_serial_early_console_write(). > > Use "meson" as the earlycon driver name, courtesy of Nicolas. > > Signed-off-by: Nicolas Saenz Julienne > Signed-off-by: Andreas Färber Acked-by: Carlo Caione Thanks, -- Carlo Caione
Re: [PATCH 7/8] ARM64: dts: amlogic: Extend GXBaby GIC node
On Tue, Mar 1, 2016 at 1:43 PM, Andre Przywara <andre.przyw...@arm.com> wrote: > Hi, > > On 01/03/16 11:18, Andreas Färber wrote: >> Hi Andre, >> >> Am 01.03.2016 um 12:01 schrieb Andre Przywara: >>> On 29/02/16 23:44, Andreas Färber wrote: >>>> Add GICH and GICV resources for HYP mode - guess based on other vendors. >>> >>> Do you know if the firmware allows the kernel to be entered in EL2 >>> (which is the arm64 name for HYP)? >>> So can we run kvm? >>> If you have a booted kernel, can you grep for "EL2" and "kvm" in the dmesg? >> >> I do not have a rootfs yet (MMC v5 patches by Carlo are still waiting >> for review), but with this change the KVM driver initializes okay - the >> purpose of this patch! >> >>> Also you should merge this patch into 3/8, same for 8/8. >> >> If people confirm this is generally or specifically for this SoC correct >> then sure. So far 3/8 seems a safe subset for lack of public documentation. > > The GIC is an integral part of the SoC, so this clearly belongs in there. > >>>> Signed-off-by: Andreas Färber <afaer...@suse.de> >>>> --- >>>> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 4 +++- > > In general I was wondering if this naming is correct? > Shouldn't it be something with "s905" in it? Because this the SoC that > is driving all those hardware and the peripherals that you describe in > there are clearly within the SoC. > So something like meson-s905.dtsi or the like? When I first submitted support for the meson8 and meson8b I picked up the names according to the Amlogic SDK. In the latest Amlogic drop this SoC is identified as meson-gxbb so probably we should stick to this name. -- Carlo Caione
Re: [PATCH 7/8] ARM64: dts: amlogic: Extend GXBaby GIC node
On Tue, Mar 1, 2016 at 1:43 PM, Andre Przywara wrote: > Hi, > > On 01/03/16 11:18, Andreas Färber wrote: >> Hi Andre, >> >> Am 01.03.2016 um 12:01 schrieb Andre Przywara: >>> On 29/02/16 23:44, Andreas Färber wrote: >>>> Add GICH and GICV resources for HYP mode - guess based on other vendors. >>> >>> Do you know if the firmware allows the kernel to be entered in EL2 >>> (which is the arm64 name for HYP)? >>> So can we run kvm? >>> If you have a booted kernel, can you grep for "EL2" and "kvm" in the dmesg? >> >> I do not have a rootfs yet (MMC v5 patches by Carlo are still waiting >> for review), but with this change the KVM driver initializes okay - the >> purpose of this patch! >> >>> Also you should merge this patch into 3/8, same for 8/8. >> >> If people confirm this is generally or specifically for this SoC correct >> then sure. So far 3/8 seems a safe subset for lack of public documentation. > > The GIC is an integral part of the SoC, so this clearly belongs in there. > >>>> Signed-off-by: Andreas Färber >>>> --- >>>> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 4 +++- > > In general I was wondering if this naming is correct? > Shouldn't it be something with "s905" in it? Because this the SoC that > is driving all those hardware and the peripherals that you describe in > there are clearly within the SoC. > So something like meson-s905.dtsi or the like? When I first submitted support for the meson8 and meson8b I picked up the names according to the Amlogic SDK. In the latest Amlogic drop this SoC is identified as meson-gxbb so probably we should stick to this name. -- Carlo Caione
Re: [PATCH 14/50] pinctrl: meson: Use devm_pinctrl_register() for pinctrl registration
On Wed, Feb 24, 2016 at 2:15 PM, Laxman Dewangan <ldewan...@nvidia.com> wrote: > Use devm_pinctrl_register() for pin control registration. > > Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com> > Cc: Carlo Caione <ca...@endlessm.com> > Cc: Beniamino Galvani <b.galv...@gmail.com> > Cc: Lee Jones <lee.jo...@linaro.org> Acked-by: <ca...@endlessm.com> -- Carlo Caione
Re: [PATCH 14/50] pinctrl: meson: Use devm_pinctrl_register() for pinctrl registration
On Wed, Feb 24, 2016 at 2:15 PM, Laxman Dewangan wrote: > Use devm_pinctrl_register() for pin control registration. > > Signed-off-by: Laxman Dewangan > Cc: Carlo Caione > Cc: Beniamino Galvani > Cc: Lee Jones Acked-by: -- Carlo Caione
[PATCH] ASoC: cht_bsw_rt5645: Enable jack detection
From: Carlo Caione <ca...@endlessm.com>
Add missing DAPM pins and enable jack detection on those pins for
Cherrytrail and Braswell.
Signed-off-by: Carlo Caione <ca...@endlessm.com>
---
sound/soc/intel/boards/cht_bsw_rt5645.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c
b/sound/soc/intel/boards/cht_bsw_rt5645.c
index e6cf800..c7af271 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -147,6 +147,17 @@ static const struct snd_kcontrol_new cht_mc_controls[] = {
SOC_DAPM_PIN_SWITCH("Ext Spk"),
};
+static struct snd_soc_jack_pin cht_bsw_jack_pins[] = {
+ {
+ .pin= "Headphone",
+ .mask = SND_JACK_HEADPHONE,
+ },
+ {
+ .pin= "Headset Mic",
+ .mask = SND_JACK_MICROPHONE,
+ },
+};
+
static int cht_aif1_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
@@ -202,9 +213,9 @@ static int cht_codec_init(struct snd_soc_pcm_runtime
*runtime)
else
jack_type = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE;
- ret = snd_soc_card_jack_new(runtime->card, "Headset Jack",
+ ret = snd_soc_card_jack_new(runtime->card, "Headset",
jack_type, >jack,
- NULL, 0);
+ cht_bsw_jack_pins,
ARRAY_SIZE(cht_bsw_jack_pins));
if (ret) {
dev_err(runtime->dev, "Headset jack creation failed %d\n", ret);
return ret;
--
1.9.1
[PATCH] ASoC: cht_bsw_rt5645: Enable jack detection
From: Carlo Caione
Add missing DAPM pins and enable jack detection on those pins for
Cherrytrail and Braswell.
Signed-off-by: Carlo Caione
---
sound/soc/intel/boards/cht_bsw_rt5645.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c
b/sound/soc/intel/boards/cht_bsw_rt5645.c
index e6cf800..c7af271 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -147,6 +147,17 @@ static const struct snd_kcontrol_new cht_mc_controls[] = {
SOC_DAPM_PIN_SWITCH("Ext Spk"),
};
+static struct snd_soc_jack_pin cht_bsw_jack_pins[] = {
+ {
+ .pin= "Headphone",
+ .mask = SND_JACK_HEADPHONE,
+ },
+ {
+ .pin= "Headset Mic",
+ .mask = SND_JACK_MICROPHONE,
+ },
+};
+
static int cht_aif1_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
@@ -202,9 +213,9 @@ static int cht_codec_init(struct snd_soc_pcm_runtime
*runtime)
else
jack_type = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE;
- ret = snd_soc_card_jack_new(runtime->card, "Headset Jack",
+ ret = snd_soc_card_jack_new(runtime->card, "Headset",
jack_type, >jack,
- NULL, 0);
+ cht_bsw_jack_pins,
ARRAY_SIZE(cht_bsw_jack_pins));
if (ret) {
dev_err(runtime->dev, "Headset jack creation failed %d\n", ret);
return ret;
--
1.9.1
Re: [PATCH] tty: serial: meson: Add support for XTAL clock input
On Mon, Feb 8, 2016 at 1:49 PM, Andreas Färber wrote: > Fix the baudrate calculation for 24 MHz XTAL clock found on gxbb platforms. > > Signed-off-by: Andreas Färber Acked-by: Carlo Caione Thanks! -- Carlo Caione
Re: [PATCH] tty: serial: meson: Add support for XTAL clock input
On Mon, Feb 8, 2016 at 1:49 PM, Andreas Färber <afaer...@suse.de> wrote: > Fix the baudrate calculation for 24 MHz XTAL clock found on gxbb platforms. > > Signed-off-by: Andreas Färber <afaer...@suse.de> Acked-by: Carlo Caione <ca...@endlessm.com> Thanks! -- Carlo Caione
Re: [PATCH] clk: meson: Fix meson_clk_register_clks() signature type mismatch
On Mon, Feb 8, 2016 at 9:34 AM, Carlo Caione wrote:
> On Sun, Feb 7, 2016 at 10:13 PM, Andreas Färber wrote:
>> As preparation for arm64 based mesongxbb, which pulls in this code once
>> enabling ARCH_MESON, fix a size_t vs. unsigned int type mismatch.
>> The loop uses a local unsigned int variable, so adopt that type,
>> matching the header.
>>
>> Fixes: 7a29a869434e ("clk: meson: Add support for Meson clock controller")
>> Signed-off-by: Andreas Färber
>> ---
>> drivers/clk/meson/clkc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/meson/clkc.c b/drivers/clk/meson/clkc.c
>> index c83ae1367abc..d920d410b51d 100644
>> --- a/drivers/clk/meson/clkc.c
>> +++ b/drivers/clk/meson/clkc.c
>> @@ -198,7 +198,7 @@ meson_clk_register_fixed_rate(const struct clk_conf
>> *clk_conf,
>> }
>>
>> void __init meson_clk_register_clks(const struct clk_conf *clk_confs,
>> - size_t nr_confs,
>> + unsigned int nr_confs,
>> void __iomem *clk_base)
>> {
>> unsigned int i;
>
> Nit: I'd prefer to fix declaration in drivers/clk/meson/clkc.h with
> size_t since we are going to use it for array indexing.
On a second thought it's ok since we use i to cycle through clocks.
Acked-by: Carlo Caione
Cheers,
--
Carlo Caione
Re: [PATCH] clk: meson: Fix meson_clk_register_clks() signature type mismatch
On Sun, Feb 7, 2016 at 10:13 PM, Andreas Färber wrote:
> As preparation for arm64 based mesongxbb, which pulls in this code once
> enabling ARCH_MESON, fix a size_t vs. unsigned int type mismatch.
> The loop uses a local unsigned int variable, so adopt that type,
> matching the header.
>
> Fixes: 7a29a869434e ("clk: meson: Add support for Meson clock controller")
> Signed-off-by: Andreas Färber
> ---
> drivers/clk/meson/clkc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/clkc.c b/drivers/clk/meson/clkc.c
> index c83ae1367abc..d920d410b51d 100644
> --- a/drivers/clk/meson/clkc.c
> +++ b/drivers/clk/meson/clkc.c
> @@ -198,7 +198,7 @@ meson_clk_register_fixed_rate(const struct clk_conf
> *clk_conf,
> }
>
> void __init meson_clk_register_clks(const struct clk_conf *clk_confs,
> - size_t nr_confs,
> + unsigned int nr_confs,
> void __iomem *clk_base)
> {
> unsigned int i;
Nit: I'd prefer to fix declaration in drivers/clk/meson/clkc.h with
size_t since we are going to use it for array indexing.
Also please, CC linux-me...@googlegroups.com on meson related patches.
Thanks!
--
Carlo Caione
Re: [PATCH] clk: meson: Fix meson_clk_register_clks() signature type mismatch
On Mon, Feb 8, 2016 at 9:34 AM, Carlo Caione <ca...@caione.org> wrote:
> On Sun, Feb 7, 2016 at 10:13 PM, Andreas Färber <afaer...@suse.de> wrote:
>> As preparation for arm64 based mesongxbb, which pulls in this code once
>> enabling ARCH_MESON, fix a size_t vs. unsigned int type mismatch.
>> The loop uses a local unsigned int variable, so adopt that type,
>> matching the header.
>>
>> Fixes: 7a29a869434e ("clk: meson: Add support for Meson clock controller")
>> Signed-off-by: Andreas Färber <afaer...@suse.de>
>> ---
>> drivers/clk/meson/clkc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/meson/clkc.c b/drivers/clk/meson/clkc.c
>> index c83ae1367abc..d920d410b51d 100644
>> --- a/drivers/clk/meson/clkc.c
>> +++ b/drivers/clk/meson/clkc.c
>> @@ -198,7 +198,7 @@ meson_clk_register_fixed_rate(const struct clk_conf
>> *clk_conf,
>> }
>>
>> void __init meson_clk_register_clks(const struct clk_conf *clk_confs,
>> - size_t nr_confs,
>> + unsigned int nr_confs,
>> void __iomem *clk_base)
>> {
>> unsigned int i;
>
> Nit: I'd prefer to fix declaration in drivers/clk/meson/clkc.h with
> size_t since we are going to use it for array indexing.
On a second thought it's ok since we use i to cycle through clocks.
Acked-by: Carlo Caione <ca...@endlessm.com>
Cheers,
--
Carlo Caione
Re: [PATCH] clk: meson: Fix meson_clk_register_clks() signature type mismatch
On Sun, Feb 7, 2016 at 10:13 PM, Andreas Färber <afaer...@suse.de> wrote:
> As preparation for arm64 based mesongxbb, which pulls in this code once
> enabling ARCH_MESON, fix a size_t vs. unsigned int type mismatch.
> The loop uses a local unsigned int variable, so adopt that type,
> matching the header.
>
> Fixes: 7a29a869434e ("clk: meson: Add support for Meson clock controller")
> Signed-off-by: Andreas Färber <afaer...@suse.de>
> ---
> drivers/clk/meson/clkc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/clkc.c b/drivers/clk/meson/clkc.c
> index c83ae1367abc..d920d410b51d 100644
> --- a/drivers/clk/meson/clkc.c
> +++ b/drivers/clk/meson/clkc.c
> @@ -198,7 +198,7 @@ meson_clk_register_fixed_rate(const struct clk_conf
> *clk_conf,
> }
>
> void __init meson_clk_register_clks(const struct clk_conf *clk_confs,
> - size_t nr_confs,
> + unsigned int nr_confs,
> void __iomem *clk_base)
> {
> unsigned int i;
Nit: I'd prefer to fix declaration in drivers/clk/meson/clkc.h with
size_t since we are going to use it for array indexing.
Also please, CC linux-me...@googlegroups.com on meson related patches.
Thanks!
--
Carlo Caione
Re: [PATCH 1/1] ARM: meson: Add status LED for Odroid-C1
On Fri, Oct 30, 2015 at 6:55 PM, Edward Cragg wrote: > On Fri, Oct 09, 2015 at 03:37:10PM +0100, Edward Cragg wrote: >> Add the blue status LED to the Hardkernel Odroid C1 board DTS. >> This is the only programmable LED on the board. >> >> Signed-off-by: Edward Cragg >> --- >> arch/arm/boot/dts/meson8b-odroidc1.dts | 13 + >> 1 file changed, 13 insertions(+) >> > > Hi, has anyone had a chance to have a look at this yet? Yes, I already replied to you on Oct 12. I was waiting for a resubmission without the added copyright but I'll modify it myself and I'll push a PR. Cheers, -- Carlo Caione -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] ARM: meson: Add status LED for Odroid-C1
On Fri, Oct 30, 2015 at 6:55 PM, Edward Cragg <edward.cr...@codethink.co.uk> wrote: > On Fri, Oct 09, 2015 at 03:37:10PM +0100, Edward Cragg wrote: >> Add the blue status LED to the Hardkernel Odroid C1 board DTS. >> This is the only programmable LED on the board. >> >> Signed-off-by: Edward Cragg <edward.cr...@codethink.co.uk> >> --- >> arch/arm/boot/dts/meson8b-odroidc1.dts | 13 + >> 1 file changed, 13 insertions(+) >> > > Hi, has anyone had a chance to have a look at this yet? Yes, I already replied to you on Oct 12. I was waiting for a resubmission without the added copyright but I'll modify it myself and I'll push a PR. Cheers, -- Carlo Caione -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] ARM: meson: Add status LED for Odroid-C1
On Fri, Oct 9, 2015 at 4:37 PM, Edward Cragg
wrote:
> Add the blue status LED to the Hardkernel Odroid C1 board DTS.
> This is the only programmable LED on the board.
>
> Signed-off-by: Edward Cragg
> ---
> arch/arm/boot/dts/meson8b-odroidc1.dts | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts
> b/arch/arm/boot/dts/meson8b-odroidc1.dts
> index a8e2911..3b03523 100644
> --- a/arch/arm/boot/dts/meson8b-odroidc1.dts
> +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
> @@ -1,6 +1,8 @@
> /*
> * Copyright 2015 Endless Mobile, Inc.
> * Author: Carlo Caione
> + * Copyright 2015 Codethink Ltd.
> + * Author: Edward Cragg
Usually we do not add a copyright line for each small addition to the DTS.
> *
> * This file is dual-licensed: you can use it either under the terms
> * of the GPL or the X11 license, at your option. Note that this dual
> @@ -46,6 +48,7 @@
>
> /dts-v1/;
> #include "meson8b.dtsi"
> +#include
>
> / {
> model = "Hardkernel ODROID-C1";
> @@ -58,6 +61,16 @@
> memory {
> reg = <0x4000 0x4000>;
> };
> +
> + leds {
> + compatible = "gpio-leds";
> + blue {
> + label = "c1:blue:alive";
> + gpios = <_ao GPIOAO_13 GPIO_ACTIVE_LOW>;
> + linux,default-trigger = "heartbeat";
> + default-state = "off";
> + };
> + };
> };
otherwise Acked-by: Carlo Caione
--
Carlo Caione
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] ARM: meson: Add status LED for Odroid-C1
On Fri, Oct 9, 2015 at 4:37 PM, Edward Cragg
<edward.cr...@codethink.co.uk> wrote:
> Add the blue status LED to the Hardkernel Odroid C1 board DTS.
> This is the only programmable LED on the board.
>
> Signed-off-by: Edward Cragg <edward.cr...@codethink.co.uk>
> ---
> arch/arm/boot/dts/meson8b-odroidc1.dts | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts
> b/arch/arm/boot/dts/meson8b-odroidc1.dts
> index a8e2911..3b03523 100644
> --- a/arch/arm/boot/dts/meson8b-odroidc1.dts
> +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
> @@ -1,6 +1,8 @@
> /*
> * Copyright 2015 Endless Mobile, Inc.
> * Author: Carlo Caione <ca...@endlessm.com>
> + * Copyright 2015 Codethink Ltd.
> + * Author: Edward Cragg <edward.cr...@codethink.co.uk>
Usually we do not add a copyright line for each small addition to the DTS.
> *
> * This file is dual-licensed: you can use it either under the terms
> * of the GPL or the X11 license, at your option. Note that this dual
> @@ -46,6 +48,7 @@
>
> /dts-v1/;
> #include "meson8b.dtsi"
> +#include
>
> / {
> model = "Hardkernel ODROID-C1";
> @@ -58,6 +61,16 @@
> memory {
> reg = <0x4000 0x4000>;
> };
> +
> + leds {
> + compatible = "gpio-leds";
> + blue {
> + label = "c1:blue:alive";
> + gpios = <_ao GPIOAO_13 GPIO_ACTIVE_LOW>;
> + linux,default-trigger = "heartbeat";
> + default-state = "off";
> + };
> + };
> };
otherwise Acked-by: Carlo Caione <ca...@endlessm.com>
--
Carlo Caione
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] [PATCH] Input: axp20x-pek: Add module alias
On Mon, Aug 3, 2015 at 9:48 AM, Chen-Yu Tsai wrote: > Add a proper module alias so the driver can be autoloaded when the > parent axp20x mfd driver registers its cells. > > Signed-off-by: Chen-Yu Tsai Acked-by: Carlo Caione Thanks, -- Carlo Caione -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] [PATCH] Input: axp20x-pek: Add module alias
On Mon, Aug 3, 2015 at 9:48 AM, Chen-Yu Tsai w...@csie.org wrote: Add a proper module alias so the driver can be autoloaded when the parent axp20x mfd driver registers its cells. Signed-off-by: Chen-Yu Tsai w...@csie.org Acked-by: Carlo Caione ca...@caione.org Thanks, -- Carlo Caione -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/41] clocksource: meson6: Migrate to new 'set-state' interface
On Thu, Jun 18, 2015 at 12:54 PM, Viresh Kumar wrote: > Migrate meson6 driver to the new 'set-state' interface provided by > clockevents core, the earlier 'set-mode' interface is marked obsolete > now. > > This also enables us to implement callbacks for new states of clockevent > devices, for example: ONESHOT_STOPPED. > > Cc: Carlo Caione > Signed-off-by: Viresh Kumar > --- > drivers/clocksource/meson6_timer.c | 50 > -- > 1 file changed, 27 insertions(+), 23 deletions(-) Acked-by: Carlo Caione -- Carlo Caione -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/41] clocksource: meson6: Migrate to new 'set-state' interface
On Thu, Jun 18, 2015 at 12:54 PM, Viresh Kumar viresh.ku...@linaro.org wrote: Migrate meson6 driver to the new 'set-state' interface provided by clockevents core, the earlier 'set-mode' interface is marked obsolete now. This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED. Cc: Carlo Caione ca...@caione.org Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- drivers/clocksource/meson6_timer.c | 50 -- 1 file changed, 27 insertions(+), 23 deletions(-) Acked-by: Carlo Caione ca...@caione.org -- Carlo Caione -- To unsubscribe from this list: send the line unsubscribe linux-kernel in Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: sunxi-nmi: Fix off-by-one for iterating over gc->num_ct
On Sun, Jun 7, 2015 at 3:33 PM, Axel Lin wrote: > Signed-off-by: Axel Lin > --- > drivers/irqchip/irq-sunxi-nmi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c > index 12f547a..eb9b59e 100644 > --- a/drivers/irqchip/irq-sunxi-nmi.c > +++ b/drivers/irqchip/irq-sunxi-nmi.c > @@ -104,7 +104,7 @@ static int sunxi_sc_nmi_set_type(struct irq_data *data, > unsigned int flow_type) > irqd_set_trigger_type(data, flow_type); > irq_setup_alt_chip(data, flow_type); > > - for (i = 0; i <= gc->num_ct; i++, ct++) > + for (i = 0; i < gc->num_ct; i++, ct++) > if (ct->type & flow_type) > ctrl_off = ct->regs.type; Acked-by: Carlo Caione -- Carlo Caione -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: sunxi-nmi: Fix off-by-one for iterating over gc-num_ct
On Sun, Jun 7, 2015 at 3:33 PM, Axel Lin axel@ingics.com wrote: Signed-off-by: Axel Lin axel@ingics.com --- drivers/irqchip/irq-sunxi-nmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c index 12f547a..eb9b59e 100644 --- a/drivers/irqchip/irq-sunxi-nmi.c +++ b/drivers/irqchip/irq-sunxi-nmi.c @@ -104,7 +104,7 @@ static int sunxi_sc_nmi_set_type(struct irq_data *data, unsigned int flow_type) irqd_set_trigger_type(data, flow_type); irq_setup_alt_chip(data, flow_type); - for (i = 0; i = gc-num_ct; i++, ct++) + for (i = 0; i gc-num_ct; i++, ct++) if (ct-type flow_type) ctrl_off = ct-regs.type; Acked-by: Carlo Caione ca...@endlessm.com -- Carlo Caione -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] media: rc: add driver for Amlogic Meson IR remote receiver
On Wed, Nov 26, 2014 at 4:04 PM, Mauro Carvalho Chehab wrote: > Em Tue, 18 Nov 2014 21:22:34 +0100 > Beniamino Galvani escreveu: > >> Amlogic Meson SoCs include a infrared remote control receiver that can >> operate in two modes: "NEC" mode in which the hardware decodes frames >> using the NEC IR protocol, and "general" mode in which the receiver >> simply reports the duration of pulses and spaces for software >> decoding. >> >> This is a driver for the IR receiver that implements software decoding >> of received frames. >> >> Signed-off-by: Beniamino Galvani >> --- >> MAINTAINERS | 1 + >> drivers/media/rc/Kconfig| 11 +++ >> drivers/media/rc/Makefile | 1 + >> drivers/media/rc/meson-ir.c | 216 >> >> 4 files changed, 229 insertions(+) >> create mode 100644 drivers/media/rc/meson-ir.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 0662378..f1bc045 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -850,6 +850,7 @@ ARM/Amlogic MesonX SoC support >> M: Carlo Caione >> L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) >> S: Maintained >> +F: drivers/media/rc/meson-ir.c >> N: meson[x68] > > Hmm... you're putting this driver at Carlo's maintenance shoulders. > > I need his ack in order to apply this patch. Acked-by: Carlo Caione -- Carlo Caione -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] media: rc: add driver for Amlogic Meson IR remote receiver
On Wed, Nov 26, 2014 at 4:04 PM, Mauro Carvalho Chehab mche...@osg.samsung.com wrote: Em Tue, 18 Nov 2014 21:22:34 +0100 Beniamino Galvani b.galv...@gmail.com escreveu: Amlogic Meson SoCs include a infrared remote control receiver that can operate in two modes: NEC mode in which the hardware decodes frames using the NEC IR protocol, and general mode in which the receiver simply reports the duration of pulses and spaces for software decoding. This is a driver for the IR receiver that implements software decoding of received frames. Signed-off-by: Beniamino Galvani b.galv...@gmail.com --- MAINTAINERS | 1 + drivers/media/rc/Kconfig| 11 +++ drivers/media/rc/Makefile | 1 + drivers/media/rc/meson-ir.c | 216 4 files changed, 229 insertions(+) create mode 100644 drivers/media/rc/meson-ir.c diff --git a/MAINTAINERS b/MAINTAINERS index 0662378..f1bc045 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -850,6 +850,7 @@ ARM/Amlogic MesonX SoC support M: Carlo Caione ca...@caione.org L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) S: Maintained +F: drivers/media/rc/meson-ir.c N: meson[x68] Hmm... you're putting this driver at Carlo's maintenance shoulders. I need his ack in order to apply this patch. Acked-by: Carlo Caione ca...@caione.org -- Carlo Caione -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: dts: meson: enable Ethernet controller
On Sun, Nov 23, 2014 at 5:08 PM, Beniamino Galvani wrote:
> Add a node for the Ethernet controller to Meson DTS file and
> enable it on the Geniatech ATV1200 board.
>
> Signed-off-by: Beniamino Galvani
> ---
> arch/arm/boot/dts/meson.dtsi | 11 +++
> arch/arm/boot/dts/meson6-atv1200.dts | 4
> 2 files changed, 15 insertions(+)
>
> diff --git a/arch/arm/boot/dts/meson.dtsi b/arch/arm/boot/dts/meson.dtsi
> index 03bcff8..4704b95 100644
> --- a/arch/arm/boot/dts/meson.dtsi
> +++ b/arch/arm/boot/dts/meson.dtsi
> @@ -143,5 +143,16 @@
> #size-cells = <0>;
> status = "disabled";
> };
> +
> + ethmac: ethernet@c941 {
> + compatible = "amlogic,meson6-dwmac", "snps,dwmac";
> + reg = <0xc941 0x1
> + 0xc1108108 0x4>;
> + interrupts = <0 8 1>;
> + interrupt-names = "macirq";
> + clocks = <>;
> + clock-names = "stmmaceth";
> + status = "disabled";
> + };
> };
> }; /* end of / */
> diff --git a/arch/arm/boot/dts/meson6-atv1200.dts
> b/arch/arm/boot/dts/meson6-atv1200.dts
> index d7d351a..1237faa 100644
> --- a/arch/arm/boot/dts/meson6-atv1200.dts
> +++ b/arch/arm/boot/dts/meson6-atv1200.dts
> @@ -64,3 +64,7 @@
> _AO {
> status = "okay";
> };
> +
> + {
> + status = "okay";
> +};
> --
> 1.9.1
>
Tested-by: Carlo Caione
Thanks,
--
Carlo Caione
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: dts: meson: enable Ethernet controller
On Sun, Nov 23, 2014 at 5:08 PM, Beniamino Galvani b.galv...@gmail.com wrote:
Add a node for the Ethernet controller to Meson DTS file and
enable it on the Geniatech ATV1200 board.
Signed-off-by: Beniamino Galvani b.galv...@gmail.com
---
arch/arm/boot/dts/meson.dtsi | 11 +++
arch/arm/boot/dts/meson6-atv1200.dts | 4
2 files changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/meson.dtsi b/arch/arm/boot/dts/meson.dtsi
index 03bcff8..4704b95 100644
--- a/arch/arm/boot/dts/meson.dtsi
+++ b/arch/arm/boot/dts/meson.dtsi
@@ -143,5 +143,16 @@
#size-cells = 0;
status = disabled;
};
+
+ ethmac: ethernet@c941 {
+ compatible = amlogic,meson6-dwmac, snps,dwmac;
+ reg = 0xc941 0x1
+ 0xc1108108 0x4;
+ interrupts = 0 8 1;
+ interrupt-names = macirq;
+ clocks = clk81;
+ clock-names = stmmaceth;
+ status = disabled;
+ };
};
}; /* end of / */
diff --git a/arch/arm/boot/dts/meson6-atv1200.dts
b/arch/arm/boot/dts/meson6-atv1200.dts
index d7d351a..1237faa 100644
--- a/arch/arm/boot/dts/meson6-atv1200.dts
+++ b/arch/arm/boot/dts/meson6-atv1200.dts
@@ -64,3 +64,7 @@
uart_AO {
status = okay;
};
+
+ethmac {
+ status = okay;
+};
--
1.9.1
Tested-by: Carlo Caione ca...@caione.org
Thanks,
--
Carlo Caione
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
