| Message ID | 20160524202059.GB18536@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } >
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; }