| Message ID | 1243582408-13084-2-git-send-email-eduardo.valentin@nokia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Douglas Landgraf |
| Headers | show |
On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > # HG changeset patch > # User Eduardo Valentin <eduardo.valentin@nokia.com> > # Date 1243414605 -10800 > # Branch export > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > # Parent 142fd6020df3b4d543068155e49a2618140efa49 > Device drivers of v4l2_subdev devices may want to have > board specific data. This patch adds an helper function > to allow bridge drivers to pass board specific data to > v4l2_subdev drivers. > > For those drivers which need to support kernel versions > bellow 2.6.26, a .s_config callback was added. The > idea of this callback is to pass board configuration > as well. In that case, subdev driver should set .s_config > properly, because v4l2_i2c_new_subdev_board will call > the .s_config directly. Of course, if we are >= 2.6.26, > the same data will be passed through i2c board info as well. Hi Eduardo, I finally had some time to look at this. After some thought I realized that the main problem is really that the API is becoming quite messy. Basically there are 9 different ways of loading and initializing a subdev: First there are three basic initialization calls: no initialization, passing irq and platform_data, and passing the i2c_board_info struct directly (preferred for drivers that don't need pre-2.6.26 compatibility). And for each flavor you would like to see three different versions as well: one with a fixed known i2c address, one where you probe for a list of addresses, and one where you can probe for a single i2c address. I propose to change the API as follows: #define V4L2_I2C_ADDRS(addr, addrs...) \ ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, const char *client_type, u8 addr, const unsigned short *addrs); struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, const char *client_type, int irq, void *platform_data, u8 addr, const unsigned short *addrs); /* Only available for kernels >= 2.6.26 */ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct i2c_board_info *info, const unsigned short *addrs); If you use a fixed address, then only set addr (or info.addr) and set addrs to NULL. If you want to probe for a list of addrs, then set addrs to the list of addrs. If you want to probe for a single addr, then use V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array with just two entries. Actually, this macro can also create arrays with more entries. Note that v4l2_i2c_new_subdev will be an inline that calls v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data. And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline calling v4l2_i2c_new_subdev_board. This approach reduces the number of functions to just one (not counting the inlines) and simplifies things all around. It does mean that all sources need to be changed, but if we go this route, then now is the time before the 2.6.31 window is closed. And I would also like to remove the '_new' from these function names. I never thought it added anything useful. Comments? If we decide to go this way, then I need to know soon so that I can make the changes before the 2.6.31 window closes. BTW, if the new s_config subdev call is present, then it should always be called. That way the subdev driver can safely do all of its initialization in s_config, no matter how it was initialized. Sorry about the long delay in replying to this: it's been very hectic lately at the expense of my v4l-dvb work. Regards, Hans > > Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com> > --- > drivers/media/video/v4l2-common.c | 37 > +++++++++++++++++++++++++++++++++++-- include/linux/v4l2-common.h | > 8 ++++++++ > include/linux/v4l2-subdev.h | 1 + > 3 files changed, 44 insertions(+), 2 deletions(-) > > diff -r 142fd6020df3 -r 4fb354645426 > linux/drivers/media/video/v4l2-common.c --- > a/linux/drivers/media/video/v4l2-common.c Mon May 18 02:31:55 2009 +0000 > +++ b/linux/drivers/media/video/v4l2-common.c Wed May 27 11:56:45 2009 > +0300 @@ -819,9 +819,10 @@ > > > /* Load an i2c sub-device. */ > -struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, > +static struct v4l2_subdev *__v4l2_i2c_new_subdev(struct v4l2_device > *v4l2_dev, struct i2c_adapter *adapter, > - const char *module_name, const char *client_type, u8 addr) > + const char *module_name, const char *client_type, u8 addr, > + int irq, void *platform_data) > { > struct v4l2_subdev *sd = NULL; > struct i2c_client *client; > @@ -840,6 +841,8 @@ > memset(&info, 0, sizeof(info)); > strlcpy(info.type, client_type, sizeof(info.type)); > info.addr = addr; > + info.irq = irq; > + info.platform_data = platform_data; > > /* Create the i2c client */ > client = i2c_new_device(adapter, &info); > @@ -877,8 +880,38 @@ > #endif > return sd; > } > + > +struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, > + struct i2c_adapter *adapter, > + const char *module_name, const char *client_type, u8 addr) > +{ > + return __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name, > + client_type, addr, 0, NULL); > +} > EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev); > > +struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > *v4l2_dev, + struct i2c_adapter *adapter, > + const char *module_name, const char *client_type, u8 addr, > + int irq, void *platform_data) > +{ > + struct v4l2_subdev *sd; > + int err = 0; > + > + sd = __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name, client_type, > + addr, irq, platform_data); > + > + /* > + * We return errors from v4l2_subdev_call only if we have the callback > + * as the .s_config is not mandatory > + */ > + if (sd && sd->ops && sd->ops->core && sd->ops->core->s_config) > + err = sd->ops->core->s_config(sd, irq, platform_data); > + > + return err < 0 ? NULL : sd; > +} > +EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board); > + > /* Probe and load an i2c sub-device. */ > struct v4l2_subdev *v4l2_i2c_new_probed_subdev(struct v4l2_device > *v4l2_dev, struct i2c_adapter *adapter, > diff -r 142fd6020df3 -r 4fb354645426 linux/include/media/v4l2-common.h > --- a/linux/include/media/v4l2-common.h Mon May 18 02:31:55 2009 +0000 > +++ b/linux/include/media/v4l2-common.h Wed May 27 11:56:45 2009 +0300 > @@ -147,6 +147,14 @@ > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, > struct i2c_adapter *adapter, > const char *module_name, const char *client_type, u8 addr); > +/* > + * Same as v4l2_i2c_new_subdev, but with the opportunity to configure > + * subdevice with board specific data (irq and platform_data). > + */ > +struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > *v4l2_dev, + struct i2c_adapter *adapter, > + const char *module_name, const char *client_type, u8 addr, > + int irq, void *platform_data); > /* Probe and load an i2c module and return an initialized v4l2_subdev > struct. Only call request_module if module_name != NULL. > The client_type argument is the name of the chip that's on the > adapter. */ diff -r 142fd6020df3 -r 4fb354645426 > linux/include/media/v4l2-subdev.h --- > a/linux/include/media/v4l2-subdev.h Mon May 18 02:31:55 2009 +0000 +++ > b/linux/include/media/v4l2-subdev.h Wed May 27 11:56:45 2009 +0300 @@ > -96,6 +96,7 @@ > struct v4l2_subdev_core_ops { > int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident > *chip); int (*log_status)(struct v4l2_subdev *sd); > + int (*s_config)(struct v4l2_subdev *sd, int irq, void *platform_data); > int (*init)(struct v4l2_subdev *sd, u32 val); > int (*load_fw)(struct v4l2_subdev *sd); > int (*reset)(struct v4l2_subdev *sd, u32 val);
On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > > # HG changeset patch > > # User Eduardo Valentin <eduardo.valentin@nokia.com> > > # Date 1243414605 -10800 > > # Branch export > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > > # Parent 142fd6020df3b4d543068155e49a2618140efa49 > > Device drivers of v4l2_subdev devices may want to have > > board specific data. This patch adds an helper function > > to allow bridge drivers to pass board specific data to > > v4l2_subdev drivers. > > > > For those drivers which need to support kernel versions > > bellow 2.6.26, a .s_config callback was added. The > > idea of this callback is to pass board configuration > > as well. In that case, subdev driver should set .s_config > > properly, because v4l2_i2c_new_subdev_board will call > > the .s_config directly. Of course, if we are >= 2.6.26, > > the same data will be passed through i2c board info as well. > > Hi Eduardo, > > I finally had some time to look at this. After some thought I realized > that the main problem is really that the API is becoming quite messy. > Basically there are 9 different ways of loading and initializing a > subdev: > > First there are three basic initialization calls: no initialization, > passing irq and platform_data, and passing the i2c_board_info struct > directly (preferred for drivers that don't need pre-2.6.26 > compatibility). > > And for each flavor you would like to see three different versions as > well: one with a fixed known i2c address, one where you probe for a list > of addresses, and one where you can probe for a single i2c address. > > I propose to change the API as follows: > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, > struct i2c_adapter *adapter, > const char *module_name, const char *client_type, > u8 addr, const unsigned short *addrs); > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev, > struct i2c_adapter *adapter, > const char *module_name, const char *client_type, > int irq, void *platform_data, > u8 addr, const unsigned short *addrs); > > /* Only available for kernels >= 2.6.26 */ > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct > i2c_board_info *info, const unsigned short *addrs); > > If you use a fixed address, then only set addr (or info.addr) and set > addrs to NULL. If you want to probe for a list of addrs, then set addrs > to the list of addrs. If you want to probe for a single addr, then use > V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array with > just two entries. Actually, this macro can also create arrays with more > entries. > > Note that v4l2_i2c_new_subdev will be an inline that calls > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data. > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline > calling v4l2_i2c_new_subdev_board. > > This approach reduces the number of functions to just one (not counting > the inlines) and simplifies things all around. It does mean that all > sources need to be changed, but if we go this route, then now is the time > before the 2.6.31 window is closed. And I would also like to remove the > '_new' from these function names. I never thought it added anything > useful. > > Comments? If we decide to go this way, then I need to know soon so that I > can make the changes before the 2.6.31 window closes. > > BTW, if the new s_config subdev call is present, then it should always be > called. That way the subdev driver can safely do all of its > initialization in s_config, no matter how it was initialized. > > Sorry about the long delay in replying to this: it's been very hectic > lately at the expense of my v4l-dvb work. I've done the initial conversion to the new API (no _cfg or _board version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies things and if nobody objects then I'd like to get this in before 2.6.31. Regards, Hans
On Sat, 2009-06-06 at 14:49 +0200, Hans Verkuil wrote: > > I propose to change the API as follows: > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > Comments? If we decide to go this way, then I need to know soon so that I > > can make the changes before the 2.6.31 window closes. > I've done the initial conversion to the new API (no _cfg or _board version > yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies things and > if nobody objects then I'd like to get this in before 2.6.31. +Alternatively, you can create the unsigned short array dynamically: + +struct v4l2_subdev *sd = v4l2_i2c_subdev(v4l2_dev, adapter, + "module_foo", "chipid", 0, V4L2_I2C_ADDRS(0x10, 0x12)); Strictly speaking, that's not "dynamically" in the sense of the generated machine code - everything is going to come from the local stack and the initialized data space. The compiler will probably be smart enough to generate an unnamed array in the initialized data space anyway, avoiding the use of local stack for the array. :) Anyway, the macro looks fine to me. But... @@ -100,16 +100,16 @@ int cx18_i2c_register(struct cx18 *cx, u if (hw == CX18_HW_TUNER) { /* special tuner group handling */ - sd = v4l2_i2c_new_probed_subdev(&cx->v4l2_dev, - adap, mod, type, cx->card_i2c->radio); + sd = v4l2_i2c_subdev(&cx->v4l2_dev, + adap, mod, type, 0, cx->card_i2c->radio); Something happened with readability for maintenance purposes. We're in cx18_i2c_register(), we're probing, we're allocating new objects, and we're registering with two subsystems (i2c and v4l). However, all we see on the surface is "foo = v4l2_i2c_subdev(blah, blah, blah, ... );" The ALSA subsystem at least uses "_create" for object constructor type functions. The v4l2 subdev framework has sophisticated constructors for convenience. I know "new" wasn't strcitly correct, as the function does probe, create, & register an object. However, the proposed name does not make it obvious that it's a constructor, IMO. Regards, Andy > Regards, > > Hans -- 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
On Saturday 06 June 2009 17:19:08 Andy Walls wrote: > On Sat, 2009-06-06 at 14:49 +0200, Hans Verkuil wrote: > > > I propose to change the API as follows: > > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > > ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > > > Comments? If we decide to go this way, then I need to know soon so > > > that I can make the changes before the 2.6.31 window closes. > > > > I've done the initial conversion to the new API (no _cfg or _board > > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies > > things and if nobody objects then I'd like to get this in before > > 2.6.31. > > +Alternatively, you can create the unsigned short array dynamically: > + > +struct v4l2_subdev *sd = v4l2_i2c_subdev(v4l2_dev, adapter, > + "module_foo", "chipid", 0, V4L2_I2C_ADDRS(0x10, 0x12)); > > Strictly speaking, that's not "dynamically" in the sense of the > generated machine code - everything is going to come from the local > stack and the initialized data space. The compiler will probably be > smart enough to generate an unnamed array in the initialized data space > anyway, avoiding the use of local stack for the array. :) 'on the fly' is perhaps a better term... > Anyway, the macro looks fine to me. > > But... > > > @@ -100,16 +100,16 @@ int cx18_i2c_register(struct cx18 *cx, u > > if (hw == CX18_HW_TUNER) { > /* special tuner group handling */ > - sd = v4l2_i2c_new_probed_subdev(&cx->v4l2_dev, > - adap, mod, type, cx->card_i2c->radio); > + sd = v4l2_i2c_subdev(&cx->v4l2_dev, > + adap, mod, type, 0, cx->card_i2c->radio); > > > Something happened with readability for maintenance purposes. We're in > cx18_i2c_register(), we're probing, we're allocating new objects, and > we're registering with two subsystems (i2c and v4l). However, all we > see on the surface is > > "foo = v4l2_i2c_subdev(blah, blah, blah, ... );" > > The ALSA subsystem at least uses "_create" for object constructor type > functions. The v4l2 subdev framework has sophisticated constructors for > convenience. I know "new" wasn't strcitly correct, as the function does > probe, create, & register an object. However, the proposed name does > not make it obvious that it's a constructor, IMO. Hmm, I should probably just leave this as v4l2_i2c_new_subdev since that corresponds to the i2c core's i2c_new_device call. Regards, Hans
On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > > > # HG changeset patch > > > # User Eduardo Valentin <eduardo.valentin@nokia.com> > > > # Date 1243414605 -10800 > > > # Branch export > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > > > # Parent 142fd6020df3b4d543068155e49a2618140efa49 > > > Device drivers of v4l2_subdev devices may want to have > > > board specific data. This patch adds an helper function > > > to allow bridge drivers to pass board specific data to > > > v4l2_subdev drivers. > > > > > > For those drivers which need to support kernel versions > > > bellow 2.6.26, a .s_config callback was added. The > > > idea of this callback is to pass board configuration > > > as well. In that case, subdev driver should set .s_config > > > properly, because v4l2_i2c_new_subdev_board will call > > > the .s_config directly. Of course, if we are >= 2.6.26, > > > the same data will be passed through i2c board info as well. > > > > Hi Eduardo, > > > > I finally had some time to look at this. After some thought I realized > > that the main problem is really that the API is becoming quite messy. > > Basically there are 9 different ways of loading and initializing a > > subdev: > > > > First there are three basic initialization calls: no initialization, > > passing irq and platform_data, and passing the i2c_board_info struct > > directly (preferred for drivers that don't need pre-2.6.26 > > compatibility). > > > > And for each flavor you would like to see three different versions as > > well: one with a fixed known i2c address, one where you probe for a > > list of addresses, and one where you can probe for a single i2c > > address. > > > > I propose to change the API as follows: > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, > > struct i2c_adapter *adapter, > > const char *module_name, const char *client_type, > > u8 addr, const unsigned short *addrs); > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > *v4l2_dev, struct i2c_adapter *adapter, > > const char *module_name, const char *client_type, > > int irq, void *platform_data, > > u8 addr, const unsigned short *addrs); > > > > /* Only available for kernels >= 2.6.26 */ > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct > > i2c_board_info *info, const unsigned short *addrs); > > > > If you use a fixed address, then only set addr (or info.addr) and set > > addrs to NULL. If you want to probe for a list of addrs, then set addrs > > to the list of addrs. If you want to probe for a single addr, then use > > V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array > > with just two entries. Actually, this macro can also create arrays with > > more entries. > > > > Note that v4l2_i2c_new_subdev will be an inline that calls > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data. > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline > > calling v4l2_i2c_new_subdev_board. > > > > This approach reduces the number of functions to just one (not counting > > the inlines) and simplifies things all around. It does mean that all > > sources need to be changed, but if we go this route, then now is the > > time before the 2.6.31 window is closed. And I would also like to > > remove the '_new' from these function names. I never thought it added > > anything useful. > > > > Comments? If we decide to go this way, then I need to know soon so that > > I can make the changes before the 2.6.31 window closes. > > > > BTW, if the new s_config subdev call is present, then it should always > > be called. That way the subdev driver can safely do all of its > > initialization in s_config, no matter how it was initialized. > > > > Sorry about the long delay in replying to this: it's been very hectic > > lately at the expense of my v4l-dvb work. > > I've done the initial conversion to the new API (no _cfg or _board > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies > things and if nobody objects then I'd like to get this in before 2.6.31. I've added the new _cfg and _board fucntions as well in this tree. It needs a bit of a cleanup before I can do a pull request (the last two patches should be merged to one), but otherwise this is the code as I think it should be: /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the fly */ #define V4L2_I2C_ADDRS(addr, addrs...) \ ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) /* Load an i2c module and return an initialized v4l2_subdev struct. Only call request_module if module_name != NULL. The client_type argument is the name of the chip that's on the adapter. */ struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, const char *client_type, int irq, void *platform_data, u8 addr, const unsigned short *addrs); static inline struct v4l2_subdev *v4l2_i2c_new_subdev( struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, const char *client_type, u8 addr, const unsigned short *addrs) { return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name, client_type, 0, NULL, addr, addrs); } #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26) struct i2c_board_info; struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct i2c_board_info *info, const unsigned short *addrs); #endif Regards, Hans
On Sat, 6 Jun 2009, Hans Verkuil wrote: > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > I propose to change the API as follows: > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, > > struct i2c_adapter *adapter, > > const char *module_name, const char *client_type, > > u8 addr, const unsigned short *addrs); > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev, > > struct i2c_adapter *adapter, > > const char *module_name, const char *client_type, > > int irq, void *platform_data, > > u8 addr, const unsigned short *addrs); > > > > /* Only available for kernels >= 2.6.26 */ > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct > > i2c_board_info *info, const unsigned short *addrs); Maybe "addrs" could be changed to something like "probed_addrs" so it's easier to tell that the two address fields are used differently? Is v4l2_i2c_new_subdev() in effect just a wrapper around v4l2_i2c_new_subdev_cfg() that calls it with NO_IRQ and NULL for irq and platform_data? And could v4l2_i2c_new_subdev_cfg() be done like this? struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, const char *client_type, int irq, void *platform_data, u8 addr, const unsigned short *addrs) { struct i2c_board_info info = { .addr = addr, .platform_data = platform_data, .irq = irq, }; strlcpy(info.type, client_type, sizeof(info.type)); return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, module_name, &info, addrs); } -- 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
On Sat, 6 Jun 2009, Andy Walls wrote: > +Alternatively, you can create the unsigned short array dynamically: > + > +struct v4l2_subdev *sd = v4l2_i2c_subdev(v4l2_dev, adapter, > + "module_foo", "chipid", 0, V4L2_I2C_ADDRS(0x10, 0x12)); > > Strictly speaking, that's not "dynamically" in the sense of the > generated machine code - everything is going to come from the local > stack and the initialized data space. The compiler will probably be > smart enough to generate an unnamed array in the initialized data space > anyway, avoiding the use of local stack for the array. :) No such luck, gcc will create an array on the stack and then initialize it with a series of move word instructions. It isn't even smart enough to turn: movw $1, (%esp) movw $2, 2(%esp) movw $3, 4(%esp) movw $-1, 6(%esp) into: movl $0x00020001, (%esp) movl $0xffff0003, 4(%esp) Now, if you use a different syntax, and change this: #define V4L2_I2C_ADDRS(addr, addrs...) \ ((const unsigned short []){ addr, ## addrs, -1 }) #define bar(addrs...) _bar(V4L2_I2C_ADDRS(addrs)) into this: #define bar(addr, addrs...) \ ({ const unsigned short _a[] = {addr, ## addrs, -1}; _bar(_a); }) If all the values are constants, then for the latter method only gcc will will create an array in the initialized data segment and use that. -- 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
Hi Hans, On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > > > > # HG changeset patch > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com> > > > > # Date 1243414605 -10800 > > > > # Branch export > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49 > > > > Device drivers of v4l2_subdev devices may want to have > > > > board specific data. This patch adds an helper function > > > > to allow bridge drivers to pass board specific data to > > > > v4l2_subdev drivers. > > > > > > > > For those drivers which need to support kernel versions > > > > bellow 2.6.26, a .s_config callback was added. The > > > > idea of this callback is to pass board configuration > > > > as well. In that case, subdev driver should set .s_config > > > > properly, because v4l2_i2c_new_subdev_board will call > > > > the .s_config directly. Of course, if we are >= 2.6.26, > > > > the same data will be passed through i2c board info as well. > > > > > > Hi Eduardo, > > > > > > I finally had some time to look at this. After some thought I realized > > > that the main problem is really that the API is becoming quite messy. > > > Basically there are 9 different ways of loading and initializing a > > > subdev: > > > > > > First there are three basic initialization calls: no initialization, > > > passing irq and platform_data, and passing the i2c_board_info struct > > > directly (preferred for drivers that don't need pre-2.6.26 > > > compatibility). > > > > > > And for each flavor you would like to see three different versions as > > > well: one with a fixed known i2c address, one where you probe for a > > > list of addresses, and one where you can probe for a single i2c > > > address. > > > > > > I propose to change the API as follows: > > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > >   ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, > > >         struct i2c_adapter *adapter, > > >         const char *module_name, const char *client_type, > > >       u8 addr, const unsigned short *addrs); > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > > *v4l2_dev, struct i2c_adapter *adapter, > > >         const char *module_name, const char *client_type, > > >         int irq, void *platform_data, > > >         u8 addr, const unsigned short *addrs); > > > > > > /* Only available for kernels >= 2.6.26 */ > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct > > > i2c_board_info *info, const unsigned short *addrs); > > > > > > If you use a fixed address, then only set addr (or info.addr) and set > > > addrs to NULL. If you want to probe for a list of addrs, then set addrs > > > to the list of addrs. If you want to probe for a single addr, then use > > > V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array > > > with just two entries. Actually, this macro can also create arrays with > > > more entries. > > > > > > Note that v4l2_i2c_new_subdev will be an inline that calls > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data. > > > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline > > > calling v4l2_i2c_new_subdev_board. > > > > > > This approach reduces the number of functions to just one (not counting > > > the inlines) and simplifies things all around. It does mean that all > > > sources need to be changed, but if we go this route, then now is the > > > time before the 2.6.31 window is closed. And I would also like to > > > remove the '_new' from these function names. I never thought it added > > > anything useful. > > > > > > Comments? If we decide to go this way, then I need to know soon so that > > > I can make the changes before the 2.6.31 window closes. > > > > > > BTW, if the new s_config subdev call is present, then it should always > > > be called. That way the subdev driver can safely do all of its > > > initialization in s_config, no matter how it was initialized. > > > > > > Sorry about the long delay in replying to this: it's been very hectic > > > lately at the expense of my v4l-dvb work. > > > > I've done the initial conversion to the new API (no _cfg or _board > > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies > > things and if nobody objects then I'd like to get this in before 2.6.31. > > I've added the new _cfg and _board fucntions as well in this tree. It needs > a bit of a cleanup before I can do a pull request (the last two patches > should be merged to one), but otherwise this is the code as I think it > should be: > > /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the fly > */ > #define V4L2_I2C_ADDRS(addr, addrs...) \ >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > /* Load an i2c module and return an initialized v4l2_subdev struct. >  Only call request_module if module_name != NULL. >  The client_type argument is the name of the chip that's on the adapter. > */ > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev, >         struct i2c_adapter *adapter, >         const char *module_name, const char *client_type, >         int irq, void *platform_data, >         u8 addr, const unsigned short *addrs); > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev( >         struct v4l2_device *v4l2_dev, >         struct i2c_adapter *adapter, >         const char *module_name, const char *client_type, >         u8 addr, const unsigned short *addrs) > { >     return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name, >             client_type, 0, NULL, addr, addrs); > } > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26) > struct i2c_board_info; > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev, >         struct i2c_adapter *adapter, const char *module_name, >         struct i2c_board_info *info, const unsigned short *addrs); > #endif > > Regards, > >     Hans I've cloned your tree and took a look at your code. Well, looks like the proper way to do this change. I didn't take this approach because it touchs other drivers. However, concentrating the code in only one function is better. I also saw that you have fixed the kernel version check in the v4l2_device_unregister function. Great! I will resend my series without this patch. I will rebase it on top of your subdev tree so the new api can be used straight. Is that ok? > > -- > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom > -- > 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 -- Eduardo Bezerra Valentin -- 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
On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote: > Hi Hans, > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > > > > > # HG changeset patch > > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com> > > > > > # Date 1243414605 -10800 > > > > > # Branch export > > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49 > > > > > Device drivers of v4l2_subdev devices may want to have > > > > > board specific data. This patch adds an helper function > > > > > to allow bridge drivers to pass board specific data to > > > > > v4l2_subdev drivers. > > > > > > > > > > For those drivers which need to support kernel versions > > > > > bellow 2.6.26, a .s_config callback was added. The > > > > > idea of this callback is to pass board configuration > > > > > as well. In that case, subdev driver should set .s_config > > > > > properly, because v4l2_i2c_new_subdev_board will call > > > > > the .s_config directly. Of course, if we are >= 2.6.26, > > > > > the same data will be passed through i2c board info as well. > > > > > > > > Hi Eduardo, > > > > > > > > I finally had some time to look at this. After some thought I > > > > realized that the main problem is really that the API is becoming > > > > quite messy. Basically there are 9 different ways of loading and > > > > initializing a subdev: > > > > > > > > First there are three basic initialization calls: no > > > > initialization, passing irq and platform_data, and passing the > > > > i2c_board_info struct directly (preferred for drivers that don't > > > > need pre-2.6.26 compatibility). > > > > > > > > And for each flavor you would like to see three different versions > > > > as well: one with a fixed known i2c address, one where you probe > > > > for a list of addresses, and one where you can probe for a single > > > > i2c address. > > > > > > > > I propose to change the API as follows: > > > > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > > >   ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > >         const char *module_name, const char *client_type, > > > >       u8 addr, const unsigned short *addrs); > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > >         const char *module_name, const char *client_type, > > > >         int irq, void *platform_data, > > > >         u8 addr, const unsigned short *addrs); > > > > > > > > /* Only available for kernels >= 2.6.26 */ > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, > > > > struct i2c_board_info *info, const unsigned short *addrs); > > > > > > > > If you use a fixed address, then only set addr (or info.addr) and > > > > set addrs to NULL. If you want to probe for a list of addrs, then > > > > set addrs to the list of addrs. If you want to probe for a single > > > > addr, then use V4L2_I2C_ADDRS(addr) as the addrs argument. This > > > > constructs an array with just two entries. Actually, this macro can > > > > also create arrays with more entries. > > > > > > > > Note that v4l2_i2c_new_subdev will be an inline that calls > > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data. > > > > > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline > > > > calling v4l2_i2c_new_subdev_board. > > > > > > > > This approach reduces the number of functions to just one (not > > > > counting the inlines) and simplifies things all around. It does > > > > mean that all sources need to be changed, but if we go this route, > > > > then now is the time before the 2.6.31 window is closed. And I > > > > would also like to remove the '_new' from these function names. I > > > > never thought it added anything useful. > > > > > > > > Comments? If we decide to go this way, then I need to know soon so > > > > that I can make the changes before the 2.6.31 window closes. > > > > > > > > BTW, if the new s_config subdev call is present, then it should > > > > always be called. That way the subdev driver can safely do all of > > > > its initialization in s_config, no matter how it was initialized. > > > > > > > > Sorry about the long delay in replying to this: it's been very > > > > hectic lately at the expense of my v4l-dvb work. > > > > > > I've done the initial conversion to the new API (no _cfg or _board > > > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really > > > simplifies things and if nobody objects then I'd like to get this in > > > before 2.6.31. > > > > I've added the new _cfg and _board fucntions as well in this tree. It > > needs a bit of a cleanup before I can do a pull request (the last two > > patches should be merged to one), but otherwise this is the code as I > > think it should be: > > > > /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the > > fly */ > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > /* Load an i2c module and return an initialized v4l2_subdev struct. > >  Only call request_module if module_name != NULL. > >  The client_type argument is the name of the chip that's on the > > adapter. */ > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > *v4l2_dev, struct i2c_adapter *adapter, > >         const char *module_name, const char *client_type, > >         int irq, void *platform_data, > >         u8 addr, const unsigned short *addrs); > > > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev( > >         struct v4l2_device *v4l2_dev, > >         struct i2c_adapter *adapter, > >         const char *module_name, const char *client_type, > >         u8 addr, const unsigned short *addrs) > > { > >     return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name, > >             client_type, 0, NULL, addr, addrs); > > } > > > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26) > > struct i2c_board_info; > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct > > i2c_board_info *info, const unsigned short *addrs); #endif > > > > Regards, > > > >     Hans > > I've cloned your tree and took a look at your code. Well, looks like > the proper way to do this change. > I didn't take this approach because it touchs other drivers. However, > concentrating the code in only one > function is better. I also saw that you have fixed the kernel version > check in the v4l2_device_unregister > function. Great! > > I will resend my series without this patch. I will rebase it on top of > your subdev tree so the new api > can be used straight. Is that ok? Yes, sure. Just be aware that there may be some small changes to my patch based on feedback I get. But it is a good test anyway of this API to see if it works well for you. Regards, Hans > > > -- > > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom > > -- > > 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 > > -- > Eduardo Bezerra Valentin > -- > 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
Em Sun, 7 Jun 2009 08:40:08 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote: > > Hi Hans, > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > > > > > > # HG changeset patch > > > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com> > > > > > > # Date 1243414605 -10800 > > > > > > # Branch export > > > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > > > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49 > > > > > > Device drivers of v4l2_subdev devices may want to have > > > > > > board specific data. This patch adds an helper function > > > > > > to allow bridge drivers to pass board specific data to > > > > > > v4l2_subdev drivers. > > > > > > > > > > > > For those drivers which need to support kernel versions > > > > > > bellow 2.6.26, a .s_config callback was added. The > > > > > > idea of this callback is to pass board configuration > > > > > > as well. In that case, subdev driver should set .s_config > > > > > > properly, because v4l2_i2c_new_subdev_board will call > > > > > > the .s_config directly. Of course, if we are >= 2.6.26, > > > > > > the same data will be passed through i2c board info as well. > > > > > > > > > > Hi Eduardo, > > > > > > > > > > I finally had some time to look at this. After some thought I > > > > > realized that the main problem is really that the API is becoming > > > > > quite messy. Basically there are 9 different ways of loading and > > > > > initializing a subdev: > > > > > > > > > > First there are three basic initialization calls: no > > > > > initialization, passing irq and platform_data, and passing the > > > > > i2c_board_info struct directly (preferred for drivers that don't > > > > > need pre-2.6.26 compatibility). > > > > > > > > > > And for each flavor you would like to see three different versions > > > > > as well: one with a fixed known i2c address, one where you probe > > > > > for a list of addresses, and one where you can probe for a single > > > > > i2c address. > > > > > > > > > > I propose to change the API as follows: > > > > > > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > > > >   ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device > > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > > >         const char *module_name, const char *client_type, > > > > >       u8 addr, const unsigned short *addrs); > > > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > > >         const char *module_name, const char *client_type, > > > > >         int irq, void *platform_data, > > > > >         u8 addr, const unsigned short *addrs); > > > > > > > > > > /* Only available for kernels >= 2.6.26 */ > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, > > > > > struct i2c_board_info *info, const unsigned short *addrs); > > > > > > > > > > If you use a fixed address, then only set addr (or info.addr) and > > > > > set addrs to NULL. If you want to probe for a list of addrs, then > > > > > set addrs to the list of addrs. If you want to probe for a single > > > > > addr, then use V4L2_I2C_ADDRS(addr) as the addrs argument. This > > > > > constructs an array with just two entries. Actually, this macro can > > > > > also create arrays with more entries. > > > > > > > > > > Note that v4l2_i2c_new_subdev will be an inline that calls > > > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data. > > > > > > > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline > > > > > calling v4l2_i2c_new_subdev_board. > > > > > > > > > > This approach reduces the number of functions to just one (not > > > > > counting the inlines) and simplifies things all around. It does > > > > > mean that all sources need to be changed, but if we go this route, > > > > > then now is the time before the 2.6.31 window is closed. And I > > > > > would also like to remove the '_new' from these function names. I > > > > > never thought it added anything useful. > > > > > > > > > > Comments? If we decide to go this way, then I need to know soon so > > > > > that I can make the changes before the 2.6.31 window closes. > > > > > > > > > > BTW, if the new s_config subdev call is present, then it should > > > > > always be called. That way the subdev driver can safely do all of > > > > > its initialization in s_config, no matter how it was initialized. > > > > > > > > > > Sorry about the long delay in replying to this: it's been very > > > > > hectic lately at the expense of my v4l-dvb work. > > > > > > > > I've done the initial conversion to the new API (no _cfg or _board > > > > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really > > > > simplifies things and if nobody objects then I'd like to get this in > > > > before 2.6.31. No please. We did already lots of change due to the i2c changes, and there are still some occasional complaints at ML about regressions that might be due to i2c changes. Let's keep 2.6.31 clean, as previously agreed, without new KABI changes. We should focus 2.6.31 on fixing any core issues that may still have. Only with 2.6.30 we'll start to have feedbacks from normal users. > > > > > > I've added the new _cfg and _board fucntions as well in this tree. It > > > needs a bit of a cleanup before I can do a pull request (the last two > > > patches should be merged to one), but otherwise this is the code as I > > > think it should be: > > > > > > /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the > > > fly */ > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END }) > > > > > > /* Load an i2c module and return an initialized v4l2_subdev struct. > > >  Only call request_module if module_name != NULL. > > >  The client_type argument is the name of the chip that's on the > > > adapter. */ > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > > *v4l2_dev, struct i2c_adapter *adapter, > > >         const char *module_name, const char *client_type, > > >         int irq, void *platform_data, > > >         u8 addr, const unsigned short *addrs); > > > > > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev( > > >         struct v4l2_device *v4l2_dev, > > >         struct i2c_adapter *adapter, > > >         const char *module_name, const char *client_type, > > >         u8 addr, const unsigned short *addrs) > > > { > > >     return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name, > > >             client_type, 0, NULL, addr, addrs); > > > } > > > > > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26) > > > struct i2c_board_info; > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct > > > i2c_board_info *info, const unsigned short *addrs); #endif > > > > > > Regards, > > > > > >     Hans > > > > I've cloned your tree and took a look at your code. Well, looks like > > the proper way to do this change. > > I didn't take this approach because it touchs other drivers. However, > > concentrating the code in only one > > function is better. I also saw that you have fixed the kernel version > > check in the v4l2_device_unregister > > function. Great! > > > > I will resend my series without this patch. I will rebase it on top of > > your subdev tree so the new api > > can be used straight. Is that ok? > > Yes, sure. Just be aware that there may be some small changes to my patch > based on feedback I get. But it is a good test anyway of this API to see if > it works well for you. Eduardo, Let's analyze and merge your changes using the current development tree. If you think that Hans approach is better (I haven't analyzed it yet), then it can later be converted to the new approach Cheers, Mauro -- 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
Hi, On Sun, 7 Jun 2009 22:29:14 -0300 Mauro Carvalho Chehab <mchehab@infradead.org> wrote: > Em Sun, 7 Jun 2009 08:40:08 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote: > > > Hi Hans, > > > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> > > > wrote: > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > > > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > > > > > > > # HG changeset patch > > > > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com> > > > > > > > # Date 1243414605 -10800 > > > > > > > # Branch export > > > > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > > > > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49 > > > > > > > Device drivers of v4l2_subdev devices may want to have > > > > > > > board specific data. This patch adds an helper function > > > > > > > to allow bridge drivers to pass board specific data to > > > > > > > v4l2_subdev drivers. > > > > > > > > > > > > > > For those drivers which need to support kernel versions > > > > > > > bellow 2.6.26, a .s_config callback was added. The > > > > > > > idea of this callback is to pass board configuration > > > > > > > as well. In that case, subdev driver should set .s_config > > > > > > > properly, because v4l2_i2c_new_subdev_board will call > > > > > > > the .s_config directly. Of course, if we are >= 2.6.26, > > > > > > > the same data will be passed through i2c board info as > > > > > > > well. > > > > > > > > > > > > Hi Eduardo, > > > > > > > > > > > > I finally had some time to look at this. After some thought > > > > > > I realized that the main problem is really that the API is > > > > > > becoming quite messy. Basically there are 9 different ways > > > > > > of loading and initializing a subdev: > > > > > > > > > > > > First there are three basic initialization calls: no > > > > > > initialization, passing irq and platform_data, and passing > > > > > > the i2c_board_info struct directly (preferred for drivers > > > > > > that don't need pre-2.6.26 compatibility). > > > > > > > > > > > > And for each flavor you would like to see three different > > > > > > versions as well: one with a fixed known i2c address, one > > > > > > where you probe for a list of addresses, and one where you > > > > > > can probe for a single i2c address. > > > > > > > > > > > > I propose to change the API as follows: > > > > > > > > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > > > > >   ((const unsigned short []){ addr, ## addrs, > > > > > > I2C_CLIENT_END }) > > > > > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device > > > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > > > >         const char *module_name, const char > > > > > > *client_type, u8 addr, const unsigned short *addrs); > > > > > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct > > > > > > v4l2_device *v4l2_dev, struct i2c_adapter *adapter, > > > > > >         const char *module_name, const char > > > > > > *client_type, int irq, void *platform_data, > > > > > >         u8 addr, const unsigned short *addrs); > > > > > > > > > > > > /* Only available for kernels >= 2.6.26 */ > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct > > > > > > v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const > > > > > > char *module_name, struct i2c_board_info *info, const > > > > > > unsigned short *addrs); > > > > > > > > > > > > If you use a fixed address, then only set addr (or > > > > > > info.addr) and set addrs to NULL. If you want to probe for > > > > > > a list of addrs, then set addrs to the list of addrs. If > > > > > > you want to probe for a single addr, then use > > > > > > V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs > > > > > > an array with just two entries. Actually, this macro can > > > > > > also create arrays with more entries. > > > > > > > > > > > > Note that v4l2_i2c_new_subdev will be an inline that calls > > > > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and > > > > > > platform_data. > > > > > > > > > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an > > > > > > inline calling v4l2_i2c_new_subdev_board. > > > > > > > > > > > > This approach reduces the number of functions to just one > > > > > > (not counting the inlines) and simplifies things all > > > > > > around. It does mean that all sources need to be changed, > > > > > > but if we go this route, then now is the time before the > > > > > > 2.6.31 window is closed. And I would also like to remove > > > > > > the '_new' from these function names. I never thought it > > > > > > added anything useful. > > > > > > > > > > > > Comments? If we decide to go this way, then I need to know > > > > > > soon so that I can make the changes before the 2.6.31 > > > > > > window closes. > > > > > > > > > > > > BTW, if the new s_config subdev call is present, then it > > > > > > should always be called. That way the subdev driver can > > > > > > safely do all of its initialization in s_config, no matter > > > > > > how it was initialized. > > > > > > > > > > > > Sorry about the long delay in replying to this: it's been > > > > > > very hectic lately at the expense of my v4l-dvb work. > > > > > > > > > > I've done the initial conversion to the new API (no _cfg or > > > > > _board version yet) in my ~hverkuil/v4l-dvb-subdev tree. It > > > > > really simplifies things and if nobody objects then I'd like > > > > > to get this in before 2.6.31. > > No please. We did already lots of change due to the i2c changes, and > there are still some occasional complaints at ML about regressions > that might be due to i2c changes. > > Let's keep 2.6.31 clean, as previously agreed, without new KABI > changes. We should focus 2.6.31 on fixing any core issues that may > still have. Only with 2.6.30 we'll start to have feedbacks from > normal users. > > > > > > > > > I've added the new _cfg and _board fucntions as well in this > > > > tree. It needs a bit of a cleanup before I can do a pull > > > > request (the last two patches should be merged to one), but > > > > otherwise this is the code as I think it should be: > > > > > > > > /* Construct an I2C_CLIENT_END-terminated array of i2c > > > > addresses on the fly */ > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > > >     ((const unsigned short []){ addr, ## addrs, > > > > I2C_CLIENT_END }) > > > > > > > > /* Load an i2c module and return an initialized v4l2_subdev > > > > struct. Only call request_module if module_name != NULL. > > > >  The client_type argument is the name of the chip that's on the > > > > adapter. */ > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > >         const char *module_name, const char *client_type, > > > >         int irq, void *platform_data, > > > >         u8 addr, const unsigned short *addrs); > > > > > > > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev( > > > >         struct v4l2_device *v4l2_dev, > > > >         struct i2c_adapter *adapter, > > > >         const char *module_name, const char *client_type, > > > >         u8 addr, const unsigned short *addrs) > > > > { > > > >     return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, > > > > module_name, client_type, 0, NULL, addr, addrs); > > > > } > > > > > > > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26) > > > > struct i2c_board_info; > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > > > *v4l2_dev, struct i2c_adapter *adapter, const char > > > > *module_name, struct i2c_board_info *info, const unsigned short > > > > *addrs); #endif > > > > > > > > Regards, > > > > > > > >     Hans > > > > > > I've cloned your tree and took a look at your code. Well, looks > > > like the proper way to do this change. > > > I didn't take this approach because it touchs other drivers. > > > However, concentrating the code in only one > > > function is better. I also saw that you have fixed the kernel > > > version check in the v4l2_device_unregister > > > function. Great! > > > > > > I will resend my series without this patch. I will rebase it on > > > top of your subdev tree so the new api > > > can be used straight. Is that ok? > > > > Yes, sure. Just be aware that there may be some small changes to my > > patch based on feedback I get. But it is a good test anyway of this > > API to see if it works well for you. > > Eduardo, > > Let's analyze and merge your changes using the current development > tree. If you think that Hans approach is better (I haven't analyzed > it yet), then it can later be converted to the new approach > I have talked with Eduardo during last week and if there is no objections, I am ready to request a pull from the current/last patches series. Cheers, Douglas -- 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
Hi guys, On Mon, Jun 08, 2009 at 05:19:22AM +0200, ext Douglas Schilling Landgraf wrote: > Hi, > > On Sun, 7 Jun 2009 22:29:14 -0300 > Mauro Carvalho Chehab <mchehab@infradead.org> wrote: > > > Em Sun, 7 Jun 2009 08:40:08 +0200 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote: > > > > Hi Hans, > > > > > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> > > > > wrote: > > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: <snip> > > > > No please. We did already lots of change due to the i2c changes, and > > there are still some occasional complaints at ML about regressions > > that might be due to i2c changes. > > > > Let's keep 2.6.31 clean, as previously agreed, without new KABI > > changes. We should focus 2.6.31 on fixing any core issues that may > > still have. Only with 2.6.30 we'll start to have feedbacks from > > normal users. <snip> > > > > > > > > I've cloned your tree and took a look at your code. Well, looks > > > > like the proper way to do this change. > > > > I didn't take this approach because it touchs other drivers. > > > > However, concentrating the code in only one > > > > function is better. I also saw that you have fixed the kernel > > > > version check in the v4l2_device_unregister > > > > function. Great! > > > > > > > > I will resend my series without this patch. I will rebase it on > > > > top of your subdev tree so the new api > > > > can be used straight. Is that ok? > > > > > > Yes, sure. Just be aware that there may be some small changes to my > > > patch based on feedback I get. But it is a good test anyway of this > > > API to see if it works well for you. > > > > Eduardo, > > > > Let's analyze and merge your changes using the current development > > tree. If you think that Hans approach is better (I haven't analyzed > > it yet), then it can later be converted to the new approach > > > > I have talked with Eduardo during last week and if there is no > objections, I am ready to request a pull from the current/last > patches series. Yes, my series is already in one of Douglas' trees and we have tested it. However, in that series there is one patch which does partially what Hans is proposing. Which is: add a way to pass platform info to i2c drivers, using v4l2 i2c helper functions. They way it is done in this patch it does not affect any other driver. Hans did also some re-factoring in existing i2c helper function, besides adding new way to pass platform data. If you agree we can use it for now and in next window we change things to have them using the way Hans did (which is more complete). What do you think? > > Cheers, > Douglas Cheers,
On Monday 08 June 2009 03:29:14 Mauro Carvalho Chehab wrote: > Em Sun, 7 Jun 2009 08:40:08 +0200 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote: > > > Hi Hans, > > > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > > > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote: > > > > > > > # HG changeset patch > > > > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com> > > > > > > > # Date 1243414605 -10800 > > > > > > > # Branch export > > > > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 > > > > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49 > > > > > > > Device drivers of v4l2_subdev devices may want to have > > > > > > > board specific data. This patch adds an helper function > > > > > > > to allow bridge drivers to pass board specific data to > > > > > > > v4l2_subdev drivers. > > > > > > > > > > > > > > For those drivers which need to support kernel versions > > > > > > > bellow 2.6.26, a .s_config callback was added. The > > > > > > > idea of this callback is to pass board configuration > > > > > > > as well. In that case, subdev driver should set .s_config > > > > > > > properly, because v4l2_i2c_new_subdev_board will call > > > > > > > the .s_config directly. Of course, if we are >= 2.6.26, > > > > > > > the same data will be passed through i2c board info as well. > > > > > > > > > > > > Hi Eduardo, > > > > > > > > > > > > I finally had some time to look at this. After some thought I > > > > > > realized that the main problem is really that the API is > > > > > > becoming quite messy. Basically there are 9 different ways of > > > > > > loading and initializing a subdev: > > > > > > > > > > > > First there are three basic initialization calls: no > > > > > > initialization, passing irq and platform_data, and passing the > > > > > > i2c_board_info struct directly (preferred for drivers that > > > > > > don't need pre-2.6.26 compatibility). > > > > > > > > > > > > And for each flavor you would like to see three different > > > > > > versions as well: one with a fixed known i2c address, one where > > > > > > you probe for a list of addresses, and one where you can probe > > > > > > for a single i2c address. > > > > > > > > > > > > I propose to change the API as follows: > > > > > > > > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > > > > >   ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END > > > > > > }) > > > > > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device > > > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > > > >         const char *module_name, const char > > > > > > *client_type, u8 addr, const unsigned short *addrs); > > > > > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > > > >         const char *module_name, const char > > > > > > *client_type, int irq, void *platform_data, > > > > > >         u8 addr, const unsigned short *addrs); > > > > > > > > > > > > /* Only available for kernels >= 2.6.26 */ > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct > > > > > > v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char > > > > > > *module_name, struct i2c_board_info *info, const unsigned short > > > > > > *addrs); > > > > > > > > > > > > If you use a fixed address, then only set addr (or info.addr) > > > > > > and set addrs to NULL. If you want to probe for a list of > > > > > > addrs, then set addrs to the list of addrs. If you want to > > > > > > probe for a single addr, then use V4L2_I2C_ADDRS(addr) as the > > > > > > addrs argument. This constructs an array with just two entries. > > > > > > Actually, this macro can also create arrays with more entries. > > > > > > > > > > > > Note that v4l2_i2c_new_subdev will be an inline that calls > > > > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and > > > > > > platform_data. > > > > > > > > > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an > > > > > > inline calling v4l2_i2c_new_subdev_board. > > > > > > > > > > > > This approach reduces the number of functions to just one (not > > > > > > counting the inlines) and simplifies things all around. It does > > > > > > mean that all sources need to be changed, but if we go this > > > > > > route, then now is the time before the 2.6.31 window is closed. > > > > > > And I would also like to remove the '_new' from these function > > > > > > names. I never thought it added anything useful. > > > > > > > > > > > > Comments? If we decide to go this way, then I need to know soon > > > > > > so that I can make the changes before the 2.6.31 window closes. > > > > > > > > > > > > BTW, if the new s_config subdev call is present, then it should > > > > > > always be called. That way the subdev driver can safely do all > > > > > > of its initialization in s_config, no matter how it was > > > > > > initialized. > > > > > > > > > > > > Sorry about the long delay in replying to this: it's been very > > > > > > hectic lately at the expense of my v4l-dvb work. > > > > > > > > > > I've done the initial conversion to the new API (no _cfg or > > > > > _board version yet) in my ~hverkuil/v4l-dvb-subdev tree. It > > > > > really simplifies things and if nobody objects then I'd like to > > > > > get this in before 2.6.31. > > No please. We did already lots of change due to the i2c changes, and > there are still some occasional complaints at ML about regressions that > might be due to i2c changes. Please point them out to me. I can't remember seeing those. Admittedly, I've been swamped with work in the past few weeks, so I might have missed them. > Let's keep 2.6.31 clean, as previously agreed, without new KABI changes. > We should focus 2.6.31 on fixing any core issues that may still have. > Only with 2.6.30 we'll start to have feedbacks from normal users. It's a new API, so it is normal that we find things that need improvement. This is the case here. Eduardo needs to pass extra initialization info to the i2c drivers, so he is adding new functions to the API. But those make an overly complex (as I've come to realize) API even more complex. Isn't is better to do it right? And I don't see the advantage of waiting with this. If there are any problems with 2.6.30 then we will get them whether we change this internal API or not. In addition, I think this is pretty much the only change in the API to be queued for 2.6.31. And how long do you intend to wait? It will take a while before all the distros take up 2.6.30. So wait until 2.6.32? 2.6.33? > > > > I've added the new _cfg and _board fucntions as well in this tree. > > > > It needs a bit of a cleanup before I can do a pull request (the > > > > last two patches should be merged to one), but otherwise this is > > > > the code as I think it should be: > > > > > > > > /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on > > > > the fly */ > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \ > > > >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END > > > > }) > > > > > > > > /* Load an i2c module and return an initialized v4l2_subdev struct. > > > >  Only call request_module if module_name != NULL. > > > >  The client_type argument is the name of the chip that's on the > > > > adapter. */ > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device > > > > *v4l2_dev, struct i2c_adapter *adapter, > > > >         const char *module_name, const char *client_type, > > > >         int irq, void *platform_data, > > > >         u8 addr, const unsigned short *addrs); > > > > > > > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev( > > > >         struct v4l2_device *v4l2_dev, > > > >         struct i2c_adapter *adapter, > > > >         const char *module_name, const char *client_type, > > > >         u8 addr, const unsigned short *addrs) > > > > { > > > >     return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, > > > > module_name, client_type, 0, NULL, addr, addrs); } > > > > > > > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26) > > > > struct i2c_board_info; > > > > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device > > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, > > > > struct i2c_board_info *info, const unsigned short *addrs); #endif > > > > > > > > Regards, > > > > > > > >     Hans > > > > > > I've cloned your tree and took a look at your code. Well, looks like > > > the proper way to do this change. > > > I didn't take this approach because it touchs other drivers. However, > > > concentrating the code in only one > > > function is better. I also saw that you have fixed the kernel version > > > check in the v4l2_device_unregister > > > function. Great! > > > > > > I will resend my series without this patch. I will rebase it on top > > > of your subdev tree so the new api > > > can be used straight. Is that ok? > > > > Yes, sure. Just be aware that there may be some small changes to my > > patch based on feedback I get. But it is a good test anyway of this API > > to see if it works well for you. > > Eduardo, > > Let's analyze and merge your changes using the current development tree. > If you think that Hans approach is better (I haven't analyzed it yet), > then it can later be converted to the new approach If you really are opposed to my changes, then I need to look at these patches again since they need to be modified a fair amount. They will have to touch the core subdev API anyway, so let's do it right rather than hacking in this new functionality and having to change it again in 2.6.32. If something is wrong, then let's just fix it instead of trying to hack it in. That only increases the chances of more errors. After implementing my changes I came to realize that it is a much cleaner approach. One alternative that I would be OK with is to wait with this until the 2.6.31 window closes. But then Eduardo's driver won't make 2.6.31 either. BTW, Eduardo isn't the only one who needs these changes. It crops up whenever you deal with embedded devices. So please let us just fix this part of the API. Regards, Hans
On Monday 08 June 2009 08:11:32 Eduardo Valentin wrote: > Hi guys, > > On Mon, Jun 08, 2009 at 05:19:22AM +0200, ext Douglas Schilling Landgraf wrote: > > Hi, > > > > On Sun, 7 Jun 2009 22:29:14 -0300 > > > > Mauro Carvalho Chehab <mchehab@infradead.org> wrote: > > > Em Sun, 7 Jun 2009 08:40:08 +0200 > > > > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote: > > > > > Hi Hans, > > > > > > > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> > > > > > > > > > > wrote: > > > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > <snip> > > > > No please. We did already lots of change due to the i2c changes, and > > > there are still some occasional complaints at ML about regressions > > > that might be due to i2c changes. > > > > > > Let's keep 2.6.31 clean, as previously agreed, without new KABI > > > changes. We should focus 2.6.31 on fixing any core issues that may > > > still have. Only with 2.6.30 we'll start to have feedbacks from > > > normal users. > > <snip> > > > > > > I've cloned your tree and took a look at your code. Well, looks > > > > > like the proper way to do this change. > > > > > I didn't take this approach because it touchs other drivers. > > > > > However, concentrating the code in only one > > > > > function is better. I also saw that you have fixed the kernel > > > > > version check in the v4l2_device_unregister > > > > > function. Great! > > > > > > > > > > I will resend my series without this patch. I will rebase it on > > > > > top of your subdev tree so the new api > > > > > can be used straight. Is that ok? > > > > > > > > Yes, sure. Just be aware that there may be some small changes to my > > > > patch based on feedback I get. But it is a good test anyway of this > > > > API to see if it works well for you. > > > > > > Eduardo, > > > > > > Let's analyze and merge your changes using the current development > > > tree. If you think that Hans approach is better (I haven't analyzed > > > it yet), then it can later be converted to the new approach > > > > I have talked with Eduardo during last week and if there is no > > objections, I am ready to request a pull from the current/last > > patches series. > > Yes, my series is already in one of Douglas' trees and we have tested it. > However, in that series there is one patch which does partially what Hans > is proposing. Which is: add a way to pass platform info to i2c drivers, > using v4l2 i2c helper functions. They way it is done in this patch it > does not affect any other driver. Hans did also some re-factoring in > existing i2c helper function, besides adding new way to pass platform > data. No, I don't agree with that. Your patch has some issues: no cleanup after s_config returns an error, and if we introduce s_config then it should be called by *all* v4l2_new_subdev* functions. That way i2c drivers that implement this can use it reliably for their initialization. I see no point in doing the same work twice. We have one clean solution into which I put quite a bit of time, and one that hacks new functionality into an already flawed API. This was also the reason why I didn't just sign off on Eduardo's patch. I strongly suspected I needed to do some proper refactoring first and when I finally had the time to look into this last Saturday I discovered it did indeed needed refactoring. > > If you agree we can use it for now and in next window we > change things to have them using the way Hans did (which is more > complete). Going with a suboptimal solution when a proper clean one is available is a really bad idea IMHO. Regards, Hans > > What do you think? > > > Cheers, > > Douglas > > Cheers,
On Mon, Jun 08, 2009 at 08:38:32AM +0200, ext Hans Verkuil wrote: > On Monday 08 June 2009 08:11:32 Eduardo Valentin wrote: > > Hi guys, > > > > On Mon, Jun 08, 2009 at 05:19:22AM +0200, ext Douglas Schilling Landgraf > wrote: > > > Hi, > > > > > > On Sun, 7 Jun 2009 22:29:14 -0300 > > > > > > Mauro Carvalho Chehab <mchehab@infradead.org> wrote: > > > > Em Sun, 7 Jun 2009 08:40:08 +0200 > > > > > > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote: > > > > > > Hi Hans, > > > > > > > > > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> > > > > > > > > > > > > wrote: > > > > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote: > > > > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote: > > > > <snip> > > > > > > No please. We did already lots of change due to the i2c changes, and > > > > there are still some occasional complaints at ML about regressions > > > > that might be due to i2c changes. > > > > > > > > Let's keep 2.6.31 clean, as previously agreed, without new KABI > > > > changes. We should focus 2.6.31 on fixing any core issues that may > > > > still have. Only with 2.6.30 we'll start to have feedbacks from > > > > normal users. > > > > <snip> > > > > > > > > I've cloned your tree and took a look at your code. Well, looks > > > > > > like the proper way to do this change. > > > > > > I didn't take this approach because it touchs other drivers. > > > > > > However, concentrating the code in only one > > > > > > function is better. I also saw that you have fixed the kernel > > > > > > version check in the v4l2_device_unregister > > > > > > function. Great! > > > > > > > > > > > > I will resend my series without this patch. I will rebase it on > > > > > > top of your subdev tree so the new api > > > > > > can be used straight. Is that ok? > > > > > > > > > > Yes, sure. Just be aware that there may be some small changes to my > > > > > patch based on feedback I get. But it is a good test anyway of this > > > > > API to see if it works well for you. > > > > > > > > Eduardo, > > > > > > > > Let's analyze and merge your changes using the current development > > > > tree. If you think that Hans approach is better (I haven't analyzed > > > > it yet), then it can later be converted to the new approach > > > > > > I have talked with Eduardo during last week and if there is no > > > objections, I am ready to request a pull from the current/last > > > patches series. > > > > Yes, my series is already in one of Douglas' trees and we have tested it. > > However, in that series there is one patch which does partially what Hans > > is proposing. Which is: add a way to pass platform info to i2c drivers, > > using v4l2 i2c helper functions. They way it is done in this patch it > > does not affect any other driver. Hans did also some re-factoring in > > existing i2c helper function, besides adding new way to pass platform > > data. > > No, I don't agree with that. Your patch has some issues: no cleanup after > s_config returns an error, and if we introduce s_config then it should be > called by *all* v4l2_new_subdev* functions. That way i2c drivers that > implement this can use it reliably for their initialization. > > I see no point in doing the same work twice. We have one clean solution into > which I put quite a bit of time, and one that hacks new functionality into > an already flawed API. Agreed. No point in doing the same work. > > This was also the reason why I didn't just sign off on Eduardo's patch. I > strongly suspected I needed to do some proper refactoring first and when I > finally had the time to look into this last Saturday I discovered it did > indeed needed refactoring. > > > > > If you agree we can use it for now and in next window we > > change things to have them using the way Hans did (which is more > > complete). > > Going with a suboptimal solution when a proper clean one is available is a > really bad idea IMHO. As I already said, I really liked your approach because it re-factors the API. No problem for me to rebase the patches on top of that. > > Regards, > > Hans > > > > > What do you think? > > > > > Cheers, > > > Douglas > > > > Cheers, > > > > -- > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
diff -r 142fd6020df3 -r 4fb354645426 linux/drivers/media/video/v4l2-common.c --- a/linux/drivers/media/video/v4l2-common.c Mon May 18 02:31:55 2009 +0000 +++ b/linux/drivers/media/video/v4l2-common.c Wed May 27 11:56:45 2009 +0300 @@ -819,9 +819,10 @@ /* Load an i2c sub-device. */ -struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, +static struct v4l2_subdev *__v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, - const char *module_name, const char *client_type, u8 addr) + const char *module_name, const char *client_type, u8 addr, + int irq, void *platform_data) { struct v4l2_subdev *sd = NULL; struct i2c_client *client; @@ -840,6 +841,8 @@ memset(&info, 0, sizeof(info)); strlcpy(info.type, client_type, sizeof(info.type)); info.addr = addr; + info.irq = irq; + info.platform_data = platform_data; /* Create the i2c client */ client = i2c_new_device(adapter, &info); @@ -877,8 +880,38 @@ #endif return sd; } + +struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, + struct i2c_adapter *adapter, + const char *module_name, const char *client_type, u8 addr) +{ + return __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name, + client_type, addr, 0, NULL); +} EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev); +struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev, + struct i2c_adapter *adapter, + const char *module_name, const char *client_type, u8 addr, + int irq, void *platform_data) +{ + struct v4l2_subdev *sd; + int err = 0; + + sd = __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name, client_type, + addr, irq, platform_data); + + /* + * We return errors from v4l2_subdev_call only if we have the callback + * as the .s_config is not mandatory + */ + if (sd && sd->ops && sd->ops->core && sd->ops->core->s_config) + err = sd->ops->core->s_config(sd, irq, platform_data); + + return err < 0 ? NULL : sd; +} +EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board); + /* Probe and load an i2c sub-device. */ struct v4l2_subdev *v4l2_i2c_new_probed_subdev(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, diff -r 142fd6020df3 -r 4fb354645426 linux/include/media/v4l2-common.h --- a/linux/include/media/v4l2-common.h Mon May 18 02:31:55 2009 +0000 +++ b/linux/include/media/v4l2-common.h Wed May 27 11:56:45 2009 +0300 @@ -147,6 +147,14 @@ struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, const char *client_type, u8 addr); +/* + * Same as v4l2_i2c_new_subdev, but with the opportunity to configure + * subdevice with board specific data (irq and platform_data). + */ +struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev, + struct i2c_adapter *adapter, + const char *module_name, const char *client_type, u8 addr, + int irq, void *platform_data); /* Probe and load an i2c module and return an initialized v4l2_subdev struct. Only call request_module if module_name != NULL. The client_type argument is the name of the chip that's on the adapter. */ diff -r 142fd6020df3 -r 4fb354645426 linux/include/media/v4l2-subdev.h --- a/linux/include/media/v4l2-subdev.h Mon May 18 02:31:55 2009 +0000 +++ b/linux/include/media/v4l2-subdev.h Wed May 27 11:56:45 2009 +0300 @@ -96,6 +96,7 @@ struct v4l2_subdev_core_ops { int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident *chip); int (*log_status)(struct v4l2_subdev *sd); + int (*s_config)(struct v4l2_subdev *sd, int irq, void *platform_data); int (*init)(struct v4l2_subdev *sd, u32 val); int (*load_fw)(struct v4l2_subdev *sd); int (*reset)(struct v4l2_subdev *sd, u32 val);
# HG changeset patch # User Eduardo Valentin <eduardo.valentin@nokia.com> # Date 1243414605 -10800 # Branch export # Node ID 4fb354645426f8b187c2c90cd8528b2518461005 # Parent 142fd6020df3b4d543068155e49a2618140efa49 Device drivers of v4l2_subdev devices may want to have board specific data. This patch adds an helper function to allow bridge drivers to pass board specific data to v4l2_subdev drivers. For those drivers which need to support kernel versions bellow 2.6.26, a .s_config callback was added. The idea of this callback is to pass board configuration as well. In that case, subdev driver should set .s_config properly, because v4l2_i2c_new_subdev_board will call the .s_config directly. Of course, if we are >= 2.6.26, the same data will be passed through i2c board info as well. Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com> --- drivers/media/video/v4l2-common.c | 37 +++++++++++++++++++++++++++++++++++-- include/linux/v4l2-common.h | 8 ++++++++ include/linux/v4l2-subdev.h | 1 + 3 files changed, 44 insertions(+), 2 deletions(-) -- 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