Message-ID: <CAJ0PZbQjRt0-3+eUUdKkxrK=Jrv-_Q0i3aUx7JHJ9XsNUFLf8w@mail.gmail.com>
Date:	Mon, 20 Feb 2012 15:17:39 +0900
From:	MyungJoo Ham <myungjoo.ham@...sung.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	linux-kernel@...r.kernel.org, NeilBrown <neilb@...e.de>,
	Randy Dunlap <rdunlap@...otime.net>,
	Mike Lockwood <lockwood@...roid.com>,
	Arve Hjønnevag <arve@...roid.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Donggeun Kim <dg77.kim@...sung.com>, Greg KH <gregkh@...e.de>,
	Arnd Bergmann <arnd@...db.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Morten CHRISTIANSEN <morten.christiansen@...ricsson.com>,
	John Stultz <john.stultz@...aro.org>,
	Joerg Roedel <joerg.roedel@....com>
Subject: Re: [PATCH v5 1/5] Extcon (external connector): import Android's
 switch class and modify.

On Mon, Feb 20, 2012 at 10:54 AM, Mark Brown
<broonie@...nsource.wolfsonmicro.com> wrote:
> On Fri, Feb 10, 2012 at 03:40:34PM +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 looks good though I've skipped some bits as it's taken me far too
> long to get round to reviewing, it'd be really good if we could get it
> into 3.4 at least in staging if not in fully.  I don't know if arm-soc
> might be a good route if there's some concerns?  A few things below but
> they're relatively minor.

Yeah. I guess arm-soc would be fine. I'll send thru arm-soc as well next time.

>
> One thing I'd suggest is splitting the GPIO implementation into a
> separate patch, mostly just to reduce the size of the initial patch for
> ease of review.

Ok, I've splitted gpio implementation for the next iteration.

>
>> +     if (edev->state != state) {
>> +             edev->state = state;
>> +
>> +             prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
>> +             if (prop_buf) {
>
> Is the cast really needed here?

Unless we have that cast, we get:
drivers/extcon/extcon_class.c:89:12: warning: assignment makes pointer
from integer without a cast

>
>> +static int create_extcon_class(void)
>> +{
>> +     if (!extcon_class) {
>> +             extcon_class = class_create(THIS_MODULE, "extcon");
>> +             if (IS_ERR(extcon_class))
>> +                     return PTR_ERR(extcon_class);
>> +             extcon_class->dev_attrs = extcon_attrs;
>
> I thought we were trying to remove classes, though I'm not sure if we're
> actually at the point where that's happening yet?  Greg?
>

Hmm.. I remember I was recommended to use classes some time ago (just
a few months ago) especially for adding sysfs entries. Things have
been changed already?

>> +static int create_extcon_class_for_android(void)
>> +{
>> +     if (!extcon_class_for_android) {
>> +             extcon_class_for_android = class_create(THIS_MODULE, "switch");
>> +             if (IS_ERR(extcon_class_for_android))
>> +                     return PTR_ERR(extcon_class_for_android);
>> +             extcon_class_for_android->dev_attrs = extcon_attrs;
>> +     }
>> +     return 0;
>> +}
>
> Might be better to put this as a separate Kconfig option or just leave
> it as an out of tree patch (given how trivial it is).  We're going to
> end up renaming a bunch of the classes anyway I expect...

Then, would it be proper to put "for-android" features surrounded by
#ifdef CONFIG_ANDROID ?

>
>> +static int __init extcon_class_init(void)
>> +{
>> +     return create_extcon_class();
>> +}
>> +
>> +static void __exit extcon_class_exit(void)
>> +{
>> +     class_destroy(extcon_class);
>> +
>> +     if (extcon_class_for_android)
>> +             class_destroy(extcon_class_for_android);
>> +}
>> +
>> +module_init(extcon_class_init);
>> +module_exit(extcon_class_exit);
>
> Ideally these should go next to the functions.

Yes..

>
>> +static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
>> +{
>> +     struct gpio_extcon_data *extcon_data =
>> +         (struct gpio_extcon_data *)dev_id;
>> +
>> +     schedule_work(&extcon_data->work);
>> +     return IRQ_HANDLED;
>> +}
>
> Given that all this does is schedule some work it'd seem useful to make
> this a threaded IRQ and just do the work directly in the interrupt
> handler.  Though on the other hand we don't have any debounce here so
> perhaps it's even better to allow the user to specify a debunce time in
> the platform data and change this to schedule_delayed_work() to
> implement it?

I looks like adding a debounce time would be useful. I'll let it use
delayed_work.
I'll do the same for adc_jack, too, though I'm thinking about
submitting adc_jack later seperatedly from this patchset.


>
>> +static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
>> +{
>> +     struct gpio_extcon_data *extcon_data =
>> +             container_of(edev, struct gpio_extcon_data, edev);
>> +     const char *state;
>> +     if (extcon_get_state(edev))
>> +             state = extcon_data->state_on;
>> +     else
>> +             state = extcon_data->state_off;
>> +
>> +     if (state)
>> +             return sprintf(buf, "%s\n", state);
>> +     return -1;
>
> -EINVAL or something?

I'll use -EINVAL and add NULL check at probe function.

>
>> +     extcon_data = kzalloc(sizeof(struct gpio_extcon_data), GFP_KERNEL);
>> +     if (!extcon_data)
>> +             return -ENOMEM;
>
> devm_kzalloc().

I'll try devm_kzalloc and devm_kfree.

>
>> +     ret = request_irq(extcon_data->irq, gpio_irq_handler,
>> +                       IRQF_TRIGGER_LOW, pdev->name, extcon_data);
>> +     if (ret < 0)
>> +             goto err_request_irq;
>
> request_any_context_irq() would allow use with any GPIO - sometimes the
> GPIOs for accessory detection are on GPIO expanders which need threaded
> context and there's nothing in the code that minds.  It would also be a
> good idea if the user could specify the triggers, lots of circuits need
> edge triggers for example.

Letting users specify flags looks much better than fixing the flag as
IRQF_TRIGGER_LOW. And I'll replease request_irq with
request_any_context_irq as you've mentioned.

>
>> +static int __init gpio_extcon_init(void)
>> +{
>> +     return platform_driver_register(&gpio_extcon_driver);
>> +}
>> +
>> +static void __exit gpio_extcon_exit(void)
>> +{
>> +     platform_driver_unregister(&gpio_extcon_driver);
>> +}
>> +
>> +module_init(gpio_extcon_init);
>> +module_exit(gpio_extcon_exit);
>
> module_platform_driver().

Oh.. yes, another modern idiom. :)



Thanks 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists