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

Groups > linux.kernel > #1642127 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Darren Hart <dvhart@infradead.org> - 2017-05-16 01:30 +0200
    Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead  of module-wide globals Jonathan Woithe <jwoithe@just42.net> - 2017-05-16 02: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-16 08:50 +0200

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

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

[toc] | [next] | [standalone]

#1642139

On Mon, May 15, 2017 at 04:27:25PM -0700, Darren Hart wrote:
> > 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.

Apologies for not getting back about this earlier.  As mentioned in my
follow up to Michael's post from a few minutes ago I agree with the above
sentiment.

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

Assuming the split happens I am happy with both of these proposals.  The
concerns raised earlier were precipitated mostly because I was unaware of
the medium term goal of splitting the driver (not because it hadn't been
mentioned, but because I had forgotten about it in the time since it was
first raised earlier in the year).

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

It seems I misinterpreted Darren's stance on this one and misrepresented him
in my previous post (sorry Darren).  Since Darren's preferred approach
is to drop them for the moment let's run with that.  As he said, once the
split has been made we can obviously revisit this to see if there value in
using them in the context of the split drivers.

Regards
  jonathan

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

#1642253

> On Mon, May 15, 2017 at 04:27:25PM -0700, Darren Hart wrote:
> > > 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.
> 
> Apologies for not getting back about this earlier.  As mentioned in my
> follow up to Michael's post from a few minutes ago I agree with the above
> sentiment.
> 
> > > 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.
> 
> Assuming the split happens I am happy with both of these proposals.  The
> concerns raised earlier were precipitated mostly because I was unaware of
> the medium term goal of splitting the driver (not because it hadn't been
> mentioned, but because I had forgotten about it in the time since it was
> first raised earlier in the year).
> 
> > >  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.
> 
> It seems I misinterpreted Darren's stance on this one and misrepresented him
> in my previous post (sorry Darren).  Since Darren's preferred approach
> is to drop them for the moment let's run with that.  As he said, once the
> split has been made we can obviously revisit this to see if there value in
> using them in the context of the split drivers.

Jonathan, Darren, thank you for all the feedback.  Silence on my behalf
has not been coincidental as I have also been busy lately and had to put
kernel stuff on the back burner.  Sadly, I can also now confirm that I
will no longer have access to the E744 I used to test my patches on as
of next Monday.  I will do my best to prepare v2 of this series before
that.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web