> From: linux-omap-owner@xxxxxxxxxxxxxxx [mailto:linux-omap- > owner@xxxxxxxxxxxxxxx] On Behalf Of Alexey Klimov > Sent: Friday, December 12, 2008 12:03 AM > On Thu, Dec 11, 2008 at 11:38 PM, Aguirre Rodriguez, Sergio Alberto > <saaguirre@xxxxxx> wrote: > > >From 1341b74f5a90e4aa079a4fcb4fe2127ff344cce7 Mon Sep 17 00:00:00 2001 > > From: Sergio Aguirre <saaguirre@xxxxxx> > > Date: Thu, 11 Dec 2008 13:35:46 -0600 > > Subject: [PATCH] OMAP: CAM: Add Lens Driver > > > > This adds the DW9710 Lens driver > > > > Signed-off-by: Sergio Aguirre <saaguirre@xxxxxx> > > --- > > +#define DRIVER_NAME "dw9710" > > Here you define DRIVER_NAME. I saw patch(for another module) that > remover MODULE_NAME and places VIVI_MODULE_NAME there. Here it is: > http://linuxtv.org/hg/v4l-dvb/rev/58b95134acb8 > > May be it's better to define something like this: #define > DW9710_DRIVER_NAME "dw9710" ? > > Probably it will help you to avoid possible namespace conflicts and > code become a bit more readible. [Aguirre, Sergio] You're right. Thanks. > > > +static int > > +dw9710_probe(struct i2c_client *client, const struct i2c_device_id > *id); > > +static int __exit dw9710_remove(struct i2c_client *client); > > + > > +struct dw9710_device { > > + const struct dw9710_platform_data *pdata; > > + struct v4l2_int_device *v4l2_int_device; > > + struct i2c_client *i2c_client; > > + int opened; > > + u16 current_lens_posn; > > + u16 saved_lens_posn; > > + int state; > > + int power_state; > > +}; > > + > > +static const struct i2c_device_id dw9710_id[] = { > > + { DW9710_NAME, 0 }, > > + { } > > +}; > > Hmmm, looks like you already defined DW9710_NAME in header-file. Why > didn't you reformat to use _one_ define for this and previous place ? > > > +MODULE_DEVICE_TABLE(i2c, dw9710_id); > > + > > +static struct i2c_driver dw9710_i2c_driver = { > > + .driver = { > > + .name = DW9710_NAME, > > Actually, the same thing here. > [Aguirre, Sergio] Ok, done. Now I use only one define: DW9710_NAME. Thanks. > > +static int > > +find_vctrl(int id) > > +{ > > + int i; > > + > > + if (id < V4L2_CID_BASE) > > + return -EDOM; > > + > > + for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--) > > + if (video_control[i].qc.id == id) > > + break; > > + if (i < 0) > > + i = -EINVAL; > > + return i; > > +} > > In this function you use return after if, and in second place you set > i equals to -EINVAL, and then return. Probably, to make it more easily > to read it's better to do such thing: > > if (i < 0) > return -EINVAL; > [Aguirre, Sergio] Ok, I agree it's a bit confusing... I have rewritten the function to be more simple, is it ok now?: static int find_vctrl(int id) { int i; if (id < V4L2_CID_BASE) return -EDOM; for (i = (ARRAY_SIZE(video_control) - 1); i >= 0; i--) { if (video_control[i].qc.id == id) return i; } return -EINVAL; } > > + if (wposn != rposn) { > > + printk(KERN_ERR "W/R MISMATCH!\n"); > > If I were writing it, I'd do the following: > > printk(KERN_ERR DRIVER_NAME "W/R MISMATCH!\n"); > > And driver-name have to be unique, see above. > To be onest it's better to provide module name to dmesg if you(or any > user) want to catch bad behavour. [Aguirre, Sergio] Ok, point taken. Thanks. > > +int dw9710_af_getfocus(u16 *value) > > +{ > > + int ret = -EINVAL; > > + u16 posn = 0; > > Why just don't use int ret; > And later in code use return -EINVAL; ? > Anyway, you change ret variable with reg_read call. [Aguirre, Sergio] Ok, Makes sense. Thanks. > > > + struct dw9710_device *af_dev = &dw9710; > > + struct i2c_client *client = af_dev->i2c_client; > > + > > + if ((af_dev->power_state == V4L2_POWER_OFF) || > > + (af_dev->power_state == V4L2_POWER_STANDBY)) > > + return ret; > > + > > + ret = camaf_reg_read(client, &posn); > > + > > + if (ret) { > > + printk(KERN_ERR "Read of current Lens position > failed\n"); > > + return ret; > > + } > > + *value = CAMAF_DW9710_DATA_R(posn); > > + dw9710.current_lens_posn = CAMAF_DW9710_DATA_R(posn); > > + return ret; > > +} > > +EXPORT_SYMBOL(dw9710_af_getfocus); > > + if ((on == V4L2_POWER_ON) && (lens->state == LENS_NOT_DETECTED)) > { > > + rval = dw9710_detect(c); > > + if (rval < 0) { > > + dev_err(&c->dev, "Unable to detect " > > + DRIVER_NAME " lens HW\n"); > > + printk(KERN_ERR "Unable to detect " > > + DRIVER_NAME " lens HW\n"); > > Two thing here. As i know it's more preferable to use dev_macro > instead of printk if possible. > As i know(may be i'm wrong) it's not good to use DRIVER_NAME in dev_err > here. > Why do you print the same text two times here ? [Aguirre, Sergio] Ok, cleaning this up. Thanks > > +static int __init dw9710_init(void) > > +{ > > + int err = -EINVAL; > > int err = something, and then changed with call to i2c.. > > This is all i can suggest, it's not about functionality, it's about > coding style.. > Please, use dev_macros if possible instead of printk. > [Aguirre, Sergio] Ok, will do that. Thanks I really appreciate all your time doing this. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
