diff mbox

[08/10,v2] v4l2-subdev: add a v4l2_i2c_subdev_board() function

Message ID Pine.LNX.4.64.0905151905440.4658@axis700.grange (mailing list archive)
State RFC
Headers show

Commit Message

Guennadi Liakhovetski May 15, 2009, 5:20 p.m. UTC
Introduce a function similar to v4l2_i2c_new_subdev() but taking a pointer to a
struct i2c_board_info as a parameter instead of a client type and an I2C
address, and make v4l2_i2c_new_subdev() a wrapper around it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Hans, renamed as you requested and updated to a (more) current state.

 drivers/media/video/v4l2-common.c |   30 +++++++++++++++++++-----------
 include/media/v4l2-common.h       |    5 +++++
 2 files changed, 24 insertions(+), 11 deletions(-)

Comments

Hans Verkuil May 21, 2009, 1:53 p.m. UTC | #1
On Friday 15 May 2009 19:20:10 Guennadi Liakhovetski wrote:
> Introduce a function similar to v4l2_i2c_new_subdev() but taking a
> pointer to a struct i2c_board_info as a parameter instead of a client
> type and an I2C address, and make v4l2_i2c_new_subdev() a wrapper around
> it.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> Hans, renamed as you requested and updated to a (more) current state.

NAK. Not because it is a bad idea, but because you need to patch against the 
version in the v4l-dvb repo. The version in the kernel is missing a lot of 
the compatibility code which we unfortunately need to keep.

Any function passing the board_info will be valid for kernels >= 2.6.26 
only.

I've been thinking of an alternative that is more 'compatibility' friendly:

The board info contains two fields that are really of interest: irq and the 
platform data. The core ops can be extended with a new .s_config function 
that receives the irq and a platform data pointer. If the new 
v4l2_i2c_subdev_board function is called, then .s_config is called 
automatically with the right parameters. This makes it possible to use 
v4l2_i2c_subdev_board with a i2c subdev that has to be backwards compatible 
(and so cannot rely on board_info). Instead that subdev will receive an 
s_config call with the needed info.

And v4l drivers that have to be backwards compatible and so cannot use 
v4l2_i2c_subdev_board can call s_config directly.

I agree, it is not elegant, but it is a fact of life that we need to stay 
backwards compatible. This new i2c API is only usable from 2.6.26 onwards, 
and that's simply too recent.

In my opinion doing it the way I proposed above will be a reasonable 
alternative that is also easy to remove when at some point in the future we 
stop supporting pre-2.6.26 kernels.

Regards,

	Hans

