led and hid class - hid_device vs led_classdev

led and hid class - hid_device vs led_classdev

[Adam Nielsen]

Hi all,

I'm writing a driver for a USB HID device (using the new hid class), but 
I'm a bit confused about how the hid_device structure relates to the 
other driver-information structures.

One of the functions the device implements is a LED.  This is pretty 
simple to control (basic on/off), so I'd like to implement a device in 
the "led" class that controls this led.

In my hid driver's probe function I'm given a struct hid_device* which 
is used with functions like hid_hw_start().

I then need to call led_classdev_register() to register my led device. 
At the moment I pass this function hid_device.dev

Then I set my custom driver data with hid_set_drvdata().

Later on, when the user wants to switch the LED on or off, a function in 
my driver gets called, with a struct led_classdev* parameter.

I'm stuck trying to figure out how to convert this led_classdev 
parameter back into a hid_device, so that I can extract the custom data 
I set with hid_set_drvdata().

Some of the led drivers use platform_set_drvdata() in their probe 
functions to get their custom data instead, but I'm afraid if I do this 
I'll overwrite any hid-related data that hid_set_drvdata() might tack on 
to my custom data.

Some pseudocode to help illustrate:

static int my_probe(struct hid_device *hid,
   const struct hid_device_id *id)
{
   hid_parse(hid);
   hid_hw_start(hid, HID_CONNECT_DEFAULT);

   led_classdev_register(&hid->dev, &my_led);

   hid_set_drvdata(hid, my_data);

   /* Don't want to use this, in case it overwrites something that
      hid_set_drvdata() includes. */
   /*platform_set_drvdata(&hid->dev, my_data);*/
}

/* Callback function when user wants to change LED state */
static void my_led_set(struct led_classdev *led_cdev,
   enum led_brightness value)
{
   /* How do I get my_data back? */
}

Hopefully this explains what I'm after - if you need me to clarify 
anything please let me know.

Many thanks,
Adam.

Re: led and hid class - hid_device vs led_classdev

[Jiri Slaby]

On 11/04/2008 08:51 AM, Adam Nielsen wrote:
> /* Callback function when user wants to change LED state */
> static void my_led_set(struct led_classdev *led_cdev,
>   enum led_brightness value)
> {
>   /* How do I get my_data back? */
> }
> 
> Hopefully this explains what I'm after - if you need me to clarify
> anything please let me know.

my_led should by part of my_data, then you do
my_data = container_of(led_cdev, /*typeof(my_data)*/, my_led);

hid class and sysfs/hwmon (was: led and hid class - hid_device vs led_classdev)

[Adam Nielsen]

>> static void my_led_set(struct led_classdev *led_cdev,
>>   enum led_brightness value)
>> {
>>   /* How do I get my_data back? */
>> }
>>
> 
> my_led should by part of my_data, then you do
> my_data = container_of(led_cdev, /*typeof(my_data)*/, my_led);

That did the trick, thanks!  I hadn't realised what container_of() was 
meant to be for - that's quite a useful little macro!

I got that working (can switch the LED on and off via the led device) 
but now I'm stuck trying to implement a hwmon interface for the device's 
sensors.

As far as I can tell I'm doing everything fine, but for some reason the 
hwmon interface gets created but with no files inside it (apart from a 
few defaults.)  It's supposed to have files like "temp1_input" to report 
temperatures, but these files never appear.

The only thing that I'm unsure of is the first parameter to 
sysfs_create_group().  All the other drivers use something like 
i2c_client.dev.kobj or platform_device.dev.kobj, but I'm using 
hid_device.dev.kobj.  I'm guessing that should work just as well, but I 
can't figure out why else the sysfs files never appear in the new hwmon 
folder in /sys/class/hwmon/

Here is the code so far, if it's useful: (I've omitted all the error 
checking code for clarity, all the functions called here return success)

--------------------------------------------
static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0);

static struct attribute *odin_attributes[] = {
   &sensor_dev_attr_temp1_input.dev_attr.attr,
   NULL
};

static const struct attribute_group odin_attr_group = {
   .attrs = odin_attributes,
};

static int odin_probe(struct hid_device *hdev,
   const struct hid_device_id *id)
{
   hid_parse(hdev);
   hid_hw_start(hdev, HID_CONNECT_DEFAULT);

   odin_psu = kzalloc(sizeof(struct odin_psu_device), GFP_KERNEL);
   odin_psu->hdev = hdev;

   hid_set_drvdata(hdev, odin_psu);

   sysfs_create_group(&hdev->dev.kobj, &odin_attr_group);
   odin_psu->hwmon_dev = hwmon_device_register(&hdev->dev);

   return 0;
}
--------------------------------------------
$ ls /sys/class/hwmon/hwmon5/
total 0
drwxr-xr-x 3 root root    0 2008-11-08 14:50 .
drwxr-xr-x 8 root root    0 2008-11-08 14:50 ..
lrwxrwxrwx 1 root root    0 2008-11-08 14:50 device -> 
../../../devices/pci0000:00/0000:00:1d.2/usb8/8-1/8-1:1.0/0003:1044:4001.0001
drwxr-xr-x 2 root root    0 2008-11-08 14:50 power
lrwxrwxrwx 1 root root    0 2008-11-08 14:50 subsystem -> ../../hwmon
-rw-r--r-- 1 root root 4.0K 2008-11-08 14:50 uevent
--------------------------------------------

