Hi!
> > + * Contact: Tuukka Toivonen
> > + * Sakari Ailus
>
> Could you put the e-mail addresses back, please?
>
> Tuukka's e-mail is tuukkat76 at gmail.com .
Ok.
> > +#include <linux/module.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/regulator/consumer.h>
>
> Alphabetical order would be nice. The same below.
I was afraid you'd ask. Ok.
> > +/**
> > + * @brief I2C write using i2c_transfer().
> > + * @param coil - the driver data structure
> > + * @param data - register value to be written
>
> This does not look entirely right. But you could just remove the entire
> comment. It's useless.
Ok.
> > + int ret = 0;
> > +
> > + /*
> > + * Go to standby first as real power off my be denied by the hardware
> > + * (single power line control for both coil and sensor).
> > + */
> > + if (standby) {
> > + coil->standby = 1;
> > + ret = ad5820_update_hw(coil);
> > + }
> > +
> > + ret |= regulator_disable(coil->vana);
>
> This is actually an error code and you shouldn't use or to combine two error
> codes. The result will make no sense.
>
> It might be the driver did this in the past but it should not be done. The
> right thing, as elsewhere, is to assign the value to ret and check it. The
> assigment in declaration may be dropped as well.
Yeah, its broken. Let me fix it.
> I think this happens in a few places in the driver.
Actually this was the only place left.
> > + struct ad5820_device *coil = to_ad5820_device(subdev);
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > +
> > + coil->vana = regulator_get(&client->dev, "VANA");
>
> Is there a reason not to acquire this in probe instead?
Yeah, new version will do that (already done, Dmitry was faster).
> > + if (IS_ERR(coil->vana)) {
> > + dev_err(&client->dev, "could not get regulator for vana\n");
> > + return -ENODEV;
>
> I wonder if -EPROBE_DEFER might be the right error code here.
..and should handle PROBE_DEFER, too.
> > +static int ad5820_probe(struct i2c_client *client,
> > + const struct i2c_device_id *devid)
> > +{
> > + struct ad5820_device *coil;
> > + int ret = 0;
>
> No need to assign ret here.
Ok.
> > +
> > + coil = kzalloc(sizeof(*coil), GFP_KERNEL);
>
> You could use devm_kzalloc() here and drop kfree() below and in _remove().
>
> The driver might be actually older than the devm_*() functions. Not sure. At
> least they were not widely used back then. :-)
Already done, Dmitry was faster.
> > +static int __exit ad5820_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct ad5820_device *coil = to_ad5820_device(subdev);
> > +
> > + 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);
>
> mutex_destroy(&coil->power_lock);
>
> Here. And in probe() error paths as well.
Ok. Can do, altrough it is pretty much a NOP in the error paths.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
