Camera sensors patches

GIT PULL Laurent Pinchart 4 participants 14 messages Apr 12, 2013 – Apr 22, 2013 Media (V4L/DVB)

lore

Share & tools

lore

Laurent Pinchart

author

2013-04-12 09:13 UTC #0 lore raw

Hi Mauro,

The following changes since commit 81e096c8ac6a064854c2157e0bf802dc4906678c:

[media] budget: Add support for Philips Semi Sylt PCI ref. design
(2013-04-08 07:28:01 -0300)

are available in the git repository at:

git://linuxtv.org/pinchartl/media.git sensors/next

for you to fetch changes up to c890926a06339944790c5c265e21e8547aa55e49:

mt9p031: Use the common clock framework (2013-04-12 11:07:07 +0200)

----------------------------------------------------------------
Laurent Pinchart (5):
mt9m032: Fix PLL setup
mt9m032: Define MT9M032_READ_MODE1 bits
mt9p031: Use devm_* managed helpers
mt9p031: Add support for regulators
mt9p031: Use the common clock framework

 drivers/media/i2c/mt9m032.c | 46 +++++++++++++++++++++++++++++++-----------
 drivers/media/i2c/mt9p031.c | 58 ++++++++++++++++++++++++++++++--------------
 include/media/mt9p031.h     |  2 --
 3 files changed, 75 insertions(+), 31 deletions(-)

Signature · 3 lines

Regards,

Laurent Pinchart

Mauro Carvalho Chehab

2013-04-14 20:23 UTC #1 lore raw

Em Fri, 12 Apr 2013 11:13:06 +0200
Laurent Pinchart <[email protected]> escreveu:

Quoted 15 lines

Hi Mauro,
The following changes since commit 81e096c8ac6a064854c2157e0bf802dc4906678c:
[media] budget: Add support for Philips Semi Sylt PCI ref. design
(2013-04-08 07:28:01 -0300)
are available in the git repository at:
git://linuxtv.org/pinchartl/media.git sensors/next
for you to fetch changes up to c890926a06339944790c5c265e21e8547aa55e49:
mt9p031: Use the common clock framework (2013-04-12 11:07:07 +0200)

Quoted 8 lines

----------------------------------------------------------------
Laurent Pinchart (5):
mt9m032: Fix PLL setup
mt9m032: Define MT9M032_READ_MODE1 bits
mt9p031: Use devm_* managed helpers
mt9p031: Add support for regulators
mt9p031: Use the common clock framework

Hmm... It seems ugly to have regulators and clock framework and other SoC calls inside an i2c driver that can be used by a device that doesn't have regulators.

I'm not sure what's the best solution for it, so, I'll be adding those two patches, but it seems that we'll need to restrict the usage of those calls only if the caller driver is a platform driver.

Regards,
Mauro

Laurent Pinchart

author

2013-04-15 10:19 UTC #2 lore raw

Hi Mauro,

Quoted 4 lines · On Sunday 14 April 2013 16:59:58 Mauro Carvalho Chehab wrote:

Em Fri, 12 Apr 2013 11:13:06 +0200 Laurent Pinchart escreveu:
Quoted 3 lines
Hi Mauro,
The following changes since commit

81e096c8ac6a064854c2157e0bf802dc4906678c:

Quoted 19 lines

Quoted 10 lines
[media] budget: Add support for Philips Semi Sylt PCI ref. design
(2013-04-08 07:28:01 -0300)
are available in the git repository at:
git://linuxtv.org/pinchartl/media.git sensors/next
for you to fetch changes up to c890926a06339944790c5c265e21e8547aa55e49:
mt9p031: Use the common clock framework (2013-04-12 11:07:07 +0200)
Quoted 8 lines
----------------------------------------------------------------
Laurent Pinchart (5):
mt9m032: Fix PLL setup
mt9m032: Define MT9M032_READ_MODE1 bits
mt9p031: Use devm_* managed helpers
mt9p031: Add support for regulators
mt9p031: Use the common clock framework

Quoted 7 lines