>
>  drivers/media/video/v4l2-common.c |   30 +++++++++++++++++++-----------
>  include/media/v4l2-common.h       |    5 +++++
>  2 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-common.c
> b/drivers/media/video/v4l2-common.c index f576ef6..ab190aa 100644
> --- a/drivers/media/video/v4l2-common.c
> +++ b/drivers/media/video/v4l2-common.c
> @@ -758,30 +758,38 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd,
> struct i2c_client *client, }
>  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
>
> -
> -
>  /* Load an i2c sub-device. */
>  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)
>  {
> -	struct v4l2_subdev *sd = NULL;
> -	struct i2c_client *client;
>  	struct i2c_board_info info;
>
> -	BUG_ON(!v4l2_dev);
> -
> -	if (module_name)
> -		request_module(module_name);
> -
>  	/* Setup the i2c board info with the device type and
>  	   the device address. */
>  	memset(&info, 0, sizeof(info));
>  	strlcpy(info.type, client_type, sizeof(info.type));
>  	info.addr = addr;
>
> +	return v4l2_i2c_subdev_board(v4l2_dev, adapter, module_name, &info);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
> +
> +/* Load an i2c sub-device using provided struct i2c_board_info. */
> +struct v4l2_subdev *v4l2_i2c_subdev_board(struct v4l2_device *v4l2_dev,
> +		struct i2c_adapter *adapter,
> +		const char *module_name, const struct i2c_board_info *info)
> +{
> +	struct v4l2_subdev *sd = NULL;
> +	struct i2c_client *client;
> +
> +	BUG_ON(!v4l2_dev);
> +
> +	if (module_name)
> +		request_module(module_name);
> +
>  	/* Create the i2c client */
> -	client = i2c_new_device(adapter, &info);
> +	client = i2c_new_device(adapter, info);
>  	/* Note: it is possible in the future that
>  	   c->driver is NULL if the driver is still being loaded.
>  	   We need better support from the kernel so that we
> @@ -808,7 +816,7 @@ error:
>  		i2c_unregister_device(client);
>  	return sd;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
> +EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_board);
>
>  /* Probe and load an i2c sub-device. */
>  struct v4l2_subdev *v4l2_i2c_new_probed_subdev(struct v4l2_device
> *v4l2_dev, diff --git a/include/media/v4l2-common.h
> b/include/media/v4l2-common.h index c48c24e..a67f5c3 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -131,6 +131,7 @@ struct i2c_driver;
>  struct i2c_adapter;
>  struct i2c_client;
>  struct i2c_device_id;
> +struct i2c_board_info;
>  struct v4l2_device;
>  struct v4l2_subdev;
>  struct v4l2_subdev_ops;
> @@ -142,6 +143,10 @@ struct v4l2_subdev_ops;
>  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 above but uses user-provided struct i2c_board_info */
> +struct v4l2_subdev *v4l2_i2c_subdev_board(struct v4l2_device *v4l2_dev,
> +		struct i2c_adapter *adapter,
> +		const char *module_name, const struct i2c_board_info *info);
>  /* 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. */
Guennadi Liakhovetski May 21, 2009, 3:33 p.m. UTC | #2
Hi Hans,

On Thu, 21 May 2009, Hans Verkuil wrote:

> On Friday 15 May 2009 19:20:10 Guennadi Liakhovetski wrote:
> > Introduce a function similar to v4l2_i2c_new_subdev() but taking a
> > pointer to a struct i2c_board_info as a parameter instead of a client
> > type and an I2C address, and make v4l2_i2c_new_subdev() a wrapper around
> > it.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > Hans, renamed as you requested and updated to a (more) current state.
> 
> NAK. Not because it is a bad idea, but because you need to patch against the 
> version in the v4l-dvb repo. The version in the kernel is missing a lot of 
> the compatibility code which we unfortunately need to keep.
> 
> Any function passing the board_info will be valid for kernels >= 2.6.26 
> only.

Here's a quote from your earlier email.

On Tue, 21 Apr 2009, Hans Verkuil wrote:

> The board_info struct didn't appear until 2.6.22, so that's certainly a
> cut-off point. Since the probe version of this call does not work on
> kernels < 2.6.26 the autoprobing mechanism is still used for those older
> kernels. I think it makes life much easier to require that everything that
> uses board_info needs kernel 2.6.26 at the minimum. I don't think that is
> an issue anyway for soc-camera. Unless there is a need to use soc-camera
> from v4l-dvb with kernels <2.6.26?

So, will this my patch build and work with >= 2.6.22 or not? I really 
would not like to consciously make code uglier now because of 
compatibility with < 2.6.26 to make it better some time later again.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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 Valentin May 22, 2009, 8:58 a.m. UTC | #3
Hi Hans and Guennadi,

On Thu, May 21, 2009 at 05:33:48PM +0200, ext Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> On Thu, 21 May 2009, Hans Verkuil wrote:
> 
> > On Friday 15 May 2009 19:20:10 Guennadi Liakhovetski wrote:
> > > Introduce a function similar to v4l2_i2c_new_subdev() but taking a
> > > pointer to a struct i2c_board_info as a parameter instead of a client
> > > type and an I2C address, and make v4l2_i2c_new_subdev() a wrapper around
> > > it.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > >
> > > Hans, renamed as you requested and updated to a (more) current state.
> > 
> > NAK. Not because it is a bad idea, but because you need to patch against the 
> > version in the v4l-dvb repo. The version in the kernel is missing a lot of 
> > the compatibility code which we unfortunately need to keep.
> > 
> > Any function passing the board_info will be valid for kernels >= 2.6.26 
> > only.
> 
> Here's a quote from your earlier email.
> 
> On Tue, 21 Apr 2009, Hans Verkuil wrote:
> 
> > The board_info struct didn't appear until 2.6.22, so that's certainly a
> > cut-off point. Since the probe version of this call does not work on
> > kernels < 2.6.26 the autoprobing mechanism is still used for those older
> > kernels. I think it makes life much easier to require that everything that
> > uses board_info needs kernel 2.6.26 at the minimum. I don't think that is
> > an issue anyway for soc-camera. Unless there is a need to use soc-camera
> > from v4l-dvb with kernels <2.6.26?
> 
> So, will this my patch build and work with >= 2.6.22 or not? I really 
> would not like to consciously make code uglier now because of 
> compatibility with < 2.6.26 to make it better some time later again.

I've to agree with Guennadi, I believe newer code should not suffer because
of compatibility code, at least if it is possible. I also agree with you that
we must keep compatibility with older drivers.

What I propose it to have the mechanism of .s_config available only for
LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 26). Newer version can take advance
of the new i2c api features.

This is slightly different from what Hans proposed. The difference here
is that we do not force newer drivers to use a callback only because
of backward compatibility.

Well, this is what I think of this problem, you may have a different point
of view. What do you think?
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> 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
Hans Verkuil May 22, 2009, 11:55 a.m. UTC | #4
On Friday 22 May 2009 10:58:27 Eduardo Valentin wrote:
> Hi Hans and Guennadi,
>
> On Thu, May 21, 2009 at 05:33:48PM +0200, ext Guennadi Liakhovetski wrote:
> > Hi Hans,
> >
> > On Thu, 21 May 2009, Hans Verkuil wrote:
> > > On Friday 15 May 2009 19:20:10 Guennadi Liakhovetski wrote:
> > > > Introduce a function similar to v4l2_i2c_new_subdev() but taking a
> > > > pointer to a struct i2c_board_info as a parameter instead of a
> > > > client type and an I2C address, and make v4l2_i2c_new_subdev() a
> > > > wrapper around it.
> > > >
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > ---
> > > >
> > > > Hans, renamed as you requested and updated to a (more) current
> > > > state.
> > >
> > > NAK. Not because it is a bad idea, but because you need to patch
> > > against the version in the v4l-dvb repo. The version in the kernel is
> > > missing a lot of the compatibility code which we unfortunately need
> > > to keep.
> > >
> > > Any function passing the board_info will be valid for kernels >=
> > > 2.6.26 only.
> >
> > Here's a quote from your earlier email.
> >
> > On Tue, 21 Apr 2009, Hans Verkuil wrote:
> > > The board_info struct didn't appear until 2.6.22, so that's certainly
> > > a cut-off point. Since the probe version of this call does not work
> > > on kernels < 2.6.26 the autoprobing mechanism is still used for those
> > > older kernels. I think it makes life much easier to require that
> > > everything that uses board_info needs kernel 2.6.26 at the minimum. I
> > > don't think that is an issue anyway for soc-camera. Unless there is a
> > > need to use soc-camera from v4l-dvb with kernels <2.6.26?
> >
> > So, will this my patch build and work with >= 2.6.22 or not? I really
> > would not like to consciously make code uglier now because of
> > compatibility with < 2.6.26 to make it better some time later again.
>
> I've to agree with Guennadi, I believe newer code should not suffer
> because of compatibility code, at least if it is possible. I also agree
> with you that we must keep compatibility with older drivers.

Let there be no doubt about it: it's very unfortunate that we have to do 
this. I really hope that in, say, one year we can just drop support for 
kernels pre-2.6.26. But my proposal from the beginning of the year to drop 
support for kernels < 2.6.22 demonstrated that not everyone is happy about 
that.

A quick note for Guennadi: the i2c_board_info and the new i2c API has been 
available since 2.6.22, but for the subdev support in v4l2 I've decided not 
to use the new i2c API for kernels < 2.6.26 due to a serious i2c core 
kernel bug that wasn't fixed until 2.6.26 (probing for the existence of an 
i2c device at certain addresses can cause an oops). Strictly speaking it 
would be possible to support board_info from 2.6.22 onwards, but going that 
way makes it very messy with lots of #ifdefs. I want to keep the simple 
rule to only support the new i2c API for 2.6.26 onwards.

> What I propose it to have the mechanism of .s_config available only for
> LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 26). Newer version can take
> advance of the new i2c api features.
>
> This is slightly different from what Hans proposed. The difference here
> is that we do not force newer drivers to use a callback only because
> of backward compatibility.
>
> Well, this is what I think of this problem, you may have a different
> point of view. What do you think?

