camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times
Commit Message
Dobry den! :-)
> > > > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > > > have focus (etc) working, but that's a start. I also had to remove
> > > > video-bus-switch support; but I guess it will be easier to use
> > > > video-multiplexer patches...
> > > >
> > > > I'll have patches over weekend.
> > >
> > > I briefly looked at what's there --- you do miss the video nodes for the
> > > non-sensor sub-devices, and they also don't show up in the media graph,
> > > right?
> >
> > Yes.
> >
> > > I guess they don't end up matching in the async list.
> >
> > How should they get to the async list?
>
> The patch you referred to does that. The problem is, it does make the bus
> configuration a pointer as well. There should be two patches. That's not a
> lot of work to separate them though. But it should be done.
>
> >
> > > I think we need to make the non-sensor sub-device support more generic;
> > > it's not just the OMAP 3 ISP that needs it. I think we need to document
> > > the property for the flash phandle as well; I can write one, or refresh
> > > an existing one that I believe already exists.
> > >
> > > How about calling it either simply "flash" or "camera-flash"? Similarly
> > > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
> > > prefix is somewhat redundant, so I'd just go for "flash" or "lens".
> >
> > Actually, I'd go for "flash" and "focus-coil". There may be other
> > lens properties, such as zoom, mirror movement, lens identification,
> > ...
>
> Good point. Still there may be other ways to move the lens than the voice
> coil (which sure is cheap), so how about "flash" and "lens-focus"?
Ok, so something like this? (Yes, needs binding documentation and you
wanted it in the core.. can fix.)
BTW, fwnode_handle_put() seems to be missing in the success path of
isp_fwnodes_parse() -- can you check that?
Best regards,
Pavel
Comments
On Sat, Mar 04, 2017 at 09:55:51AM +0100, Pavel Machek wrote:
> Dobry den! :-)
Huomenta! :-)
>
> > > > > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > > > > have focus (etc) working, but that's a start. I also had to remove
> > > > > video-bus-switch support; but I guess it will be easier to use
> > > > > video-multiplexer patches...
> > > > >
> > > > > I'll have patches over weekend.
> > > >
> > > > I briefly looked at what's there --- you do miss the video nodes for the
> > > > non-sensor sub-devices, and they also don't show up in the media graph,
> > > > right?
> > >
> > > Yes.
> > >
> > > > I guess they don't end up matching in the async list.
> > >
> > > How should they get to the async list?
> >
> > The patch you referred to does that. The problem is, it does make the bus
> > configuration a pointer as well. There should be two patches. That's not a
> > lot of work to separate them though. But it should be done.
> >
> > >
> > > > I think we need to make the non-sensor sub-device support more generic;
> > > > it's not just the OMAP 3 ISP that needs it. I think we need to document
> > > > the property for the flash phandle as well; I can write one, or refresh
> > > > an existing one that I believe already exists.
> > > >
> > > > How about calling it either simply "flash" or "camera-flash"? Similarly
> > > > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
> > > > prefix is somewhat redundant, so I'd just go for "flash" or "lens".
> > >
> > > Actually, I'd go for "flash" and "focus-coil". There may be other
> > > lens properties, such as zoom, mirror movement, lens identification,
> > > ...
> >
> > Good point. Still there may be other ways to move the lens than the voice
> > coil (which sure is cheap), so how about "flash" and "lens-focus"?
>
> Ok, so something like this? (Yes, needs binding documentation and you
> wanted it in the core.. can fix.)
>
> BTW, fwnode_handle_put() seems to be missing in the success path of
> isp_fwnodes_parse() -- can you check that?
Where exactly? I noticed that if notifier->num_subdevs hits the limit the
last node isn't put properly. I'll fix that. Is that what you meant?
>
> Best regards,
> Pavel
>
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index c80397a..6f6fbed 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2114,7 +2114,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> buscfg->bus.ccp2.lanecfg.data[0].pol =
> vfwn.bus.mipi_csi1.lane_polarity[1];
>
> - dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> + dev_dbg(dev, "data lane %u polarity %u, pos %u\n", 0,
Why?
> buscfg->bus.ccp2.lanecfg.data[0].pol,
> buscfg->bus.ccp2.lanecfg.data[0].pos);
>
> @@ -2162,10 +2162,64 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> return 0;
> }
>
> +static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> + const char *key)
> +{
> + struct device_node *node;
> + struct isp_async_subdev *isd;
> +
> + printk("Looking for %s\n", key);
> +
> + node = of_parse_phandle(dev->of_node, key, 0);
There may be more than one flash associated with a sensor. Speaking of which
--- how is it associated to the sensors?
One way to do this could be to simply move the flash property to the sensor
OF node. We could have it here, too, if the flash was not associated with
any sensor, but I doubt that will ever be needed.
This really calls fork moving this part to the framework away from drivers.
> + if (!node)
> + return 0;
> +
> + printk("Having subdevice: %p\n", node);
> +
> + isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> + if (!isd)
> + return -ENOMEM;
> +
> + notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> +
> + isd->asd.match.of.node = node;
> + if (!isd->asd.match.of.node) {
You should check node here first.
> + dev_warn(dev, "bad remote port parent\n");
> + return -EIO;
> + }
> +
And then assign it here.
isd->asd.match.fwnode.fwn = of_fwnode_handle(node);
> + isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
V4L2_ASYNC_MATCH_FWNODE, please.
> + notifier->num_subdevs++;
> +
> + return 0;
> +}
> +
> +static int camera_subdevs_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> + int max)
> +{
> + int res = 0;
No need to assign res here.
> +
> + printk("Going through camera-flashes\n");
> + if (notifier->num_subdevs < max) {
> + res = camera_subdev_parse(dev, notifier, "flash");
> + if (res)
> + return res;
> + }
> +
> + if (notifier->num_subdevs < max) {
> + res = camera_subdev_parse(dev, notifier, "lens-focus");
> + if (res)
> + return res;
> + }
> +
> + return 0;
> +}
> +
> static int isp_fwnodes_parse(struct device *dev,
> struct v4l2_async_notifier *notifier)
> {
> struct fwnode_handle *fwn = NULL;
> + int res = 0;
>
> notifier->subdevs = devm_kcalloc(
> dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
> @@ -2199,6 +2253,15 @@ static int isp_fwnodes_parse(struct device *dev,
> notifier->num_subdevs++;
> }
>
> + /* FIXME: missing put in the success path? */
> +
> + res = camera_subdevs_parse(dev, notifier, ISP_MAX_SUBDEVS);
> + if (res)
> + goto error;
> +
> + if (notifier->num_subdevs == ISP_MAX_SUBDEVS) {
> + printk("isp: Maybe too many devices?\n");
> + }
> return notifier->num_subdevs;
>
> error:
>
>
On Sat 2017-03-04 14:30:11, Sakari Ailus wrote:
> On Sat, Mar 04, 2017 at 09:55:51AM +0100, Pavel Machek wrote:
> > Dobry den! :-)
>
> Huomenta! :-)
Dobry vecer! :-).
> > > Good point. Still there may be other ways to move the lens than the voice
> > > coil (which sure is cheap), so how about "flash" and "lens-focus"?
> >
> > Ok, so something like this? (Yes, needs binding documentation and you
> > wanted it in the core.. can fix.)
> >
> > BTW, fwnode_handle_put() seems to be missing in the success path of
> > isp_fwnodes_parse() -- can you check that?
>
> Where exactly? I noticed that if notifier->num_subdevs hits the limit the
> last node isn't put properly. I'll fix that. Is that what you meant?
I guess I'm confused. I see no put on the success path. Maybe it is
put somewhere out of the function where I did not look... is the
reference held while the driver is running
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2114,7 +2114,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> > buscfg->bus.ccp2.lanecfg.data[0].pol =
> > vfwn.bus.mipi_csi1.lane_polarity[1];
> >
> > - dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> > + dev_dbg(dev, "data lane %u polarity %u, pos %u\n", 0,
>
> Why?
I was printing uninitialized / unused variable, which is a no-no (and
gcc complains). I guess I should do a separate patch.
> > buscfg->bus.ccp2.lanecfg.data[0].pol,
> > buscfg->bus.ccp2.lanecfg.data[0].pos);
> >
> > @@ -2162,10 +2162,64 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> > return 0;
> > }
> >
> > +static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> > + const char *key)
> > +{
> > + struct device_node *node;
> > + struct isp_async_subdev *isd;
> > +
> > + printk("Looking for %s\n", key);
> > +
> > + node = of_parse_phandle(dev->of_node, key, 0);
>
> There may be more than one flash associated with a sensor. Speaking of which
> --- how is it associated to the sensors?
Ok, more then one flash I can understand (will fix).
> One way to do this could be to simply move the flash property to the sensor
> OF node. We could have it here, too, if the flash was not associated with
> any sensor, but I doubt that will ever be needed.
>
> This really calls fork moving this part to the framework away from
> drivers.
The rest I don't get :-(. The flash is likely connected over i2c, so
it should not become child node of omap3isp.
And yes, I agree we want to move it into the framework. Lets agree on
how it should work and where to put it, I'll debug it here then move it...
> > + if (!node)
> > + return 0;
> > +
> > + printk("Having subdevice: %p\n", node);
> > +
> > + isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> > + if (!isd)
> > + return -ENOMEM;
> > +
> > + notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> > +
> > + isd->asd.match.of.node = node;
> > + if (!isd->asd.match.of.node) {
>
> You should check node here first.
Umm. I did, above. This can't happen, AFAICT.
> > + dev_warn(dev, "bad remote port parent\n");
> > + return -EIO;
> > + }
> > +
>
> And then assign it here.
>
> isd->asd.match.fwnode.fwn = of_fwnode_handle(node);
>
> > + isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
>
> V4L2_ASYNC_MATCH_FWNODE, please.
Ok. Lets see if it still works after the changes :-)... it does, good.
> > +static int camera_subdevs_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> > + int max)
> > +{
> > + int res = 0;
>
> No need to assign res here.
Ok.
Thanks and best regards,
Pavel
@@ -2114,7 +2114,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
buscfg->bus.ccp2.lanecfg.data[0].pol =
vfwn.bus.mipi_csi1.lane_polarity[1];
- dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+ dev_dbg(dev, "data lane %u polarity %u, pos %u\n", 0,
buscfg->bus.ccp2.lanecfg.data[0].pol,
buscfg->bus.ccp2.lanecfg.data[0].pos);
@@ -2162,10 +2162,64 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
return 0;
}
+static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
+ const char *key)
+{
+ struct device_node *node;
+ struct isp_async_subdev *isd;
+
+ printk("Looking for %s\n", key);
+
+ node = of_parse_phandle(dev->of_node, key, 0);
+ if (!node)
+ return 0;
+
+ printk("Having subdevice: %p\n", node);
+
+ isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
+ if (!isd)
+ return -ENOMEM;
+
+ notifier->subdevs[notifier->num_subdevs] = &isd->asd;
+
+ isd->asd.match.of.node = node;
+ if (!isd->asd.match.of.node) {
+ dev_warn(dev, "bad remote port parent\n");
+ return -EIO;
+ }
+
+ isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
+ notifier->num_subdevs++;
+
+ return 0;
+}
+
+static int camera_subdevs_parse(struct device *dev, struct v4l2_async_notifier *notifier,
+ int max)
+{
+ int res = 0;
+
+ printk("Going through camera-flashes\n");
+ if (notifier->num_subdevs < max) {
+ res = camera_subdev_parse(dev, notifier, "flash");
+ if (res)
+ return res;
+ }
+
+ if (notifier->num_subdevs < max) {
+ res = camera_subdev_parse(dev, notifier, "lens-focus");
+ if (res)
+ return res;
+ }
+
+ return 0;
+}
+
static int isp_fwnodes_parse(struct device *dev,
struct v4l2_async_notifier *notifier)
{
struct fwnode_handle *fwn = NULL;
+ int res = 0;
notifier->subdevs = devm_kcalloc(
dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
@@ -2199,6 +2253,15 @@ static int isp_fwnodes_parse(struct device *dev,
notifier->num_subdevs++;
}
+ /* FIXME: missing put in the success path? */
+
+ res = camera_subdevs_parse(dev, notifier, ISP_MAX_SUBDEVS);
+ if (res)
+ goto error;
+
+ if (notifier->num_subdevs == ISP_MAX_SUBDEVS) {
+ printk("isp: Maybe too many devices?\n");
+ }
return notifier->num_subdevs;
error: