Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups

From	Michał Kępień <kernel@kempniu.pl>
Newsgroups	linux.kernel
Subject	Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups
Date	2017-02-09 08:40 +0100
Message-ID	<t8RuG-3aL-9@gated-at.bofh.it> (permalink)
References	<t8Bgd-1nA-5@gated-at.bofh.it> <t8Jx8-6AK-19@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

> Hi Michael
> 
> Thanks very much for the work you've put in to clean up these patches.  I
> very much appreciate it.  I will go through them myself in the next day or
> so, and most importantly test them on my hardware to confirm there are no
> regressions.

Thanks!  I did my best to keep track of everything, but something surely
could have slipped through, given the number of changes Alan's patches
introduce.

> Some initial comments follow.
> 
> On Wed, Feb 08, 2017 at 02:46:23PM +0100, Micha?? K??pie?? wrote:
> > Some of these patches raise checkpatch warnings.  This is intentional,
> > in order to make renames clean and easy to review.
> 
> For what it's worth, I was also planning to go through and fix the
> checkpatch warnings in this driver, but held off until the issues raised by
> Alan's patches were addressed (essentially to make it easier to apply those
> old patches.  If you are planning on doing this then I won't start it. :-)

I have a few draft patch series prepared that contain "real" cleanups
and incidentally also lower the total count of checkpatch issues in the
driver from ~70 to ~20 (ballpark figures).  I think it is way better to
fix as many checkpatch issues as possible while also fixing actual code
and then iron out the leftovers than to first generate a lot of churn
which fixes spaces and newlines and then fix the code anyway.  So yeah,
I would hold off pure checkpatch fixes until the more pressing matters
get sorted out.

> 
> On to the ommitted changes from Alan's orginal patch series.
> 
> >   - Restructure fujitsu_init().  Quite frankly, I am pretty sure Alan
> >     did not test his series on Fujitsu hardware with a logo lamp and/or
> >     keyboard lamps.
> 
> Back in 2009 I don't think there were many models with these features.  This
> observation is therefore quite possible.  I'm more than happy to leave
> fujitsu_init()'s structure as is, especially given that related issues will
> be addressed in your furture patch series.

Yes, they will.  How these fixes will exactly look depends on the
feedback I get from Darren and Andy.

> 
> >   - Bail out when FUJ02B1 is not present.  It could have been deemed
> >     correct back in the day, but we now know that Fujitsu started
> >     shipping devices without that ACPI device present, though with
> >     FUJ02E3 still in place.  These two ACPI devices are independent and
> >     thus should not rely on each other's presence.
> 
> Agreed.  You are right, in 2009 I think it seemed like a good idea but it's
> clearly no longer valid.
> 
> >   - Move keycode[1-5] fields to struct fujitsu_laptop.  Doing this
> >     causes ordering issues inside fujitsu_init(), while a patch series I
> >     have queued that makes fujitsu-laptop use a sparse keymap removes
> >     these fields altogether.
> 
> This makes sense.
> 
> >   - Allocate and free fujitsu_bl and fujitsu_laptop inside ACPI
> >     callbacks.  While elegant and correct, this also causes ordering
> >     issues inside fujitsu_init() and can only be done once platform
> >     device registration is properly fixed.
> 
> Agreed.
> 
> >   - Sync backlight power status in acpi_fujitsu_bl_add().  The long-term
> >     objective for fujitsu-laptop should be to achieve a clean split
> >     between the backlight-related part and the laptop-related part.
> >     This change keeps both parts intertwined.  My fujitsu_init() cleanup
> >     series contains a similar fix, but I have since found a different
> >     solution to this problem which I will post once Alan's rebased
> >     series gets applied.
> 
> I'm happy to wait to see this alternative solution.  In the meantime,
> keeping the two parts independent is a good idea.
> 
> >   - Remove dmi_check_cb_common().  The sparse keymap series I have
> >     queued gets rid of this function without introducing an additional
> >     field inside struct fujitsu_laptop.
> 
> Ok.
> 
> > Moreover, some other minor changes present in original patch 1/4 were
> > left out.  A brief discussion of each such case follows.
> > 
> > -#define ACPI_FUJITSU_BL_NOTIFY_CODE1    0x80
> > +#define ACPI_FUJITSU_NOTIFY_CODE1     0x80
> > 
> > Notify code 0x80 is used by both parts of the driver (the
> > brightness-related one and the laptop-related one) and thus it should
> > not be annotated using the "_BL" infix specific to brightness-related
> > code.
> 
> Agreed.  Perhaps back in the day it was thought that it did only apply to
> the backlight but with the benefit of hindsight this is clearly not the
> case.
> 
> > -#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
> > ...
> > -#endif
> > 
> > This does not really bring any benefit while breaking consistency with
> > other parts of the driver.
> 
> Fair enough.
> 
> > 
> > -       dmi_check_system(fujitsu_laptop_dmi_table);
> > +       dmi_check_system(fujitsu_dmi_table);
> > 
> > The original patches moved this dmi_check_system() call to
> > acpi_fujitsu_laptop_add(), which justifies renaming fujitsu_dmi_table to
> > fujitsu_laptop_dmi_table.  However, for reasons discussed above, the
> > rebased patches leave the dmi_check_system() call inside fujitsu_init(),
> > thus making the rename dubious.
> 
> Agreed.  In light of the reasons given this makes no sense.
> 
> > -                    DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
> > +                    DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU_LAPTOP SIEMENS"),
> > 
> > There are multiple changes like this in original patch 1/4.  They are
> > obviously wrong and were probably introduced by an unreviewed automatic
> > replacement.
> 
> I noticed this myself; they are clearly incorrect and have the hallmarks of
> a global search/replace operation.  They were omitted in the initial RFC
> series I sent through for this reason.
> 
> [Blank line removal]
> > There are four instances of such a removal in original patch 1/4.  They
> > are obviously correct, though they all happen inside functions related
> > to platform device attributes which I hope will soon get removed
> > altogether.  Thus I refrained from applying these to reduce churn (at
> > least a bit).
> 
> I'm happy with that.  The future checkpatch cleanup series could address
> these if they are deemed important enough.

