Re: [PATCH v2] RFC: staging: greybus: shape up greybus GPIO

[Johan Hovold]

On Mon, Oct 10, 2016 at 10:39:32AM +0200, Linus Walleij wrote:
> Greybus GPIO seems to reimplement the already existing generic
> gpiolib irqchip. Also use gpiochip_get_data(). Also use
> devm_gpiochip_add_data().
> 
> Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> Cc: Axel Haslam <haslam_axel@xxxxxxxxxxxxxx>
> Cc: Johan Hovold <johan@xxxxxxxxxxxxxxxxxxxx>
> Cc: Sandeep Patil <patil_sandeep@xxxxxxxxxxxxxx>
> Cc: Rui Miguel Silva <rui.silva@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - Add the hunk adding select GPIOLIB_IRQCHIP to Kconfig,
>   sorry for sending patches too late in the night.
> 
> Greybus folks: please look at this. I expect something like this
> to be applied before migrating from staging, I can't test it
> obviously.

Yeah, ripping out the inline gpiolib-irqchip backport has been on the
TODO list for quite some time now. This was used to support some ancient
vendor kernels, which we no longer need to worry about. Thanks for
taking a stab at it.

But please split this up in two patches for the gpiolib-irqchip
conversion and the switch to using the new gpiochip_add_data()
interface.

>  static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  			 const struct gbphy_device_id *id)
>  {
> @@ -690,7 +556,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	gpio->get = gb_gpio_get;
>  	gpio->set = gb_gpio_set;
>  	gpio->set_debounce = gb_gpio_set_debounce;
> -	gpio->to_irq = gb_gpio_to_irq;
>  	gpio->base = -1;		/* Allocate base dynamically */
>  	gpio->ngpio = ggc->line_max + 1;
>  	gpio->can_sleep = true;
> @@ -699,26 +564,26 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	if (ret)
>  		goto exit_line_free;
>  
> -	ret = gb_gpio_irqchip_add(gpio, irqc, 0,
> -				   handle_level_irq, IRQ_TYPE_NONE);
> +	ret = devm_gpiochip_add_data(&connection->bundle->dev,

As Viresh already noted, this should use the gbphy_dev device that is
being probed rather than the parent bundle device.

> +				     gpio, ggc);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,
> -			"failed to add irq chip: %d\n", ret);
> +			"failed to add gpio chip: %d\n", ret);
>  		goto exit_line_free;
>  	}
>  
> -	ret = gpiochip_add(gpio);
> +	ret = gpiochip_irqchip_add(gpio, irqc,
> +				   0, handle_level_irq,
> +				   IRQ_TYPE_NONE);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,
> -			"failed to add gpio chip: %d\n", ret);
> -		goto exit_gpio_irqchip_remove;
> +			"failed to add gpio irqchip: %d\n", ret);
> +		goto exit_line_free;
>  	}
>  
>  	gbphy_runtime_put_autosuspend(gbphy_dev);
>  	return 0;
>  
> -exit_gpio_irqchip_remove:
> -	gb_gpio_irqchip_remove(ggc);

You need to deregister the gpiochip here, before parts of our state is
freed.

>  exit_line_free:
>  	kfree(ggc->lines);
>  exit_connection_disable:

And the connection should really have been disabled before freeing lines
as well (separate prior bug).

> @@ -741,8 +606,6 @@ static void gb_gpio_remove(struct gbphy_device *gbphy_dev)
>  		gbphy_runtime_get_noresume(gbphy_dev);
>  
>  	gb_connection_disable_rx(connection);
> -	gpiochip_remove(&ggc->chip);
> -	gb_gpio_irqchip_remove(ggc);

This is similarly problematic, as again parts of our private state is
released below while the gpiochip is still registered.

I suggest simply not using the devres interface in probe.

>  	gb_connection_disable(connection);
>  	gb_connection_destroy(connection);
>  	kfree(ggc->lines);

Thanks,
Johan