| Message ID | 1366320945-21591-24-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote: > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows > the driver to use generic functions to manage sensor power supplies. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> A small addition to this one too: to be absolutely honest, I also had to replace 12-bit formats with their 8-bit counterparts, because only 8 data lanes are connected to my camera host. We'll need to somehow properly solve this too. Thanks Guennadi > --- > drivers/media/i2c/mt9p031.c | 1 + > include/media/mt9p031.h | 3 +++ > 2 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c > index 70f4525..ca2cc6e 100644 > --- a/drivers/media/i2c/mt9p031.c > +++ b/drivers/media/i2c/mt9p031.c > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client, > goto done; > > mt9p031->subdev.dev = &client->dev; > + mt9p031->subdev.pdata = &pdata->sd_pdata; > ret = v4l2_async_register_subdev(&mt9p031->subdev); > > done: > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h > index 0c97b19..7bf7b53 100644 > --- a/include/media/mt9p031.h > +++ b/include/media/mt9p031.h > @@ -1,6 +1,8 @@ > #ifndef MT9P031_H > #define MT9P031_H > > +#include <media/v4l2-subdev.h> > + > struct v4l2_subdev; > > /* > @@ -15,6 +17,7 @@ struct mt9p031_platform_data { > int reset; > int ext_freq; > int target_freq; > + struct v4l2_subdev_platform_data sd_pdata; > }; > > #endif > -- > 1.7.2.5 > > -- > 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, 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
Hi Guennadi, On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote: > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote: > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows > > the driver to use generic functions to manage sensor power supplies. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > A small addition to this one too: to be absolutely honest, I also had to > replace 12-bit formats with their 8-bit counterparts, because only 8 data > lanes are connected to my camera host. We'll need to somehow properly > solve this too. That information should be conveyed by platform/DT data for the host, and be used to convert the 12-bit media bus code into a 8-bit media bus code in the host (a core helper function would probably be helpful). > > --- > > > > drivers/media/i2c/mt9p031.c | 1 + > > include/media/mt9p031.h | 3 +++ > > 2 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c > > index 70f4525..ca2cc6e 100644 > > --- a/drivers/media/i2c/mt9p031.c > > +++ b/drivers/media/i2c/mt9p031.c > > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client, > > goto done; > > > > mt9p031->subdev.dev = &client->dev; > > + mt9p031->subdev.pdata = &pdata->sd_pdata; > > ret = v4l2_async_register_subdev(&mt9p031->subdev); > > > > done: > > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h > > index 0c97b19..7bf7b53 100644 > > --- a/include/media/mt9p031.h > > +++ b/include/media/mt9p031.h > > @@ -1,6 +1,8 @@ > > #ifndef MT9P031_H > > #define MT9P031_H > > > > +#include <media/v4l2-subdev.h> > > + > > struct v4l2_subdev; > > /* > > @@ -15,6 +17,7 @@ struct mt9p031_platform_data { > > int reset; > > int ext_freq; > > int target_freq; > > + struct v4l2_subdev_platform_data sd_pdata; > > }; > > > > #endif
On Mon, 22 Apr 2013, Laurent Pinchart wrote: > Hi Guennadi, > > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote: > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote: > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows > > > the driver to use generic functions to manage sensor power supplies. > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > A small addition to this one too: to be absolutely honest, I also had to > > replace 12-bit formats with their 8-bit counterparts, because only 8 data > > lanes are connected to my camera host. We'll need to somehow properly > > solve this too. > > That information should be conveyed by platform/DT data for the host, and be > used to convert the 12-bit media bus code into a 8-bit media bus code in the > host (a core helper function would probably be helpful). Yes, and we discussed this before too, I think. I proposed based then to implement some compatibility table of "trivial" transformations, like a 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just needs to be implemented... Sure, I'd be happy to move soc_mediabus.c to drivers/media/v4l2-core/v4l2-mediabus.c. Thanks Guennadi > > > --- > > > > > > drivers/media/i2c/mt9p031.c | 1 + > > > include/media/mt9p031.h | 3 +++ > > > 2 files changed, 4 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c > > > index 70f4525..ca2cc6e 100644 > > > --- a/drivers/media/i2c/mt9p031.c > > > +++ b/drivers/media/i2c/mt9p031.c > > > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client, > > > goto done; > > > > > > mt9p031->subdev.dev = &client->dev; > > > + mt9p031->subdev.pdata = &pdata->sd_pdata; > > > ret = v4l2_async_register_subdev(&mt9p031->subdev); > > > > > > done: > > > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h > > > index 0c97b19..7bf7b53 100644 > > > --- a/include/media/mt9p031.h > > > +++ b/include/media/mt9p031.h > > > @@ -1,6 +1,8 @@ > > > #ifndef MT9P031_H > > > #define MT9P031_H > > > > > > +#include <media/v4l2-subdev.h> > > > + > > > struct v4l2_subdev; > > > /* > > > @@ -15,6 +17,7 @@ struct mt9p031_platform_data { > > > int reset; > > > int ext_freq; > > > int target_freq; > > > + struct v4l2_subdev_platform_data sd_pdata; > > > }; > > > > > > #endif > -- > Regards, > > Laurent Pinchart > --- 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
Hi Guennadi, On Thursday 18 April 2013 23:35:44 Guennadi Liakhovetski wrote: > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows > the driver to use generic functions to manage sensor power supplies. The mt9p031 driver now handles its regulators explicitly, please see commit 97f212767a4d0fbddbf4786ccedacb47fc210548 Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Date: Tue May 8 10:10:36 2012 -0300 [media] mt9p031: Add support for regulators Enable the regulators when powering the sensor up, and disable them when powering it down. The regulators are mandatory. Boards that don't allow controlling the sensor power lines must provide fixed voltage regulators. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > drivers/media/i2c/mt9p031.c | 1 + > include/media/mt9p031.h | 3 +++ > 2 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c > index 70f4525..ca2cc6e 100644 > --- a/drivers/media/i2c/mt9p031.c > +++ b/drivers/media/i2c/mt9p031.c > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client, > goto done; > > mt9p031->subdev.dev = &client->dev; > + mt9p031->subdev.pdata = &pdata->sd_pdata; > ret = v4l2_async_register_subdev(&mt9p031->subdev); > > done: > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h > index 0c97b19..7bf7b53 100644 > --- a/include/media/mt9p031.h > +++ b/include/media/mt9p031.h > @@ -1,6 +1,8 @@ > #ifndef MT9P031_H > #define MT9P031_H > > +#include <media/v4l2-subdev.h> > + > struct v4l2_subdev; > > /* > @@ -15,6 +17,7 @@ struct mt9p031_platform_data { > int reset; > int ext_freq; > int target_freq; > + struct v4l2_subdev_platform_data sd_pdata; > }; > > #endif
On Monday 22 April 2013 14:39:57 Guennadi Liakhovetski wrote: > On Mon, 22 Apr 2013, Laurent Pinchart wrote: > > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote: > > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote: > > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data > > > > allows the driver to use generic functions to manage sensor power > > > > supplies. > > > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > > > A small addition to this one too: to be absolutely honest, I also had to > > > replace 12-bit formats with their 8-bit counterparts, because only 8 > > > data lanes are connected to my camera host. We'll need to somehow > > > properly solve this too. > > > > That information should be conveyed by platform/DT data for the host, and > > be used to convert the 12-bit media bus code into a 8-bit media bus code > > in the host (a core helper function would probably be helpful). > > Yes, and we discussed this before too, I think. I proposed based then to > implement some compatibility table of "trivial" transformations, like a > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just > needs to be implemented... > > Sure, I'd be happy to move soc_mediabus.c to > drivers/media/v4l2-core/v4l2-mediabus.c. And the OMAP3 ISP driver has something similiar in drivers/media/platform/omap3isp/ispvideo.c > > > > --- > > > > > > > > drivers/media/i2c/mt9p031.c | 1 + > > > > include/media/mt9p031.h | 3 +++ > > > > 2 files changed, 4 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c > > > > index 70f4525..ca2cc6e 100644 > > > > --- a/drivers/media/i2c/mt9p031.c > > > > +++ b/drivers/media/i2c/mt9p031.c > > > > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client > > > > *client, > > > > > > > > goto done; > > > > > > > > mt9p031->subdev.dev = &client->dev; > > > > > > > > + mt9p031->subdev.pdata = &pdata->sd_pdata; > > > > > > > > ret = v4l2_async_register_subdev(&mt9p031->subdev); > > > > > > > > done: > > > > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h > > > > index 0c97b19..7bf7b53 100644 > > > > --- a/include/media/mt9p031.h > > > > +++ b/include/media/mt9p031.h > > > > @@ -1,6 +1,8 @@ > > > > > > > > #ifndef MT9P031_H > > > > #define MT9P031_H > > > > > > > > +#include <media/v4l2-subdev.h> > > > > + > > > > > > > > struct v4l2_subdev; > > > > /* > > > > > > > > @@ -15,6 +17,7 @@ struct mt9p031_platform_data { > > > > > > > > int reset; > > > > int ext_freq; > > > > int target_freq; > > > > > > > > + struct v4l2_subdev_platform_data sd_pdata; > > > > > > > > }; > > > > > > > > #endif
Hi Guennadi, On Mon, Apr 22, 2013 at 02:39:57PM +0200, Guennadi Liakhovetski wrote: > On Mon, 22 Apr 2013, Laurent Pinchart wrote: > > > Hi Guennadi, > > > > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote: > > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote: > > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows > > > > the driver to use generic functions to manage sensor power supplies. > > > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > > > A small addition to this one too: to be absolutely honest, I also had to > > > replace 12-bit formats with their 8-bit counterparts, because only 8 data > > > lanes are connected to my camera host. We'll need to somehow properly > > > solve this too. > > > > That information should be conveyed by platform/DT data for the host, and be > > used to convert the 12-bit media bus code into a 8-bit media bus code in the > > host (a core helper function would probably be helpful). > > Yes, and we discussed this before too, I think. I proposed based then to > implement some compatibility table of "trivial" transformations, like a > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just > needs to be implemented... These "trivial" transformations may turn out not to be so trivial. In the devicetree we would then need kind of 'shift-4-bit-left' properties. How about instead describing the sensor node with: mbus-formats = <0x3010, 0x2013>; and the corresponding host interface with: mbus-formats = <0x3013, 0x2001>; This would allow to describe arbitrary transformations without having to limit to the 'trivial' ones. The result would be easier to understand also I think. Sascha
Hi Sascha On Fri, 26 Apr 2013, Sascha Hauer wrote: > Hi Guennadi, > > On Mon, Apr 22, 2013 at 02:39:57PM +0200, Guennadi Liakhovetski wrote: > > On Mon, 22 Apr 2013, Laurent Pinchart wrote: > > > > > Hi Guennadi, > > > > > > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote: > > > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote: > > > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows > > > > > the driver to use generic functions to manage sensor power supplies. > > > > > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > > > > > > A small addition to this one too: to be absolutely honest, I also had to > > > > replace 12-bit formats with their 8-bit counterparts, because only 8 data > > > > lanes are connected to my camera host. We'll need to somehow properly > > > > solve this too. > > > > > > That information should be conveyed by platform/DT data for the host, and be > > > used to convert the 12-bit media bus code into a 8-bit media bus code in the > > > host (a core helper function would probably be helpful). > > > > Yes, and we discussed this before too, I think. I proposed based then to > > implement some compatibility table of "trivial" transformations, like a > > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer > > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just > > needs to be implemented... > > These "trivial" transformations may turn out not to be so trivial. In > the devicetree we would then need kind of 'shift-4-bit-left' properties. We already have a "data-shift" property exactly for this purpose. > How about instead describing the sensor node with: > > mbus-formats = <0x3010, 0x2013>; > > and the corresponding host interface with: > > mbus-formats = <0x3013, 0x2001>; How would this describe _how_ the transformation should be performed? And why does the host driver need mbus formats? The translation is from mbus formats to fourcc formats (in memory). If you use those as bridge DT node properties, that would only tell the bridge driver which mbus format to request from the subdevice when requested fourcc format X. This decision soc-camera currently performs automatically, if this is ever needed as a configuration parameter, we'll think then where and how to put it. Thanks Guennadi > This would allow to describe arbitrary transformations without having to > limit to the 'trivial' ones. The result would be easier to understand > also I think. --- 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
On Fri, Apr 26, 2013 at 10:43:28AM +0200, Guennadi Liakhovetski wrote: > Hi Sascha > > On Fri, 26 Apr 2013, Sascha Hauer wrote: > > > > > > > > > That information should be conveyed by platform/DT data for the host, and be > > > > used to convert the 12-bit media bus code into a 8-bit media bus code in the > > > > host (a core helper function would probably be helpful). > > > > > > Yes, and we discussed this before too, I think. I proposed based then to > > > implement some compatibility table of "trivial" transformations, like a > > > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer > > > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just > > > needs to be implemented... > > > > These "trivial" transformations may turn out not to be so trivial. In > > the devicetree we would then need kind of 'shift-4-bit-left' properties. > > We already have a "data-shift" property exactly for this purpose. > > > How about instead describing the sensor node with: > > > > mbus-formats = <0x3010, 0x2013>; > > > > and the corresponding host interface with: > > > > mbus-formats = <0x3013, 0x2001>; > > How would this describe _how_ the transformation should be performed? nth index in the sensor array matches nth index in the csi array. The above describes: V4L2_MBUS_FMT_SGBRG12_1X12 on the sensor matches V4L2_MBUS_FMT_SGBRG8_1X8 on the host V4L2_MBUS_FMT_Y12_1X12 on the sensor matches V4L2_MBUS_FMT_Y8_1X8 on the host effectively implementing a shift by four bits. But also more complicated transformations could be described, like a colour space converter implemented in a DSP (not sure if anyone does this, but you get the idea) > And why does the host driver need mbus formats? Because mbus formats are effectively the input of a host driver. I assumed that we translate the mbus formats the sensor can output into the corresponding mbus formats that arrive at the host interface. Then afterwards the usual translation from mbus to fourcc a host interface can do is performed. I think what you aim at instead is a translation directly from the sensor to memory which I think is more complicated to build correctly. Sascha
Hi Sascha, On Friday 26 April 2013 11:15:56 Sascha Hauer wrote: > On Fri, Apr 26, 2013 at 10:43:28AM +0200, Guennadi Liakhovetski wrote: > > On Fri, 26 Apr 2013, Sascha Hauer wrote: > > > > > That information should be conveyed by platform/DT data for the > > > > > host, and be used to convert the 12-bit media bus code into a 8-bit > > > > > media bus code in the host (a core helper function would probably > > > > > be helpful). > > > > > > > > Yes, and we discussed this before too, I think. I proposed based then > > > > to implement some compatibility table of "trivial" transformations, > > > > like a 12-bit Bayer, right-shifted by 4 bits, produces a respective 8- > > > > bit Bayer etc. Such transformations would fit nicely in soc_mediabus.c > > > > ;-) This just needs to be implemented... > > > > > > These "trivial" transformations may turn out not to be so trivial. In > > > the devicetree we would then need kind of 'shift-4-bit-left' properties. > > > > We already have a "data-shift" property exactly for this purpose. > > > > > How about instead describing the sensor node with: > > > mbus-formats = <0x3010, 0x2013>; > > > > > > and the corresponding host interface with: > > > mbus-formats = <0x3013, 0x2001>; > > > > How would this describe _how_ the transformation should be performed? > > nth index in the sensor array matches nth index in the csi array. The > above describes: > > V4L2_MBUS_FMT_SGBRG12_1X12 on the sensor matches V4L2_MBUS_FMT_SGBRG8_1X8 on > the host V4L2_MBUS_FMT_Y12_1X12 on the sensor matches V4L2_MBUS_FMT_Y8_1X8 > on the host > > effectively implementing a shift by four bits. But also more complicated > transformations could be described, like a colour space converter > implemented in a DSP (not sure if anyone does this, but you get the > idea) If there's a component on the board between the sensor and the host it should be modeled as a proper subdev. I don't think trying to describe anything more complex than shifting the lanes in the device tree sensor and/or bridge data is a good idea. > > And why does the host driver need mbus formats? > > Because mbus formats are effectively the input of a host driver. I assumed > that we translate the mbus formats the sensor can output into the > corresponding mbus formats that arrive at the host interface. Then > afterwards the usual translation from mbus to fourcc a host interface can do > is performed. I think what you aim at instead is a translation directly from > the sensor to memory which I think is more complicated to build correctly. The sensor knows what mbus formats it supports at its output, and the host knows what mbus formats it supports at its inputs. Both information can be queried from user space and kernel space. With the data lanes shift property the host can then compute the mbus format it will receive at its input. I don't think we need to specify that in DT.
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c index 70f4525..ca2cc6e 100644 --- a/drivers/media/i2c/mt9p031.c +++ b/drivers/media/i2c/mt9p031.c @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client, goto done; mt9p031->subdev.dev = &client->dev; + mt9p031->subdev.pdata = &pdata->sd_pdata; ret = v4l2_async_register_subdev(&mt9p031->subdev); done: diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h index 0c97b19..7bf7b53 100644 --- a/include/media/mt9p031.h +++ b/include/media/mt9p031.h @@ -1,6 +1,8 @@ #ifndef MT9P031_H #define MT9P031_H +#include <media/v4l2-subdev.h> + struct v4l2_subdev; /* @@ -15,6 +17,7 @@ struct mt9p031_platform_data { int reset; int ext_freq; int target_freq; + struct v4l2_subdev_platform_data sd_pdata; }; #endif
Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows the driver to use generic functions to manage sensor power supplies. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/media/i2c/mt9p031.c | 1 + include/media/mt9p031.h | 3 +++ 2 files changed, 4 insertions(+), 0 deletions(-)