Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1604512 > unrolled thread

[PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling Michał Kępień <kernel@kempniu.pl> - 2017-03-20 13:50 +0100
    [PATCH 4/8] platform/x86: fujitsu-laptop: move hotkey input device setup to a separate function Michał Kępień <kernel@kempniu.pl> - 2017-03-20 13:50 +0100
      Re: [PATCH 4/8] platform/x86: fujitsu-laptop: move hotkey input  device setup to a separate function Darren Hart <dvhart@infradead.org> - 2017-03-29 22:20 +0200
    Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Jonathan Woithe <jwoithe@just42.net> - 2017-03-24 12:00 +0100
      Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Jonathan Woithe <jwoithe@just42.net> - 2017-03-28 02:00 +0200
        Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Michał Kępień <kernel@kempniu.pl> - 2017-03-28 08:20 +0200
          Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Jonathan Woithe <jwoithe@just42.net> - 2017-03-29 01:10 +0200
            Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Michał Kępień <kernel@kempniu.pl> - 2017-03-29 09:20 +0200
              Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-29 18:40 +0200
                Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Darren Hart <dvhart@infradead.org> - 2017-03-30 05:40 +0200
                  Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Jonathan Woithe <jwoithe@just42.net> - 2017-03-30 06:00 +0200
                    Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Darren Hart <dvhart@infradead.org> - 2017-03-30 07:10 +0200
                      Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Michał Kępień <kernel@kempniu.pl> - 2017-03-30 08:50 +0200
                        Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Jonathan Woithe <jwoithe@just42.net> - 2017-03-31 00:30 +0200
            Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event  handling Darren Hart <dvhart@infradead.org> - 2017-03-29 21:40 +0200

#1604512 — [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

This series simplifies handling of both brightness key and hotkey input
events on Fujitsu laptops by making use of sparse keymaps.  This not
only makes the driver shorter and, hopefully, cleaner, but also enables
us to get rid of the keycodeX fields inside struct fujitsu_bl, which
facilitates further cleanups.  Also, to simplify error handling, input
devices registered by fujitsu-laptop are migrated to the devres API
along the way.

This series was tested on a Lifebook S7020 and a Lifebook E744.

This series depends on the platform cleanup series I posted last week.
While that series has not yet been merged into testing, Jonathan has
reviewed it and Darren also seemed to be okay with it, so I just assumed
it will get merged soon.  I wanted to post this one as soon as possible
as it requires a bit more thorough review and testing compared to the
previous series I posted for fujitsu-laptop.

 drivers/platform/x86/Kconfig          |   1 +
 drivers/platform/x86/fujitsu-laptop.c | 355 +++++++++++++++-------------------
 2 files changed, 154 insertions(+), 202 deletions(-)

-- 
2.12.0

[toc] | [next] | [standalone]

#1604513 — [PATCH 4/8] platform/x86: fujitsu-laptop: move hotkey input device setup to a separate function

Simplify error handling in acpi_fujitsu_laptop_add() by moving code
responsible for setting up the input device to a separate function.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 74 ++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 31 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3483ac37bee5..b1a08d83330b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -756,6 +756,46 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 
 /* ACPI device for hotkey handling */
 
+static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
+{
+	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+	struct input_dev *input;
+	int error;
+
+	fujitsu_laptop->input = input = input_allocate_device();
+	if (!input)
+		return -ENOMEM;
+
+	snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
+		 "%s/video/input0", acpi_device_hid(device));
+
+	input->name = acpi_device_name(device);
+	input->phys = fujitsu_laptop->phys;
+	input->id.bustype = BUS_HOST;
+	input->id.product = 0x06;
+	input->dev.parent = &device->dev;
+
+	set_bit(EV_KEY, input->evbit);
+	set_bit(fujitsu_bl->keycode1, input->keybit);
+	set_bit(fujitsu_bl->keycode2, input->keybit);
+	set_bit(fujitsu_bl->keycode3, input->keybit);
+	set_bit(fujitsu_bl->keycode4, input->keybit);
+	set_bit(fujitsu_bl->keycode5, input->keybit);
+	set_bit(KEY_TOUCHPAD_TOGGLE, input->keybit);
+	set_bit(KEY_UNKNOWN, input->keybit);
+
+	error = input_register_device(input);
+	if (error)
+		goto err_free_input_dev;
+
+	return 0;
+
+err_free_input_dev:
+	input_free_device(input);
+
+	return error;
+}
+
 static int fujitsu_laptop_platform_add(void)
 {
 	int ret;
@@ -794,7 +834,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 {
 	int result = 0;
 	int state = 0;
-	struct input_dev *input;
 	int error;
 	int i;
 
@@ -816,33 +855,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 		goto err_stop;
 	}
 
-	fujitsu_laptop->input = input = input_allocate_device();
-	if (!input) {
-		error = -ENOMEM;
-		goto err_free_fifo;
-	}
-
-	snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
-		 "%s/video/input0", acpi_device_hid(device));
-
-	input->name = acpi_device_name(device);
-	input->phys = fujitsu_laptop->phys;
-	input->id.bustype = BUS_HOST;
-	input->id.product = 0x06;
-	input->dev.parent = &device->dev;
-
-	set_bit(EV_KEY, input->evbit);
-	set_bit(fujitsu_bl->keycode1, input->keybit);
-	set_bit(fujitsu_bl->keycode2, input->keybit);
-	set_bit(fujitsu_bl->keycode3, input->keybit);
-	set_bit(fujitsu_bl->keycode4, input->keybit);
-	set_bit(fujitsu_bl->keycode5, input->keybit);
-	set_bit(KEY_TOUCHPAD_TOGGLE, input->keybit);
-	set_bit(KEY_UNKNOWN, input->keybit);
-
-	error = input_register_device(input);
+	error = acpi_fujitsu_laptop_input_setup(device);
 	if (error)
