[RFC] Per-subdev, host-specific data

[RFC] Per-subdev, host-specific data

[Laurent Pinchart]

Hi everybody,

Trying to implement support for multiple sensors connected to the same OMAP3 
ISP input (all but one of the sensors need to be kept in reset obviously), I 
need to associate host-specific data to the sensor subdevs.

The terms host and bridge are considered as synonyms in the rest of this e-
mail.

The OMAP3 ISP platform data has interface configuration parameters for the two 
CSI2 (a and c), CCP2 and parallel interfaces. The parameters are used to 
configure the bus when a sensor is selected. To support multiple sensors on 
the same input, the parameters need to be specified per-sensor, and not ISP-
wide.

No issue in the platform data. Board codes declare an array of structures that 
embed a struct v4l2_subdev_i2c_board_info instance and an OMAP3 ISP-specific 
interface configuration structure.

At runtime, when a sensor is selected, I need to access the OMAP3 ISP-specific 
interface configuration structure for the selected sensor. At that point all I 
have is a v4l2_subdev structure pointer, without a way to get back to the 
interface configuration structure.

The only point in the code where the v4l2_subdev and the interface 
configuration data are both known and could be linked together is in the host 
driver's probe function, where the v4l2_subdev instances are created. I have 
two solutions there:

- store the v4l2_subdev pointer and the interface configuration data pointer 
in a host-specific array, and perform a an array lookup operation at runtime 
with the v4l2_subdev pointer as a key

- add a void *host_priv field to the v4l2_subdev structure, store the 
interface configuration data pointer in that field, and use the field at 
runtime

The second solution seems cleaner but requires an additional field in 
v4l2_subdev. Opinions and other comments will be appreciated.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Per-subdev, host-specific data

[Hans Verkuil]

On Friday 23 July 2010 15:01:29 Laurent Pinchart wrote:
> Hi everybody,
> 
> Trying to implement support for multiple sensors connected to the same OMAP3 
> ISP input (all but one of the sensors need to be kept in reset obviously), I 
> need to associate host-specific data to the sensor subdevs.
> 
> The terms host and bridge are considered as synonyms in the rest of this e-
> mail.
> 
> The OMAP3 ISP platform data has interface configuration parameters for the two 
> CSI2 (a and c), CCP2 and parallel interfaces. The parameters are used to 
> configure the bus when a sensor is selected. To support multiple sensors on 
> the same input, the parameters need to be specified per-sensor, and not ISP-
> wide.
> 
> No issue in the platform data. Board codes declare an array of structures that 
> embed a struct v4l2_subdev_i2c_board_info instance and an OMAP3 ISP-specific 
> interface configuration structure.
> 
> At runtime, when a sensor is selected, I need to access the OMAP3 ISP-specific 
> interface configuration structure for the selected sensor. At that point all I 
> have is a v4l2_subdev structure pointer, without a way to get back to the 
> interface configuration structure.
> 
> The only point in the code where the v4l2_subdev and the interface 
> configuration data are both known and could be linked together is in the host 
> driver's probe function, where the v4l2_subdev instances are created. I have 
> two solutions there:
> 
> - store the v4l2_subdev pointer and the interface configuration data pointer 
> in a host-specific array, and perform a an array lookup operation at runtime 
> with the v4l2_subdev pointer as a key
> 
> - add a void *host_priv field to the v4l2_subdev structure, store the 
> interface configuration data pointer in that field, and use the field at 
> runtime
> 
> The second solution seems cleaner but requires an additional field in 
> v4l2_subdev. Opinions and other comments will be appreciated.

There is a third option: the grp_id field is owned by the host driver, so you
could make that an index into a host specific array.

That said, I think having a host_priv field isn't a bad idea. Although if we
do this, then I think the existing priv field should be renamed to drv_priv
to prevent confusion.

Regards,

            Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC] Per-subdev, host-specific data

[Laurent Pinchart]

Hi Hans,

On Friday 23 July 2010 15:46:29 Hans Verkuil wrote:
> On Friday 23 July 2010 15:01:29 Laurent Pinchart wrote:
> > Hi everybody,
> > 
> > Trying to implement support for multiple sensors connected to the same
> > OMAP3 ISP input (all but one of the sensors need to be kept in reset
> > obviously), I need to associate host-specific data to the sensor
> > subdevs.
> > 
> > The terms host and bridge are considered as synonyms in the rest of this
> > e- mail.
> > 
> > The OMAP3 ISP platform data has interface configuration parameters for
> > the two CSI2 (a and c), CCP2 and parallel interfaces. The parameters are
> > used to configure the bus when a sensor is selected. To support multiple
> > sensors on the same input, the parameters need to be specified
> > per-sensor, and not ISP- wide.
> > 
> > No issue in the platform data. Board codes declare an array of structures
> > that embed a struct v4l2_subdev_i2c_board_info instance and an OMAP3
> > ISP-specific interface configuration structure.
> > 
> > At runtime, when a sensor is selected, I need to access the OMAP3
> > ISP-specific interface configuration structure for the selected sensor.
> > At that point all I have is a v4l2_subdev structure pointer, without a
> > way to get back to the interface configuration structure.
> > 
> > The only point in the code where the v4l2_subdev and the interface
> > configuration data are both known and could be linked together is in the
> > host driver's probe function, where the v4l2_subdev instances are
> > created. I have two solutions there:
> > 
> > - store the v4l2_subdev pointer and the interface configuration data
> > pointer in a host-specific array, and perform a an array lookup
> > operation at runtime with the v4l2_subdev pointer as a key
> > 
> > - add a void *host_priv field to the v4l2_subdev structure, store the
> > interface configuration data pointer in that field, and use the field at
> > runtime
> > 
> > The second solution seems cleaner but requires an additional field in
> > v4l2_subdev. Opinions and other comments will be appreciated.
> 
> There is a third option: the grp_id field is owned by the host driver, so
> you could make that an index into a host specific array.

