Date:   Thu, 11 May 2017 15:40:28 +0200
From:   Michał Kępień <kernel@...pniu.pl>
To:     Darren Hart <dvhart@...radead.org>,
        Jonathan Woithe <jwoithe@...t42.net>
Cc:     Rafael Wysocki <rjw@...ysocki.net>,
        Andy Shevchenko <andy@...radead.org>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead
 of module-wide globals

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

Powered by blists - more mailing lists