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

[Darren Hart]

On Thu, May 11, 2017 at 03:40:28PM +0200, MichaÅ KÄpieÅ wrote:
> > 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 :)
> 

In the interest of keeping this moving... As I'm not sure there is a "right
answer" to split or not, and nobody screamed out against splitting, and this is
the direction Michal seems to prefer, and he is doing the work, let's proceed
with the split of -backlight and -laptop.

> 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.

Both of these are easily discussed in the next series which will most likely
have at least one respin anyway.

>  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?

I was torn on this as well - I didn't think they added much value. Let's focus
on splitting the driver, and we can revisit this later for the -laptop driver if
there is interest.

-- 
Darren Hart
VMware Open Source Technology Center