lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <57441BF8.60606@gmail.com> Date: Tue, 24 May 2016 12:16:40 +0300 From: Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com> To: Pavel Machek <pavel@....cz> Cc: pali.rohar@...il.com, sre@...nel.org, kernel list <linux-kernel@...r.kernel.org>, linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>, linux-omap@...r.kernel.org, tony@...mide.com, khilman@...nel.org, aaro.koskinen@....fi, patrikbachan@...il.com, serge@...lyn.com, linux-media@...r.kernel.org, mchehab@....samsung.com, sakari.ailus@....fi Subject: Re: [PATCHv3] support for AD5820 camera auto-focus coil On 24.05.2016 12:04, Pavel Machek wrote: > Hi! > >>> +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"); >> >> 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: 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. >>> +#define AD5820_RAMP_MODE_LINEAR (0 << 3) >>> +#define AD5820_RAMP_MODE_64_16 (1 << 3) >>> + >>> +struct ad5820_platform_data { >>> + int (*set_xshutdown)(struct v4l2_subdev *subdev, int set); >>> +}; >>> + >>> +#define to_ad5820_device(sd) container_of(sd, struct ad5820_device, subdev) >>> + >>> +struct ad5820_device { >>> + struct v4l2_subdev subdev; >>> + struct ad5820_platform_data *platform_data; >>> + struct regulator *vana; >>> + >>> + struct v4l2_ctrl_handler ctrls; >>> + u32 focus_absolute; >>> + u32 focus_ramp_time; >>> + u32 focus_ramp_mode; >>> + >>> + struct mutex power_lock; >>> + int power_count; >>> + >>> + int standby : 1; >>> +}; >>> + >> >> The same for struct ad5820_device, is it really part of the public API? > > Let me check what can be done with it. > Pavel >
Powered by blists - more mailing lists