Hi Michael Apologies for the delayed response - things have been a bit crazy lately. Darren's mail just now has reminded me that you were awaiting feedback.
On Thu, May 11, 2017 at 03:40:28PM +0200, Micha?? K??pie?? wrote: > > 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. Agreed - we don't need duplicate implementations. > > 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 :) I'm pretty much sitting on the fence regarding the split. Darren has provided a compelling argument for proceeding with the split so I guess we can pursue that and see where it leads. It still feels odd to me that two different drivers will end up touching the same hardware control, but at the end of the day the fact that this is necessary says more about the hardware design than anything else. There isn't really a clean solution which ticks every box so we just need to pick one and run with it. > 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. With separate drivers I don't have a problem with the generic "priv" since the ambiguity drops away due to the driver encapsulation. > 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. My primary concern was the presence of two ACPI handles. If there are two such handles, calling one of them by a very generic term tends to make the code confusing IMHO. However, since the split avoids the need for either driver to carry two ACPI handles this becomes a moot point and the plain "handle" can be justified. This is of course contingent on the split happening. > 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? The extended explanation of the rationale for these functions (provided a few days later) makes their presence clearer. In some ways though it still feels like this is being done solely due to the (somewhat arbitrary) checkpatch 80 column thing, rather than being driven by a bona fide code structuring issue. Having said that, the number of parameters to call_fext_func() has grown to 5 as you said, and that is arguably an issue in its own right. On balance (taking your detailed explanation into consideration), since Darren hasn't voiced any concern about these functions and they do permit a clean up of the call sites I am happy for you to retain them in v2. Regards jonathan
