Messages in this thread |  | | Date | Fri, 10 Feb 2012 16:25:12 +0000 | From | Mark Brown <> | Subject | Re: [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] |  |