Re: [PATCH v5 2/5] usb: gadget: configfs: pass UDC name via usb_gadget_driver struct

[Ruslan Bilovol]

Hi Krzysztof,

On Tue, Jun 23, 2015 at 9:54 AM, Krzysztof Opasiak
<k.opasiak@xxxxxxxxxxx> wrote:
> Hello,
>
> On 06/23/2015 12:01 AM, Ruslan Bilovol wrote:
>>
>> Now when udc-core supports binding to specific UDC by passing
>> its name via 'udc_name' member of usb_gadget_driver struct,
>> switch to this generic approach.
>>
>> Tested-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@xxxxxxxxx>
>> ---
>>   drivers/usb/gadget/configfs.c | 27 ++++++++++++++-------------
>>   1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index c42765b..efad021 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -54,7 +54,6 @@ struct gadget_info {
>>         struct list_head string_list;
>>         struct list_head available_func;
>>
>> -       const char *udc_name;
>>   #ifdef CONFIG_USB_OTG
>>         struct usb_otg_descriptor otg;
>>   #endif
>> @@ -230,21 +229,21 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct
>> gadget_info *gi,
>>
>>   static ssize_t gadget_dev_desc_UDC_show(struct gadget_info *gi, char
>> *page)
>>   {
>> -       return sprintf(page, "%s\n", gi->udc_name ?: "");
>> +       return sprintf(page, "%s\n", gi->composite.gadget_driver.udc_name
>> ?: "");
>>   }
>>
>>   static int unregister_gadget(struct gadget_info *gi)
>>   {
>>         int ret;
>>
>> -       if (!gi->udc_name)
>> +       if (!gi->composite.gadget_driver.udc_name)
>>                 return -ENODEV;
>>
>>         ret = usb_gadget_unregister_driver(&gi->composite.gadget_driver);
>>         if (ret)
>>                 return ret;
>> -       kfree(gi->udc_name);
>> -       gi->udc_name = NULL;
>> +       kfree(gi->composite.gadget_driver.udc_name);
>> +       gi->composite.gadget_driver.udc_name = NULL;
>>         return 0;
>>   }
>>
>> @@ -267,14 +266,16 @@ static ssize_t gadget_dev_desc_UDC_store(struct
>> gadget_info *gi,
>>                 if (ret)
>>                         goto err;
>>         } else {
>> -               if (gi->udc_name) {
>> +               if (gi->composite.gadget_driver.udc_name) {
>
>
> You are using this very long if condition in a few places, maybe it would be
> more suitable to define a macro or inline function for this? Something like
> gadget_dev_bound() or gadget_dev_is_active() or some other more suitable
> name.

Yes, it makes sense, since there are 11 places where this long access
to udc_name is used.
However, it is used not only in if conditions but also for changing
udc_name, so need to add some getters/setters, something like
#define gi_to_udc_name(gi)
((gi)->composite.gadget_driver.udc_name)
#define gi_set_udc_name(gi,name)
((gi)->composite.gadget_driver.udc_name = (name))

Best regards,
Ruslan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/