lkml.org 
[lkml]   [2012]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 5/5] Extcon: adc-jack driver to support 3.5 pi or simliar devices
On Fri, Feb 10, 2012 at 03:40:38PM +0900, MyungJoo Ham wrote:
> External connector devices that decides connection information based on
> ADC values may use adc-jack device driver. The user simply needs to
> provide a table of adc range and connection states. Then, extcon
> framework will automatically notify others.

This really should be done in terms of the IIO in-kernel framework.

> + /* Check the length of array and set num_cables */
> + for (i = 0; data->edev.supported_cable[i]; i++)
> + ;
> + if (i == 0 || i > SUPPORTED_CABLE_MAX) {

Can we not avoid the hard limit?

> + /* Check the length of array and set num_conditions */
> + for (i = 0; data->adc_condition[i].state; i++)
> + ;
> + data->num_conditions = i;

It'd seem nicer to just specify the length in the parameters, less error
prone.

> + if (pdata->irq) {
> + data->irq = pdata->irq;
> +
> + err = request_threaded_irq(data->irq, NULL,
> + adc_jack_irq_thread,
> + pdata->irq_flags, pdata->name,
> + data);

Since all this does is schedule a delayed work there's no reason to
bother with a threaded IRQ and this could be request_any_context_irq().

> + err = extcon_dev_register(&data->edev, &pdev->dev);
> + if (err)
> + goto err_irq;

What happens if the interrupt fires prior to this...

> + data->ready = true;

...ah, that's what this is about. It'd seem cleaner to just reverse the
request and registration.

> + goto out;

More idiomatic to have a return statement here.

> +static int __devexit adc_jack_remove(struct platform_device *pdev)
> +{
> + struct adc_jack_data *data = platform_get_drvdata(pdev);
> +
> + extcon_dev_unregister(&data->edev);
> + if (data->irq)
> + free_irq(data->irq, data);

The interrupt needs to be freed prior to the device unregistration
otherwise they may race.

> +static int __init adc_jack_init(void)
> +{
> + return platform_driver_register(&adc_jack_driver);
> +}
> +
> +static void __exit adc_jack_exit(void)
> +{
> + platform_driver_unregister(&adc_jack_driver);
> +}

module_platform_driver().

> + unsigned long handling_delay; /* in jiffies */

I'd suggest calling this "debounce" instead, it seems more idiomatic.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-02-10 17:27    [W:0.146 / U:2.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site