Em Sun, 7 Jun 2009 08:40:08 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > > > # 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
