[PATCH 0/2] fixes for adt7410

[PATCH 0/2] fixes for adt7410

[Hartmut Knaack]

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

Device instanciation, reading sysfs-values and removal work smoothly, now. Based on recent official git kernel.

Remaining issue: writing any value to sysfs (mode, resolution, ...) files causes infinite call of the appropriate store function. I have no clue where to look for a start. For demonstration, I added some printk's to the store function. This is a piece of the results, it keeps repeating forever:

[ 3257.158462] Set Resolution 1
[ 3257.158478] Set Resolution 2, data: 16
[ 3257.174151] Set Resolution 3, ret: 0
[ 3257.174157] Set Resolution 4, config: 0
[ 3257.174160] Set Resolution 4.2
[ 3257.182143] Set Resolution 6
[ 3257.182148] Set Resolution 7, ret: 0
[ 3257.182159] Set Resolution 1
[ 3257.182163] Set Resolution 2, data: 16
[ 3257.198140] Set Resolution 3, ret: 0
[ 3257.198145] Set Resolution 4, config: 0
[ 3257.198147] Set Resolution 4.2
[ 3257.206139] Set Resolution 6
[ 3257.206144] Set Resolution 7, ret: 0
[ 3257.206150] Set Resolution 1
[ 3257.206154] Set Resolution 2, data: 16
[ 3257.222147] Set Resolution 3, ret: 0
[ 3257.222158] Set Resolution 4, config: 0
[ 3257.222161] Set Resolution 4.2
[ 3257.230135] Set Resolution 6
[ 3257.230141] Set Resolution 7, ret: 0
[ 3257.230154] Set Resolution 1
[ 3257.230159] Set Resolution 2, data: 16
[ 3257.246132] Set Resolution 3, ret: 0
[ 3257.246137] Set Resolution 4, config: 0
[ 3257.246140] Set Resolution 4.2
[ 3257.254130] Set Resolution 6
[ 3257.254135] Set Resolution 7, ret: 0

The function looks like this:

static ssize_t adt7410_store_resolution(struct device *dev,
        struct device_attribute *attr,
        const char *buf,
        size_t len)
{
    struct iio_dev *dev_info = dev_to_iio_dev(dev);
    struct adt7410_chip_info *chip = iio_priv(dev_info);
    unsigned long data;
    u16 config;
    int ret;

    printk(KERN_ERR "Set Resolution 1\n");
    ret = strict_strtoul(buf, 10, &data);
    printk(KERN_ERR "Set Resolution 2, data: %u\n", data);
    if (ret)
        return -EINVAL;

    ret = adt7410_i2c_read_byte(chip, ADT7410_CONFIG, &chip->config);
    printk(KERN_ERR "Set Resolution 3, ret: %i\n", ret);
    if (ret)
        return -EIO;

    config = chip->config & (~ADT7410_RESOLUTION);
    printk(KERN_ERR "Set Resolution 4, config: %i\n", config);
    switch (data) {
        case 13:
            config |= 0;
            printk(KERN_ERR "Set Resolution 4.1,\n");
            break;
        case 16:
            config |= ADT7410_RESOLUTION;
            printk(KERN_ERR "Set Resolution 4.2\n");
            break;
        default:
            printk(KERN_ERR "Invalid value, valid are 13 and 16\n");
    }
/*    if (data) {
        printk(KERN_ERR "Set Resolution 5\n");
   
        config |= ADT7410_RESOLUTION;
    }
*/
    ret = adt7410_i2c_write_byte(chip, ADT7410_CONFIG, config);
    printk(KERN_ERR "Set Resolution 6\n");
    if (ret)
        return -EIO;

    chip->config = config;
    printk(KERN_ERR "Set Resolution 7, ret: %i\n", ret);
   

    return ret;
}

Re: [PATCH 0/2] fixes for adt7410

[Lars-Peter Clausen]

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

