On Tue, Feb 28, 2017 at 09:44:26PM +1030, Jonathan Woithe wrote:
> On Tue, Feb 28, 2017 at 09:07:04AM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Feb 27, 2017 at 10:17:55PM -0800, Darren Hart wrote:
> > > > > > > > The problem I have with this driver is that it is generally
> > > > > > > > oblivious to
> > > > > > > > the user's chosen backlight provider. It was written before
> > > > > > > > acpi_video
> > > > > > > > support for backlight control was automatically detected by the
> > > > > > > > kernel
> > > > > > > > and as such it required a fix which was allegedly provided by
> > > > > > > >
> > > > > > > > 7d5c89a615c5 ("fujitsu-laptop: fingers off backlight if
> > > > > > > > video.ko is
> > > > > > > > serving this functionality"). However, that fix was
> > > > > > > > superficial as the
> > > > > > > > module still registered its vendor-specific ACPI driver for
> > > > > > > > backlight
> > > > > > > > control. That in turn caused SBLL/SBL2 to be called when
> > > > > > > > brightness
> > > > > > > > hotkeys were pressed even when acpi_video was supposed to
> > > > > > > > handle
> > > > > > > > brightness changes, which is exactly what that patch was hoping
> > > > > > > > to
> > > > > > > > prevent. Moreover, the module unconditionally exports
> > > > > > > > backlight-related
> > > > > > > > sysfs attributes which enable userspace to change the
> > > > > > > > brightness level,
> > > > > > > > which should also only be possible when vendor backlight
> > > > > > > > control is
> > > > > > > > used. Fast forward several years and on modern Fujitsu
> > > > > > > > machines (e.g.
> > > > > > > > Lifebook E744), the kernel automatically detects that native
> > > > > > > > backlight
> > > > > > > > control [1] should be used and SBLL becomes a noop [2]. This
> > > > > > > > creates
> > > > > > > > confusion, because the driver claims that it can get/set LCD
> > > > > > > > brightness
> > > > > > > > level via the platform device's sysfs attributes, but it
> > > > > > > > actually
> > > > > > > > cannot. It gets even more confusing on Skylake machines on
> > > > > > > > which the
> > > > > > > > FUJ02B1 ACPI device is not present at all.
> > > > > > > >
> > >
> > > Right, evolved.... time to take a step back and clean it up.
> > >
> > > > > > > > The proposal I put forward in regard to the above is to remove
> > > > > > > > the three
> > > > > > > > backlight-related attributes (brightness_changed, lcd_level,
> > > > > > > > max_brightness) from the platform driver's sysfs tree. I
> > > > > > > > believe the
> > > > > > > > functions they implement should only be exposed through the
> > > > > > > > backlight
> > > > > > > > device, because the latter is only created when the user
> > > > > > > > explicitly
> > > > > > > > selects vendor backlight control or it is automatically
> > > > > > > > selected by the
> > > > > > > > kernel (this should be the case for older machines).
> > >
> > > This seems to be a good approach as in combination with the discussion
> > > below re
> > > the ACPI device, it eliminates the sysfs attributes from the platform
> > > device
> > > completely and makes the driver more consistent with others in the
> > > subsystem.
>
> I have now had a chance to revisit the historical background, especially as
> it relates to the three sysfs attributes mentioned (brightness_changed,
> lcd_level, max_brightness). It seems to me that these are most likely left
> overs from a very early version of fujitsu-laptop, originally out-of-tree,
> merged to mainline by myself and others. Like Michael I doubt there is any
> userspace utility which makes use of these. Checking the scripts which I
> personally use on my S7020 I see that I only ever use the attributes
> provided under/sys/devices/virtual/backlight/fujitsu-laptop when controlling
> the backlight. Like Michael said, it's impossible to completely rule out
> the possiblity of an obscure Fujitsu-only utility somewhere in the universe
> which makes use of the /sys/devices/platform/fujitsu-laptop/ backlight
> attributes but I think the probability of such a thing existing is
> vanishingly small. Even if there was, there is a mostly trivial mapping
> from old to new (lcd_level -> brightness, max_brightness -> max_brightness).
> Only brightness_changed isn't replicated but I'm sure something could be
> hooked if necessary.
>
> What this means is that from my perspective it is highly unlikely that
> removing the three sysfs attributes as proposed by Michael
> (brightness_changed, lcd_level, max_brightness) will break userspace in a
> practical sense. Instead, it will improve the structure of the
> fujitsu-laptop driver and make it more consistent with other platform
> drivers. Putting all this together, I have no objections to the removal as
> proposed by Michael: there appears to be far more gains than losses.
As the person who wrote the original driver, for Fujitsu, I can almost
guarantee that there is no specific "fujitsu utility" that uses that
platform driver. We just used the "normal" backlight/led interface
instead, so all should be fine there.
thanks,
greg k-h