Hmm... It seems ugly to have regulators and clock framework and other SoC calls inside an i2c driver that can be used by a device that doesn't have regulators.
I'm not sure what's the best solution for it, so, I'll be adding those two patches, but it seems that we'll need to restrict the usage of those calls only if the caller driver is a platform driver.

The MT9P031 needs power supplies and a clock on all platforms, regardless of the bridge bus type. I suppose the use case that mostly concerns you here is USB webcams where the power supplies and the clock are controlled automatically by the device. If we ever need to support such a device in the future we can of course revisit the driver then, and one possible solution would be to register fixed voltage regulators and a fixed clock.

Signature · 3 lines

Regards,

Laurent Pinchart

Mauro Carvalho Chehab

2013-04-15 12:42 UTC #3 lore raw

Em Mon, 15 Apr 2013 12:19:23 +0200
Laurent Pinchart <[email protected]> escreveu:

Quoted 31 lines

Hi Mauro,
Quoted 4 lines · On Sunday 14 April 2013 16:59:58 Mauro Carvalho Chehab wrote:
Em Fri, 12 Apr 2013 11:13:06 +0200 Laurent Pinchart escreveu:
Quoted 3 lines
Hi Mauro,
The following changes since commit
81e096c8ac6a064854c2157e0bf802dc4906678c:
Quoted 23 lines
Quoted 10 lines
[media] budget: Add support for Philips Semi Sylt PCI ref. design
(2013-04-08 07:28:01 -0300)
are available in the git repository at:
git://linuxtv.org/pinchartl/media.git sensors/next
for you to fetch changes up to c890926a06339944790c5c265e21e8547aa55e49:
mt9p031: Use the common clock framework (2013-04-12 11:07:07 +0200)
Quoted 8 lines
----------------------------------------------------------------
Laurent Pinchart (5):
mt9m032: Fix PLL setup
mt9m032: Define MT9M032_READ_MODE1 bits
mt9p031: Use devm_* managed helpers
mt9p031: Add support for regulators
mt9p031: Use the common clock framework
Hmm... It seems ugly to have regulators and clock framework and other SoC calls inside an i2c driver that can be used by a device that doesn't have regulators.

Quoted 6 lines

Quoted 3 lines
I'm not sure what's the best solution for it, so, I'll be adding those two patches, but it seems that we'll need to restrict the usage of those calls only if the caller driver is a platform driver.
The MT9P031 needs power supplies and a clock on all platforms, regardless of the bridge bus type.

Well, all digital devices require clock and power. If power is either a simple electric circuit, a battery or a regulator, that depends on the board.

Quoted 2 lines

I suppose the use case that mostly concerns you here is USB webcams

Yes.

Quoted 2 lines

where the power supplies and the clock are controlled automatically by the device.

Or could be not controlled at all. It could be a simple XTAL attached to the sensor or a clock signal provided by the bridge obtained from a fixed XTAL, and a resistor bridge or a Zenner diode providing the needed power voltage.

Quoted 3 lines

If we ever need to support such a device in the
future we can of course revisit the driver then, and one possible solution would be to register fixed voltage regulators and a fixed clock.

That is an overkill: devices were the power supply/xtal clock can't be controlled should not require extra software that pretend to control it.

Regards,
Mauro

Laurent Pinchart

author

2013-04-16 15:30 UTC #4 lore raw

Hi Mauro,

Quoted 16 lines · On Monday 15 April 2013 09:42:48 Mauro Carvalho Chehab wrote:

Em Mon, 15 Apr 2013 12:19:23 +0200 Laurent Pinchart escreveu:
Quoted 15 lines · On Sunday 14 April 2013 16:59:58 Mauro Carvalho Chehab wrote:
Quoted 4 lines
Em Fri, 12 Apr 2013 11:13:06 +0200 Laurent Pinchart escreveu:
Quoted 3 lines
Hi Mauro,
The following changes since commit
81e096c8ac6a064854c2157e0bf802dc4906678c:
Quoted 8 lines
Quoted 8 lines
[media] budget: Add support for Philips Semi Sylt PCI ref. design
(2013-04-08 07:28:01 -0300)
are available in the git repository at:
git://linuxtv.org/pinchartl/media.git sensors/next
for you to fetch changes up to