No, that's not going to work. None of the USB and PCI drivers use 
i2c_board_info since they all can run on older kernels as well. So all 
those drivers need an s_config ops that they can call. Now, embedded 
drivers usually have no need to support older kernels, so these can use 
i2c_board_info directly.

If you know that an i2c driver is only ever called using this new 
v4l2_i2c_new_subdev_board_info function (I prefer the shorter name 
v4l2_i2c_subdev_board() BTW), then you can choose to not implement s_config 
in that i2c driver.

But i2c drivers that have to support older kernels as well should use 
s_config.

Note that s_config also needs an 'int irq' argument besides the 'void 
*platform_data'. And that we should also add a 
v4l2_i2c_probed_subdev_board() call.

Regards,

	Hans

>
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > --
> > 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
Guennadi Liakhovetski May 22, 2009, 12:16 p.m. UTC | #5
On Fri, 22 May 2009, Hans Verkuil wrote:

> A quick note for Guennadi: the i2c_board_info and the new i2c API has been 
> available since 2.6.22, but for the subdev support in v4l2 I've decided not 
> to use the new i2c API for kernels < 2.6.26 due to a serious i2c core 
> kernel bug that wasn't fixed until 2.6.26 (probing for the existence of an 
> i2c device at certain addresses can cause an oops). Strictly speaking it 
> would be possible to support board_info from 2.6.22 onwards, but going that 
> way makes it very messy with lots of #ifdefs. I want to keep the simple 
> rule to only support the new i2c API for 2.6.26 onwards.