If anyone can see why this might result in no sysfs files, please let me 
know!  I previously had most of this code working with a platform_device 
instead of the hid_device, which is what makes me wonder about 
hdev->dev.kobj.  (Not sure how to test if that variable is accurate, 
either.)  Or perhaps it has already been used elsewhere and it can only 
be used once?

Thanks,
Adam.

Re: hid class and sysfs/hwmon

[Jiri Slaby]

On 11/08/2008 07:18 AM, Adam Nielsen wrote:
> Here is the code so far, if it's useful: (I've omitted all the error
> checking code for clarity, all the functions called here return success)
> 
> --------------------------------------------
> static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0);
> 
> static struct attribute *odin_attributes[] = {
>   &sensor_dev_attr_temp1_input.dev_attr.attr,
>   NULL
> };
> 
> static const struct attribute_group odin_attr_group = {
>   .attrs = odin_attributes,
> };
> 
> static int odin_probe(struct hid_device *hdev,
>   const struct hid_device_id *id)
> {
>   hid_parse(hdev);
>   hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> 
>   odin_psu = kzalloc(sizeof(struct odin_psu_device), GFP_KERNEL);
>   odin_psu->hdev = hdev;
> 
>   hid_set_drvdata(hdev, odin_psu);
> 
>   sysfs_create_group(&hdev->dev.kobj, &odin_attr_group);
>   odin_psu->hwmon_dev = hwmon_device_register(&hdev->dev);
> 
>   return 0;
> }
> --------------------------------------------
> If anyone can see why this might result in no sysfs files, please let me
> know!  I previously had most of this code working with a platform_device
> instead of the hid_device, which is what makes me wonder about
> hdev->dev.kobj.  (Not sure how to test if that variable is accurate,
> either.)  Or perhaps it has already been used elsewhere and it can only
> be used once?

I suppose it's under /sys/bus/hid/devices/.../, isn't it?

Re: hid class and sysfs/hwmon

[Adam Nielsen]

>> If anyone can see why this might result in no sysfs files, please let me
>> know!  I previously had most of this code working with a platform_device
>> instead of the hid_device, which is what makes me wonder about
>> hdev->dev.kobj.  (Not sure how to test if that variable is accurate,
>> either.)  Or perhaps it has already been used elsewhere and it can only
>> be used once?
> 
> I suppose it's under /sys/bus/hid/devices/.../, isn't it?

Aha, yes!  I didn't even think of checking there.  I suppose this means 
I have to create another device (like a platform_device) in order to 
have the sysfs files appear in the correct place?

Is a platform_device the correct type to create, or is there another way?

Thanks,
Adam.

Re: hid class and sysfs/hwmon

[Jiri Slaby]

Adam Nielsen napsal(a):
>>> If anyone can see why this might result in no sysfs files, please let me
>>> know!  I previously had most of this code working with a platform_device
>>> instead of the hid_device, which is what makes me wonder about
>>> hdev->dev.kobj.  (Not sure how to test if that variable is accurate,
>>> either.)  Or perhaps it has already been used elsewhere and it can only
>>> be used once?
>>
>> I suppose it's under /sys/bus/hid/devices/.../, isn't it?
> 
> Aha, yes!  I didn't even think of checking there.  I suppose this means
> I have to create another device (like a platform_device) in order to
> have the sysfs files appear in the correct place?
> 
> Is a platform_device the correct type to create, or is there another way?

I don't know much about hwmon, but I would guess you should use hwmon_dev
for registering the attributes if you want it under /sys/class/hwmon/...

Such as:
sysfs_create_group(&odin_psu->hwmon_dev->kobj, &odin_attr_group);

No?

Re: hid class and sysfs/hwmon

[Adam Nielsen]

> I don't know much about hwmon, but I would guess you should use hwmon_dev
> for registering the attributes if you want it under /sys/class/hwmon/...
> 
> Such as:
> sysfs_create_group(&odin_psu->hwmon_dev->kobj, &odin_attr_group);
> 
> No?

Oh, that worked!  All the other hwmon drivers seem to have it the other 
way around, i.e. you attach sysfs to your original bus/class device kobj 
instead of the hwmon kobj.

Hopefully doing it this way won't have any side effects...

Thanks for your help Jiri!

Cheers,
Adam.