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

From: Jonathan Cameron <jic23@kernel.org>
To: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: gupt21@gmail.com, linux-iio@vger.kernel.org,
	linux-input@vger.kernel.org, linux-i2c@vger.kernel.org
Subject: Re: [PATCH v3 4/5] HID: mcp2221: switch i2c registration to devm functions
Date: Sun, 18 Sep 2022 16:39:33 +0100	[thread overview]
Message-ID: <20220918163933.0e52b581@jic23-huawei> (raw)
In-Reply-To: <20220912173202.16723-5-matt.ranostay@konsulko.com>

On Mon, 12 Sep 2022 10:32:01 -0700
Matt Ranostay <matt.ranostay@konsulko.com> 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>

This doesn't necessarily make things worse, but I'm not keen on the
potential ordering issues that result form mixed devm / non-devm
in this function.  It's too hard too think about!

Easiest way to avoid people staring at the code to figure out if
there are nasty issues would be to take the whole thing devm
with a couple of devm_add_action_or_reset() to handle the
hid_hw_stop()/hid_hw_close() at right points in the error / remove
flows.

Jonathan

> ---
>  drivers/hid/hid-mcp2221.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index de52e9f7bb8c..29e69576c3d4 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -873,7 +873,7 @@ 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;
> @@ -884,7 +884,7 @@ static int mcp2221_probe(struct hid_device *hdev,
>  	mcp->gc = devm_kzalloc(&hdev->dev, sizeof(*mcp->gc), GFP_KERNEL);
>  	if (!mcp->gc) {
>  		ret = -ENOMEM;
> -		goto err_gc;
> +		goto err_i2c;
>  	}
>  
>  	mcp->gc->label = "mcp2221_gpio";
> @@ -900,12 +900,10 @@ static int mcp2221_probe(struct hid_device *hdev,
>  
>  	ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
>  	if (ret)
> -		goto err_gc;
> +		goto err_i2c;
>  
>  	return 0;
>  
> -err_gc:
> -	i2c_del_adapter(&mcp->adapter);
>  err_i2c:
>  	hid_hw_close(mcp->hdev);
>  err_hstop:
> @@ -917,7 +915,6 @@ 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);
>  }