[1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

  From: Sebastian Reichel <sre@kernel.org>

If v4l2_device_register_subdev_nodes() is called multiple times, it is
better to return early than corrupt memory.

Without this, exposure / gain controls do not work in the camera
application on N900.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/v4l2-core/v4l2-device.c | 3 +++
 1 file changed, 3 insertions(+)
  

Hi!

On Tue 2017-02-14 23:38:49, Pavel Machek wrote:
> From: Sebastian Reichel <sre@kernel.org>
> 
> If v4l2_device_register_subdev_nodes() is called multiple times, it is
> better to return early than corrupt memory.
> 
> Without this, exposure / gain controls do not work in the camera
> application on N900.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

Can I get some updates/feedback here?

You liked this one and whole series should be ready...

Best regards,
									Pavel
  

Hi Pavel,

On Mon, Feb 20, 2017 at 11:31:14AM +0100, Pavel Machek wrote:
> Hi!
> 
> On Tue 2017-02-14 23:38:49, Pavel Machek wrote:
> > From: Sebastian Reichel <sre@kernel.org>
> > 
> > If v4l2_device_register_subdev_nodes() is called multiple times, it is
> > better to return early than corrupt memory.
> > 
> > Without this, exposure / gain controls do not work in the camera
> > application on N900.
> > 
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> Can I get some updates/feedback here?
> 
> You liked this one and whole series should be ready...

:-)

I was just rebasing the CCP2 support on the fwnode patchset.

I'm just pushing the result here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>

I've tested ACPI, will test DT soon...
  

On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> I've tested ACPI, will test DT soon...

DT case works, too (Nokia N9).
  

On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > I've tested ACPI, will test DT soon...
> 
> DT case works, too (Nokia N9).

Hmm. Good to know. Now to figure out how to get N900 case to work...

AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
seem to be in, so I'll need to figure out which, and will still need
omap3isp modifications...

Best regards,
									Pavel
  

On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > I've tested ACPI, will test DT soon...
> > 
> > DT case works, too (Nokia N9).
> 
> Hmm. Good to know. Now to figure out how to get N900 case to work...
> 
> AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> seem to be in, so I'll need to figure out which, and will still need
> omap3isp modifications...

Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.

It's essentially the functionality in the four patches. The data-lane and
clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
plural) to match the property documentation.
  

Hi!

On Tue 2017-02-21 13:11:04, Sakari Ailus wrote:
> On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > I've tested ACPI, will test DT soon...
> > > 
> > > DT case works, too (Nokia N9).
> > 
> > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > 
> > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > seem to be in, so I'll need to figure out which, and will still need
> > omap3isp modifications...
> 
> Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> 
> It's essentially the functionality in the four patches. The data-lane and
> clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> plural) to match the property documentation.

Ok, thanks, I got CSI-1 support to compile.

I'm now fighting with subdevices support. Camera flash and autofocus
coil really should be subdevices of the ISP, right?

Do you have any solution for that? [I need it for my userspace to
work, and porting the old one looks like lot of fun (tm) :-(].

Best regards,
									Pavel
  

On Tue 2017-02-21 13:11:04, Sakari Ailus wrote:
> On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > I've tested ACPI, will test DT soon...
> > > 
> > > DT case works, too (Nokia N9).
> > 
> > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > 
> > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > seem to be in, so I'll need to figure out which, and will still need
> > omap3isp modifications...
> 
> Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> 
> It's essentially the functionality in the four patches. The data-lane and
> clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> plural) to match the property documentation.

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.

Best regards,
									Pavel
  

On Sat, Feb 25, 2017 at 01:09:18AM +0100, Pavel Machek wrote:
> On Tue 2017-02-21 13:11:04, Sakari Ailus wrote:
> > On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > > I've tested ACPI, will test DT soon...
> > > > 
> > > > DT case works, too (Nokia N9).
> > > 
> > > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > > 
> > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > > seem to be in, so I'll need to figure out which, and will still need
> > > omap3isp modifications...
> > 
> > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> > 
> > It's essentially the functionality in the four patches. The data-lane and
> > clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> > plural) to match the property documentation.
> 
> 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?

I guess they don't end up matching in the async list.

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

At the very least the property names must be generic (not hardware
dependent) as this kind of functionality should be present in the
framework rather than in individual drivers. That'll be for later though.

Making the sub-device bus configuration a pointer should be in a separate
patch. It makes sense since the entire configuration is not valid for all
sub-devices attached to the ISP anymore. I think it originally was a
separate patch, but they probably have been merged at some point. I can't
find it right now anyway.
  

Hi!

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

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

> At the very least the property names must be generic (not hardware
> dependent) as this kind of functionality should be present in the
> framework rather than in individual drivers. That'll be for later
> though.

Agreed, that would be nice.

> Making the sub-device bus configuration a pointer should be in a separate
> patch. It makes sense since the entire configuration is not valid for all
> sub-devices attached to the ISP anymore. I think it originally was a
> separate patch, but they probably have been merged at some point. I can't
> find it right now anyway.

I believe I can find the patch. But I'm not sure if I can port it to
the fwnode infrastructure anytime soon...

									Pavel
  

Hi!

> > Making the sub-device bus configuration a pointer should be in a separate
> > patch. It makes sense since the entire configuration is not valid for all
> > sub-devices attached to the ISP anymore. I think it originally was a
> > separate patch, but they probably have been merged at some point. I can't
> > find it right now anyway.
> 
> I believe I can find the patch. But I'm not sure if I can port it to
> the fwnode infrastructure anytime soon...

Here is the (unported) patch.

https://git.kernel.org/cgit/linux/kernel/git/pavel/linux-n900.git/commit/?h=camera-sa-5&id=e705712683b99fec282f87ed3e80bb58c95cf726

Maybe this helps,
									Pavel
  

Moi! :-)

On Sat, Feb 25, 2017 at 10:53:22PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > 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"?
  

Ahoj! :-)

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

Well... This is the line I'm fighting with:

+ of_parse_phandle(dev->of_node, "ti,camera-flashes",
+							flash++)

If someone told me its fwnode equivalent, I might be able to get it to
work. Knowing what group_id is and if I could ignore it would help a
bit, too :-).

(Also, I'll be glad to test patches :-))

> > > 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"?

Sounds good to me.
									Pavel
  

Hyvää iltaa!

On Sun, Feb 26, 2017 at 09:38:51AM +0100, Pavel Machek wrote:
> Ahoj! :-)
> 
> > > > > 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.
> 
> Well... This is the line I'm fighting with:
> 
> + of_parse_phandle(dev->of_node, "ti,camera-flashes",
> +							flash++)
> 
> If someone told me its fwnode equivalent, I might be able to get it to
> work. Knowing what group_id is and if I could ignore it would help a
> bit, too :-).

Right.

ACPI does not have equivalents for OF phandles. That's the background of the
problem. The port and endpoint references are a bit special: there'a a
device reference and indices of the port and the endpoint nodes.

I think you can just use the OF API for the time being until we define how
to describe flash devices with ACPI. The difference with ACPI will be
visible there almost no matter what do you do there, which is one more
reason to have that functionality in the framework (and not drivers).