Hm, I am afraid, I do not understand.

My patch doesn't change any behaviour. It just adds one more way of 
calling the same function, which is already there, just with different 
parameters. All existing (or new) drivers can call v4l2_i2c_new_subdev() 
just like before - nothing changes for them. Only internally this function 
now will use its "struct i2c_board_info info" which it _already_ has, to 
call a new function - v4l2_i2c_subdev_board(). No change whatsoever! 
Drivers, that know _will_ work this way, e.g., if they don't care about 
_any_ other kernel versions, except for the one they are compiled for, can 
call that function - v4l2_i2c_subdev_board() directly. Are you concerned, 
that some drivers, that do want to work with older kernels, will switch to 
using this function and then fail for older kernels? Well, we can put the

EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_board);

line under an ifdef if you want, but even that I don't think is necessary, 
we just have to catch those careless drivers. Or am I missing something?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
Hans Verkuil May 22, 2009, 12:58 p.m. UTC | #6
On Friday 22 May 2009 14:16:13 Guennadi Liakhovetski wrote:
> On Fri, 22 May 2009, Hans Verkuil wrote:
> > A quick note for Guennadi: the i2c_board_info and the new i2c API has
> > been available since 2.6.22, but for the subdev support in v4l2 I've
> > decided not to use the new i2c API for kernels < 2.6.26 due to a
> > serious i2c core kernel bug that wasn't fixed until 2.6.26 (probing for
> > the existence of an i2c device at certain addresses can cause an oops).
> > Strictly speaking it would be possible to support board_info from
> > 2.6.22 onwards, but going that way makes it very messy with lots of
> > #ifdefs. I want to keep the simple rule to only support the new i2c API
> > for 2.6.26 onwards.
>
> Hm, I am afraid, I do not understand.
>
> My patch doesn't change any behaviour. It just adds one more way of
> calling the same function, which is already there, just with different
> parameters. All existing (or new) drivers can call v4l2_i2c_new_subdev()
> just like before - nothing changes for them. Only internally this
> function now will use its "struct i2c_board_info info" which it _already_
> has, to call a new function - v4l2_i2c_subdev_board(). No change
> whatsoever! Drivers, that know _will_ work this way, e.g., if they don't
> care about _any_ other kernel versions, except for the one they are
> compiled for, can call that function - v4l2_i2c_subdev_board() directly.
> Are you concerned, that some drivers, that do want to work with older
> kernels, will switch to using this function and then fail for older
> kernels? Well, we can put the
>
> EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_board);
>
> line under an ifdef if you want, but even that I don't think is
> necessary, we just have to catch those careless drivers. Or am I missing
> something?