Good point.

> That said, I think having a host_priv field isn't a bad idea. Although if
> we do this, then I think the existing priv field should be renamed to
> drv_priv to prevent confusion.

As Guennadi, Sakari and you all agree that the host_priv field is a good idea, 
I'll submit a patch.

Thanks.

-- 
Regards,

Laurent Pinchart

RE: [RFC] Per-subdev, host-specific data

[Sylwester Nawrocki]

Hello,

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Friday, July 23, 2010 3:01 PM
> To: Linux Media Mailing List
> Cc: Sakari Ailus; Guennadi Liakhovetski
> Subject: [RFC] Per-subdev, host-specific data
> 
> Hi everybody,
> 
> Trying to implement support for multiple sensors connected to the same
> OMAP3
> ISP input (all but one of the sensors need to be kept in reset
> obviously), I
> need to associate host-specific data to the sensor subdevs.
> 
> The terms host and bridge are considered as synonyms in the rest of
> this e-
> mail.
> 
> The OMAP3 ISP platform data has interface configuration parameters for
> the two
> CSI2 (a and c), CCP2 and parallel interfaces. The parameters are used
> to
> configure the bus when a sensor is selected. To support multiple
> sensors on
> the same input, the parameters need to be specified per-sensor, and not
> ISP-
> wide.
> 
> No issue in the platform data. Board codes declare an array of
> structures that
> embed a struct v4l2_subdev_i2c_board_info instance and an OMAP3 ISP-
> specific
> interface configuration structure.
> 
> At runtime, when a sensor is selected, I need to access the OMAP3 ISP-
> specific
> interface configuration structure for the selected sensor. At that
> point all I
> have is a v4l2_subdev structure pointer, without a way to get back to
> the
> interface configuration structure.
> 
> The only point in the code where the v4l2_subdev and the interface
> configuration data are both known and could be linked together is in
> the host
> driver's probe function, where the v4l2_subdev instances are created. I
> have
> two solutions there:
> 
> - store the v4l2_subdev pointer and the interface configuration data
> pointer
> in a host-specific array, and perform a an array lookup operation at
> runtime
> with the v4l2_subdev pointer as a key
> 
> - add a void *host_priv field to the v4l2_subdev structure, store the
> interface configuration data pointer in that field, and use the field
> at
> runtime
> 
> The second solution seems cleaner but requires an additional field in
> v4l2_subdev. Opinions and other comments will be appreciated.
> 

I like the solution with an additional void *host_priv field,
it could also possibly be useful for the notify() callback to v4l2_subdev
parent.
On our SoCs we also need some camera host interface specific data to be
attached
to image sensor subdevice and later passed to host driver. So host_priv
field in v4l2_subdev would be nice feature to have.

> --
> Regards,
> 
> Laurent Pinchart
> --
> 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

Regards,
--
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [RFC] Per-subdev, host-specific data

[Andy Walls]

On Fri, 2010-07-23 at 16:31 +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 23 July 2010 15:46:29 Hans Verkuil wrote:
> > On Friday 23 July 2010 15:01:29 Laurent Pinchart wrote:
> > > Hi everybody,
> > > 
> > > Trying to implement support for multiple sensors connected to the same
> > > OMAP3 ISP input (all but one of the sensors need to be kept in reset
> > > obviously), I need to associate host-specific data to the sensor
> > > subdevs.
> > > 
> > > The terms host and bridge are considered as synonyms in the rest of this
> > > e- mail.
> > > 
> > > The OMAP3 ISP platform data has interface configuration parameters for
> > > the two CSI2 (a and c), CCP2 and parallel interfaces. The parameters are
> > > used to configure the bus when a sensor is selected. To support multiple
> > > sensors on the same input, the parameters need to be specified
> > > per-sensor, and not ISP- wide.
> > > 
> > > No issue in the platform data. Board codes declare an array of structures
> > > that embed a struct v4l2_subdev_i2c_board_info instance and an OMAP3
> > > ISP-specific interface configuration structure.
> > > 
> > > At runtime, when a sensor is selected, I need to access the OMAP3
> > > ISP-specific interface configuration structure for the selected sensor.
> > > At that point all I have is a v4l2_subdev structure pointer, without a
> > > way to get back to the interface configuration structure.
> > > 
> > > The only point in the code where the v4l2_subdev and the interface
> > > configuration data are both known and could be linked together is in the
> > > host driver's probe function, where the v4l2_subdev instances are
> > > created. I have two solutions there:
> > > 
> > > - store the v4l2_subdev pointer and the interface configuration data
> > > pointer in a host-specific array, and perform a an array lookup
> > > operation at runtime with the v4l2_subdev pointer as a key
> > > 
> > > - add a void *host_priv field to the v4l2_subdev structure, store the
> > > interface configuration data pointer in that field, and use the field at
> > > runtime
> > > 
> > > The second solution seems cleaner but requires an additional field in
> > > v4l2_subdev. Opinions and other comments will be appreciated.

