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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 4193 bytes --]

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().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]