c890926a06339944790c5c265e21e8547aa55e49:

Quoted 38 lines

Quoted 15 lines
Quoted 15 lines
Quoted 10 lines
mt9p031: Use the common clock framework (2013-04-12 11:07:07 +0200)
----------------------------------------------------------------
Laurent Pinchart (5):
mt9m032: Fix PLL setup
mt9m032: Define MT9M032_READ_MODE1 bits
mt9p031: Use devm_* managed helpers
mt9p031: Add support for regulators
mt9p031: Use the common clock framework
Hmm... It seems ugly to have regulators and clock framework and other SoC calls inside an i2c driver that can be used by a device that doesn't have regulators.
Quoted 6 lines
Quoted 3 lines
I'm not sure what's the best solution for it, so, I'll be adding those two patches, but it seems that we'll need to restrict the usage of those calls only if the caller driver is a platform driver.
The MT9P031 needs power supplies and a clock on all platforms, regardless of the bridge bus type.
Well, all digital devices require clock and power. If power is either a simple electric circuit, a battery or a regulator, that depends on the board.
Quoted 2 lines
I suppose the use case that mostly concerns you here is USB webcams
Yes.
Quoted 2 lines
where the power supplies and the clock are controlled automatically by the device.
Or could be not controlled at all. It could be a simple XTAL attached to the sensor or a clock signal provided by the bridge obtained from a fixed XTAL, and a resistor bridge or a Zenner diode providing the needed power voltage.

Quoted 6 lines

Quoted 3 lines
If we ever need to support such a device in the future we can of course revisit the driver then, and one possible solution would be to register fixed voltage regulators and a fixed clock.
That is an overkill: devices were the power supply/xtal clock can't be controlled should not require extra software that pretend to control it.

If I'm not mistaken that's however the recommended way on embedded devices at the moment. I don't have a strong opinion on the subject for now, but this will need to be at least discussed with core clock and regulator developers.

Signature · 3 lines

Regards,

Laurent Pinchart

Mauro Carvalho Chehab

2013-04-16 17:36 UTC #5 lore raw

Em 16-04-2013 12:30, Laurent Pinchart escreveu:

Quoted 62 lines

Hi Mauro,
Quoted 16 lines · On Monday 15 April 2013 09:42:48 Mauro Carvalho Chehab wrote:
Em Mon, 15 Apr 2013 12:19:23 +0200 Laurent Pinchart escreveu:
Quoted 15 lines · On Sunday 14 April 2013 16:59:58 Mauro Carvalho Chehab wrote:
Quoted 4 lines
Em Fri, 12 Apr 2013 11:13:06 +0200 Laurent Pinchart escreveu:
Quoted 3 lines
Hi Mauro,
The following changes since commit
81e096c8ac6a064854c2157e0bf802dc4906678c:
Quoted 8 lines
Quoted 8 lines
[media] budget: Add support for Philips Semi Sylt PCI ref. design
(2013-04-08 07:28:01 -0300)
are available in the git repository at:
git://linuxtv.org/pinchartl/media.git sensors/next
for you to fetch changes up to
c890926a06339944790c5c265e21e8547aa55e49:
Quoted 42 lines
Quoted 15 lines
Quoted 15 lines
Quoted 10 lines
   mt9p031: Use the common clock framework (2013-04-12 11:07:07 +0200)
