On 08/06/2012 06:59 PM, anish kumar wrote:
> From: anish kumar <anish198519851985@xxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
> ---
> [...]
> +
> +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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