> 
> Device instanciation, reading sysfs-values and removal work smoothly, now. Based on recent official git kernel.
> 
> Remaining issue: writing any value to sysfs (mode, resolution, ...) files causes infinite call of the appropriate store function. I have no clue where to look for a start. For demonstration, I added some printk's to the store function. This is a piece of the results, it keeps repeating forever:
> 
> [ 3257.158462] Set Resolution 1
> [ 3257.158478] Set Resolution 2, data: 16
> [ 3257.174151] Set Resolution 3, ret: 0
> [ 3257.174157] Set Resolution 4, config: 0
> [ 3257.174160] Set Resolution 4.2
> [ 3257.182143] Set Resolution 6
> [ 3257.182148] Set Resolution 7, ret: 0
> [ 3257.182159] Set Resolution 1
> [ 3257.182163] Set Resolution 2, data: 16
> [ 3257.198140] Set Resolution 3, ret: 0
> [ 3257.198145] Set Resolution 4, config: 0
> [ 3257.198147] Set Resolution 4.2
> [ 3257.206139] Set Resolution 6
> [ 3257.206144] Set Resolution 7, ret: 0
> [ 3257.206150] Set Resolution 1
> [ 3257.206154] Set Resolution 2, data: 16
> [ 3257.222147] Set Resolution 3, ret: 0
> [ 3257.222158] Set Resolution 4, config: 0
> [ 3257.222161] Set Resolution 4.2
> [ 3257.230135] Set Resolution 6
> [ 3257.230141] Set Resolution 7, ret: 0
> [ 3257.230154] Set Resolution 1
> [ 3257.230159] Set Resolution 2, data: 16
> [ 3257.246132] Set Resolution 3, ret: 0
> [ 3257.246137] Set Resolution 4, config: 0
> [ 3257.246140] Set Resolution 4.2
> [ 3257.254130] Set Resolution 6
> [ 3257.254135] Set Resolution 7, ret: 0
> 
> The function looks like this:
> 
> static ssize_t adt7410_store_resolution(struct device *dev,
>         struct device_attribute *attr,
>         const char *buf,
>         size_t len)
> {
>     struct iio_dev *dev_info = dev_to_iio_dev(dev);
>     struct adt7410_chip_info *chip = iio_priv(dev_info);
>     unsigned long data;
>     u16 config;
>     int ret;
> 
>     printk(KERN_ERR "Set Resolution 1\n");
>     ret = strict_strtoul(buf, 10, &data);
>     printk(KERN_ERR "Set Resolution 2, data: %u\n", data);
>     if (ret)
>         return -EINVAL;
> 
>     ret = adt7410_i2c_read_byte(chip, ADT7410_CONFIG, &chip->config);
>     printk(KERN_ERR "Set Resolution 3, ret: %i\n", ret);
>     if (ret)
>         return -EIO;
> 
>     config = chip->config & (~ADT7410_RESOLUTION);
>     printk(KERN_ERR "Set Resolution 4, config: %i\n", config);
>     switch (data) {
>         case 13:
>             config |= 0;
>             printk(KERN_ERR "Set Resolution 4.1,\n");
>             break;
>         case 16:
>             config |= ADT7410_RESOLUTION;
>             printk(KERN_ERR "Set Resolution 4.2\n");
>             break;
>         default:
>             printk(KERN_ERR "Invalid value, valid are 13 and 16\n");
>     }
> /*    if (data) {
>         printk(KERN_ERR "Set Resolution 5\n");
>    
>         config |= ADT7410_RESOLUTION;
>     }
> */
>     ret = adt7410_i2c_write_byte(chip, ADT7410_CONFIG, config);
>     printk(KERN_ERR "Set Resolution 6\n");
>     if (ret)
>         return -EIO;
> 
>     chip->config = config;
>     printk(KERN_ERR "Set Resolution 7, ret: %i\n", ret);
>    
> 
>     return ret;
> }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] fixes for adt7410

[Sascha Hauer]

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.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/2] fixes for adt7410

[Jonathan Cameron]

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.

Re: [PATCH 0/2] fixes for adt7410

[Hartmut Knaack]

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.
>
Well, never mind then.

Hartmut

Re: [PATCH 0/2] fixes for adt7410

[Hartmut Knaack]

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

Hartmut

Re: [PATCH 0/2] fixes for adt7410

[Lars-Peter Clausen]

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

- Lars

Re: [PATCH 0/2] fixes for adt7410

[Jonathan Cameron]

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

Re: [PATCH 0/2] fixes for adt7410

[Lars-Peter Clausen]

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

Hartmut any suggestions from your side how to solve this?

Thanks
 - Lars

Re: [PATCH 0/2] fixes for adt7410

[Guenter Roeck]

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.

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

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

Guenter

Re: [PATCH 0/2] fixes for adt7410

[Lars-Peter Clausen]

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

Re: [PATCH 0/2] fixes for adt7410

[Guenter Roeck]

On Wed, Dec 12, 2012 at 04:15:55PM +0100, Lars-Peter Clausen wrote:
> 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.
> 
No idea where the "PC-like" comes from. It is supposed to be used for hardware
monitoring on any hardware, and for anything that needs to be monitored
(voltage, current, power, energy, temperature, humidity, fans).

Now, of course one can use all the associated sensors for other purposes,
such as for ambient or remote temperature measurements, external voltage/current
measurements, or remote humidity measurements, but that isn't really the point.

> > 
> >> 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.
> 
Ok with me as well. Just let me know if there is anything you want me to do.

Thanks,
Guenter

Re: [PATCH 0/2] fixes for adt7410

[Lars-Peter Clausen]

On 12/12/2012 05:51 PM, Guenter Roeck wrote:
> On Wed, Dec 12, 2012 at 04:15:55PM +0100, Lars-Peter Clausen wrote:
>> 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.
>>
> No idea where the "PC-like" comes from. It is supposed to be used for hardware
> monitoring on any hardware, and for anything that needs to be monitored
> (voltage, current, power, energy, temperature, humidity, fans).

Ok, maybe it's just my memory playing tricks on me.

> 
> Now, of course one can use all the associated sensors for other purposes,
> such as for ambient or remote temperature measurements, external voltage/current
> measurements, or remote humidity measurements, but that isn't really the point.
> 
>>>
>>>> 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.
>>
> Ok with me as well. Just let me know if there is anything you want me to do.
> 

I'll try to send a couple of patches migrating the missing features from the
IIO driver to the hwmon driver and then we can remove the IIO driver.
Probably won't be getting to it before mid January though.

- Lars

Re: [PATCH 0/2] fixes for adt7410

[Hartmut Knaack]

Guenter Roeck schrieb:
> 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.
>
>> 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 ? 
>
>> 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.
>From what I could see, most issues have been discussed already by now. Well, I also assumed that there was no more discussion going on, since Jonathan stated quite clearly that the iio driver would never make it out of staging. What I still have in mind to add are some configuration options, probably using devicetree, but there are a few other tasks for me to work on, before I can dig into that topic. For the moment I'm glad to have an adt7410 driver outside staging.
> Let me know if you want me to revert the hwmon driver; no problem.
>
> Guenter
>