Re: Extcon: adc_jack: adc-jack driver to support 3.5 pi or simliar devices

[Lars-Peter Clausen <lars@metafoo.de>]

On 08/06/2012 06:59 PM, anish kumar wrote:
> From: anish kumar <anish198519851985@gmail.com>
> 
> 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.
> 
> Changes in this version:
> added Lars-Peter Clausen suggested changes:
> Using macros to get rid of boiler plate code such as devm_kzalloc
> and module_platform_driver.Other changes suggested are related to
> coding guidelines.

Looks mostly good.

> 
> Signed-off-by: anish kumar <anish.singh@samsung.com>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> ---
> [...]
> +
> +static int adc_jack_probe(struct platform_device *pdev)

__devinit

> +{
> [...]
> +
> +	data->irq = platform_get_irq(pdev, 0);

platform_get_irq may return an error, you should check that.

> +
> +	err = request_any_context_irq(data->irq, adc_jack_irq_thread,
> +				pdata->irq_flags, pdata->name, data);
> +
> +	if (err) {
> +		dev_err(&pdev->dev, "error: irq %d\n", data->irq);
> +		err = -EINVAL;
> +		goto err_irq;
> +	}
> +
> +	goto out;
> +
> +err_irq:
> +	extcon_dev_unregister(&data->edev);
> +err_initwork:
> +	cancel_delayed_work_sync(&data->handler);

It does not hurt, but since the work is only scheduled from the interrupt
handler this should not be necessary.

> +err_alloc:
> +	kfree(data);

Since it is allocated with devm_kzalloc now you shouldn't manually free it.

> +out:
> +	return err;
> +}
> +
> +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)

Since the IRQ is now requested unconditionally this check can also be remove.

> +		free_irq(data->irq, data);

Since you access the extcon device from within the IRQ handler it makes
sense to free the IRQ handler before the extcon device.

> +
> +	return 0;
> +}
> +
[...]
> diff --git a/include/linux/extcon/adc_jack.h b/include/linux/extcon/adc_jack.h
> new file mode 100644
> index 0000000..ca4d1cd
> --- /dev/null
> +++ b/include/linux/extcon/adc_jack.h
> @@ -0,0 +1,77 @@
> +/*
> [...]
> +struct adc_jack_pdata {
> +	const char *name;
> +	const char *consumer_channel;
> +	/*
> +	 * NULL if standard extcon names are used.
> +	 * The last entry should be NULL
> +	 */
> +	const char **cable_names;
> +	/* The last entry's state should be 0 */
> +	struct adc_jack_cond *adc_condition;
> +
> +	unsigned long irq_flags;
> +	unsigned long handling_delay_ms; /* in ms */
> +
> +	/* When we have ADC subsystem, this can be generalized. */
> +	int (*get_adc)(u32 *value);

Huh, looks like it sneaked in again.


> +};
> +
> +#endif /* _EXTCON_ADC_JACK_H */