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.
>
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
