[PATCHv3] support for AD5820 camera auto-focus coil
Commit Message
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.
> 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?
Comments
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;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -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;
}