linux-kernel - Re: [PATCH v5 5/5] Extcon: adc-jack driver to support 3.5 pi or simliar devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120210162512.GF6472@opensource.wolfsonmicro.com>
Date:	Fri, 10 Feb 2012 16:25:12 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:	linux-kernel@...r.kernel.org, NeilBrown <neilb@...e.de>,
	Randy Dunlap <rdunlap@...otime.net>,
	Mike Lockwood <lockwood@...roid.com>,
	Arve Hjønnevag <arve@...roid.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Donggeun Kim <dg77.kim@...sung.com>, Greg KH <gregkh@...e.de>,
	Arnd Bergmann <arnd@...db.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Morten CHRISTIANSEN <morten.christiansen@...ricsson.com>,
	John Stultz <john.stultz@...aro.org>,
	Joerg Roedel <joerg.roedel@....com>, myungjoo.ham@...il.com
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.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)