----------------------------------------------------------------
Laurent Pinchart (5):
mt9m032: Fix PLL setup
mt9m032: Define MT9M032_READ_MODE1 bits
mt9p031: Use devm_* managed helpers
mt9p031: Add support for regulators
mt9p031: Use the common clock framework
Hmm... It seems ugly to have regulators and clock framework and other SoC calls inside an i2c driver that can be used by a device that doesn't have regulators.
Quoted 6 lines
Quoted 3 lines
I'm not sure what's the best solution for it, so, I'll be adding those two patches, but it seems that we'll need to restrict the usage of those calls only if the caller driver is a platform driver.
The MT9P031 needs power supplies and a clock on all platforms, regardless of the bridge bus type.
Well, all digital devices require clock and power. If power is either a simple electric circuit, a battery or a regulator, that depends on the board.
Quoted 2 lines
I suppose the use case that mostly concerns you here is USB webcams
Yes.
Quoted 2 lines
where the power supplies and the clock are controlled automatically by the device.
Or could be not controlled at all. It could be a simple XTAL attached to the sensor or a clock signal provided by the bridge obtained from a fixed XTAL, and a resistor bridge or a Zenner diode providing the needed power voltage.
Quoted 3 lines
If we ever need to support such a device in the future we can of course revisit the driver then, and one possible solution would be to register fixed voltage regulators and a fixed clock.

Quoted 6 lines

Quoted 2 lines
That is an overkill: devices were the power supply/xtal clock can't be controlled should not require extra software that pretend to control it.
If I'm not mistaken that's however the recommended way on embedded devices at the moment. I don't have a strong opinion on the subject for now, but this will need to be at least discussed with core clock and regulator developers.

Well, a customer's webcam is not an embedded device at all. That's why I think that putting it at the I2C driver is wrong: those drivers are not to be used only by embedded hardware.

Regards,
Mauro

Quoted 1 line

Sylwester Nawrocki

2013-04-16 18:04 UTC #6 lore raw

Cc Mark, Mike

Quoted 58 lines · Mauro Carvalho Chehab

Em 16-04-2013 12:30, Laurent Pinchart escreveu:
Quoted 57 lines
Hi Mauro,
Quoted 16 lines · On Monday 15 April 2013 09:42:48 Mauro Carvalho Chehab wrote:
Em Mon, 15 Apr 2013 12:19:23 +0200 Laurent Pinchart escreveu:
Quoted 15 lines · On Sunday 14 April 2013 16:59:58 Mauro Carvalho Chehab wrote:
Quoted 4 lines
Em Fri, 12 Apr 2013 11:13:06 +0200 Laurent Pinchart escreveu:
Quoted 3 lines
Hi Mauro,
The following changes since commit
81e096c8ac6a064854c2157e0bf802dc4906678c:
Quoted 8 lines
Quoted 8 lines
[media] budget: Add support for Philips Semi Sylt PCI ref. design
(2013-04-08 07:28:01 -0300)
are available in the git repository at:
git://linuxtv.org/pinchartl/media.git sensors/next
for you to fetch changes up to
c890926a06339944790c5c265e21e8547aa55e49:
Quoted 37 lines
Quoted 15 lines
Quoted 15 lines
Quoted 10 lines
   mt9p031: Use the common clock framework (2013-04-12 11:07:07 +0200)
----------------------------------------------------------------
Laurent Pinchart (5):
mt9m032: Fix PLL setup
mt9m032: Define MT9M032_READ_MODE1 bits
mt9p031: Use devm_* managed helpers
mt9p031: Add support for regulators
mt9p031: Use the common clock framework
Hmm... It seems ugly to have regulators and clock framework and other SoC calls inside an i2c driver that can be used by a device that doesn't have regulators.
Quoted 6 lines
Quoted 3 lines
I'm not sure what's the best solution for it, so, I'll be adding those two patches, but it seems that we'll need to restrict the usage of those calls only if the caller driver is a platform driver.
The MT9P031 needs power supplies and a clock on all platforms, regardless of the bridge bus type.
Well, all digital devices require clock and power. If power is either a simple electric circuit, a battery or a regulator, that depends on the board.
Quoted 2 lines
I suppose the use case that mostly concerns you here is USB webcams
Yes.
Quoted 2 lines
where the power supplies and the clock are controlled automatically by the device.
Or could be not controlled at all. It could be a simple XTAL attached to the sensor or a clock signal provided by the bridge obtained from a fixed XTAL, and a resistor bridge or a Zenner diode providing the needed power voltage.

