adv7180 as SoC camera device

adv7180 as SoC camera device

[Rodolfo Giometti]

Hello,

on my pxa27x based board I have a adv7180 connected with the CIF
interface. Due this fact I'm going to use the pxa_camera.c driver
which in turn registers a soc_camera_host.

In the latest kernel I found your driver for the ADV7180, but it
registers the chip as a v4l sub device.

I suppose these two interfaces are not compatible, aren't they?

In this situation, should I write a new driver for the
soc_camera_device? Which is The-Right-Thing(TM) to do? :)

Thanks in advance,

Rodolfo Giometti

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: adv7180 as SoC camera device

[Guennadi Liakhovetski]

On Fri, 19 Feb 2010, Rodolfo Giometti wrote:

> Hello,
> 
> on my pxa27x based board I have a adv7180 connected with the CIF
> interface. Due this fact I'm going to use the pxa_camera.c driver
> which in turn registers a soc_camera_host.
> 
> In the latest kernel I found your driver for the ADV7180, but it
> registers the chip as a v4l sub device.
> 
> I suppose these two interfaces are not compatible, aren't they?

Congratulations! Thereby you're in a position to develop the first 
v4l2-subdev / soc-camera universal driver;) The answer to this your 
question is - they are... kinda. This means - yes, soc-camera is also 
using the v4l2-subdev API, but - with a couple of additions. Basically, 
there are two things you have to change in the adv7180 driver to make it 
compatible with soc-camera - (1) add bus-configuration methods, even if 
they don't do much (see .query_bus_param() and .set_bus_param() methods 
from struct soc_camera_ops), and (2) migrate the driver to the mediabus 
API. The latter one requires some care - in principle, mediabus should be 
the future API to negotiate parameters on the video bus between bridges 
(in your case PXA CIF) and clients, but for you this means you also have 
to migrate any other bridge drivers in the mainline to that API, and, if 
they also interface to some other subdevices - those too, and if those can 
also work with other bridges - those too...;) But, I think, that chain 
will terminate quite soon, in fact, I cannot find any users of that driver 
currently in the mainline, Richard?

> In this situation, should I write a new driver for the
> soc_camera_device? Which is The-Right-Thing(TM) to do? :)

Please, have a look and try to convert the driver as described above. All 
the APIs and a few examples are in the mainline, so, you should have 
enough copy-paste sources;) Ask on the list (with me on cc) if anything is 
still unclear.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: adv7180 as SoC camera device

[Rodolfo Giometti]

On Fri, Feb 19, 2010 at 08:36:38PM +0100, Guennadi Liakhovetski wrote:
> On Fri, 19 Feb 2010, Rodolfo Giometti wrote:
> 
> > Hello,
> > 
> > on my pxa27x based board I have a adv7180 connected with the CIF
> > interface. Due this fact I'm going to use the pxa_camera.c driver
> > which in turn registers a soc_camera_host.
> > 
> > In the latest kernel I found your driver for the ADV7180, but it
> > registers the chip as a v4l sub device.
> > 
> > I suppose these two interfaces are not compatible, aren't they?
> 
> Congratulations! Thereby you're in a position to develop the first 
> v4l2-subdev / soc-camera universal driver;) The answer to this your 
> question is - they are... kinda. This means - yes, soc-camera is also 
> using the v4l2-subdev API, but - with a couple of additions. Basically, 
> there are two things you have to change in the adv7180 driver to make it 
> compatible with soc-camera - (1) add bus-configuration methods, even if 
> they don't do much (see .query_bus_param() and .set_bus_param() methods 
> from struct soc_camera_ops), and (2) migrate the driver to the mediabus 
> API. The latter one requires some care - in principle, mediabus should be 
> the future API to negotiate parameters on the video bus between bridges 
> (in your case PXA CIF) and clients, but for you this means you also have 
> to migrate any other bridge drivers in the mainline to that API, and, if 
> they also interface to some other subdevices - those too, and if those can 
> also work with other bridges - those too...;) But, I think, that chain 
> will terminate quite soon, in fact, I cannot find any users of that driver 
> currently in the mainline, Richard?
> 
> > In this situation, should I write a new driver for the
> > soc_camera_device? Which is The-Right-Thing(TM) to do? :)
> 
> Please, have a look and try to convert the driver as described above. All 
> the APIs and a few examples are in the mainline, so, you should have 
> enough copy-paste sources;) Ask on the list (with me on cc) if anything is 
> still unclear.

Thanks for your quick answer! :)

What I still don't understand is if should I move the driver form
v4l2-subdev to a soc_camera device or trying to support both API...

It seems to me that the driver is not used by any machines into
mainline so if soc-camera is also using the v4l2-subdev API but with a
couple of additions I suppose I can move it to soc_camera API...

Is that right?

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: adv7180 as SoC camera device

[Hans Verkuil]

On Monday 22 February 2010 17:01:39 Rodolfo Giometti wrote:
> On Fri, Feb 19, 2010 at 08:36:38PM +0100, Guennadi Liakhovetski wrote:
> > On Fri, 19 Feb 2010, Rodolfo Giometti wrote:
> > 
> > > Hello,
> > > 
> > > on my pxa27x based board I have a adv7180 connected with the CIF
> > > interface. Due this fact I'm going to use the pxa_camera.c driver
> > > which in turn registers a soc_camera_host.
> > > 
> > > In the latest kernel I found your driver for the ADV7180, but it
> > > registers the chip as a v4l sub device.
> > > 
> > > I suppose these two interfaces are not compatible, aren't they?
> > 
> > Congratulations! Thereby you're in a position to develop the first 
> > v4l2-subdev / soc-camera universal driver;) The answer to this your 
> > question is - they are... kinda. This means - yes, soc-camera is also 
> > using the v4l2-subdev API, but - with a couple of additions. Basically, 
> > there are two things you have to change in the adv7180 driver to make it 
> > compatible with soc-camera - (1) add bus-configuration methods, even if 
> > they don't do much (see .query_bus_param() and .set_bus_param() methods 
> > from struct soc_camera_ops), and (2) migrate the driver to the mediabus 
> > API. The latter one requires some care - in principle, mediabus should be 
> > the future API to negotiate parameters on the video bus between bridges 
> > (in your case PXA CIF) and clients, but for you this means you also have 
> > to migrate any other bridge drivers in the mainline to that API, and, if 
> > they also interface to some other subdevices - those too, and if those can 
> > also work with other bridges - those too...;) But, I think, that chain 
> > will terminate quite soon, in fact, I cannot find any users of that driver 
> > currently in the mainline, Richard?
> > 
> > > In this situation, should I write a new driver for the
> > > soc_camera_device? Which is The-Right-Thing(TM) to do? :)
> > 
> > Please, have a look and try to convert the driver as described above. All 
> > the APIs and a few examples are in the mainline, so, you should have 
> > enough copy-paste sources;) Ask on the list (with me on cc) if anything is 
> > still unclear.
> 
> Thanks for your quick answer! :)
> 
> What I still don't understand is if should I move the driver form
> v4l2-subdev to a soc_camera device or trying to support both API...
> 
> It seems to me that the driver is not used by any machines into
> mainline so if soc-camera is also using the v4l2-subdev API but with a
> couple of additions I suppose I can move it to soc_camera API...

The long-term goal is to remove the last soc-camera API dependencies from
the sensor subdev drivers. Subdevice (usually i2c) drivers should be fully
reusable and a dependency on soc-camera defeats that goal.

I think the only missing piece is low-level bus setup (i.e. sync polarities,
rising/falling edge sampling, etc.). Some proposals were made, but basically
nobody has had the time to actually implement this.

Right now, if you want to use your sensor with soc-camera, then you need to
support the soc-camera API (or what is left of it) in your subdev driver as
well.

Regards,

	Hans

> 
> Is that right?
> 
> Ciao,
> 
> Rodolfo
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: adv7180 as SoC camera device

[Rodolfo Giometti]

On Mon, Feb 22, 2010 at 05:11:18PM +0100, Hans Verkuil wrote:
> 
> The long-term goal is to remove the last soc-camera API dependencies from
> the sensor subdev drivers. Subdevice (usually i2c) drivers should be fully
> reusable and a dependency on soc-camera defeats that goal.
> 
> I think the only missing piece is low-level bus setup (i.e. sync polarities,
> rising/falling edge sampling, etc.). Some proposals were made, but basically
> nobody has had the time to actually implement this.
> 
> Right now, if you want to use your sensor with soc-camera, then you need to
> support the soc-camera API (or what is left of it) in your subdev driver as
> well.

But with the goal to remove the last soc-camera API dependencies I
suppose is better I try to change the pxa_camera driver in something
compatible with the API of the adv7180 driver...

I'm sorry but I'm a bit confused. :)

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: adv7180 as SoC camera device

[Guennadi Liakhovetski]

On Mon, 22 Feb 2010, Rodolfo Giometti wrote:

> On Mon, Feb 22, 2010 at 05:11:18PM +0100, Hans Verkuil wrote:
> > 
> > The long-term goal is to remove the last soc-camera API dependencies from
> > the sensor subdev drivers. Subdevice (usually i2c) drivers should be fully
> > reusable and a dependency on soc-camera defeats that goal.
> > 
> > I think the only missing piece is low-level bus setup (i.e. sync polarities,
> > rising/falling edge sampling, etc.). Some proposals were made, but basically
> > nobody has had the time to actually implement this.
> > 
> > Right now, if you want to use your sensor with soc-camera, then you need to
> > support the soc-camera API (or what is left of it) in your subdev driver as
> > well.
> 
> But with the goal to remove the last soc-camera API dependencies I
> suppose is better I try to change the pxa_camera driver in something
> compatible with the API of the adv7180 driver...

No. As Hans said, one of important things, that is present in soc-camera, 
but absent from v4l2-subdev is bus-parameter configuration. So, to remove 
those dependencies one would have to develop a generic bus-configuration 
API for V4L2, and then convert soc-camera core and all soc-camera drivers 
to it. Feel free to submit patches.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: adv7180 as SoC camera device

[Guennadi Liakhovetski]

On Mon, 22 Feb 2010, Rodolfo Giometti wrote:

> On Fri, Feb 19, 2010 at 08:36:38PM +0100, Guennadi Liakhovetski wrote:
> > On Fri, 19 Feb 2010, Rodolfo Giometti wrote:
> > 
> > > Hello,
> > > 
> > > on my pxa27x based board I have a adv7180 connected with the CIF
> > > interface. Due this fact I'm going to use the pxa_camera.c driver
> > > which in turn registers a soc_camera_host.
> > > 
> > > In the latest kernel I found your driver for the ADV7180, but it
> > > registers the chip as a v4l sub device.
> > > 
> > > I suppose these two interfaces are not compatible, aren't they?
> > 
> > Congratulations! Thereby you're in a position to develop the first 
> > v4l2-subdev / soc-camera universal driver;) The answer to this your 
> > question is - they are... kinda. This means - yes, soc-camera is also 
> > using the v4l2-subdev API, but - with a couple of additions. Basically, 
> > there are two things you have to change in the adv7180 driver to make it 
> > compatible with soc-camera - (1) add bus-configuration methods, even if 
> > they don't do much (see .query_bus_param() and .set_bus_param() methods 
> > from struct soc_camera_ops), and (2) migrate the driver to the mediabus 
> > API. The latter one requires some care - in principle, mediabus should be 
> > the future API to negotiate parameters on the video bus between bridges 
> > (in your case PXA CIF) and clients, but for you this means you also have 
> > to migrate any other bridge drivers in the mainline to that API, and, if 
> > they also interface to some other subdevices - those too, and if those can 
> > also work with other bridges - those too...;) But, I think, that chain 
> > will terminate quite soon, in fact, I cannot find any users of that driver 
> > currently in the mainline, Richard?
> > 
> > > In this situation, should I write a new driver for the
> > > soc_camera_device? Which is The-Right-Thing(TM) to do? :)
> > 
> > Please, have a look and try to convert the driver as described above. All 
> > the APIs and a few examples are in the mainline, so, you should have 
> > enough copy-paste sources;) Ask on the list (with me on cc) if anything is 
> > still unclear.
> 
> Thanks for your quick answer! :)
> 
> What I still don't understand is if should I move the driver form
> v4l2-subdev to a soc_camera device or trying to support both API...

Both. It is just one (v4l2-subdev) API, but soc-camera is using some 
extensions to it.

> It seems to me that the driver is not used by any machines into
> mainline

That makes your task even easier - you do not have to convert any bridge 
drivers to mediabus API.

> so if soc-camera is also using the v4l2-subdev API but with a
> couple of additions I suppose I can move it to soc_camera API...

Not move, extend. But preserve an ability to function without soc-camera 
additions too. I.e., the use of soc-camera extensions should be optional 
in your driver. Look here 
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/11486/focus=11493 
for an example.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: adv7180 as SoC camera device

[Jonathan Cameron]

On 02/22/10 16:16, Guennadi Liakhovetski wrote:
> On Mon, 22 Feb 2010, Rodolfo Giometti wrote:
> 
>> On Fri, Feb 19, 2010 at 08:36:38PM +0100, Guennadi Liakhovetski wrote:
>>> On Fri, 19 Feb 2010, Rodolfo Giometti wrote:
>>>
>>>> Hello,
>>>>
>>>> on my pxa27x based board I have a adv7180 connected with the CIF
>>>> interface. Due this fact I'm going to use the pxa_camera.c driver
>>>> which in turn registers a soc_camera_host.
>>>>
>>>> In the latest kernel I found your driver for the ADV7180, but it
>>>> registers the chip as a v4l sub device.
>>>>
>>>> I suppose these two interfaces are not compatible, aren't they?
>>>
>>> Congratulations! Thereby you're in a position to develop the first 
>>> v4l2-subdev / soc-camera universal driver;) The answer to this your 
>>> question is - they are... kinda. This means - yes, soc-camera is also 
>>> using the v4l2-subdev API, but - with a couple of additions. Basically, 
>>> there are two things you have to change in the adv7180 driver to make it 
>>> compatible with soc-camera - (1) add bus-configuration methods, even if 
>>> they don't do much (see .query_bus_param() and .set_bus_param() methods 
>>> from struct soc_camera_ops), and (2) migrate the driver to the mediabus 
>>> API. The latter one requires some care - in principle, mediabus should be 
>>> the future API to negotiate parameters on the video bus between bridges 
>>> (in your case PXA CIF) and clients, but for you this means you also have 
>>> to migrate any other bridge drivers in the mainline to that API, and, if 
>>> they also interface to some other subdevices - those too, and if those can 
>>> also work with other bridges - those too...;) But, I think, that chain 
>>> will terminate quite soon, in fact, I cannot find any users of that driver 
>>> currently in the mainline, Richard?
>>>
>>>> In this situation, should I write a new driver for the
>>>> soc_camera_device? Which is The-Right-Thing(TM) to do? :)
>>>
>>> Please, have a look and try to convert the driver as described above. All 
>>> the APIs and a few examples are in the mainline, so, you should have 
>>> enough copy-paste sources;) Ask on the list (with me on cc) if anything is 
>>> still unclear.
>>
>> Thanks for your quick answer! :)
>>
>> What I still don't understand is if should I move the driver form
>> v4l2-subdev to a soc_camera device or trying to support both API...
> 
> Both. It is just one (v4l2-subdev) API, but soc-camera is using some 
> extensions to it.
> 
>> It seems to me that the driver is not used by any machines into
>> mainline
> 
> That makes your task even easier - you do not have to convert any bridge 
> drivers to mediabus API.
Indeed.  Having time to do that is what is delaying the ov7670 conversion.
(that and having time in general!)  For info I did post some untested
patches for the ov7670 a while back that show the minimal changes I think
are needed under these circumstances.

> 
>> so if soc-camera is also using the v4l2-subdev API but with a
>> couple of additions I suppose I can move it to soc_camera API...
> 
> Not move, extend. But preserve an ability to function without soc-camera 
> additions too. I.e., the use of soc-camera extensions should be optional 
> in your driver. Look here 
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/11486/focus=11493 
> for an example.
> 
> 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
> 

Re: adv7180 as SoC camera device

[Richard Röjfors]

On 2/22/10 5:01 PM, Rodolfo Giometti wrote:
> On Fri, Feb 19, 2010 at 08:36:38PM +0100, Guennadi Liakhovetski wrote:
>> On Fri, 19 Feb 2010, Rodolfo Giometti wrote:
>>
>>> Hello,
>>>
>>> on my pxa27x based board I have a adv7180 connected with the CIF
>>> interface. Due this fact I'm going to use the pxa_camera.c driver
>>> which in turn registers a soc_camera_host.
>>>
>>> In the latest kernel I found your driver for the ADV7180, but it
>>> registers the chip as a v4l sub device.
>>>
>>> I suppose these two interfaces are not compatible, aren't they?
>>
>> Congratulations! Thereby you're in a position to develop the first 
>> v4l2-subdev / soc-camera universal driver;) The answer to this your 
>> question is - they are... kinda. This means - yes, soc-camera is also 
>> using the v4l2-subdev API, but - with a couple of additions. Basically, 
>> there are two things you have to change in the adv7180 driver to make it 
>> compatible with soc-camera - (1) add bus-configuration methods, even if 
>> they don't do much (see .query_bus_param() and .set_bus_param() methods 
>> from struct soc_camera_ops), and (2) migrate the driver to the mediabus 
>> API. The latter one requires some care - in principle, mediabus should be 
>> the future API to negotiate parameters on the video bus between bridges 
>> (in your case PXA CIF) and clients, but for you this means you also have 
>> to migrate any other bridge drivers in the mainline to that API, and, if 
>> they also interface to some other subdevices - those too, and if those can 
>> also work with other bridges - those too...;) But, I think, that chain 
>> will terminate quite soon, in fact, I cannot find any users of that driver 
>> currently in the mainline, Richard?
>>
>>> In this situation, should I write a new driver for the
>>> soc_camera_device? Which is The-Right-Thing(TM) to do? :)
>>
>> Please, have a look and try to convert the driver as described above. All 
>> the APIs and a few examples are in the mainline, so, you should have 
>> enough copy-paste sources;) Ask on the list (with me on cc) if anything is 
>> still unclear.
> 
> Thanks for your quick answer! :)
> 
> What I still don't understand is if should I move the driver form
> v4l2-subdev to a soc_camera device or trying to support both API...
> 
> It seems to me that the driver is not used by any machines into
> mainline so if soc-camera is also using the v4l2-subdev API but with a
> couple of additions I suppose I can move it to soc_camera API...
> 
> Is that right?

We use it as a subdev to a driver not yet committed from us. So I think
you should extend it, not move it.

--Richard

Re: adv7180 as SoC camera device

[Rodolfo Giometti]

On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
> 
> We use it as a subdev to a driver not yet committed from us. So I think
> you should extend it, not move it.

Finally I got something functional... but I'm puzzled to know how I
can add platform data configuration struct by using the I2C's
platform_data pointer if it is already used to hold struct
soc_camera_device... O_o

In fact my probe function looks like:

static __devinit int adv7180_probe(struct i2c_client *client,
                        const struct i2c_device_id *id)
{
        struct adv7180_state *state;
        struct soc_camera_device *icd = client->dev.platform_data;
        struct soc_camera_link *icl;
        struct v4l2_subdev *sd;
        int ret;

        /* Check if the adapter supports the needed features */
        if (!i2c_check_functionality(client->adapter,
	I2C_FUNC_SMBUS_BYTE_DATA))
                return -EIO;

        v4l_info(client, "chip found @ 0x%02x (%s)\n",
                        client->addr << 1, client->adapter->name);

        if (icd) {
                icl = to_soc_camera_link(icd);
                if (!icl)
                        return -EINVAL;

                icd->ops = &adv7180_soc_ops;
                v4l_info(client, "soc-camera support enabled\n");
        } else
                pdata = client->dev.platform_data;

        state = kzalloc(sizeof(struct adv7180_state), GFP_KERNEL);
        if (state == NULL) {
                ret = -ENOMEM;
                goto err;
        }

        state->irq = client->irq;
        INIT_WORK(&state->work, adv7180_work);
        mutex_init(&state->mutex);
        state->autodetect = true;
        sd = &state->sd;
        v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
	...

Thanks in advance,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: adv7180 as SoC camera device

[Guennadi Liakhovetski]

On Tue, 30 Mar 2010, Rodolfo Giometti wrote:

> On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
> > 
> > We use it as a subdev to a driver not yet committed from us. So I think
> > you should extend it, not move it.
> 
> Finally I got something functional... but I'm puzzled to know how I
> can add platform data configuration struct by using the I2C's
> platform_data pointer if it is already used to hold struct
> soc_camera_device... O_o

As usual, looking at existing examples helps, e.g., in ov772x:

	priv->info = icl->priv;

i.e., icl->priv is where you pass subdevice-driver specific data with 
the soc-camera API.

> In fact my probe function looks like:
> 
> static __devinit int adv7180_probe(struct i2c_client *client,
>                         const struct i2c_device_id *id)
> {
>         struct adv7180_state *state;
>         struct soc_camera_device *icd = client->dev.platform_data;
>         struct soc_camera_link *icl;
>         struct v4l2_subdev *sd;
>         int ret;
> 
>         /* Check if the adapter supports the needed features */
>         if (!i2c_check_functionality(client->adapter,
> 	I2C_FUNC_SMBUS_BYTE_DATA))
>                 return -EIO;
> 
>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>                         client->addr << 1, client->adapter->name);
> 
>         if (icd) {
>                 icl = to_soc_camera_link(icd);
>                 if (!icl)
>                         return -EINVAL;
> 
>                 icd->ops = &adv7180_soc_ops;
>                 v4l_info(client, "soc-camera support enabled\n");
>         } else
>                 pdata = client->dev.platform_data;

This is a complicated way to say

		pdata = NULL;

Thanks
Guennadi

> 
>         state = kzalloc(sizeof(struct adv7180_state), GFP_KERNEL);
>         if (state == NULL) {
>                 ret = -ENOMEM;
>                 goto err;
>         }
> 
>         state->irq = client->irq;
>         INIT_WORK(&state->work, adv7180_work);
>         mutex_init(&state->mutex);
>         state->autodetect = true;
>         sd = &state->sd;
>         v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
> 	...
> 
> Thanks in advance,
> 
> Rodolfo
> 
> -- 
> 
> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
> Linux Device Driver                          giometti@linux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming                     skype:  rodolfo.giometti
> Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
> 

---
Guennadi Liakhovetski

Re: adv7180 as SoC camera device

[Rodolfo Giometti]

On Tue, Mar 30, 2010 at 04:40:17PM +0200, Guennadi Liakhovetski wrote:
> On Tue, 30 Mar 2010, Rodolfo Giometti wrote:
> 
> > On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
> > > 
> > > We use it as a subdev to a driver not yet committed from us. So I think
> > > you should extend it, not move it.
> > 
> > Finally I got something functional... but I'm puzzled to know how I
> > can add platform data configuration struct by using the I2C's
> > platform_data pointer if it is already used to hold struct
> > soc_camera_device... O_o
> 
> As usual, looking at existing examples helps, e.g., in ov772x:
> 
> 	priv->info = icl->priv;
> 
> i.e., icl->priv is where you pass subdevice-driver specific data with 
> the soc-camera API.

This driver was developed as v4l2-sub device and I just add the
soc-camera support.

Doing what are you suggesting is not compatible with the v4l2-sub
device support, in fact the I2C platform_data is used to know if the
driver must enable soc-camera or not...

Look the code:

   static __devinit int adv7180_probe(struct i2c_client *client,
                           const struct i2c_device_id *id)
   {
           struct adv7180_state *state;
           struct soc_camera_device *icd = client->dev.platform_data;

icd is set as client->dev.platform_data. This can be use for normal
I2C devices to hold custom platform_data initialization.

           struct soc_camera_link *icl;
           struct adv7180_platform_data *pdata = NULL;
           struct v4l2_subdev *sd;
           int ret;

           /* Check if the adapter supports the needed features */
           if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
                   return -EIO;

           v4l_info(client, "chip found @ 0x%02x (%s)\n",
                           client->addr << 1, client->adapter->name);

           if (icd) {

If the icd is set we assume it holds a soc-camera struct.

                   icl = to_soc_camera_link(icd);
                   if (!icl || !icl->priv) {
                           v4l_err(client, "missing platform data!\n");
                           return -EINVAL;
                   }

                   icd->ops = &adv7180_soc_ops;

If soc-camera is defined we set some info in order to enable it...

                   v4l_info(client, "soc-camera support enabled\n");
           }
...otherwise the driver works in the old way.

           state = kzalloc(sizeof(struct adv7180_state), GFP_KERNEL);
           if (state == NULL) {
                   ret = -ENOMEM;
                   goto err;
           }

           state->irq = client->irq;
           INIT_WORK(&state->work, adv7180_work);
           mutex_init(&state->mutex);
           state->autodetect = true;
           sd = &state->sd;
           v4l2_i2c_subdev_init(sd, client, &adv7180_ops);

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: adv7180 as SoC camera device

[Guennadi Liakhovetski]

On Tue, 30 Mar 2010, Rodolfo Giometti wrote:

> On Tue, Mar 30, 2010 at 04:40:17PM +0200, Guennadi Liakhovetski wrote:
> > On Tue, 30 Mar 2010, Rodolfo Giometti wrote:
> > 
> > > On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
> > > > 
> > > > We use it as a subdev to a driver not yet committed from us. So I think
> > > > you should extend it, not move it.
> > > 
> > > Finally I got something functional... but I'm puzzled to know how I
> > > can add platform data configuration struct by using the I2C's
> > > platform_data pointer if it is already used to hold struct
> > > soc_camera_device... O_o
> > 
> > As usual, looking at existing examples helps, e.g., in ov772x:
> > 
> > 	priv->info = icl->priv;
> > 
> > i.e., icl->priv is where you pass subdevice-driver specific data with 
> > the soc-camera API.
> 
> This driver was developed as v4l2-sub device and I just add the
> soc-camera support.
> 
> Doing what are you suggesting is not compatible with the v4l2-sub
> device support, in fact the I2C platform_data is used to know if the
> driver must enable soc-camera or not...

Yes, this is a known limitation, I have tried to discuss this on this ML 
in the past to get some reasonable solution for this, i.e., to standardise 
the use of the platform-data pointer. However, this hasn't worked until 
now. Maybe we manage it this time. So, what we would need is some standard 
optional struct, to which platform-data may point. If it is not NULL, then 
there could be either a "priv" member, that we would then use for 
soc-camera specific data, or we could embed that common struct in an 
soc-camera specific one. I would prefer a priv pointer.

> 
> Look the code:
> 
>    static __devinit int adv7180_probe(struct i2c_client *client,
>                            const struct i2c_device_id *id)
>    {
>            struct adv7180_state *state;
>            struct soc_camera_device *icd = client->dev.platform_data;
> 
> icd is set as client->dev.platform_data. This can be use for normal
> I2C devices to hold custom platform_data initialization.
> 
>            struct soc_camera_link *icl;
>            struct adv7180_platform_data *pdata = NULL;
>            struct v4l2_subdev *sd;
>            int ret;
> 
>            /* Check if the adapter supports the needed features */
>            if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>                    return -EIO;
> 
>            v4l_info(client, "chip found @ 0x%02x (%s)\n",
>                            client->addr << 1, client->adapter->name);
> 
>            if (icd) {
> 
> If the icd is set we assume it holds a soc-camera struct.
> 
>                    icl = to_soc_camera_link(icd);
>                    if (!icl || !icl->priv) {
>                            v4l_err(client, "missing platform data!\n");
>                            return -EINVAL;
>                    }
> 
>                    icd->ops = &adv7180_soc_ops;
> 
> If soc-camera is defined we set some info in order to enable it...
> 
>                    v4l_info(client, "soc-camera support enabled\n");
>            }
> ...otherwise the driver works in the old way.

Exactly, now we just have to standardise the use of 
client->dev.platform_data for v4l(2) I2C devices.

> 
>            state = kzalloc(sizeof(struct adv7180_state), GFP_KERNEL);
>            if (state == NULL) {
>                    ret = -ENOMEM;
>                    goto err;
>            }
> 
>            state->irq = client->irq;
>            INIT_WORK(&state->work, adv7180_work);
>            mutex_init(&state->mutex);
>            state->autodetect = true;
>            sd = &state->sd;
>            v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
> 
> Ciao,
> 
> Rodolfo

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: adv7180 as SoC camera device

[Rodolfo Giometti]

On Tue, Mar 30, 2010 at 04:06:11PM +0200, Rodolfo Giometti wrote:
> On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
> > 
> > We use it as a subdev to a driver not yet committed from us. So I think
> > you should extend it, not move it.
> 
> Finally I got something functional... but I'm puzzled to know how I
> can add platform data configuration struct by using the I2C's
> platform_data pointer if it is already used to hold struct
> soc_camera_device... O_o

Here my solution:

static __devinit int adv7180_probe(struct i2c_client *client,
                        const struct i2c_device_id *id)
{
        struct adv7180_state *state;
#if defined(CONFIG_SOC_CAMERA)
        struct soc_camera_device *icd = client->dev.platform_data;
        struct soc_camera_link *icl;
        struct adv7180_platform_data *pdata = NULL;
#else
        struct adv7180_platform_data *pdata =
	client->dev.platform_data;
#endif
        struct v4l2_subdev *sd;
        int i, ret;

        /* Check if the adapter supports the needed features */
        if (!i2c_check_functionality(client->adapter,
	I2C_FUNC_SMBUS_BYTE_DATA))
                return -EIO;

        v4l_info(client, "chip found @ 0x%02x (%s)\n",
                        client->addr << 1, client->adapter->name);

#if defined(CONFIG_SOC_CAMERA)
        if (icd) {
                icl = to_soc_camera_link(icd);
                if (!icl || !icl->priv) {
                        v4l_err(client, "missing platform data!\n");
                        return -EINVAL;
                }
                pdata = icl->priv;

                icd->ops = &adv7180_soc_ops;
                v4l_info(client, "soc-camera support enabled\n");
        }
#endif

        state = kzalloc(sizeof(struct adv7180_state), GFP_KERNEL);
        if (state == NULL) {
                ret = -ENOMEM;
                goto err;
        }

        state->irq = client->irq;
        INIT_WORK(&state->work, adv7180_work);
        mutex_init(&state->mutex);
        state->autodetect = true;
        sd = &state->sd;
        v4l2_i2c_subdev_init(sd, client, &adv7180_ops);

        if (pdata)
                for (i = 0; pdata[i].reg >= 0; i++) {
                        printk("----> %x %x\n", pdata[i].reg,
			pdata[i].val);
                        ret = i2c_smbus_write_byte_data(client,
                                        pdata[i].reg, pdata[i].val);
                        if (ret < 0)
                                goto err_unreg_subdev;
                }

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: adv7180 as SoC camera device

[Guennadi Liakhovetski]

On Thu, 1 Apr 2010, Rodolfo Giometti wrote:

> On Tue, Mar 30, 2010 at 04:06:11PM +0200, Rodolfo Giometti wrote:
> > On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
> > > 
> > > We use it as a subdev to a driver not yet committed from us. So I think
> > > you should extend it, not move it.
> > 
> > Finally I got something functional... but I'm puzzled to know how I
> > can add platform data configuration struct by using the I2C's
> > platform_data pointer if it is already used to hold struct
> > soc_camera_device... O_o
> 
> Here my solution:
> 
> static __devinit int adv7180_probe(struct i2c_client *client,
>                         const struct i2c_device_id *id)
> {
>         struct adv7180_state *state;
> #if defined(CONFIG_SOC_CAMERA)
>         struct soc_camera_device *icd = client->dev.platform_data;
>         struct soc_camera_link *icl;
>         struct adv7180_platform_data *pdata = NULL;
> #else
>         struct adv7180_platform_data *pdata =
> 	client->dev.platform_data;
> #endif

No, we don't want any ifdefs in drivers. And the current driver doesn't 
use the platform data, so, I would just leave it as it is - if NULL - no 
soc-camera and no platform data, if it set - soc-camera environment is 
used. That's until we standardise the use of platform data for v4l i2c 
devices.

>         struct v4l2_subdev *sd;
>         int i, ret;
> 
>         /* Check if the adapter supports the needed features */
>         if (!i2c_check_functionality(client->adapter,
> 	I2C_FUNC_SMBUS_BYTE_DATA))
>                 return -EIO;
> 
>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>                         client->addr << 1, client->adapter->name);
> 
> #if defined(CONFIG_SOC_CAMERA)
>         if (icd) {
>                 icl = to_soc_camera_link(icd);
>                 if (!icl || !icl->priv) {
>                         v4l_err(client, "missing platform data!\n");
>                         return -EINVAL;
>                 }
>                 pdata = icl->priv;
> 
>                 icd->ops = &adv7180_soc_ops;
>                 v4l_info(client, "soc-camera support enabled\n");
>         }
> #endif
> 
>         state = kzalloc(sizeof(struct adv7180_state), GFP_KERNEL);
>         if (state == NULL) {
>                 ret = -ENOMEM;
>                 goto err;
>         }
> 
>         state->irq = client->irq;
>         INIT_WORK(&state->work, adv7180_work);
>         mutex_init(&state->mutex);
>         state->autodetect = true;
>         sd = &state->sd;
>         v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
> 
>         if (pdata)
>                 for (i = 0; pdata[i].reg >= 0; i++) {
>                         printk("----> %x %x\n", pdata[i].reg,
> 			pdata[i].val);
>                         ret = i2c_smbus_write_byte_data(client,
>                                         pdata[i].reg, pdata[i].val);
>                         if (ret < 0)
>                                 goto err_unreg_subdev;
>                 }

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: adv7180 as SoC camera device

[Rodolfo Giometti]

On Fri, Apr 02, 2010 at 09:09:30AM +0200, Guennadi Liakhovetski wrote:
> On Thu, 1 Apr 2010, Rodolfo Giometti wrote:
> 
> > On Tue, Mar 30, 2010 at 04:06:11PM +0200, Rodolfo Giometti wrote:
> > > On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
> > > > 
> > > > We use it as a subdev to a driver not yet committed from us. So I think
> > > > you should extend it, not move it.
> > > 
> > > Finally I got something functional... but I'm puzzled to know how I
> > > can add platform data configuration struct by using the I2C's
> > > platform_data pointer if it is already used to hold struct
> > > soc_camera_device... O_o
> > 
> > Here my solution:
> > 
> > static __devinit int adv7180_probe(struct i2c_client *client,
> >                         const struct i2c_device_id *id)
> > {
> >         struct adv7180_state *state;
> > #if defined(CONFIG_SOC_CAMERA)
> >         struct soc_camera_device *icd = client->dev.platform_data;
> >         struct soc_camera_link *icl;
> >         struct adv7180_platform_data *pdata = NULL;
> > #else
> >         struct adv7180_platform_data *pdata =
> > 	client->dev.platform_data;
> > #endif
> 
> No, we don't want any ifdefs in drivers. And the current driver doesn't 
> use the platform data, so, I would just leave it as it is - if NULL - no 
> soc-camera and no platform data, if it set - soc-camera environment is 
> used. That's until we standardise the use of platform data for v4l i2c 
> devices.

Ok. Thanks! :)

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it