Why isn't

	v4l2_set_subdevdata(sd, private_ptr);

sufficient?

The cx18-av-core.[ch] files use that to get a bridge instance pointer
from a v4l2_subdev.

Or is it that the v4l2_subdev infrastructure help functions for I2C
connected devices already claim that?

Regards,
Andy

> > There is a third option: the grp_id field is owned by the host driver, so
> > you could make that an index into a host specific array.



> Good point.
> 
> > That said, I think having a host_priv field isn't a bad idea. Although if
> > we do this, then I think the existing priv field should be renamed to
> > drv_priv to prevent confusion.
> 
> As Guennadi, Sakari and you all agree that the host_priv field is a good idea, 
> I'll submit a patch.
> 
> Thanks.
> 

Re: [RFC] Per-subdev, host-specific data

[Hans Verkuil]

On Friday 23 July 2010 22:52:48 Andy Walls wrote:
> On Fri, 2010-07-23 at 16:31 +0200, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Friday 23 July 2010 15:46:29 Hans Verkuil wrote:
> > > On Friday 23 July 2010 15:01:29 Laurent Pinchart wrote:
> > > > Hi everybody,
> > > > 
> > > > Trying to implement support for multiple sensors connected to the same
> > > > OMAP3 ISP input (all but one of the sensors need to be kept in reset
> > > > obviously), I need to associate host-specific data to the sensor
> > > > subdevs.
> > > > 
> > > > The terms host and bridge are considered as synonyms in the rest of this
> > > > e- mail.
> > > > 
> > > > The OMAP3 ISP platform data has interface configuration parameters for
> > > > the two CSI2 (a and c), CCP2 and parallel interfaces. The parameters are
> > > > used to configure the bus when a sensor is selected. To support multiple
> > > > sensors on the same input, the parameters need to be specified
> > > > per-sensor, and not ISP- wide.
> > > > 
> > > > No issue in the platform data. Board codes declare an array of structures
> > > > that embed a struct v4l2_subdev_i2c_board_info instance and an OMAP3
> > > > ISP-specific interface configuration structure.
> > > > 
> > > > At runtime, when a sensor is selected, I need to access the OMAP3
> > > > ISP-specific interface configuration structure for the selected sensor.
> > > > At that point all I have is a v4l2_subdev structure pointer, without a
> > > > way to get back to the interface configuration structure.
> > > > 
> > > > The only point in the code where the v4l2_subdev and the interface
> > > > configuration data are both known and could be linked together is in the
> > > > host driver's probe function, where the v4l2_subdev instances are
> > > > created. I have two solutions there:
> > > > 
> > > > - store the v4l2_subdev pointer and the interface configuration data
> > > > pointer in a host-specific array, and perform a an array lookup
> > > > operation at runtime with the v4l2_subdev pointer as a key
> > > > 
> > > > - add a void *host_priv field to the v4l2_subdev structure, store the
> > > > interface configuration data pointer in that field, and use the field at
> > > > runtime
> > > > 
> > > > The second solution seems cleaner but requires an additional field in
> > > > v4l2_subdev. Opinions and other comments will be appreciated.
> 
> Why isn't
> 
> 	v4l2_set_subdevdata(sd, private_ptr);
> 
> sufficient?
> 
> The cx18-av-core.[ch] files use that to get a bridge instance pointer
> from a v4l2_subdev.
> 
> Or is it that the v4l2_subdev infrastructure help functions for I2C
> connected devices already claim that?

Yes, they do. It is used to store the struct i2c_client pointer.

Regards,

	Hans

> 
> Regards,
> Andy
> 
> > > There is a third option: the grp_id field is owned by the host driver, so
> > > you could make that an index into a host specific array.
> 
> 
> 
> > Good point.
> > 
> > > That said, I think having a host_priv field isn't a bad idea. Although if
> > > we do this, then I think the existing priv field should be renamed to
> > > drv_priv to prevent confusion.
> > 
> > As Guennadi, Sakari and you all agree that the host_priv field is a good idea, 
> > I'll submit a patch.
> > 
> > Thanks.
> > 
> 
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco