Messages in this thread | | | Subject | Re: [PATCHv3] support for AD5820 camera auto-focus coil | From | Ivaylo Dimitrov <> | Date | Thu, 26 May 2016 06:46:08 +0300 |
| |
On 24.05.2016 23:20, Pavel Machek wrote: > Hi! > >>>> devm_regulator_get()? >>> >>> I'd rather avoid devm_ here. Driver is simple enough to allow it. >>> >> >> Now thinking about it, what would happen here if regulator_get() returns >> -EPROBE_DEFER? Wouldn't it be better to move regulator_get to the probe() >> function, something like: > > Ok, I can do it. > > Oh, and don't try to complain about newlines before returns. It looks > better this way. >
All I complain about is missing empty line before a final return at the end of a function :). I guess it is a matter of preference though.
>> static int ad5820_probe(struct i2c_client *client, >> const struct i2c_device_id *devid) >> { >> struct ad5820_device *coil; >> int ret = 0; >> >> coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL); >> if (coil == NULL) >> return -ENOMEM; >> >> coil->vana = devm_regulator_get(&client->dev, NULL); >> if (IS_ERR(coil->vana)) { >> ret = PTR_ERR(coil->vana); >> if (ret != -EPROBE_DEFER) >> dev_err(&client->dev, "could not get regulator for vana\n"); >> return ret; >> } >> >> mutex_init(&coil->power_lock); >> ... >> >> with the appropriate changes to remove() because of the devm API >> usage. > > Something like this? >
Didn't try to apply it, but yes, looks right. Besides the missing mutex_destroy(&coil->power_lock); pointed out by Sakari.
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c > index f956bd3..f871366 100644 > --- a/drivers/media/i2c/ad5820.c > +++ b/drivers/media/i2c/ad5820.c > @@ -8,7 +8,7 @@ > * Copyright (C) 2016 Pavel Machek <pavel@ucw.cz> > * > * Contact: Tuukka Toivonen > - * Sakari Ailus > + * Sakari Ailus > * > * Based on af_d88.c by Texas Instruments. > * > @@ -263,13 +263,6 @@ static int ad5820_init_controls(struct ad5820_device *coil) > static int ad5820_registered(struct v4l2_subdev *subdev) > { > struct ad5820_device *coil = to_ad5820_device(subdev); > - struct i2c_client *client = v4l2_get_subdevdata(subdev); > - > - coil->vana = regulator_get(&client->dev, "VANA"); > - if (IS_ERR(coil->vana)) { > - dev_err(&client->dev, "could not get regulator for vana\n"); > - return -ENODEV; > - } > > return ad5820_init_controls(coil); > } > @@ -367,10 +360,18 @@ static int ad5820_probe(struct i2c_client *client, > struct ad5820_device *coil; > int ret = 0; > > - coil = kzalloc(sizeof(*coil), GFP_KERNEL); > + coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL); > if (!coil) > return -ENOMEM; > > + coil->vana = devm_regulator_get(&client->dev, NULL); > + if (IS_ERR(coil->vana)) { > + ret = PTR_ERR(coil->vana); > + if (ret != -EPROBE_DEFER) > + dev_err(&client->dev, "could not get regulator for vana\n"); > + return ret; > + } > + > mutex_init(&coil->power_lock); > > v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops); > @@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client, > > cleanup: > media_entity_cleanup(&coil->subdev.entity); > - > -free: > - kfree(coil); > - > return ret; > } > > @@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client) > v4l2_device_unregister_subdev(&coil->subdev); > v4l2_ctrl_handler_free(&coil->ctrls); > media_entity_cleanup(&coil->subdev.entity); > - if (coil->vana) > - regulator_put(coil->vana); > - > - kfree(coil); > - > return 0; > } >
| |