There are two 'missing somethings': the first is that you can have a v4l2 
driver that uses v4l2_i2c_subdev_board, but the i2c driver that is loaded 
by that call can also by used by other drivers need to be backwards 
compatible with older kernels.

So the i2c driver cannot in general obtain the irq and platform_data from 
i2c_client during probe, since it won't be set during probe (actually, the 
irq field in i2c_client doesn't even exist on pre-2.6.22 kernels).

But if v4l2_i2c_subdev_board calls s_config explicitly, then this data can 
be passed to the i2c driver in a manner that works for all kernels.

The second 'something' is that I need something like s_config anyway for 
those drivers that cannot use this new v4l2_i2c_subdev_board because they 
have to compile for all kernels. Those drivers can call the existing 
functions, and then call s_config explicitly.

For the embedded platforms it really doesn't matter: there you just use 
i2c_board_info, and the only time s_config comes into play is when the i2c 
driver can also be used by bridge drivers that cannot use 
v4l2_i2c_subdev_board.

Regards,

	Hans
Eduardo Valentin May 22, 2009, 1:14 p.m. UTC | #7
Hi,

On Fri, May 22, 2009 at 02:58:49PM +0200, ext Hans Verkuil wrote:
> On Friday 22 May 2009 14:16:13 Guennadi Liakhovetski wrote:
> > On Fri, 22 May 2009, Hans Verkuil wrote:
> > > A quick note for Guennadi: the i2c_board_info and the new i2c API has
> > > been available since 2.6.22, but for the subdev support in v4l2 I've
> > > decided not to use the new i2c API for kernels < 2.6.26 due to a
> > > serious i2c core kernel bug that wasn't fixed until 2.6.26 (probing for
> > > the existence of an i2c device at certain addresses can cause an oops).
> > > Strictly speaking it would be possible to support board_info from
> > > 2.6.22 onwards, but going that way makes it very messy with lots of
> > > #ifdefs. I want to keep the simple rule to only support the new i2c API
> > > for 2.6.26 onwards.
> >
> > Hm, I am afraid, I do not understand.
> >
> > My patch doesn't change any behaviour. It just adds one more way of
> > calling the same function, which is already there, just with different
> > parameters. All existing (or new) drivers can call v4l2_i2c_new_subdev()
> > just like before - nothing changes for them. Only internally this
> > function now will use its "struct i2c_board_info info" which it _already_
> > has, to call a new function - v4l2_i2c_subdev_board(). No change
> > whatsoever! Drivers, that know _will_ work this way, e.g., if they don't
> > care about _any_ other kernel versions, except for the one they are
> > compiled for, can call that function - v4l2_i2c_subdev_board() directly.
> > Are you concerned, that some drivers, that do want to work with older
> > kernels, will switch to using this function and then fail for older
> > kernels? Well, we can put the
> >
> > EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_board);
> >
> > line under an ifdef if you want, but even that I don't think is
> > necessary, we just have to catch those careless drivers. Or am I missing
> > something?
> 
> There are two 'missing somethings': the first is that you can have a v4l2 
> driver that uses v4l2_i2c_subdev_board, but the i2c driver that is loaded 
> by that call can also by used by other drivers need to be backwards 
> compatible with older kernels.
> 
> So the i2c driver cannot in general obtain the irq and platform_data from 
> i2c_client during probe, since it won't be set during probe (actually, the 
> irq field in i2c_client doesn't even exist on pre-2.6.22 kernels).
> 
> But if v4l2_i2c_subdev_board calls s_config explicitly, then this data can 
> be passed to the i2c driver in a manner that works for all kernels.
> 
> The second 'something' is that I need something like s_config anyway for 
> those drivers that cannot use this new v4l2_i2c_subdev_board because they 
> have to compile for all kernels. Those drivers can call the existing 
> functions, and then call s_config explicitly.
> 
> For the embedded platforms it really doesn't matter: there you just use 
> i2c_board_info, and the only time s_config comes into play is when the i2c 
> driver can also be used by bridge drivers that cannot use 
> v4l2_i2c_subdev_board.

I think this is heading to other direction that I first understood. I don't
see why one i2c driver which implements the new i2c api would not setup
its irq and platform data other then during probe time. But let's assume that.

In that case, i2c drivers, even though they implement the new i2c api, should
not receive the board specific info during probe, but should wait until s_config time.

That would work for all kernels, but then i2c drivers would not have opportunity to
communicate with device during probe time and do silly checks there.

Why can not v4l2_i2c_subdev_board be able to pass i2c_board_info using normal
i2c api if we are >= 2.6.26, and let s_config to bridge driver? Or even,
call s_config only for < 2.6.26 kernels ?

In both cases, I still see that i2c drivers should be able to be configured
both using normal i2c probes (with board data) or by using s_config.

Besides that, I think s_config can be passing a void *, something like:
+	int (*s_config)(struct v4l2_subdev *sd, void *config_data);

As in my last rfc patch. This way you don't need to care which board data
need to be passed. We can even pass a i2c board info there. That should just
be agreed between bridge and sub dev drivers.

> 
> Regards,
> 
> 	Hans
> 
> -- 
> Hans Verkuil - video4linux developer - sponsored by TANDBERG
Eduardo Valentin May 25, 2009, 10:18 a.m. UTC | #8
Hi,

On Fri, May 22, 2009 at 03:14:52PM +0200, Valentin Eduardo (Nokia-D/Helsinki) wrote:
> Hi,
> 
> > >
> > 
> > There are two 'missing somethings': the first is that you can have a v4l2 
> > driver that uses v4l2_i2c_subdev_board, but the i2c driver that is loaded 
> > by that call can also by used by other drivers need to be backwards 
> > compatible with older kernels.
> > 
> > So the i2c driver cannot in general obtain the irq and platform_data from 
> > i2c_client during probe, since it won't be set during probe (actually, the 
> > irq field in i2c_client doesn't even exist on pre-2.6.22 kernels).
> > 
> > But if v4l2_i2c_subdev_board calls s_config explicitly, then this data can 
> > be passed to the i2c driver in a manner that works for all kernels.
> > 
> > The second 'something' is that I need something like s_config anyway for 
> > those drivers that cannot use this new v4l2_i2c_subdev_board because they 
> > have to compile for all kernels. Those drivers can call the existing 
> > functions, and then call s_config explicitly.
> > 
> > For the embedded platforms it really doesn't matter: there you just use 
> > i2c_board_info, and the only time s_config comes into play is when the i2c 
> > driver can also be used by bridge drivers that cannot use 
> > v4l2_i2c_subdev_board.
> 
> I think this is heading to other direction that I first understood. I don't
> see why one i2c driver which implements the new i2c api would not setup
> its irq and platform data other then during probe time. But let's assume that.
> 
> In that case, i2c drivers, even though they implement the new i2c api, should
> not receive the board specific info during probe, but should wait until s_config time.
> 
> That would work for all kernels, but then i2c drivers would not have opportunity to
> communicate with device during probe time and do silly checks there.
> 
> Why can not v4l2_i2c_subdev_board be able to pass i2c_board_info using normal
> i2c api if we are >= 2.6.26, and let s_config to bridge driver? Or even,
> call s_config only for < 2.6.26 kernels ?
> 
> In both cases, I still see that i2c drivers should be able to be configured
> both using normal i2c probes (with board data) or by using s_config.
> 
> Besides that, I think s_config can be passing a void *, something like:
> +	int (*s_config)(struct v4l2_subdev *sd, void *config_data);
> 
> As in my last rfc patch. This way you don't need to care which board data
> need to be passed. We can even pass a i2c board info there. That should just
> be agreed between bridge and sub dev drivers.