Yes, and this are details of the whole system, which IMHO are supposed to be abstracted outside of a clock/power supply consumer driver. What resources are needed is well defined by a chip architecture, driver of a chip knows what voltage supply/clocks, etc. it needs and it should request that. Now on some systems there is no need to explicitly enable/disable, change parameters of some clocks/voltage regulators. And the question is, IIUC, at what layer we choose to abstract such differences. I don't think it is a good idea to push it into a sub-device driver. A sub-device driver would need to know details of its host driver, wouldn't it ? AFAICT it's not something subdev drivers are really supposed to deal with.

I'm not sure if the regulators and the clock framework are "SoC calls". These are generic APIs that have well defined semantic of abstracting platform differences. At least it can be said about the regulators API IMHO.

It's probably more clean to provide a dummy clock/regulator in a host driver (platform) than to add something in a sub-device drivers that would resolve which resources should be requested and which not.

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
If we ever need to support such a device in the future we can of course revisit the driver then, and one possible solution would be to register fixed voltage regulators and a fixed clock.
That is an overkill: devices were the power supply/xtal clock can't be controlled should not require extra software that pretend to control it.

Quoted 8 lines

Quoted 3 lines
If I'm not mistaken that's however the recommended way on embedded devices at the moment. I don't have a strong opinion on the subject for now, but this will need to be at least discussed with core clock and regulator developers.
Well, a customer's webcam is not an embedded device at all. That's why I think that putting it at the I2C driver is wrong: those drivers are not to be used only by embedded hardware.

Regards,
Sylwester

Mark Brown

2013-04-17 13:55 UTC #7 lore raw

Quoted 3 lines · Sylwester Nawrocki

It's probably more clean to provide a dummy clock/regulator in a host driver (platform) than to add something in a sub-device drivers that would resolve which resources should be requested and which not.

Yes, that's the general theory for regulators at least - it allows the device driver to just trundle along and not worry about how the board is hooked up. The other issue it resolves that you didn't mention is that it avoids just ignoring errors which isn't terribly clever.

Mauro Carvalho Chehab

2013-04-17 14:36 UTC #8 lore raw

Em Wed, 17 Apr 2013 14:55:03 +0100
Mark Brown <[email protected]> escreveu:

Quoted 10 lines · Sylwester Nawrocki

Quoted 4 lines
It's probably more clean to provide a dummy clock/regulator in a host driver (platform) than to add something in a sub-device drivers that would resolve which resources should be requested and which not.
Yes, that's the general theory for regulators at least - it allows the device driver to just trundle along and not worry about how the board is hooked up. The other issue it resolves that you didn't mention is that it avoids just ignoring errors which isn't terribly clever.

I agree. Adding dummy clock/regulator at the host platform driver makes sense, as the platform driver knows how the board is hooked up; keeping it at the I2C driver doesn't make sense, so the code needs to be moved away from it.

Laurent,

Could you please work on a patch moving that code to the host platform driver?

Thanks!
Mauro

Laurent Pinchart

author

2013-04-21 23:14 UTC #9 lore raw

Hi Mauro,

Quoted 16 lines · On Wednesday 17 April 2013 11:36:39 Mauro Carvalho Chehab wrote:

Em Wed, 17 Apr 2013 14:55:03 +0100 Mark Brown escreveu:
Quoted 9 lines · Sylwester Nawrocki
Quoted 3 lines
It's probably more clean to provide a dummy clock/regulator in a host driver (platform) than to add something in a sub-device drivers that would resolve which resources should be requested and which not.
Yes, that's the general theory for regulators at least - it allows the device driver to just trundle along and not worry about how the board is hooked up. The other issue it resolves that you didn't mention is that it avoids just ignoring errors which isn't terribly clever.
I agree. Adding dummy clock/regulator at the host platform driver makes sense, as the platform driver knows how the board is hooked up; keeping it at the I2C driver doesn't make sense, so the code needs to be moved away from it.

Quoted 4 lines

