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

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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

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);
>  }

On Sun, Sep 18, 2022 at 8:39 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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.

Good idea. Even if .remove() may be valid with the device resource
management it is a bit confusing.

- Matt

>
> 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);
> >  }
>