I think I understood your point. Maybe what you are trying to keep is the same
v4l2 api so that can be used in all kernel versions. Based on that we cannot
pass i2c_board_info as v4l2 api function (at least for now).

So, I think I'm sending a patch with this in mind. Which is:
* Addition of s_config(irq, platform_data) into core callbacks set:
+       int (*s_config)(struct v4l2_subdev *sd, int irq, void *platform_data);

* Addition of v4l2_i2c_new_subdev_board, which is basically the same
of v4l2_i2c_new_subdev, but with two additional parameters: irq and platform_data.
This function will pass these data to i2c normal probe procedure (if we are >= 2.6.26),
and will call s_config with these two additional parameters.

So, both older i2c version compatible and newer i2c version only drivers can
call it. Is that what you were thinking?

I'm sending that patch as well in other email.


> 
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > -- 
> > Hans Verkuil - video4linux developer - sponsored by TANDBERG
> 
> -- 
> Eduardo Valentin
diff mbox

Patch

diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
index f576ef6..ab190aa 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -758,30 +758,38 @@  void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
 }
 EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
 
-
-
 /* Load an i2c sub-device. */
 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)
 {
-	struct v4l2_subdev *sd = NULL;
-	struct i2c_client *client;
 	struct i2c_board_info info;
 
-	BUG_ON(!v4l2_dev);
-
-	if (module_name)
-		request_module(module_name);
-
 	/* Setup the i2c board info with the device type and
 	   the device address. */
 	memset(&info, 0, sizeof(info));
 	strlcpy(info.type, client_type, sizeof(info.type));
 	info.addr = addr;
 
+	return v4l2_i2c_subdev_board(v4l2_dev, adapter, module_name, &info);
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
+
+/* Load an i2c sub-device using provided struct i2c_board_info. */
+struct v4l2_subdev *v4l2_i2c_subdev_board(struct v4l2_device *v4l2_dev,
+		struct i2c_adapter *adapter,
+		const char *module_name, const struct i2c_board_info *info)
+{
+	struct v4l2_subdev *sd = NULL;
+	struct i2c_client *client;
+
+	BUG_ON(!v4l2_dev);
+
+	if (module_name)
+		request_module(module_name);
+
 	/* Create the i2c client */
-	client = i2c_new_device(adapter, &info);
+	client = i2c_new_device(adapter, info);
 	/* Note: it is possible in the future that
 	   c->driver is NULL if the driver is still being loaded.
 	   We need better support from the kernel so that we
@@ -808,7 +816,7 @@  error:
 		i2c_unregister_device(client);
 	return sd;
 }
-EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
+EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_board);
 
 /* Probe and load an i2c sub-device. */
 struct v4l2_subdev *v4l2_i2c_new_probed_subdev(struct v4l2_device *v4l2_dev,
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index c48c24e..a67f5c3 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -131,6 +131,7 @@  struct i2c_driver;
 struct i2c_adapter;
 struct i2c_client;
 struct i2c_device_id;
+struct i2c_board_info;
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
@@ -142,6 +143,10 @@  struct v4l2_subdev_ops;
 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 above but uses user-provided struct i2c_board_info */
+struct v4l2_subdev *v4l2_i2c_subdev_board(struct v4l2_device *v4l2_dev,
+		struct i2c_adapter *adapter,
+		const char *module_name, const struct i2c_board_info *info);
 /* 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. */