On Sun, Sep 18, 2022 at 8:39 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Mon, 12 Sep 2022 10:32:01 -0700
> Matt Ranostay <matt.ranostay@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> 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);
> > }
>
