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

Groups > linux.kernel > #1639462

Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals

Show all headers | View raw

> Perhaps so (overly zealous). Regarding the globals, let's be clear on the
> motivation. We want to follow good sw engineering practice, use data
> encapsulation, etc. However, using an explicit path to an ACPI device to avoid
> having a static file-level global doesn't really improve encapsulation in any
> way - it just shifts the blame :-)

Indeed, thanks for a clear-headed opinion.  I got a bit carried away :)

> Another reason to eliminate globals is to allow one driver to handle multiple
> devices - all device-specific data must be bound to the device, not the driver.
> In our case, there literally cannot be more than one _SB.FEXT. While there could
> theoretically be more than one FUJ02E3, I think we all agree that is highly
> improbable - and if it did happen, the explicit ACPI path approach would also be
> broken.

Good point.

> The motivation to divide the drivers was to provide functional encapsulation,
> accurately represent the system in the device tree, and to improve readability
> and maintainability of the driver code. So long as we can keep coupling to a
> minimum, I still think this makes sense.
> 
> So - static global variable for a driver with exactly one device that needs
> offer services to another driver... not really all that horrible.
> 
> You could accomplish this by making call_fext_func() not static and calling it
> from fujitsu-backlight. Or, you could further restrict it by exporting a
> fujitsu_backlight_power() function which wraps call_fext_func() providing a
> specific interface for fujitsu-backlight. This makes the ownership very explicit
> and ensures the usage doesn't grow without explicit changes to fujitsu-laptop.

I like the latter option more.  Exporting call_fext_func() as it is
would mean enabling other modules to reimplement fujitsu-laptop's
features and we do not want that.

> That is probably the most practical solution IFF we still feel it is worth
> splitting the driver into two separate modules. We need to develop a more robust
> and objective decision making process on module granularity (when to split, when
> to keep together). Will continue to give this more thought.

In light of the above, I still feel the split is worth going through
with.  The question is whether Jonathan feels the same :)

Jonathan, assuming the objective of splitting the module in two, allow
me to pick your brain a bit:

 1. Would you be okay with leaving "priv" as the variable name for
    device-specific data in both drivers?  If they are to be separated,
    "priv" would soon become unambiguous.  I do not have any strong
    feelings about this, though.

 2. Would you be okay with renaming "acpi_handle" to "handle"?  Darren
    seems to like this idea and in light of the above we would not have
    another ACPI handle inside struct fujitsu_bl any more.

 3. You mentioned earlier that you were not really fond of the fext_*()
    helper functions.  Would you like me to drop them and simply use
    call_fext_func() with five arguments everywhere?  Or should I keep
    the helper functions in v2?

Thanks,

-- 
Best regards,
Michał Kępień

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-01 15:10 +0200
  Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-02 15:30 +0200
    Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-05 01:50 +0200
      Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-05 18:20 +0200
        Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-06 14:40 +0200
          Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-06 14:50 +0200
            Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of  module-wide globals Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-06 16:30 +0200
            Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of  module-wide globals Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-06 16:30 +0200
            Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-08 18:10 +0200
              Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-09 11:40 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-09 14:20 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-09 18:50 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-09 23:40 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-11 16:00 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-11 16:50 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-11 17:40 +0200
                Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Michał Kępień <kernel@kempniu.pl> - 2017-05-11 15:50 +0200

csiph-web