Laurent,
Could you please work on a patch moving that code to the host platform driver?

I think that Mark's point was that the regulators should be provided by platform code (in the generic sense, it could be DT on ARM, board code, or a USB bridge driver for a webcam that uses the mt9p031 sensor) and used by the sensor driver. That's exactly what my mt9p031 patch does.

Signature · 3 lines

Regards,

Laurent Pinchart

Mark Brown

2013-04-22 10:03 UTC #10 lore raw

Quoted 4 lines · Laurent Pinchart

I think that Mark's point was that the regulators should be provided by platform code (in the generic sense, it could be DT on ARM, board code, or a USB bridge driver for a webcam that uses the mt9p031 sensor) and used by the sensor driver. That's exactly what my mt9p031 patch does.

Yes, you understood me perfectly - to a good approximation the matching up should be done by whatever the chip is soldered down to.

Mauro Carvalho Chehab

2013-04-22 12:46 UTC #11 lore raw

Em 22-04-2013 07:03, Mark Brown escreveu:

Quoted 10 lines · Laurent Pinchart

Quoted 5 lines
I think that Mark's point was that the regulators should be provided by platform code (in the generic sense, it could be DT on ARM, board code, or a USB bridge driver for a webcam that uses the mt9p031 sensor) and used by the sensor driver. That's exactly what my mt9p031 patch does.
Yes, you understood me perfectly - to a good approximation the matching up should be done by whatever the chip is soldered down to.

That doesn't make any sense to me. I2C devices can be used anywere, as they can be soldered either internally on an USB webcam without any regulators or any other platform code on it or could be soldered to some platform-specific bus.

Also, what best describes "soldered" here is the binding between an I2C driver and the I2C adapter. The I2C adapter is a platform driver on embedded devices, where, on an usual USB camera, it is just a USB->I2C bridge.

Also, requiring that simple USB cameras to have regulators will prevent its usual usage, as non-platform distros don't set config REGULATOR, and it shouldn't.

Regards,
Mauro

Mark Brown

2013-04-22 12:56 UTC #12 lore raw

Quoted 9 lines · Mauro Carvalho Chehab

Em 22-04-2013 07:03, Mark Brown escreveu:
Quoted 2 lines
Yes, you understood me perfectly - to a good approximation the matching up should be done by whatever the chip is soldered down to.
That doesn't make any sense to me. I2C devices can be used anywere, as they can be soldered either internally on an USB webcam without any regulators or any other platform code on it or could be soldered to some platform-specific bus.

If it's running on Linux on a visible I2C bus it ought to be shown as an I2C bus on Linux and the thing doing that plumbing ought to be worrying about hooking up anything the driver needs.

Quoted 4 lines

Also, what best describes "soldered" here is the binding between an I2C driver and the I2C adapter. The I2C adapter is a platform driver on embedded devices, where, on an usual USB camera, it is just a USB->I2C bridge.

Sure, but there's no meaningful difference between these things as far as plumbing things together goes.

Quoted 3 lines

Also, requiring that simple USB cameras to have regulators will prevent its usual usage, as non-platform distros don't set config REGULATOR, and it shouldn't.

No problem there, the regulator API stubs itself out if it's not enabled.

Mauro Carvalho Chehab

2013-04-22 12:46 UTC #13 lore raw

In reply to Mark Brown

Em 22-04-2013 07:03, Mark Brown escreveu:

Quoted 10 lines · Laurent Pinchart

Quoted 5 lines
I think that Mark's point was that the regulators should be provided by platform code (in the generic sense, it could be DT on ARM, board code, or a USB bridge driver for a webcam that uses the mt9p031 sensor) and used by the sensor driver. That's exactly what my mt9p031 patch does.
Yes, you understood me perfectly - to a good approximation the matching up should be done by whatever the chip is soldered down to.

Also, requiring that simple USB cameras to have regulators will prevent its usual usage, as non-platform distros don't set config REGULATOR (and they shouldn't, as that would just increase the Kernel's footprint for a code that will never ever be needed there).

Regards,
Mauro