Re: [PATCH v5 5/5] Extcon: adc-jack driver to support 3.5 pi orsimliar devices

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature