adv7180 as SoC camera device

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? :)

Quoted 10 lines · 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?

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?

Quoted 2 lines

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.

Quoted 30 lines · Guennadi Liakhovetski

Quoted 10 lines · 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?

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?

Quoted 7 lines

Quoted 2 lines

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

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.

Quoted 9 lines · Hans Verkuil

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.

Quoted 3 lines

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...

Quoted 30 lines · Guennadi Liakhovetski

Quoted 10 lines · 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?

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?

Quoted 7 lines

Quoted 2 lines

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...

Quoted 39 lines · Rodolfo Giometti

Quoted 30 lines · Guennadi Liakhovetski

Quoted 10 lines · 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?

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?

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 30 lines · Guennadi Liakhovetski

Quoted 10 lines · 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?

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?

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines · Richard Röjfors

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

Signature · 5 lines

GNU/Linux Solutions                  e-mail: [email protected]
Linux Device Driver                          [email protected]
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Quoted 9 lines · Rodolfo Giometti

Quoted 3 lines · Richard Röjfors

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.

Quoted 12 lines · Guennadi Liakhovetski

Quoted 9 lines · Rodolfo Giometti

Quoted 3 lines · Richard Röjfors

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

Quoted 6 lines

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

Quoted 3 lines · Richard Röjfors

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

Quoted 9 lines · Rodolfo Giometti

Quoted 3 lines · Richard Röjfors

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;
                }

Quoted 21 lines · Rodolfo Giometti

Quoted 9 lines · Rodolfo Giometti

Quoted 3 lines · Richard Röjfors

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;

Quoted 4 lines

#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.