Re: [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

[foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
the series, as you will need ack from us and we don't necessarily monitor
every single message on linux-input]

On Sep 20 2022, Matt Ranostay wrote:
> Switch from i2c_add_adapter() to resource managed devm_i2c_add_adapter()
> for matching rest of driver initialization, and more concise code.
> 
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/hid/hid-mcp2221.c | 45 +++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index de52e9f7bb8c..7ba63bcd66de 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  	return 1;
>  }
>  
> +static void mcp2221_hid_remove(void *ptr)
> +{
> +	struct hid_device *hdev = ptr;
> +
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);

By default, if you remove the .remove() callback, hid_hw_stop() will get
automatically called by hid-core.c. So we are now calling it twice,
which, in a way is not a big deal but it might be an issue in the long
run.

Generally speaking, in the HID subsystem, that situation doesn't happen
a lot because hid_hw_start() is usually the last command of probe, and
we don't need to open the device in the driver itself.

Here, I guess as soon as you add the i2c adapter, you might want to have
the communication channels ready, and thus you need to have it open
*before* i2c_add_adapter.

I would suggest the following if you want to keep the devm release of
stop and close: please put a big fat warning before mcp2221_hid_remove()
explaining that this is called in devm management, *and* add a function
that would just return 0 as the .remove() callback with another big fat
warning explaining that we don't want hid-core.c to call hid_hw_stop()
because we are doing it ourself through devres.

Last, in the HID subsystem, we often interleave non devres with devres
for resource allocation, given that .remove() will be called before any
devres release. But that is assuming this ordering is OK, which doesn't
seem to be the case here. We first need to unregister the i2c adapter
and then close/stop the HID device.

> +}
> +
>  static int mcp2221_probe(struct hid_device *hdev,
>  					const struct hid_device_id *id)
>  {
> @@ -849,7 +857,8 @@ static int mcp2221_probe(struct hid_device *hdev,
>  	ret = hid_hw_open(hdev);
>  	if (ret) {
>  		hid_err(hdev, "can't open device\n");
> -		goto err_hstop;
> +		hid_hw_stop(hdev);
> +		return ret;
>  	}
>  
>  	mutex_init(&mcp->lock);
> @@ -857,6 +866,10 @@ static int mcp2221_probe(struct hid_device *hdev,
>  	hid_set_drvdata(hdev, mcp);
>  	mcp->hdev = hdev;
>  
> +	ret = devm_add_action_or_reset(&hdev->dev, mcp2221_hid_remove, hdev);
> +	if (ret)
> +		return ret;
> +
>  	/* Set I2C bus clock diviser */
>  	if (i2c_clk_freq > 400)
>  		i2c_clk_freq = 400;
> @@ -873,19 +886,17 @@ static int mcp2221_probe(struct hid_device *hdev,
>  			"MCP2221 usb-i2c bridge on hidraw%d",
>  			((struct hidraw *)hdev->hidraw)->minor);
>  
> -	ret = i2c_add_adapter(&mcp->adapter);
> +	ret = devm_i2c_add_adapter(&hdev->dev, &mcp->adapter);
>  	if (ret) {
>  		hid_err(hdev, "can't add usb-i2c adapter: %d\n", ret);
> -		goto err_i2c;
> +		return ret;
>  	}
>  	i2c_set_adapdata(&mcp->adapter, mcp);
>  
>  	/* Setup GPIO chip */
>  	mcp->gc = devm_kzalloc(&hdev->dev, sizeof(*mcp->gc), GFP_KERNEL);
> -	if (!mcp->gc) {
> -		ret = -ENOMEM;
> -		goto err_gc;
> -	}
> +	if (!mcp->gc)
> +		return -ENOMEM;
>  
>  	mcp->gc->label = "mcp2221_gpio";
>  	mcp->gc->direction_input = mcp_gpio_direction_input;
> @@ -900,26 +911,9 @@ static int mcp2221_probe(struct hid_device *hdev,
>  
>  	ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
>  	if (ret)
> -		goto err_gc;
> +		return ret;
>  
>  	return 0;
> -
> -err_gc:
> -	i2c_del_adapter(&mcp->adapter);
> -err_i2c:
> -	hid_hw_close(mcp->hdev);
> -err_hstop:
> -	hid_hw_stop(mcp->hdev);
> -	return ret;
> -}
> -
> -static void mcp2221_remove(struct hid_device *hdev)
> -{
> -	struct mcp2221 *mcp = hid_get_drvdata(hdev);
> -
> -	i2c_del_adapter(&mcp->adapter);
> -	hid_hw_close(mcp->hdev);
> -	hid_hw_stop(mcp->hdev);
>  }
>  
>  static const struct hid_device_id mcp2221_devices[] = {
> @@ -932,7 +926,6 @@ static struct hid_driver mcp2221_driver = {
>  	.name		= "mcp2221",
>  	.id_table	= mcp2221_devices,
>  	.probe		= mcp2221_probe,
> -	.remove		= mcp2221_remove,
>  	.raw_event	= mcp2221_raw_event,
>  };
>  
> -- 
> 2.37.2
> 

Cheers,
Benjamin