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

[Stefan Bader]

Tim Gardner wrote:
> 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.
>

It sounds correct to me. The kernel hdaps does not need an arbitration layer as 
it is the only driver accessing it (well maybe not 100% true, as thinkpad-acpi 
also does some access to the EC for backlight issues).

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

We could dump the alternate battery tool and update the kernel hdaps. The 
differences there are not so big I believe. But as heard on #u-k, the whi..., 
err, requests for that will come as soon as we do.

> 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) ?
>

If my suspicion about thinkpad-acpi is true, we might as well discuss the 
generic ec driver with Henrique, the maintainer of thinkpad-acpi. Given a 
little breath, Andy and I wanted to get into touch with him for the whole 
sounds and backlight issues.

Stefan

-- 

When all other means of communication fail, try words!