Re: [PATCH 0/4] fujitsu_init() cleanup

[Darren Hart]

On Tue, Feb 28, 2017 at 03:01:52PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 28, 2017 at 02:24:40PM +0100, MichaÅ KÄpieÅ wrote:
> > > On Tue, Feb 28, 2017 at 09:33:28AM +0100, Micha?? K??pie?? wrote:
> > > > GregKH wrote:
> > > > > On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > > > > GregKH - Can you please confirm the above? Moving an attribute is different than
> > > > > > the format and contents, which is what I explicitly documented in
> > > > > > Documentation/admin-guide/sysfs-rules.rst (last section).
> > > > > 
> > > > > Moving an attribute to a different device structure is usually a bad
> > > > > idea, if the userspace tool counting on it to be present in a specific
> > > > > place breaks.
> > > > 
> > > > Yes, I am familiar with that premise.  Here is the thing though: I am
> > > > unaware of any userspace tool which uses these attributes.  Though,
> > > > obviously, that does not mean such tools do not exist.
> > > 
> > > For what it's worth I too am unaware of any utilities which use the
> > > /sys/devices/platform/fujitsu-laptop/ attributes associated with the
> > > backlight - this after using the S7020 since 2005.  I would be surprised if
> > > any existed since they would have to be specifically for the Fujitsu
> > > hardware.  If writing any utility to control the backlight the logical thing
> > > to do would be to use the standard backlight attributes in
> > > /sys/devices/virtual/backlight/fujitsu-laptop/.
> > > 
> > > > > But, as you can't be consistent here, don't break userspace please, I'd
> > > > > recommend just leaving it alone for now.
> > > > 
> > > > Darren, in the light of the above I will be awaiting your final call on
> > > > this before posting any further patches touching this area.  My number
> > > > one priority was to drop the broken backlight-related attributes,
> > > > because leaving the other attributes where they currently are does not
> > > > prevent achieving a clean split between the two drivers registered by
> > > > fujitsu-laptop, which is the ultimate objective of all these cleanups.
> > > 
> > > As I explained in my response to GregKH earlier and having investigated this
> > > in more detail, I have no objection to the removal of the non-standard
> > > backlight-related sysfs attributes (brightness_changed, lcd_level and
> > > max_brightness).  They are almost certainly unused and their removal will
> > > allow a significant cleanup of fujitsu-laptop.  Obviously however there are
> > > competing viewpoints which take a bigger picture view so I will defer to
> > > Darren's judgement.
> > 
> > Okay, so it looks like I did the best I could to explain my intentions
> > and some confusion crept in anyway, sorry about that.  Let me try again,
> > I will be as concise as I can.
> > 
> > Current custom attributes attached to the platform device are:
> > 
> >     /sys/bus/platform/devices/fujitsu-laptop
> >     `- brightness_changed [*]
> >     `- dock
> >     `- lcd_level [*]
> >     `- lid
> >     `- max_brightness [*]
> >     `- radios
> > 
> > AFAICT, all four of us agree that attributes marked with an asterisk [*]
> > should be removed from the platform device because they are exposed by
> > the backlight device anyway and do not work correctly under certain
> > circumstances.

Yes, please kill these.

> > 
> > However, Darren's question to Greg did not apply to these attributes.
> > It was about the other three attributes: dock, lid and radios.
> > 
> > In other words, my proposal was to change this:
> > 
> >     /sys/bus/platform/devices/fujitsu-laptop
> >     `- dock
> >     `- lid
> >     `- radios
> > 
> > into this:
> > 
> >     /sys/bus/acpi/devices/FUJ02E3:00
> >     `- dock
> >     `- lid
> >     `- radios
> > 
> > That would enable us to completely rip the platform driver and platform
> > device out of fujitsu-laptop, cutting the driver down by ca. 40 lines.
> > 
> > Greg objected to that, arguing that there might be userspace
> > applications using these attributes under their current path.  I do not
> > know of such an application.  My question to Darren was thus: do we move
> > dock, lid and radios and rip the platform driver and platform device out
> > of fujitsu-laptop or should these attributes rather stay where they are?
> 
> I would say just to leave them as-is, there's no harm in those 40 lines,
> and you really don't want to break someone's existing setup for no good
> reason.

In a clean room I'd definitely opt for the move to eliminate the extra
infrastructure of the platform driver - and binding to a HID is a superior
enumeration mechanism.

However, changing this interface doesn't fix any bugs and it breaks a user space
interface. Greg made it clear this commitment is stronger than I had positioned
it for sysfs attributes. Please leave the platform driver code for dock, lid,
and radios in place.

Thank you everyone for the time to clarify.

-- 
Darren Hart
Intel Open Source Technology Center