RE: [PATCH/RFC 9/9] mt9t031: make the use of the soc-camera client API optional
Guennadi,
Thanks for the reply.
+};
+
+static struct soc_camera_ops mt9t031_ops = {
+ .set_bus_param = mt9t031_set_bus_param,
+ .query_bus_param= mt9t031_query_bus_param,
+ .controls = mt9t031_controls,
+ .num_controls = ARRAY_SIZE(mt9t031_controls),
+};
+
[MK] Why don't you implement queryctrl ops in core? query_bus_param
set_bus_param() can be implemented as a sub device operation as well
right? I think we need to get the bus parameter RFC implemented and
this driver could be targeted for it's first use so that we could
work together to get it accepted. I didn't get a chance to study your
bus image format RFC, but plan to review it soon and to see if it can be
used in my platform as well. For use of this driver in our platform,
all reference to soc_ must be removed. I am ok if the structure is
re-used, but if this driver calls any soc_camera function, it canot
be used in my platform.
Why? Some soc-camera functions are just library functions, you just have
to build soc-camera into your kernel. (also see below)
My point is that the control is for the sensor device, so why to implement
queryctrl in SoC camera? Just for this I need to include SOC camera in my
build? That doesn't make any sense at all. IMHO, queryctrl() logically belongs
to this sensor driver which can be called from the bridge driver using sudev
API call. Any reverse dependency from MT9T031 to SoC camera to be removed if it
is to be re-used across other platforms. Can we agree on this? Did you have a
chance to compare the driver file that I had sent to you?
Thanks.
Murali
BTW, I am attaching a version of the driver that we use in our kernel
tree for your reference which will give you an idea of my requirement.
[snip]
@@ -565,7 +562,6 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd,
struct v4l2_control *ctrl)
{
struct i2c_client *client = sd-priv;
struct mt9t031 *mt9t031 = to_mt9t031(client);
- struct soc_camera_device *icd = client-dev.platform_data;
const struct v4l2_queryctrl *qctrl;
int data;
@@ -657,7 +653,8 @@ static int mt9t031_s_ctrl(struct v4l2_subdev *sd,
struct v4l2_control *ctrl)
if (set_shutter(client, total_h) 0)
return -EIO;
- qctrl = soc_camera_find_qctrl(icd-ops,
V4L2_CID_EXPOSURE);
+ qctrl = soc_camera_find_qctrl(mt9t031_ops,
+ V4L2_CID_EXPOSURE);
[MK] Why do we still need this call? In my version of the sensor driver,
I just implement the queryctrl() operation in core_ops. This cannot work
since soc_camera_find_qctrl() is implemented only in SoC camera.
As mentioned above, that's just a library function without any further
dependencies, so, why reimplement it?
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH/RFC 9/9] mt9t031: make the use of the soc-camera client API optional
Guennadi,
Thanks for your time for updating this driver. But I still don't think
it is in a state to be re-used on TI's VPFE platform. Please see
below for my comments.
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
email: m-kariche...@ti.com
-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Friday, October 30, 2009 10:02 AM
To: Linux Media Mailing List
Cc: Hans Verkuil; Laurent Pinchart; Sakari Ailus; Karicheri, Muralidharan
Subject: [PATCH/RFC 9/9] mt9t031: make the use of the soc-camera client API
optional
Now that we have moved most of the functions over to the v4l2-subdev API,
only
quering and setting bus parameters are still performed using the legacy
soc-camera client API. Make the use of this API optional for mt9t031.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
Muralidharan, this one is for you to test. To differentiate between the
soc-camera case and a generic user I check i2c client's platform data
(client-dev.platform_data), so, you have to make sure your user doesn't
use that field for something else.
Currently I am using this field for bus parameters such as pclk polarity.
If there is an API (bus parameter) to set this after probing the sensor, that
may work too. I will check your latest driver and let you know if
I see an issue in migrating to this version.
One more note: I'm not sure about where v4l2_device_unregister_subdev()
should be called. In soc-camera the core calls
v4l2_i2c_new_subdev_board(), which then calls
v4l2_device_register_subdev(). Logically, it's also the core that then
calls v4l2_device_unregister_subdev(). Whereas I see many other client
drivers call v4l2_device_unregister_subdev() internally. So, if your
bridge driver does not call v4l2_device_unregister_subdev() itself and
expects the client to call it, there will be a slight problem with that
too.
In my bridge driver also v4l2_i2c_new_subdev_board() is called to load
up the sub device. When the bridge driver is removed (remove() call), it calls
v4l2_device_unregister() which will unregister the v4l2 device and all
sub devices (in turn calls v4l2_device_unregister_subdev()). But most
of the sub devices also calls v4l2_device_unregister_subdev() in the
remove() function of the module (so also the version of the mt9t031
that I use). So even if that call is kept in the mt9t031 sensor driver (not
sure if someone use it as a standalone driver), it would just return since the
v4l2_dev ptr in sd ptr would have been set to null as a result of the bridge
driver remove() call. What do you think?
See also some comments below..
drivers/media/video/mt9t031.c | 146 -
1 files changed, 70 insertions(+), 76 deletions(-)
diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index c95c277..49357bd 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -204,6 +204,59 @@ static unsigned long mt9t031_query_bus_param(struct
soc_camera_device *icd)
return soc_camera_apply_sensor_flags(icl, MT9T031_BUS_PARAM);
}
+static const struct v4l2_queryctrl mt9t031_controls[] = {
+ {
+ .id = V4L2_CID_VFLIP,
+ .type = V4L2_CTRL_TYPE_BOOLEAN,
+ .name = Flip Vertically,
+ .minimum= 0,
+ .maximum= 1,
+ .step = 1,
+ .default_value = 0,
+ }, {
+ .id = V4L2_CID_HFLIP,
+ .type = V4L2_CTRL_TYPE_BOOLEAN,
+ .name = Flip Horizontally,
+ .minimum= 0,
+ .maximum= 1,
+ .step = 1,
+ .default_value = 0,
+ }, {
+ .id = V4L2_CID_GAIN,
+ .type = V4L2_CTRL_TYPE_INTEGER,
+ .name = Gain,
+ .minimum= 0,
+ .maximum= 127,
+ .step = 1,
+ .default_value = 64,
+ .flags = V4L2_CTRL_FLAG_SLIDER,
+ }, {
+ .id = V4L2_CID_EXPOSURE,
+ .type = V4L2_CTRL_TYPE_INTEGER,
+ .name = Exposure,
+ .minimum= 1,
+ .maximum= 255,
+ .step = 1,
+ .default_value = 255,
+ .flags = V4L2_CTRL_FLAG_SLIDER,
+ }, {
+ .id = V4L2_CID_EXPOSURE_AUTO,
+ .type = V4L2_CTRL_TYPE_BOOLEAN,
+ .name = Automatic Exposure,
+ .minimum= 0,
+ .maximum= 1,
+ .step = 1,
+ .default_value = 1,
+ }
+};
+
+static struct soc_camera_ops mt9t031_ops = {
+ .set_bus_param = mt9t031_set_bus_param,
+
RE: [PATCH/RFC 9/9] mt9t031: make the use of the soc-camera client API optional
On Fri, 30 Oct 2009, Karicheri, Muralidharan wrote:
Guennadi,
Thanks for your time for updating this driver. But I still don't think
it is in a state to be re-used on TI's VPFE platform. Please see
below for my comments.
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
email: m-kariche...@ti.com
-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Friday, October 30, 2009 10:02 AM
To: Linux Media Mailing List
Cc: Hans Verkuil; Laurent Pinchart; Sakari Ailus; Karicheri, Muralidharan
Subject: [PATCH/RFC 9/9] mt9t031: make the use of the soc-camera client API
optional
Now that we have moved most of the functions over to the v4l2-subdev API,
only
quering and setting bus parameters are still performed using the legacy
soc-camera client API. Make the use of this API optional for mt9t031.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
Muralidharan, this one is for you to test. To differentiate between the
soc-camera case and a generic user I check i2c client's platform data
(client-dev.platform_data), so, you have to make sure your user doesn't
use that field for something else.
Currently I am using this field for bus parameters such as pclk polarity.
If there is an API (bus parameter) to set this after probing the sensor,
that may work too. I will check your latest driver and let you know if
I see an issue in migrating to this version.
No, that shall come with the bus-configuration API. I was already thinking
about switching to passing a pointer to struct soc_camera_link or
something similar in platform_data, because that's exactly what that
struct is for. Of course, we would have to agree on a specific object with
platform parameters there also for non soc-camera drivers.
One more note: I'm not sure about where v4l2_device_unregister_subdev()
should be called. In soc-camera the core calls
v4l2_i2c_new_subdev_board(), which then calls
v4l2_device_register_subdev(). Logically, it's also the core that then
calls v4l2_device_unregister_subdev(). Whereas I see many other client
drivers call v4l2_device_unregister_subdev() internally. So, if your
bridge driver does not call v4l2_device_unregister_subdev() itself and
expects the client to call it, there will be a slight problem with that
too.
In my bridge driver also v4l2_i2c_new_subdev_board() is called to load
up the sub device. When the bridge driver is removed (remove() call), it
calls v4l2_device_unregister() which will unregister the v4l2 device and
all sub devices (in turn calls v4l2_device_unregister_subdev()). But
most of the sub devices also calls v4l2_device_unregister_subdev() in
the remove() function of the module (so also the version of the mt9t031
that I use). So even if that call is kept in the mt9t031 sensor driver
(not sure if someone use it as a standalone driver), it would just
return since the v4l2_dev ptr in sd ptr would have been set to null as a
result of the bridge driver remove() call. What do you think?
...as long as sd has not been freed yet by then. But in case of mt9t031
the subdevice is embedded in driver-instance object, which is freed, when
the respective i2c device gets unregistered or the driver unloaded. So,
you could call it twice here, yes.
See also some comments below..
drivers/media/video/mt9t031.c | 146 -
1 files changed, 70 insertions(+), 76 deletions(-)
diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
index c95c277..49357bd 100644
--- a/drivers/media/video/mt9t031.c
+++ b/drivers/media/video/mt9t031.c
@@ -204,6 +204,59 @@ static unsigned long mt9t031_query_bus_param(struct
soc_camera_device *icd)
return soc_camera_apply_sensor_flags(icl, MT9T031_BUS_PARAM);
}
+static const struct v4l2_queryctrl mt9t031_controls[] = {
+{
+.id = V4L2_CID_VFLIP,
+.type = V4L2_CTRL_TYPE_BOOLEAN,
+.name = Flip Vertically,
+.minimum= 0,
+.maximum= 1,
+.step = 1,
+.default_value = 0,
+}, {
+.id = V4L2_CID_HFLIP,
+.type = V4L2_CTRL_TYPE_BOOLEAN,
+.name = Flip Horizontally,
+.minimum= 0,
+.maximum= 1,
+.step = 1,
+.default_value = 0,
+}, {
+.id = V4L2_CID_GAIN,
+.type = V4L2_CTRL_TYPE_INTEGER,
+.name = Gain,
+.minimum= 0,
+.maximum= 127,
+.step = 1,
+.default_value = 64,
+.flags = V4L2_CTRL_FLAG_SLIDER,
+}, {
+.id = V4L2_CID_EXPOSURE,
+.type =
