[Qemu-devel] [PATCH] hw/input/lm832x: set device category of lm832x

[Qemu-devel] [PATCH] hw/input/lm832x: set device category of lm832x

[kumar sourav]

Sets the category of lm832x as DEVICE_CATEGORY_INPUT
Devices should be assigned to one of DEVICE_CATEGORY_XXXX

Signed-off-by: kumar sourav <sourav.jb1988@gmail.com>
---
 hw/input/lm832x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index cffbf586d4..07ae5e0aee 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -509,6 +509,7 @@ static void lm8323_class_init(ObjectClass *klass, void *data)
     k->recv = lm_i2c_rx;
     k->send = lm_i2c_tx;
     dc->vmsd = &vmstate_lm_kbd;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
 static const TypeInfo lm8323_info = {
-- 
2.17.1

Re: [Qemu-devel] [PATCH] hw/input/lm832x: set device category of lm832x

[Thomas Huth]

 Hi,

On 2019-01-27 11:18, kumar sourav wrote:
> Sets the category of lm832x as DEVICE_CATEGORY_INPUT
> Devices should be assigned to one of DEVICE_CATEGORY_XXXX
> 
> Signed-off-by: kumar sourav <sourav.jb1988@gmail.com>
> ---
>  hw/input/lm832x.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
> index cffbf586d4..07ae5e0aee 100644
> --- a/hw/input/lm832x.c
> +++ b/hw/input/lm832x.c
> @@ -509,6 +509,7 @@ static void lm8323_class_init(ObjectClass *klass, void *data)
>      k->recv = lm_i2c_rx;
>      k->send = lm_i2c_tx;
>      dc->vmsd = &vmstate_lm_kbd;
> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }

This device already has DEVICE_CATEGORY_MISC set since it is derived
from TYPE_I2C_SLAVE. If you now set another category bit here, the
device shows up twice in the output of "-device help". That's not so nice.

I see multiple options here:

1) Drop this patch since the device already has a category

2) Make sure to clear_bit() the MISC category here again

3) Remove the set_bit(DEVICE_CATEGORY_MISC, k->categories) in
   hw/i2c/core.c - it does not make that much sense to set the
   MISC category for an abstract parent class.

4) Introduce a new DEVICE_CATEGORY_I2C which could be used
   instead of the DEVICE_CATEGORY_MISC in hw/i2c/core.c

One of the last two options sound most appealing to me.

 Thomas

Re: [Qemu-devel] [PATCH] hw/input/lm832x: set device category of lm832x

[Philippe Mathieu-Daudé]

Hi Thomas,

On 1/28/19 9:37 AM, Thomas Huth wrote:
>  Hi,
> 
> On 2019-01-27 11:18, kumar sourav wrote:
>> Sets the category of lm832x as DEVICE_CATEGORY_INPUT
>> Devices should be assigned to one of DEVICE_CATEGORY_XXXX
>>
>> Signed-off-by: kumar sourav <sourav.jb1988@gmail.com>
>> ---
>>  hw/input/lm832x.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
>> index cffbf586d4..07ae5e0aee 100644
>> --- a/hw/input/lm832x.c
>> +++ b/hw/input/lm832x.c
>> @@ -509,6 +509,7 @@ static void lm8323_class_init(ObjectClass *klass, void *data)
>>      k->recv = lm_i2c_rx;
>>      k->send = lm_i2c_tx;
>>      dc->vmsd = &vmstate_lm_kbd;
>> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>>  }
> 
> This device already has DEVICE_CATEGORY_MISC set since it is derived
> from TYPE_I2C_SLAVE. If you now set another category bit here, the
> device shows up twice in the output of "-device help". That's not so nice.
> 
> I see multiple options here:
> 
> 1) Drop this patch since the device already has a category
> 
> 2) Make sure to clear_bit() the MISC category here again
> 
> 3) Remove the set_bit(DEVICE_CATEGORY_MISC, k->categories) in
>    hw/i2c/core.c - it does not make that much sense to set the
>    MISC category for an abstract parent class.
> 
> 4) Introduce a new DEVICE_CATEGORY_I2C which could be used
>    instead of the DEVICE_CATEGORY_MISC in hw/i2c/core.c
> 
> One of the last two options sound most appealing to me.

Can we go with 0/3/4 together? 0 being this patch:

  0) This devices inherited DEVICE_CATEGORY_I2C from his
     abstract parent, also select the DEVICE_CATEGORY_INPUT
     bit.

Re: [Qemu-devel] [PATCH] hw/input/lm832x: set device category of lm832x

[Thomas Huth]

On 2019-01-28 12:10, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 1/28/19 9:37 AM, Thomas Huth wrote:
>>  Hi,
>>
>> On 2019-01-27 11:18, kumar sourav wrote:
>>> Sets the category of lm832x as DEVICE_CATEGORY_INPUT
>>> Devices should be assigned to one of DEVICE_CATEGORY_XXXX
>>>
>>> Signed-off-by: kumar sourav <sourav.jb1988@gmail.com>
>>> ---
>>>  hw/input/lm832x.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
>>> index cffbf586d4..07ae5e0aee 100644
>>> --- a/hw/input/lm832x.c
>>> +++ b/hw/input/lm832x.c
>>> @@ -509,6 +509,7 @@ static void lm8323_class_init(ObjectClass *klass, void *data)
>>>      k->recv = lm_i2c_rx;
>>>      k->send = lm_i2c_tx;
>>>      dc->vmsd = &vmstate_lm_kbd;
>>> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>>>  }
>>
>> This device already has DEVICE_CATEGORY_MISC set since it is derived
>> from TYPE_I2C_SLAVE. If you now set another category bit here, the
>> device shows up twice in the output of "-device help". That's not so nice.
>>
>> I see multiple options here:
>>
>> 1) Drop this patch since the device already has a category
>>
>> 2) Make sure to clear_bit() the MISC category here again
>>
>> 3) Remove the set_bit(DEVICE_CATEGORY_MISC, k->categories) in
>>    hw/i2c/core.c - it does not make that much sense to set the
>>    MISC category for an abstract parent class.
>>
>> 4) Introduce a new DEVICE_CATEGORY_I2C which could be used
>>    instead of the DEVICE_CATEGORY_MISC in hw/i2c/core.c
>>
>> One of the last two options sound most appealing to me.
> 
> Can we go with 0/3/4 together? 0 being this patch:
> 
>   0) This devices inherited DEVICE_CATEGORY_I2C from his
>      abstract parent, also select the DEVICE_CATEGORY_INPUT
>      bit.

Yes, I guess... Thinking about this again ... dc->categories is a
bitfield, so there is or at least was the original intention that a
device can go into multiple categories. Question: Is this what we want,
and if yes, is everybody fine with the fact that a device shows up
multiple times in the output of "-device help" ? If not, shall we change
the bitfield into a normal enum value?

Anyway, a DEVICE_CATEGORY_I2C is likely a good idea, at least it's
better than MISC here (and we already have DEVICE_CATOGORY_USB, too).
Anybody want to send a patch?

 Thomas