-		goto err_free_input_dev;
+		goto err_free_fifo;
 
 	error = acpi_bus_update_power(fujitsu_laptop->acpi_handle, &state);
 	if (error) {
@@ -960,10 +975,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	return result;
 
 err_unregister_input_dev:
-	input_unregister_device(input);
-	input = NULL;
-err_free_input_dev:
-	input_free_device(input);
+	input_unregister_device(fujitsu_laptop->input);
 err_free_fifo:
 	kfifo_free(&fujitsu_laptop->fifo);
 err_stop:
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1612292 — Re: [PATCH 4/8] platform/x86: fujitsu-laptop: move hotkey input device setup to a separate function

On Mon, Mar 20, 2017 at 10:32:20AM +0100, Michał Kępień wrote:
> Simplify error handling in acpi_fujitsu_laptop_add() by moving code
> responsible for setting up the input device to a separate function.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 74 ++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 3483ac37bee5..b1a08d83330b 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c



> @@ -794,7 +834,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
>  {
>  	int result = 0;
>  	int state = 0;
> -	struct input_dev *input;
>  	int error;

This patch highlights the odd error handling / return path with the odd mix of
result and error. Not introduced here, but something for a future cleanup
perhaps.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1608310 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> This series simplifies handling of both brightness key and hotkey input
> events on Fujitsu laptops by making use of sparse keymaps.  This not
> only makes the driver shorter and, hopefully, cleaner, but also enables
> us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> facilitates further cleanups.  Also, to simplify error handling, input
> devices registered by fujitsu-laptop are migrated to the devres API
> along the way.
> 
> This series was tested on a Lifebook S7020 and a Lifebook E744.
> 
> This series depends on the platform cleanup series I posted last week.
> While that series has not yet been merged into testing, Jonathan has
> reviewed it and Darren also seemed to be okay with it, so I just assumed
> it will get merged soon.  I wanted to post this one as soon as possible
> as it requires a bit more thorough review and testing compared to the
> previous series I posted for fujitsu-laptop.

Thanks for posting this.  I have started going through this and hope to
complete my review by the end of this weekend.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1610172 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > This series simplifies handling of both brightness key and hotkey input
> > events on Fujitsu laptops by making use of sparse keymaps.  This not
> > only makes the driver shorter and, hopefully, cleaner, but also enables
> > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > facilitates further cleanups.  Also, to simplify error handling, input
> > devices registered by fujitsu-laptop are migrated to the devres API
> > along the way.
> > 
> > This series was tested on a Lifebook S7020 and a Lifebook E744.
> > 
> > This series depends on the platform cleanup series I posted last week.
> > While that series has not yet been merged into testing, Jonathan has
> > reviewed it and Darren also seemed to be okay with it, so I just assumed
> > it will get merged soon.  I wanted to post this one as soon as possible
> > as it requires a bit more thorough review and testing compared to the
> > previous series I posted for fujitsu-laptop.
> 
> Thanks for posting this.  I have started going through this and hope to
> complete my review by the end of this weekend.

I have completed my initial review of this patch series.  Aside from the
single recommendation about patch 7/8 (posted separately) it looks good.
I await your thoughts regarding patch 7/8 so we can finalise and sign off on
this series.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1610353 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

> On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > This series simplifies handling of both brightness key and hotkey input
> > > events on Fujitsu laptops by making use of sparse keymaps.  This not
> > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > facilitates further cleanups.  Also, to simplify error handling, input
> > > devices registered by fujitsu-laptop are migrated to the devres API
> > > along the way.
> > > 
> > > This series was tested on a Lifebook S7020 and a Lifebook E744.
> > > 
> > > This series depends on the platform cleanup series I posted last week.
> > > While that series has not yet been merged into testing, Jonathan has
> > > reviewed it and Darren also seemed to be okay with it, so I just assumed
> > > it will get merged soon.  I wanted to post this one as soon as possible
> > > as it requires a bit more thorough review and testing compared to the
> > > previous series I posted for fujitsu-laptop.
> > 
> > Thanks for posting this.  I have started going through this and hope to
> > complete my review by the end of this weekend.
> 
> I have completed my initial review of this patch series.  Aside from the
> single recommendation about patch 7/8 (posted separately) it looks good.
> I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> this series.

Thanks for the review, Jonathan.  I agree with your remark regarding the
potentially confusing name of the variable holding the S64x0 keymap.

As in the past, we have at least two options: I can either post v2 of
all eight patches with three characters changed or you can provide your
Reviewed-by for v1, in which case I will kindly ask the maintainers to
run:

    sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c

after applying patch 7/8.  Given that this series does a bit more than
the cleanup series I posted previously, I sense it might be a good idea
to defer submitting v2 until after subsystem maintainers review v1, just
in case they find more issues.  If that happens, I will post v2 to avoid
confusion.  If not, then v1 can be applied with the one-liner above
taken into account (or I can post v2 anyway if that would be preferred
by Darren and Andy).

In other words, I am happy to follow whatever route you and the
subsystem maintainers suggest and I just want to avoid spamming the
mailing list.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1611429 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

On Tue, Mar 28, 2017 at 08:16:18AM +0200, Micha?? K??pie?? wrote:
> > On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > > This series simplifies handling of both brightness key and hotkey input
> > > > events on Fujitsu laptops by making use of sparse keymaps.  This not
> > > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > > facilitates further cleanups.  Also, to simplify error handling, input
> > > > devices registered by fujitsu-laptop are migrated to the devres API
> > > > along the way.
> > > > :
> > I have completed my initial review of this patch series.  Aside from the
> > single recommendation about patch 7/8 (posted separately) it looks good.
> > I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> > this series.
> 
> Thanks for the review, Jonathan.  I agree with your remark regarding the
> potentially confusing name of the variable holding the S64x0 keymap.
> 
> As in the past, we have at least two options: I can either post v2 of
> all eight patches with three characters changed or you can provide your
> Reviewed-by for v1, in which case I will kindly ask the maintainers to
> run:
> 
>     sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c
> 
> after applying patch 7/8.  Given that this series does a bit more than
> the cleanup series I posted previously, I sense it might be a good idea
> to defer submitting v2 until after subsystem maintainers review v1, just
> in case they find more issues.  If that happens, I will post v2 to avoid
> confusion.  If not, then v1 can be applied with the one-liner above
> taken into account (or I can post v2 anyway if that would be preferred
> by Darren and Andy).
> 
> In other words, I am happy to follow whatever route you and the
> subsystem maintainers suggest and I just want to avoid spamming the
> mailing list.

I would be interested to hear what Darren and Andy's preference would be,
and have no issue with following that.  My personal thought is that it's
best to have a patch series v2 submitted with the keymap fix since I think
that reduces the potential confusion now and in the future.  It makes it
clear what exactly is being signed off on in any Reviewed-By tags.  This
approach also removes the need for Darren or Andy to special case the
eventual merge of the series (having to remember to do the replacement),
which in turn reduces the chances of little errors creeping in.  However, if
Darren and Andy are happy with an alternative then we can go with that.

As to whether v2 is held until Darren or Andy do their own review, I guess
it makes sense in case they have other suggestions which are similarly
trivial to implement.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1611637 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

> On Tue, Mar 28, 2017 at 08:16:18AM +0200, Micha?? K??pie?? wrote:
> > > On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > > > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > > > This series simplifies handling of both brightness key and hotkey input
> > > > > events on Fujitsu laptops by making use of sparse keymaps.  This not
> > > > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > > > facilitates further cleanups.  Also, to simplify error handling, input
> > > > > devices registered by fujitsu-laptop are migrated to the devres API
> > > > > along the way.
> > > > > :
> > > I have completed my initial review of this patch series.  Aside from the
> > > single recommendation about patch 7/8 (posted separately) it looks good.
> > > I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> > > this series.
> > 
> > Thanks for the review, Jonathan.  I agree with your remark regarding the
> > potentially confusing name of the variable holding the S64x0 keymap.
> > 
> > As in the past, we have at least two options: I can either post v2 of
> > all eight patches with three characters changed or you can provide your
> > Reviewed-by for v1, in which case I will kindly ask the maintainers to
> > run:
> > 
> >     sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c
> > 
> > after applying patch 7/8.  Given that this series does a bit more than
> > the cleanup series I posted previously, I sense it might be a good idea
> > to defer submitting v2 until after subsystem maintainers review v1, just
> > in case they find more issues.  If that happens, I will post v2 to avoid
> > confusion.  If not, then v1 can be applied with the one-liner above
> > taken into account (or I can post v2 anyway if that would be preferred
> > by Darren and Andy).
> > 
> > In other words, I am happy to follow whatever route you and the
> > subsystem maintainers suggest and I just want to avoid spamming the
> > mailing list.
> 
> I would be interested to hear what Darren and Andy's preference would be,
> and have no issue with following that.  My personal thought is that it's
> best to have a patch series v2 submitted with the keymap fix since I think
> that reduces the potential confusion now and in the future.  It makes it
> clear what exactly is being signed off on in any Reviewed-By tags.  This
> approach also removes the need for Darren or Andy to special case the
> eventual merge of the series (having to remember to do the replacement),
> which in turn reduces the chances of little errors creeping in.  However, if
> Darren and Andy are happy with an alternative then we can go with that.

Agreed.

> As to whether v2 is held until Darren or Andy do their own review, I guess
> it makes sense in case they have other suggestions which are similarly
> trivial to implement.

Darren, Andy, in light of the above I will be awaiting your review of
this series.  I will submit v2 afterwards, with all remarks from both
you and Jonathan taken into account.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1612143

On Wed, Mar 29, 2017 at 10:19 AM, Michał Kępień <kernel@kempniu.pl> wrote:

> Darren, Andy, in light of the above I will be awaiting your review of
> this series.  I will submit v2 afterwards, with all remarks from both
> you and Jonathan taken into account.

Darren marked this series under his name to review, so, I let him to
speak for us.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1612537 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> On Wed, Mar 29, 2017 at 10:19 AM, Michał Kępień <kernel@kempniu.pl> wrote:
> 
> > Darren, Andy, in light of the above I will be awaiting your review of
> > this series.  I will submit v2 afterwards, with all remarks from both
> > you and Jonathan taken into account.
> 
> Darren marked this series under his name to review, so, I let him to
> speak for us.

The series looks good to me. Nice work Michał. They are logically divided and
address issues in a procedural way (so I stopped commenting until I read the
full series through as a couple of times you addressed a concern from a move in
a cleanup to follow).

I've applied the noted change to 7/8 and will run this through my tests, but
don't anticipate any problems. Jonathan, if you don't have any additional
concerns, let me know if I can add your Reviewed-by.

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1612543 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

On Wed, Mar 29, 2017 at 08:36:50PM -0700, Darren Hart wrote:
> On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> > On Wed, Mar 29, 2017 at 10:19 AM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
> > 
> > > Darren, Andy, in light of the above I will be awaiting your review of
> > > this series.  I will submit v2 afterwards, with all remarks from both
> > > you and Jonathan taken into account.
> > 
> > Darren marked this series under his name to review, so, I let him to
> > speak for us.
> 
> The series looks good to me. Nice work Micha??. They are logically divided and
> address issues in a procedural way (so I stopped commenting until I read the
> full series through as a couple of times you addressed a concern from a move in
> a cleanup to follow).
> 
> I've applied the noted change to 7/8 and will run this through my tests, but
> don't anticipate any problems. Jonathan, if you don't have any additional
> concerns, let me know if I can add your Reviewed-by.

With the noted change to 7/8 applied I'm happy with the resulting series. 
As you noted, there is still some scope for making things more consistent,
especially with regard to error handling.  However, that is really a
separate task which can be addressed in a later series.  This present series
doesn't impact on this issue in any significant way so it makes sense that
be applied.

Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1612562 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

On Thu, Mar 30, 2017 at 02:26:26PM +1030, Jonathan Woithe wrote:
> On Wed, Mar 29, 2017 at 08:36:50PM -0700, Darren Hart wrote:
> > On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> > > On Wed, Mar 29, 2017 at 10:19 AM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
> > > 
> > > > Darren, Andy, in light of the above I will be awaiting your review of
> > > > this series.  I will submit v2 afterwards, with all remarks from both
> > > > you and Jonathan taken into account.
> > > 
> > > Darren marked this series under his name to review, so, I let him to
> > > speak for us.
> > 
> > The series looks good to me. Nice work Micha??. They are logically divided and
> > address issues in a procedural way (so I stopped commenting until I read the
> > full series through as a couple of times you addressed a concern from a move in
> > a cleanup to follow).
> > 
> > I've applied the noted change to 7/8 and will run this through my tests, but
> > don't anticipate any problems. Jonathan, if you don't have any additional
> > concerns, let me know if I can add your Reviewed-by.
> 
> With the noted change to 7/8 applied I'm happy with the resulting series. 
> As you noted, there is still some scope for making things more consistent,
> especially with regard to error handling.  However, that is really a
> separate task which can be addressed in a later series.  This present series
> doesn't impact on this issue in any significant way so it makes sense that
> be applied.
> 
> Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

Merged, thanks!

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1612604 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

> On Thu, Mar 30, 2017 at 02:26:26PM +1030, Jonathan Woithe wrote:
> > On Wed, Mar 29, 2017 at 08:36:50PM -0700, Darren Hart wrote:
> > > On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Mar 29, 2017 at 10:19 AM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
> > > > 
> > > > > Darren, Andy, in light of the above I will be awaiting your review of
> > > > > this series.  I will submit v2 afterwards, with all remarks from both
> > > > > you and Jonathan taken into account.
> > > > 
> > > > Darren marked this series under his name to review, so, I let him to
> > > > speak for us.
> > > 
> > > The series looks good to me. Nice work Micha??. They are logically divided and
> > > address issues in a procedural way (so I stopped commenting until I read the
> > > full series through as a couple of times you addressed a concern from a move in
> > > a cleanup to follow).
> > > 
> > > I've applied the noted change to 7/8 and will run this through my tests, but
> > > don't anticipate any problems. Jonathan, if you don't have any additional
> > > concerns, let me know if I can add your Reviewed-by.
> > 
> > With the noted change to 7/8 applied I'm happy with the resulting series. 
> > As you noted, there is still some scope for making things more consistent,
> > especially with regard to error handling.  However, that is really a
> > separate task which can be addressed in a later series.  This present series
> > doesn't impact on this issue in any significant way so it makes sense that
> > be applied.
> > 
> > Reviewed-by: Jonathan Woithe <jwoithe@just42.net>
> 
> Merged, thanks!

Thanks to everyone involved.  I noted your concerns about error handling
in the driver, rest assured that I share them.  I have a local git
branch with miscellaneous fixes that I plan to submit (as one or more
series) in the future.  I think error handling improvements fall right
into that "miscellaneous" category of fixes.  For the time being I would
rather concentrate on the issues I personally consider more pressing as
I will most likely lose access to modern Fujitsu hardware in about six
weeks and I would really like to be able to test my patches on hardware
on which the code touched by these patches actually runs (LEDs come to
mind, specifically).

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1613525 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

Hi Michael

On Thu, Mar 30, 2017 at 08:41:15AM +0200, Micha?? K??pie?? wrote:
> I noted your concerns about error handling in the driver ...  For the time
> being I would rather concentrate on the issues I personally consider more
> pressing as I will most likely lose access to modern Fujitsu hardware in
> about six weeks and I would really like to be able to test my patches on
> hardware on which the code touched by these patches actually runs (LEDs
> come to mind, specifically).

I have no problem with this.  Fixing consistency in connection with error
handling is certainly something which can be done by me (or anyone else)
at a later date.  I agree that the other work is more important.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1612273 — Re: [PATCH 0/8] fujitsu-laptop: use sparse keymaps for input event handling

On Wed, Mar 29, 2017 at 09:30:29AM +1030, Jonathan Woithe wrote:
> On Tue, Mar 28, 2017 at 08:16:18AM +0200, Micha?? K??pie?? wrote:
> > > On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > > > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > > > This series simplifies handling of both brightness key and hotkey input
> > > > > events on Fujitsu laptops by making use of sparse keymaps.  This not
> > > > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > > > facilitates further cleanups.  Also, to simplify error handling, input
> > > > > devices registered by fujitsu-laptop are migrated to the devres API
> > > > > along the way.
> > > > > :
> > > I have completed my initial review of this patch series.  Aside from the
> > > single recommendation about patch 7/8 (posted separately) it looks good.
> > > I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> > > this series.
> > 
> > Thanks for the review, Jonathan.  I agree with your remark regarding the
> > potentially confusing name of the variable holding the S64x0 keymap.
> > 
> > As in the past, we have at least two options: I can either post v2 of
> > all eight patches with three characters changed or you can provide your
> > Reviewed-by for v1, in which case I will kindly ask the maintainers to
> > run:
> > 
> >     sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c
> > 
> > after applying patch 7/8.  Given that this series does a bit more than
> > the cleanup series I posted previously, I sense it might be a good idea
> > to defer submitting v2 until after subsystem maintainers review v1, just
> > in case they find more issues.  If that happens, I will post v2 to avoid
> > confusion.  If not, then v1 can be applied with the one-liner above
> > taken into account (or I can post v2 anyway if that would be preferred
> > by Darren and Andy).
> > 
> > In other words, I am happy to follow whatever route you and the
> > subsystem maintainers suggest and I just want to avoid spamming the
> > mailing list.
> 
> I would be interested to hear what Darren and Andy's preference would be,
> and have no issue with following that.  My personal thought is that it's
> best to have a patch series v2 submitted with the keymap fix since I think
> that reduces the potential confusion now and in the future.  It makes it
> clear what exactly is being signed off on in any Reviewed-By tags.  This

No concerns applying the minor fix. It's very common to say "With XXX,
Reviewed-by: ...", so I that's fine.

> approach also removes the need for Darren or Andy to special case the
> eventual merge of the series (having to remember to do the replacement),
> which in turn reduces the chances of little errors creeping in.  However, if
> Darren and Andy are happy with an alternative then we can go with that.
> 
> As to whether v2 is held until Darren or Andy do their own review, I guess
> it makes sense in case they have other suggestions which are similarly
> trivial to implement.

Completing my review now...

> 
> Regards
>   jonathan
> 

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web