linux-kernel - Re: [PATCH v5 1/5] Extcon (external connector): import Android's switch class and modify.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20120220015400.GD3194@opensource.wolfsonmicro.com>
Date:	Sun, 19 Feb 2012 17:54:02 -0800
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 1/5] Extcon (external connector): import Android's
 switch class and modify.

On Fri, Feb 10, 2012 at 03:40:34PM +0900, MyungJoo Ham wrote:
> External connector class (extcon) is based on and an extension of Android
> kernel's switch class located at linux/drivers/switch/.

This looks good though I've skipped some bits as it's taken me far too
long to get round to reviewing, it'd be really good if we could get it
into 3.4 at least in staging if not in fully.  I don't know if arm-soc
might be a good route if there's some concerns?  A few things below but
they're relatively minor.  

One thing I'd suggest is splitting the GPIO implementation into a
separate patch, mostly just to reduce the size of the initial patch for
ease of review.

> +	if (edev->state != state) {
> +		edev->state = state;
> +
> +		prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
> +		if (prop_buf) {

Is the cast really needed here?

> +static int create_extcon_class(void)
> +{
> +	if (!extcon_class) {
> +		extcon_class = class_create(THIS_MODULE, "extcon");
> +		if (IS_ERR(extcon_class))
> +			return PTR_ERR(extcon_class);
> +		extcon_class->dev_attrs = extcon_attrs;

I thought we were trying to remove classes, though I'm not sure if we're
actually at the point where that's happening yet?  Greg?

> +static int create_extcon_class_for_android(void)
> +{
> +	if (!extcon_class_for_android) {
> +		extcon_class_for_android = class_create(THIS_MODULE, "switch");
> +		if (IS_ERR(extcon_class_for_android))
> +			return PTR_ERR(extcon_class_for_android);
> +		extcon_class_for_android->dev_attrs = extcon_attrs;
> +	}
> +	return 0;
> +}

Might be better to put this as a separate Kconfig option or just leave
it as an out of tree patch (given how trivial it is).  We're going to
end up renaming a bunch of the classes anyway I expect...

> +static int __init extcon_class_init(void)
> +{
> +	return create_extcon_class();
> +}
> +
> +static void __exit extcon_class_exit(void)
> +{
> +	class_destroy(extcon_class);
> +
> +	if (extcon_class_for_android)
> +		class_destroy(extcon_class_for_android);
> +}
> +
> +module_init(extcon_class_init);
> +module_exit(extcon_class_exit);

Ideally these should go next to the functions.

> +static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> +{
> +	struct gpio_extcon_data *extcon_data =
> +	    (struct gpio_extcon_data *)dev_id;
> +
> +	schedule_work(&extcon_data->work);
> +	return IRQ_HANDLED;
> +}

Given that all this does is schedule some work it'd seem useful to make
this a threaded IRQ and just do the work directly in the interrupt
handler.  Though on the other hand we don't have any debounce here so
perhaps it's even better to allow the user to specify a debunce time in
the platform data and change this to schedule_delayed_work() to
implement it?

> +static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
> +{
> +	struct gpio_extcon_data	*extcon_data =
> +		container_of(edev, struct gpio_extcon_data, edev);
> +	const char *state;
> +	if (extcon_get_state(edev))
> +		state = extcon_data->state_on;
> +	else
> +		state = extcon_data->state_off;
> +
> +	if (state)
> +		return sprintf(buf, "%s\n", state);
> +	return -1;

-EINVAL or something?

> +	extcon_data = kzalloc(sizeof(struct gpio_extcon_data), GFP_KERNEL);
> +	if (!extcon_data)
> +		return -ENOMEM;

devm_kzalloc().

> +	ret = request_irq(extcon_data->irq, gpio_irq_handler,
> +			  IRQF_TRIGGER_LOW, pdev->name, extcon_data);
> +	if (ret < 0)
> +		goto err_request_irq;

request_any_context_irq() would allow use with any GPIO - sometimes the
GPIOs for accessory detection are on GPIO expanders which need threaded
context and there's nothing in the code that minds.  It would also be a
good idea if the user could specify the triggers, lots of circuits need
edge triggers for example.

> +static int __init gpio_extcon_init(void)
> +{
> +	return platform_driver_register(&gpio_extcon_driver);
> +}
> +
> +static void __exit gpio_extcon_exit(void)
> +{
> +	platform_driver_unregister(&gpio_extcon_driver);
> +}
> +
> +module_init(gpio_extcon_init);
> +module_exit(gpio_extcon_exit);

module_platform_driver().

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