Re: [PATCH v2 1/3] Extcon (external connector): import Android'sswitch class and modify.

[MyungJoo Ham]

On Thu, Dec 15, 2011 at 10:01 AM, Greg KH <gregkh@xxxxxxx> wrote:
> On Wed, Dec 14, 2011 at 07:28:26PM +0900, MyungJoo Ham wrote:
>> External connector class (extcon) is based on and an extension of Android
>> kernel's switch class located at linux/drivers/switch/.
>> This patch provides the before-extension switch class moved to the
>> location where the extcon will be located (linux/drivers/extcon/).
>>
>> The before-extension class, switch class of Android kernel, commits
>> imported are:
>>
>> switch: switch class and GPIO drivers.
>> Author: Mike Lockwood <lockwood@xxxxxxxxxxx>
>>
>> switch: gpio: Don't call request_irq with interrupts disabled
>> Author: Arve Hjønnevåg <arve@xxxxxxxxxxx>
>>
>> switch: Use device_create instead of device_create_drvdata.
>> Author: Arve Hjønnevåg <arve@xxxxxxxxxxx>
>>
>> switch_gpio: Add missing #include <linux/interrupt.h>
>> Author: Mike Lockwood <lockwood@xxxxxxxxxxx>
>>
>> In this patch, upon the commits of Android kernel, we have added:
>> - Relocated and renamed for extcon.
>> - Comments, module name, and author information are updated
>> - Code clean for successing patches
>> - Bugfix: enabling write access without write functions
>
> Nice, but if we accept this, will someone also make the needed changes
> to the Android userspace code to handle the user api changes that this
> causes?

I have no idea about how Android will react to this as I have no
developmental experiences with Android.
However, from the perspective of general userspace, this modification
incurs path changes (/sys/class/switch/.... to /sys/class/extcon/...)
only.

>
>> --- /dev/null
>> +++ b/drivers/extcon/Kconfig
>> @@ -0,0 +1,22 @@
>> +menuconfig EXTCON
>> +     bool "External Connector Class (extcon) support"
>
> Why can't this be a module?
>

I also think this can be a module as well. I'm wondering why I've
changed it to bool from RFC patchset's tristate. ;(

>> +
>> +struct class *extcon_class;
>> +static atomic_t device_count;
>
> this should use the idr subsystem, not just an atomic variable, as you
> aren't properly handling devices being removed (which is something
> overall this core code needs to properly handle.)
>

It seems that I need to rewrite extcon_dev_register/unregister
functions. I was simply reusing that part from android kernel;
however, this time, they do look ugly.

>
>> +
>> +static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>> +             char *buf)
>> +{
>> +     struct extcon_dev *edev = (struct extcon_dev *)
>> +             dev_get_drvdata(dev);
>> +
>> +     if (edev->print_state) {
>> +             int ret = edev->print_state(edev, buf);
>> +
>> +             if (ret >= 0)
>> +                     return ret;
>> +             /* Use default if failed */
>> +     }
>> +     return sprintf(buf, "%u\n", edev->state);
>> +}
>> +
>> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>> +             char *buf)
>> +{
>> +     struct extcon_dev *edev = (struct extcon_dev *)
>> +             dev_get_drvdata(dev);
>> +
>> +     if (edev->print_name) {
>> +             int ret = edev->print_name(edev, buf);
>> +             if (ret >= 0)
>> +                     return ret;
>> +     }
>> +     return sprintf(buf, "%s\n", edev->name);
>> +}
>
> Why is the name somehow different from the device name in the first
> place?

I'll let dev_get_name() return what "name_show" wants. I guess I'll
keep name_show sysfs entry to be compatible with Android kernel
(except for the path) and keep the optional "print_name" callback.


>
>> +/**
>> + * extcon_set_state() - Set the cable attach states of the extcon device.
>> + * @edev:    the extcon device
>> + * @state:   new cable attach status for @edev
>> + */
>> +void extcon_set_state(struct extcon_dev *edev, u32 state)
>> +{
>> +     char name_buf[120];
>> +     char state_buf[120];
>> +     char *prop_buf;
>> +     char *envp[3];
>> +     int env_offset = 0;
>> +     int length;
>> +
>> +     if (edev->state != state) {
>> +             edev->state = state;
>> +
>> +             prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
>> +             if (prop_buf) {
>> +                     length = name_show(edev->dev, NULL, prop_buf);
>
> Heh, nice hack, but wouldn't it just be simpler to call the
> evdev->print_name() function directly here?

I didn't do so because print_name callback is optional.

>
>> +                     if (length > 0) {
>> +                             if (prop_buf[length - 1] == '\n')
>> +                                     prop_buf[length - 1] = 0;
>> +                             snprintf(name_buf, sizeof(name_buf),
>> +                                     "SWITCH_NAME=%s", prop_buf);
>> +                             envp[env_offset++] = name_buf;
>> +                     }
>> +                     length = state_show(edev->dev, NULL, prop_buf);
>
> Same here.

Same here. print_state is also optional. (and I expect many won't use
these optional callbacks at least including MAX8997, MAX77693, and
FSA9480)

>> +                     printk(KERN_ERR "out of memory in extcon_set_state\n");
>
> dev_err()?

Yes, that looks better.

>
>> +                     kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
>
> I really dislike using uevents, what is listening for them?  Are you
> hooked into udev's event chain in userspace to properly handle this?  If
> not, what is the point of sending them?

It is to let userspace processes get notified for the events in extcon.
Do you think sysfs_notify() would be better for this purpose?

>
>> +
>> +     ret = device_create_file(edev->dev, &dev_attr_state);
>> +     if (ret < 0)
>> +             goto err_create_file_1;
>> +     ret = device_create_file(edev->dev, &dev_attr_name);
>> +     if (ret < 0)
>> +             goto err_create_file_2;
>
> Why not use default attributes for the class?  That makes this code go
> away, and solves the error handling issues as well.

Thanks for the remind! I'll use extcon_class->dev_attrs.

>> + */
>> +void extcon_dev_unregister(struct extcon_dev *edev)
>> +{
>> +     device_remove_file(edev->dev, &dev_attr_name);
>> +     device_remove_file(edev->dev, &dev_attr_state);
>> +     device_destroy(extcon_class, MKDEV(0, edev->index));
>> +     dev_set_drvdata(edev->dev, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>
> As stated before, you forgot to decrement the count, but use the idr
> subsystem and remember to decrement it here also.
>

Sure, I'll revise unregister function.


Thank you so much.


Cheers!
MyungJoo

-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
--
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/