| Message ID | 55D259A1.6090102@tul.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Petr Cvek <petr.cvek@tul.cz> writes:
> + mdelay(3);
Okay, there are several mdelay(3) in this file.
Could you please test if removing these mdelays prevents the camera from working
? If not, I'd like to have them removed.
Cheers.
--
Robert
On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: > Petr Cvek <petr.cvek@tul.cz> writes: > > > + mdelay(3); > Okay, there are several mdelay(3) in this file. > > Could you please test if removing these mdelays prevents the camera from working > ? If not, I'd like to have them removed. > > FWIW, it might be possible to replace some of them with msleep() calls, potentially after replacing spinlocks with mutexes where necessary. Arnd
Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a): > On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: >> Petr Cvek <petr.cvek@tul.cz> writes: >> >>> + mdelay(3); >> Okay, there are several mdelay(3) in this file. >> >> Could you please test if removing these mdelays prevents the camera from working >> ? If not, I'd like to have them removed. >> >> > > FWIW, it might be possible to replace some of them with msleep() calls, > potentially after replacing spinlocks with mutexes where necessary. > > Arnd > Datasheet says: tS:RESET Setting time after software/hardware reset 1 ms So at least one ~1 ms should be left there. Are msleep less than 20ms valid? (checkpatch: msleep < 20ms can sleep for up to 20ms) Petr
On Friday 21 August 2015 00:39:30 Petr Cvek wrote: > Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a): > > On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: > >> Petr Cvek <petr.cvek@tul.cz> writes: > >> > >>> + mdelay(3); > >> Okay, there are several mdelay(3) in this file. > >> > >> Could you please test if removing these mdelays prevents the camera from working > >> ? If not, I'd like to have them removed. > >> > >> > > > > FWIW, it might be possible to replace some of them with msleep() calls, > > potentially after replacing spinlocks with mutexes where necessary. > > > > Arnd > > > > Datasheet says: > > tS:RESET Setting time after software/hardware reset 1 ms > > So at least one ~1 ms should be left there. Are msleep less than 20ms valid? > > (checkpatch: msleep < 20ms can sleep for up to 20ms) On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds (but could be much longer), while an mdelay(1) tries to sleep around one millisecond, more or less. Arnd
I shrunk a bit the mailing list. Arnd Bergmann <arnd@arndb.de> writes: > On Friday 21 August 2015 00:39:30 Petr Cvek wrote: >> Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a): >> > On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: >> >> Petr Cvek <petr.cvek@tul.cz> writes: >> Datasheet says: >> >> tS:RESET Setting time after software/hardware reset 1 ms >> >> So at least one ~1 ms should be left there. Are msleep less than 20ms valid? >> >> (checkpatch: msleep < 20ms can sleep for up to 20ms) > > On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds > (but could be much longer), while an mdelay(1) tries to sleep around one > millisecond, more or less. I have rethought of that a bit more. I'm convinced a delay of "at least 1ms" is necessary according to the specification, it can also be more. Moreover, I came to the conclusion this reset sequence is not something that is "magician" specific (see palmz72_camera_reset() in .../mach-pxa/palmz72.c). Actually it's not even mach-pxa specific, it's "ov9640" specific. Now add this to the fact that it would be good to have a solution working for devicetree as well, and on any board, and the conclusion I came to was that this handling deserves to be in ov9640 driver (please correct me if I'm wrong). The idea behind it is have the reset handled in ov9640, and the gpio provided by platform data or devicetree. So Guennadi, is it possible to add a gpio through platform data to ov9640 driver, does it make sense, and would you accept to have the reset handled there ? And if yes, would you, Petr, accept to revamp your patch to have the reset and power handled in ov9640 ? Please note that it is a proposal, I'm not forcing anybody, I'd like to choose a path that agrees with the future push to remove as many machine files as possible. Cheers.
Dne 21.8.2015 v 19:36 Robert Jarzmik napsal(a): > I shrunk a bit the mailing list. > > Arnd Bergmann <arnd@arndb.de> writes: > >> On Friday 21 August 2015 00:39:30 Petr Cvek wrote: >>> Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a): >>>> On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: >>>>> Petr Cvek <petr.cvek@tul.cz> writes: >>> Datasheet says: >>> >>> tS:RESET Setting time after software/hardware reset 1 ms >>> >>> So at least one ~1 ms should be left there. Are msleep less than 20ms valid? >>> >>> (checkpatch: msleep < 20ms can sleep for up to 20ms) >> >> On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds >> (but could be much longer), while an mdelay(1) tries to sleep around one >> millisecond, more or less. > > I have rethought of that a bit more. I'm convinced a delay of "at least 1ms" is > necessary according to the specification, it can also be more. > > Moreover, I came to the conclusion this reset sequence is not something that is > "magician" specific (see palmz72_camera_reset() in > .../mach-pxa/palmz72.c). Actually it's not even mach-pxa specific, it's "ov9640" > specific. > > Now add this to the fact that it would be good to have a solution working for > devicetree as well, and on any board, and the conclusion I came to was that this > handling deserves to be in ov9640 driver (please correct me if I'm wrong). > > The idea behind it is have the reset handled in ov9640, and the gpio provided by > platform data or devicetree. > > So Guennadi, is it possible to add a gpio through platform data to ov9640 > driver, does it make sense, and would you accept to have the reset handled there > ? And if yes, would you, Petr, accept to revamp your patch to have the reset and > power handled in ov9640 ? > OK, why not, so power and reset gpio with polarity settings? > Please note that it is a proposal, I'm not forcing anybody, I'd like to choose a > path that agrees with the future push to remove as many machine files as possible. Anyway I'm planning to send patch for OV9640 in future too (color correction matrix is not complete and some registers too). > > Cheers. >
Petr Cvek <petr.cvek@tul.cz> writes: >> So Guennadi, is it possible to add a gpio through platform data to ov9640 >> driver, does it make sense, and would you accept to have the reset handled there >> ? And if yes, would you, Petr, accept to revamp your patch to have the reset and >> power handled in ov9640 ? >> > > OK, why not, so power and reset gpio with polarity settings? Even better pass ... nothing through platform-data. I think there is now a gpio binding for devices which works this way : - in ov9640 : devm_gpiod_get(dev, "reset") - in magician: GPIO_LOOKUP() and gpiod_add_lookup_table() In this way I think we'll gather at the same time : - no new platform data - ov9640 will follow the same path for devicetree and platform data > Anyway I'm planning to send patch for OV9640 in future too (color correction > matrix is not complete and some registers too). Good. Cheers.
On Fri, 21 Aug 2015, Arnd Bergmann wrote: > On Friday 21 August 2015 00:39:30 Petr Cvek wrote: > > Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a): > > > On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: > > >> Petr Cvek <petr.cvek@tul.cz> writes: > > >> > > >>> + mdelay(3); > > >> Okay, there are several mdelay(3) in this file. > > >> > > >> Could you please test if removing these mdelays prevents the camera from working > > >> ? If not, I'd like to have them removed. > > >> > > >> > > > > > > FWIW, it might be possible to replace some of them with msleep() calls, > > > potentially after replacing spinlocks with mutexes where necessary. > > > > > > Arnd > > > > > > > Datasheet says: > > > > tS:RESET Setting time after software/hardware reset 1 ms > > > > So at least one ~1 ms should be left there. Are msleep less than 20ms valid? > > > > (checkpatch: msleep < 20ms can sleep for up to 20ms) > > On most kernels, an msleep(1) will sleep somewhere between 1 and 3 milliseconds > (but could be much longer), while an mdelay(1) tries to sleep around one > millisecond, more or less. If high-res timers are available I'd suggest using usleep_range() instead. By providing suitably divergent values usleep_range de-restricts the kernel, allowing the scheduler to bundle your wake-up along with others.
diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c index 7331b34..63604e1 100644 --- a/arch/arm/mach-pxa/magician.c +++ b/arch/arm/mach-pxa/magician.c @@ -91,6 +91,9 @@ #include <linux/spi/pxa2xx_spi.h> #include <linux/spi/ads7846.h> +#include <linux/platform_data/camera-pxa.h> +#include <media/soc_camera.h> + #include "devices.h" #include "generic.h" @@ -634,6 +637,60 @@ static struct platform_device pasic3 = { }; /* + * SoC Camera + */ + +static struct pxacamera_platform_data magician_pxacamera_pdata = { + .flags = PXA_CAMERA_MASTER | PXA_CAMERA_DATAWIDTH_8 | + PXA_CAMERA_PCLK_EN | PXA_CAMERA_MCLK_EN, + .mclk_10khz = 4800, +}; + +static int magician_camera_power(struct device *dev, int power) +{ + pr_debug("Camera power = %i\n", power); + + gpio_set_value(GPIO116_MAGICIAN_nCAM_EN, !power); + mdelay(3); + + return 0; +} + +static int magician_camera_reset(struct device *dev) +{ + pr_debug("Camera reset\n"); + + gpio_set_value(GPIO57_MAGICIAN_CAM_RESET, 1); + mdelay(3); + gpio_set_value(GPIO57_MAGICIAN_CAM_RESET, 0); + mdelay(3); + + return 0; +} + +static struct i2c_board_info magician_camera_i2c_board_info = { + I2C_BOARD_INFO("ov9640", 0x30), + .flags = I2C_CLIENT_SCCB, +}; + +static struct soc_camera_link magician_camera_iclink = { + .bus_id = 0, + .flags = SOCAM_DATAWIDTH_8, + .i2c_adapter_id = 0, + .board_info = &magician_camera_i2c_board_info, + .power = magician_camera_power, + .reset = magician_camera_reset, +}; + +static struct platform_device magician_camera = { + .name = "soc-camera-pdrv", + .id = 0, + .dev = { + .platform_data = &magician_camera_iclink, + }, +}; + +/* * USB "Transceiver" */ @@ -1038,6 +1095,7 @@ static struct platform_device *devices[] __initdata = { &gpio_vbus, &power_supply, &leds_gpio, + &magician_camera, /* NOTICE mutually exclusive with PXA I2C */ &i2c_gpio_bus_alt, @@ -1058,6 +1116,10 @@ static struct gpio magician_global_gpios[] = { "LCD VOFF enable" }, { GPIO105_MAGICIAN_LCD_VON_EN, GPIOF_OUT_INIT_HIGH, "LCD VON enable" }, + { GPIO57_MAGICIAN_CAM_RESET, GPIOF_OUT_INIT_HIGH, + "Camera reset" }, + { GPIO116_MAGICIAN_nCAM_EN, GPIOF_OUT_INIT_HIGH, + "Camera power" }, }; static void __init magician_init(void) @@ -1105,6 +1167,7 @@ static void __init magician_init(void) pxa2xx_set_spi_info(2, &magician_spi_info); spi_register_board_info(ARRAY_AND_SIZE(ads7846_spi_board_info)); + pxa_set_camera_info(&magician_pxacamera_pdata); } MACHINE_START(MAGICIAN, "HTC Magician")
Add OV9640 camera support for HTC Magician. Signed-off-by: Petr Cvek <petr.cvek@tul.cz> --- arch/arm/mach-pxa/magician.c | 63 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)