[PATCH 0/1] UBUNTU: Add hdaps_ec driver to support newer ThinkPads (Hard Drive Active Protection System)

[Tim Gardner]

Stefan Bader wrote:
> Tim Gardner wrote:
>> Amit Kucheria wrote:
>>> On Mon, Apr 27, 2009 at 09:43:34AM +0200, Stefan Bader wrote:
>>>> Tim Gardner wrote:
>>>>> Brad Figg wrote:
>>>>>> Please pull from : git://kernel.ubuntu.com/bradf/ubuntu-karmic
>>>>>> master
>>>>>>
>>>>>> Bug: #297213
>>>>>>
>>>>>> Hardy had two drivers which handled the HardDisk Active Protection 
>>>>>> System (hdaps and hdaps_ec) which is present in IBM ThinkPads.  
>>>>>> One of the two drivers was dropped when Intrepid development began 
>>>>>> and due to that there are a number of
>>>>>> ThinkPads (newer models) for which this is not supported.
>>>>>>
>>>>>> This patch brings back in the missing driver.
>>>>>>
>>>>>> The code for this patch was taken from
>>>>>> http://tpctl.sourceforge.net and is the 0.40 release of
>>>>>> December 16, 2008.
>>>>>>
>>>>>> Brad Figg (1): UBUNTU: Add hdaps_ec driver to support newer
>>>>>> ThinkPads (Hard Drive Active Protection System)
>>>>>>
>>>>>> ubuntu/misc/Kconfig    |   13 + ubuntu/misc/Makefile   |    2
>>>>>> +- ubuntu/misc/hdaps_ec.c |  880
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files
>>>>>> changed, 894 insertions(+), 1 deletions(-) create mode 100644
>>>>>> ubuntu/misc/hdaps_ec.c
>>>>>>
>>>>>>
>>>>> How is this one better then drivers/hwmon/hdaps.c ? The in-kernel
>>>>> driver appears to contain a superset of DMI table information,
>>>>> i.e., supported platforms. Furthermore, it corrects some problems
>>>>> that exist in ubuntu/misc/hdaps_ec.c, such as I/O space
>>>>> registration, only loads if at least one DMI match is found, and
>>>>> uses in-kernel functionality for polled input devices.
>>>>>
>>>>> So, NAK from me unless you can convince me otherwise.
>>>>>
>>>>> rtg
>>>> This might not be completely true anymore but at the time I did
>>>> that driver for Hardy LUM the following was:
>>>>
>>>> - The in kernel driver could not do independent axis inversion
>>>> which was required for some models - The driver from tpctl would
>>>> use thinkpad_ec to channel acces to the embedded controller, which
>>>> allowed the other tool from that package concurrent access. 
>>>> (arguable benefit)
>>>>
>>>> Upstreaming sounded always like "soonish" without anything
>>>> happening. The problem is/was the questionable origins (unclear).
>>>> As we not necessarily need the other part of tpctl, on option would
>>>> be to update the in kernel driver to support more models (do the
>>>> additional inversion logic and merge the DMi information). An
>>>> interesting question there would be whether access to the ec needs
>>>> syncing with other stuff (like thinpad-acpi)
>>>
>>> NACK adding this alternative driver to our tree. This driver's
>>> upstream had made no known effort to get the driver upstream. We
>>> should ask them to submit it for inclusion into staging atleast. We
>>> had a discussion about this with a user (Whoopie) on irc today.
>>>
>>> Now if we really want to support this feature, someone should find
>>> the changes compared to the in-kernel one and try to get that pushed
>>> upstream.
>>>
>>> Regards, Amit
>>>
>>
>>
>> I'm confused by your comment about "This driver's upstream had made no 
>> known effort to get the driver upstream". Are we talking about 
>> drivers/hwmon/hdaps.c ? If so, its been upstream since Aug 2005. From 
>> a quick perusal it seems to be the same driver, though improved from 
>> the original, and with a slightly different file name.
>>
>> rtg
> 
> The slightly different filename is, err, my invention for Hardy to have 
> the upstream driver live in peace with the other one.
> The "other one" is part of the tpctl package. A sourceforge project that 
> offers the hdaps driver and another driver that allows to control the 
> battery in thinkpads more intelligently (you can define when to charge 
> the battery with some hi- low- watermarks or let them discharge even on 
> ac).
> In order for the two to work (both access the embedded controller. There 
> is the thinkpad_ec driver (also not upstream).
> So Amits comment I would take was meant to bring the full functionality 
> upstream, not only the hdaps part.
> 
> Stefan
> 

Oh, I think I get it now. The support in the in-kernel driver is _just_ 
for the hard drive accelerometer which does not provide shared access to 
the Thinkpad EC. What we _need_ is a Thinkpad EC driver which arbitrates 
access via the EC for battery function as well as HD accelerometer.

Is my summary correct? In any case, I stand by my original NACK since 
the proposed hdaps_ec driver doesn't improve the situation, and in fact 
appears to be a regression.

Before proposing that we add the alternative Thinkpad driver stack to 
staging, I think we'd first have to re-architect the existing hdaps.c 
driver to provide arbitrated EC access, which will probably snowball 
into a general EC API (which is likely the correct approach anyways).

Is anyone with a Thinkpad interested in taking this on? Does it even 
make sense with respect to the age of the platform, i.e., is this a 
one-off thing (in which case I don't really care that much) ?

rtg
-- 
Tim Gardner tim.gardner at canonical.com