Yes, I have this specific cleanup queued in a branch containing
miscellaneous fixes anyway, so it should not get lost in the noise.

> 
> > Finally, some of the changes suggested by Alan were already applied
> > along the way:
> > 
> >   - kmalloc() + memset() occurences were changed to kzalloc() by commit
> >     6c75dd0f965b ("drivers/platform/x86: Use kzalloc").
> > 
> >   - Unused debug macros were removed by commit 00816e1b3839
> >     ("fujitsu-laptop: Remove unused macros").
> 
> The entire debug structure has been revised since Alan's original patch was
> made, making most if not all of the debug changes unnecessary.
> 
> My ack will follow once I've tested this series on my hardware.  I will try
> to get to this in the next 24 hours.

Great, thanks!

-- 
Best regards,
Michał Kępień

Thread

[PATCH 00/10] fujitsu-laptop: renames and cleanups Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:30 +0100
  [PATCH 01/10] platform/x86: fujitsu-laptop: clearly denote backlight-related symbols Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:30 +0100
  [PATCH 07/10] platform/x86: fujitsu-laptop: simplify acpi_bus_register_driver() error handling Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:40 +0100
  [PATCH 06/10] platform/x86: fujitsu-laptop: remove redundant forward declarations Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:50 +0100
  Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-08 16:30 +0100
    Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Darren Hart <dvhart@infradead.org> - 2017-02-09 02:40 +0100
      Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-09 02:50 +0100
    Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Michał Kępień <kernel@kempniu.pl> - 2017-02-09 08:00 +0100
  [PATCH 10/10] platform/x86: fujitsu-laptop: remove redundant MODULE_ALIAS entries Michał Kępień <kernel@kempniu.pl> - 2017-02-08 16:50 +0100
  [PATCH 04/10] platform/x86: fujitsu-laptop: rename FUNC_RFKILL to FUNC_FLAGS Michał Kępień <kernel@kempniu.pl> - 2017-02-08 17:40 +0100
  [PATCH 02/10] platform/x86: fujitsu-laptop: replace "hotkey" with "laptop" in symbol names Michał Kępień <kernel@kempniu.pl> - 2017-02-08 20:10 +0100
  Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Jonathan Woithe <jwoithe@just42.net> - 2017-02-09 00:10 +0100
    Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Michał Kępień <kernel@kempniu.pl> - 2017-02-09 08:40 +0100
  Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Jonathan Woithe <jwoithe@just42.net> - 2017-02-10 01:20 +0100
    Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-10 01:50 +0100

csiph-web