Re: [PATCH 0/2] fixes for adt7410

[Lars-Peter Clausen <lars@metafoo.de>]

On 12/12/2012 03:47 PM, Guenter Roeck wrote:
> On Wed, Dec 12, 2012 at 11:08:23AM +0100, Lars-Peter Clausen wrote:
>> Added Guenter Roeck to Cc, since he merged the hwmon patch.
>>
>> On 12/12/2012 09:59 AM, Jonathan Cameron wrote:
>>> On 11/12/12 20:10, Lars-Peter Clausen wrote:
>>>> On 12/11/2012 08:57 PM, Hartmut Knaack wrote:
>>>>> Jonathan Cameron schrieb:
>>>>>> On 7/16/2012 9:34 AM, Sascha Hauer wrote:
>>>>>>> On Sun, Jul 15, 2012 at 11:11:52PM +0200, Lars-Peter Clausen wrote:
>>>>>>>> On 07/15/2012 10:40 PM, Hartmut Knaack wrote:
>>>>>>>>> This patchset makes the ADT7410 usable. Main reason was the following
>>>>>>>>> error when trying to register one of these devices:
>>>>>>>>> [  180.945561] BUG: unable to handle kernel NULL pointer dereference
>>>>>>>>> at   (null)
>>>>>>>>> [  180.945592] IP: [<f84b1e80>]
>>>>>>>>> iio_device_register_eventset+0x380/0x3a0 [industrialio]
>>>>>>>>> This happens, since in industrialio-events.c
>>>>>>>>> __iio_add_event_config_attrs (which does a INIT_LIST_HEAD) is not
>>>>>>>>> called, if the channels attribute of the device is not set. As a
>>>>>>>>> result, list_for_each_entry causes those NULL pointer dereference
>>>>>>>>> problems. So, before trying to access those list elements, we check
>>>>>>>>> for their existence (also in unregister functions).
>>>>>>>>> Now the adt7410.c ended in some complex changes - my apologies for this:
>>>>>>>>>
>>>>>>>>>     * check for exact values provided through sysfs, otherwise output
>>>>>>>>> a verbose error/usage message for: sample mode, resolution, event mode
>>>>>>>>>     * adt7410_show_id: I don't see a point in masking out some LSBs,
>>>>>>>>> if they get shifted to oblivion, anyway -> no more need for
>>>>>>>>> ADT7410_MANUFACTORY_ID_MASK
>>>>>>>>>     * adt7410_convert_temperature: in 13 bit mode, the driver assumed
>>>>>>>>> the data to be stored in bits 0-12, but according to the data sheet,
>>>>>>>>> it is stored in bits 3-15. Temperature readings were always around
>>>>>>>>> 200(°C), while real temperature was something around 20(°C). Simple
>>>>>>>>> fix: mask out bits 0-2 in 13 bit mode and use the 16 bit routines
>>>>>>>>> from that point on.
>>>>>>>>>     * adt7410_set_t_bound: handle float values provided through sysfs
>>>>>>>>> properly. Also, simplify treatment of 13 bit values by masking out
>>>>>>>>> bits 0-2.
>>>>>>>>>     * adt7410_probe, adt7410_remove: fix another possible NULL
>>>>>>>>> pointer dereference issue, in case no platform data is provided. Sync
>>>>>>>>> chip->config with the config register
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Sascha Hauer posted a couple of patches a while ago which seem to fix a
>>>>>>>> similar set of issues.
>>>>>>>>
>>>>>>>> See: http://www.spinics.net/lists/linux-iio/msg05947.html and
>>>>>>>> http://www.spinics.net/lists/linux-iio/msg05955.html
>>>>>>> Indeed, most if not all of the mentioned issues should be fixed already
>>>>>>> with these patches.
>>>>>>>
>>>>>> Just to reiterate what I said to Sascha at the time...  This driver does
>>>>>> not belong in IIO. It is very much a hardware monitoring part and so
>>>>>> wants to move to hwmon. We aren't going to take it out of staging into
>>>>>> drivers/iio and I'm rather unwilling to take any 'new' features into the
>>>>>> staging version. Someone with hardware who is interesting needs to bite
>>>>>> the bullet and convert / rewrite this driver as a hwmon device.
>>>>>>
>>>>> FYI, I created a basic hwmon driver for ADT7410, which now made it into
>>>>> linux 3.7.
>>>>> Take care
>>>>
>>>> Ah, great, now we have two drivers which bind to the same device...
>>>>
>>> Yup, can one of you take a look at the two drivers and see where they
>>> differ in functionality. We obviously don't want this messy situation
>>> to go on for long!  Then again, we don't want to end up with missing
>>> functionality in one of them either...
>>
>> The IIO driver supports more. It has support for the similar adt7310 and it
>> also has support for reporting over- or undertemperature, something which
>> can't really be implemented that nicely in the hwmon framework. We did think
> 
> hwmon supports configuring over- and undertemperature and supports attributes
> for reporting it.
> 
>> about moving this driver to hwmon, but since it does not really support
>> threshold events and also is more meant for PC-style hardware monitoring and
> 
> There is nothing preventing you from creating such events. See gpio-fan for an
> example. That it isn't implemented for most drivers doesn't mean it is not
> possible or supported.
> 

Ah ok, via kobject_uevent. Hadn't seen this before. This definitely lowers
the barrier for moving the driver to hwmon.

>> the adt7410 has primarily other applications I did decide against it.
>> Another reason was that we do have the IIO-to-hwmon bridge which would allow
>> to instantiate a hwmon userspace interface for the driver if necessary.
>>
>> Hartmut you knew of the existence of the IIO driver it would have really
>> been nice if you had included us on Cc when you submitted the hwmon driver
>> that would have allowed us to avoid this messy situation.
>>
>> One solution could be to just sent a patch to revert the hwmon driver patch,
>> but I guess that would be kind of counterproductive.
>>
> 
> I see two contradicting comments above: "very much a hardware monitoring
> part" and "needs to ... rewrite the driver as hwmon device" vs. "the adt7410
> has primarily other applications". Which one is it ?

My understanding is (or at least was) that the hwmon framework is primarily
intended to be used for PC-like hardware monitoring. E.g. your CPU
temperature or core voltage. The ADT7410 is a temperature sensor and while
it also could be used as a CPU temperature monitor it not necessarily is.
The datasheet list a wide variety of applications for example industrial
process control.

> 
>> Hartmut any suggestions from your side how to solve this?
>>
> Sorry, I had assumed the discussion was closed and the decision was made to move
> the driver to hwmon.
> 
> Let me know if you want me to revert the hwmon driver; no problem.
> 

Since hwmon has support for alarms we may as well add the missing
functionality to the hwmon driver and than remove the IIO driver.

- Lars