From: Pavel Machek <pavel@ucw.cz> To: Sakari Ailus <sakari.ailus@iki.fi> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>, pali.rohar@gmail.com, sre@kernel.org, kernel list <linux-kernel@vger.kernel.org>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, linux-omap@vger.kernel.org, tony@atomide.com, khilman@kernel.org, aaro.koskinen@iki.fi, patrikbachan@gmail.com, serge@hallyn.com, linux-media@vger.kernel.org, mchehab@osg.samsung.com Subject: Re: [PATCHv4] support for AD5820 camera auto-focus coil Date: Fri, 27 May 2016 22:33:11 +0200 [thread overview] Message-ID: <20160527203311.GA13282@amd> (raw) In-Reply-To: <20160525212659.GK26360@valkosipuli.retiisi.org.uk> 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
WARNING: multiple messages have this Message-ID (diff)
From: pavel@ucw.cz (Pavel Machek) To: linux-arm-kernel@lists.infradead.org Subject: [PATCHv4] support for AD5820 camera auto-focus coil Date: Fri, 27 May 2016 22:33:11 +0200 [thread overview] Message-ID: <20160527203311.GA13282@amd> (raw) In-Reply-To: <20160525212659.GK26360@valkosipuli.retiisi.org.uk> 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
next prev parent reply other threads:[~2016-05-27 20:33 UTC|newest] Thread overview: 158+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-05-17 18:19 [PATCH] support for AD5820 camera auto-focus coil Pavel Machek 2016-05-17 18:19 ` Pavel Machek 2016-05-17 18:33 ` Marcus Folkesson 2016-05-17 18:33 ` Marcus Folkesson 2016-05-18 8:30 ` Pavel Machek 2016-05-18 8:30 ` Pavel Machek 2016-05-21 5:43 ` [PATCHv2] " Pavel Machek 2016-05-21 5:43 ` Pavel Machek 2016-05-21 6:25 ` Ivaylo Dimitrov 2016-05-21 6:25 ` Ivaylo Dimitrov 2016-05-21 10:56 ` [PATCHv3] " Pavel Machek 2016-05-21 10:56 ` Pavel Machek 2016-05-21 11:43 ` Ivaylo Dimitrov 2016-05-21 11:43 ` Ivaylo Dimitrov 2016-05-23 7:41 ` Pali Rohár 2016-05-23 7:41 ` Pali Rohár 2016-05-23 7:41 ` Pali Rohár 2016-05-24 9:04 ` Pavel Machek 2016-05-24 9:04 ` Pavel Machek 2016-05-24 9:16 ` Ivaylo Dimitrov 2016-05-24 9:16 ` Ivaylo Dimitrov 2016-05-24 20:20 ` Pavel Machek 2016-05-24 20:20 ` Pavel Machek 2016-05-26 3:46 ` Ivaylo Dimitrov 2016-05-26 3:46 ` Ivaylo Dimitrov 2016-05-24 9:17 ` [PATCHv4] " Pavel Machek 2016-05-24 9:17 ` Pavel Machek 2016-05-25 21:26 ` Sakari Ailus 2016-05-25 21:26 ` Sakari Ailus 2016-05-27 20:33 ` Pavel Machek [this message] 2016-05-27 20:33 ` Pavel Machek 2016-05-27 20:51 ` [PATCHv5] " Pavel Machek 2016-05-27 20:51 ` Pavel Machek 2016-05-31 21:22 ` Sakari Ailus 2016-05-31 21:22 ` Sakari Ailus 2016-05-31 21:34 ` Pavel Machek 2016-05-31 21:34 ` Pavel Machek 2016-06-01 15:24 ` Sakari Ailus 2016-06-01 15:24 ` Sakari Ailus 2016-06-01 22:08 ` Pavel Machek 2016-06-01 22:08 ` Pavel Machek 2016-06-02 7:45 ` Sakari Ailus 2016-06-02 7:45 ` Sakari Ailus 2016-06-02 19:27 ` Pavel Machek 2016-06-02 19:27 ` Pavel Machek 2016-06-02 21:23 ` Sakari Ailus 2016-06-02 21:23 ` Sakari Ailus 2016-06-02 19:30 ` [PATCH] device tree description " Pavel Machek 2016-06-02 19:30 ` Pavel Machek 2016-06-02 19:30 ` Pavel Machek 2016-06-02 21:27 ` Sakari Ailus 2016-06-02 21:27 ` Sakari Ailus 2016-06-03 6:19 ` Pavel Machek 2016-06-03 6:19 ` Pavel Machek 2016-06-05 19:07 ` [PATCH] userspace API definitions for " Pavel Machek 2016-06-05 19:07 ` Pavel Machek 2016-06-05 19:07 ` Pavel Machek 2016-06-06 6:06 ` Ivaylo Dimitrov 2016-06-06 6:06 ` Ivaylo Dimitrov 2016-06-06 7:21 ` Pavel Machek 2016-06-06 7:21 ` Pavel Machek 2016-06-11 22:06 ` Sakari Ailus 2016-06-11 22:06 ` Sakari Ailus 2016-06-11 22:06 ` Sakari Ailus 2016-06-12 7:54 ` Pavel Machek 2016-06-12 7:54 ` Pavel Machek 2016-06-17 21:28 ` Sakari Ailus 2016-06-17 21:28 ` Sakari Ailus 2016-06-17 21:28 ` Sakari Ailus 2016-06-12 8:48 ` Pavel Machek 2016-06-12 8:48 ` Pavel Machek 2016-06-12 11:22 ` Sakari Ailus 2016-06-12 11:22 ` Sakari Ailus 2016-06-12 11:22 ` Sakari Ailus 2016-06-13 19:17 ` Pavel Machek 2016-06-13 19:17 ` Pavel Machek 2016-06-13 19:17 ` Pavel Machek 2016-06-17 21:35 ` Sakari Ailus 2016-06-17 21:35 ` Sakari Ailus 2016-06-18 15:37 ` [PATCHv4] support for AD5820 camera " Pavel Machek 2016-06-18 15:37 ` Pavel Machek 2016-06-18 15:38 ` [PATCH] userspace API definitions for " Pavel Machek 2016-06-18 15:38 ` Pavel Machek 2016-07-12 23:32 ` Mauro Carvalho Chehab 2016-07-12 23:32 ` Mauro Carvalho Chehab 2016-07-12 23:32 ` Mauro Carvalho Chehab 2016-07-13 6:57 ` Pavel Machek 2016-07-13 6:57 ` Pavel Machek 2016-07-13 7:26 ` Pavel Machek 2016-07-13 7:26 ` Pavel Machek 2016-06-06 13:29 ` [PATCH] device tree description for AD5820 camera " Rob Herring 2016-06-06 13:29 ` Rob Herring 2016-06-07 7:10 ` [PATCHv2] " Pavel Machek 2016-06-07 7:10 ` Pavel Machek 2016-06-09 22:37 ` Sakari Ailus 2016-06-09 22:37 ` Sakari Ailus 2016-06-04 23:27 ` [PATCHv5] support " kbuild test robot 2016-06-04 23:27 ` kbuild test robot 2016-06-05 0:46 ` kbuild test robot 2016-06-05 0:46 ` kbuild test robot 2016-06-12 20:41 ` [PATCH 1/1] v4l: Add camera voice coil lens control class, current control Sakari Ailus 2016-06-12 21:48 ` Ivaylo Dimitrov 2016-06-17 22:11 ` Pavel Machek 2016-06-17 22:39 ` Laurent Pinchart 2016-06-18 11:28 ` Pavel Machek 2016-06-17 22:06 ` Pavel Machek 2016-08-05 10:26 ` [PATCHv6] support for AD5820 camera auto-focus coil Pavel Machek 2016-08-05 10:26 ` Pavel Machek 2016-08-05 10:30 ` Pali Rohár 2016-08-05 10:30 ` Pali Rohár 2016-08-08 8:09 ` Sakari Ailus 2016-08-08 8:09 ` Sakari Ailus 2016-08-08 21:41 ` Pavel Machek 2016-08-08 21:41 ` Pavel Machek 2016-08-08 21:41 ` Pavel Machek 2016-08-10 12:01 ` Sakari Ailus 2016-08-10 12:01 ` Sakari Ailus 2016-08-10 12:01 ` Sakari Ailus 2016-08-08 23:23 ` Pavel Machek 2016-08-08 23:23 ` Pavel Machek 2016-08-08 23:23 ` Pavel Machek 2016-08-11 11:16 ` Sakari Ailus 2016-08-11 11:16 ` Sakari Ailus 2016-08-18 10:45 ` Pavel Machek 2016-08-18 10:45 ` Pavel Machek 2016-08-18 20:26 ` Sakari Ailus 2016-08-18 20:26 ` Sakari Ailus 2016-08-18 20:26 ` Sakari Ailus 2016-08-18 21:28 ` Pavel Machek 2016-08-18 21:28 ` Pavel Machek 2016-08-18 21:28 ` Pavel Machek 2016-11-03 10:27 ` Pavel Machek 2016-11-03 10:27 ` Pavel Machek 2016-11-03 21:49 ` Sakari Ailus 2016-11-03 21:49 ` Sakari Ailus 2016-11-04 7:45 ` Pavel Machek 2016-11-04 7:45 ` Pavel Machek 2016-11-04 14:49 ` Tony Lindgren 2016-11-04 14:49 ` Tony Lindgren 2016-12-14 13:38 ` Pali Rohár 2016-12-14 13:38 ` Pali Rohár 2016-12-14 15:08 ` Tony Lindgren 2016-12-14 15:08 ` Tony Lindgren 2016-12-14 15:08 ` Tony Lindgren 2016-12-15 6:50 ` Sakari Ailus 2016-12-15 6:50 ` Sakari Ailus 2016-12-15 6:50 ` Sakari Ailus 2016-12-19 22:23 ` Pavel Machek 2016-12-19 22:23 ` Pavel Machek 2016-08-10 7:34 ` Pali Rohár 2016-08-10 7:34 ` Pali Rohár 2016-08-10 7:34 ` Pali Rohár 2016-08-08 21:40 ` Pavel Machek 2016-08-08 21:40 ` Pavel Machek 2016-08-08 21:40 ` Pavel Machek 2016-08-10 12:00 ` Sakari Ailus 2016-08-10 12:00 ` Sakari Ailus 2016-08-10 12:00 ` Sakari Ailus
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160527203311.GA13282@amd \ --to=pavel@ucw.cz \ --cc=aaro.koskinen@iki.fi \ --cc=ivo.g.dimitrov.75@gmail.com \ --cc=khilman@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=mchehab@osg.samsung.com \ --cc=pali.rohar@gmail.com \ --cc=patrikbachan@gmail.com \ --cc=sakari.ailus@iki.fi \ --cc=serge@hallyn.com \ --cc=sre@kernel.org \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.