Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sakari Ailus]

On Tue, Jul 18, 2017 at 12:53:12PM +, Hugues FRUCHET wrote:
> 
> 
> On 07/18/2017 02:17 PM, H. Nikolaus Schaller wrote:
> > Hi,
> > 
> >> Am 18.07.2017 um 13:59 schrieb Hans Verkuil :
> >>
> >> On 12/07/17 22:01, Sylwester Nawrocki wrote:
> >>> Hi Hugues,
> >>>
> >>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>  This patchset enables OV9655 camera support.
> 
>  OV9655 support has been tested using STM32F4DIS-CAM extension board
>  plugged on connector P1 of STM32F746G-DISCO board.
>  Due to lack of OV9650/52 hardware support, the modified related code
>  could not have been checked for non-regression.
> 
>  First patches upgrade current support of OV9650/52 to prepare then
>  introduction of OV9655 variant patch.
>  Because of OV9655 register set slightly different from OV9650/9652,
>  not all of the driver features are supported (controls). Supported
>  resolutions are limited to VGA, QVGA, QQVGA.
>  Supported format is limited to RGB565.
>  Controls are limited to color bar test pattern for test purpose.
> >>>
> >>> I appreciate your efforts towards making a common driver but IMO it would 
> >>> be
> >>> better to create a separate driver for the OV9655 sensor.  The original 
> >>> driver
> >>> is 1576 lines of code, your patch set adds half of that (816).  There are
> >>> significant differences in the feature set of both sensors, there are
> >>> differences in the register layout.  I would go for a separate driver, we
> >>> would then have code easier to follow and wouldn't need to worry about 
> >>> possible
> >>> regressions.  I'm afraid I have lost the camera module and won't be able
> >>> to test the patch set against regressions.
> >>>
> >>> IMHO from maintenance POV it's better to make a separate driver. In the 
> >>> end
> >>> of the day we wouldn't be adding much more code than it is being done now.
> >>
> >> I agree. We do not have great experiences in the past with trying to 
> >> support
> >> multiple variants in a single driver (unless the diffs are truly small).
> > 
> > Well,
> > IMHO the diffs in ov965x are smaller (but untestable because nobody seems
> > to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
> > an old pdata based separate ov9655 driver and extend that to become DT 
> > compatible.
> > 
> > I had abandoned that separate approach in favour of extending the ov965x 
> > driver.
> > 
> > Have to discuss with Hugues how to proceed.
> > 
> > BR and thanks,
> > Nikolaus
> > 
> 
> As Sylwester and Hans, I'm also in flavour of a separate driver, the 
> fact that register set seems similar but in fact is not and that we 
> cannot test for non-regression of 9650/52 are killer for me to continue 
> on a single driver.
> We can now restart from a new fresh state of the art sensor driver 
> getting rid of legacy (pdata, old gpio, etc...).

Agreed. I bet the result will look cleaner indeed although this wasn't one
of the complex drivers.

It'd be nice that someone was able to test the ov9650/2, too, drivers that
are never used tend to break...

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hugues FRUCHET]

On 07/18/2017 02:17 PM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 18.07.2017 um 13:59 schrieb Hans Verkuil :
>>
>> On 12/07/17 22:01, Sylwester Nawrocki wrote:
>>> Hi Hugues,
>>>
>>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
 This patchset enables OV9655 camera support.

 OV9655 support has been tested using STM32F4DIS-CAM extension board
 plugged on connector P1 of STM32F746G-DISCO board.
 Due to lack of OV9650/52 hardware support, the modified related code
 could not have been checked for non-regression.

 First patches upgrade current support of OV9650/52 to prepare then
 introduction of OV9655 variant patch.
 Because of OV9655 register set slightly different from OV9650/9652,
 not all of the driver features are supported (controls). Supported
 resolutions are limited to VGA, QVGA, QQVGA.
 Supported format is limited to RGB565.
 Controls are limited to color bar test pattern for test purpose.
>>>
>>> I appreciate your efforts towards making a common driver but IMO it would be
>>> better to create a separate driver for the OV9655 sensor.  The original 
>>> driver
>>> is 1576 lines of code, your patch set adds half of that (816).  There are
>>> significant differences in the feature set of both sensors, there are
>>> differences in the register layout.  I would go for a separate driver, we
>>> would then have code easier to follow and wouldn't need to worry about 
>>> possible
>>> regressions.  I'm afraid I have lost the camera module and won't be able
>>> to test the patch set against regressions.
>>>
>>> IMHO from maintenance POV it's better to make a separate driver. In the end
>>> of the day we wouldn't be adding much more code than it is being done now.
>>
>> I agree. We do not have great experiences in the past with trying to support
>> multiple variants in a single driver (unless the diffs are truly small).
> 
> Well,
> IMHO the diffs in ov965x are smaller (but untestable because nobody seems
> to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
> an old pdata based separate ov9655 driver and extend that to become DT 
> compatible.
> 
> I had abandoned that separate approach in favour of extending the ov965x 
> driver.
> 
> Have to discuss with Hugues how to proceed.
> 
> BR and thanks,
> Nikolaus
> 

As Sylwester and Hans, I'm also in flavour of a separate driver, the 
fact that register set seems similar but in fact is not and that we 
cannot test for non-regression of 9650/52 are killer for me to continue 
on a single driver.
We can now restart from a new fresh state of the art sensor driver 
getting rid of legacy (pdata, old gpio, etc...).

BR,
Hugues.

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[H. Nikolaus Schaller]

Hi,

> Am 18.07.2017 um 13:59 schrieb Hans Verkuil :
> 
> On 12/07/17 22:01, Sylwester Nawrocki wrote:
>> Hi Hugues,
>> 
>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>>> This patchset enables OV9655 camera support.
>>> 
>>> OV9655 support has been tested using STM32F4DIS-CAM extension board
>>> plugged on connector P1 of STM32F746G-DISCO board.
>>> Due to lack of OV9650/52 hardware support, the modified related code
>>> could not have been checked for non-regression.
>>> 
>>> First patches upgrade current support of OV9650/52 to prepare then
>>> introduction of OV9655 variant patch.
>>> Because of OV9655 register set slightly different from OV9650/9652,
>>> not all of the driver features are supported (controls). Supported
>>> resolutions are limited to VGA, QVGA, QQVGA.
>>> Supported format is limited to RGB565.
>>> Controls are limited to color bar test pattern for test purpose.
>> 
>> I appreciate your efforts towards making a common driver but IMO it would be 
>> better to create a separate driver for the OV9655 sensor.  The original 
>> driver 
>> is 1576 lines of code, your patch set adds half of that (816).  There are
>> significant differences in the feature set of both sensors, there are 
>> differences in the register layout.  I would go for a separate driver, we  
>> would then have code easier to follow and wouldn't need to worry about 
>> possible
>> regressions.  I'm afraid I have lost the camera module and won't be able 
>> to test the patch set against regressions.
>> 
>> IMHO from maintenance POV it's better to make a separate driver. In the end 
>> of the day we wouldn't be adding much more code than it is being done now.
> 
> I agree. We do not have great experiences in the past with trying to support
> multiple variants in a single driver (unless the diffs are truly small).

Well,
IMHO the diffs in ov965x are smaller (but untestable because nobody seems
to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
an old pdata based separate ov9655 driver and extend that to become DT 
compatible.

I had abandoned that separate approach in favour of extending the ov965x driver.

Have to discuss with Hugues how to proceed.

BR and thanks,
Nikolaus

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hans Verkuil]

On 12/07/17 22:01, Sylwester Nawrocki wrote:
> Hi Hugues,
> 
> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>> This patchset enables OV9655 camera support.
>>
>> OV9655 support has been tested using STM32F4DIS-CAM extension board
>> plugged on connector P1 of STM32F746G-DISCO board.
>> Due to lack of OV9650/52 hardware support, the modified related code
>> could not have been checked for non-regression.
>>
>> First patches upgrade current support of OV9650/52 to prepare then
>> introduction of OV9655 variant patch.
>> Because of OV9655 register set slightly different from OV9650/9652,
>> not all of the driver features are supported (controls). Supported
>> resolutions are limited to VGA, QVGA, QQVGA.
>> Supported format is limited to RGB565.
>> Controls are limited to color bar test pattern for test purpose.
> 
> I appreciate your efforts towards making a common driver but IMO it would be 
> better to create a separate driver for the OV9655 sensor.  The original 
> driver 
> is 1576 lines of code, your patch set adds half of that (816).  There are
> significant differences in the feature set of both sensors, there are 
> differences in the register layout.  I would go for a separate driver, we  
> would then have code easier to follow and wouldn't need to worry about 
> possible
> regressions.  I'm afraid I have lost the camera module and won't be able 
> to test the patch set against regressions.
> 
> IMHO from maintenance POV it's better to make a separate driver. In the end 
> of the day we wouldn't be adding much more code than it is being done now.

I agree. We do not have great experiences in the past with trying to support
multiple variants in a single driver (unless the diffs are truly small).

Regards,

Hans

> 
>>   .../devicetree/bindings/media/i2c/ov965x.txt   |  45 ++
>>   drivers/media/i2c/Kconfig  |   6 +-
>>   drivers/media/i2c/ov9650.c | 816 
>> +
>>   3 files changed, 736 insertions(+), 131 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
> 
> --
> Thanks,
> Sylwester
> 

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sylwester Nawrocki]

Hi Hugues,

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> This patchset enables OV9655 camera support.
> 
> OV9655 support has been tested using STM32F4DIS-CAM extension board
> plugged on connector P1 of STM32F746G-DISCO board.
> Due to lack of OV9650/52 hardware support, the modified related code
> could not have been checked for non-regression.
> 
> First patches upgrade current support of OV9650/52 to prepare then
> introduction of OV9655 variant patch.
> Because of OV9655 register set slightly different from OV9650/9652,
> not all of the driver features are supported (controls). Supported
> resolutions are limited to VGA, QVGA, QQVGA.
> Supported format is limited to RGB565.
> Controls are limited to color bar test pattern for test purpose.

I appreciate your efforts towards making a common driver but IMO it would be 
better to create a separate driver for the OV9655 sensor.  The original driver 
is 1576 lines of code, your patch set adds half of that (816).  There are
significant differences in the feature set of both sensors, there are 
differences in the register layout.  I would go for a separate driver, we  
would then have code easier to follow and wouldn't need to worry about possible
regressions.  I'm afraid I have lost the camera module and won't be able 
to test the patch set against regressions.

IMHO from maintenance POV it's better to make a separate driver. In the end 
of the day we wouldn't be adding much more code than it is being done now.

>   .../devicetree/bindings/media/i2c/ov965x.txt   |  45 ++
>   drivers/media/i2c/Kconfig  |   6 +-
>   drivers/media/i2c/ov9650.c | 816 
> +
>   3 files changed, 736 insertions(+), 131 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

--
Thanks,
Sylwester

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sylwester Nawrocki]

Hi Hugues,

On 07/06/2017 09:51 AM, Hugues FRUCHET wrote:

Hi Sylwester,

Do you have the possibility to check for non-regression of this patchset
on 9650/52 camera ?


I will try to test your patch set once I find the camera module for
my Micro2440SDK board. I've spent already a day on setting up everything 
and fixing multiple regressions in the kernel. I will likely try your 
patch series in coming week.


--
Thanks,
Sylwester

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hugues FRUCHET]

Hi Sylwester,

Do you have the possibility to check for non-regression of this patchset 
on 9650/52 camera ?

Best regards,
Hugues.

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> This patchset enables OV9655 camera support.
> 
> OV9655 support has been tested using STM32F4DIS-CAM extension board
> plugged on connector P1 of STM32F746G-DISCO board.
> Due to lack of OV9650/52 hardware support, the modified related code
> could not have been checked for non-regression.
> 
> First patches upgrade current support of OV9650/52 to prepare then
> introduction of OV9655 variant patch.
> Because of OV9655 register set slightly different from OV9650/9652,
> not all of the driver features are supported (controls). Supported
> resolutions are limited to VGA, QVGA, QQVGA.
> Supported format is limited to RGB565.
> Controls are limited to color bar test pattern for test purpose.
> 
> OV9655 initial support is based on a driver written by H. Nikolaus Schaller 
> [1].
> OV9655 registers sequences come from STM32CubeF7 embedded software [2].
> 
> [1] 
> http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
> [2] 
> https://developer.mbed.org/teams/ST/code/BSP_DISCO_F746NG/file/e1d9da7fe856/Drivers/BSP/Components/ov9655/ov9655.c
> 
> ===
> = history =
> ===
> version 2:
>- Remove some unneeded semicolons (kbuild test robot):
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114616.html
>- Remove patch [media] ov9650: select the nearest higher resolution:
>  it is up to the application to find the best matching resolution
>  using ENUM_FRAMESIZES/S_FMT/S_SELECTION (S_CROP), see
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114667.html
>- dt-bindings: Fix remarks from Rob Herring about polarity:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114705.html
>- dt-bindings: Add optional regulators avdd, dvdd, dovdd:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114785.html
>- fix missing semicolons in if condition:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114611.html
>- move ov965x_pixfmt relocation in right patch:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114849.html
>- revisit OV965x renaming to ov965x for device id names and DT compatible 
> strings,
>  drop of_device_id .data device identification
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114635.html
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114738.html
>- Add analog power supply and clock gating, needed for GTA04 platform:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114519.html
> 
> version 1:
>- Initial submission.
> 
> H. Nikolaus Schaller (1):
>DT bindings: add bindings for ov965x camera module
> 
> Hugues Fruchet (6):
>[media] ov9650: switch i2c device id to lower case
>[media] ov9650: add device tree support
>[media] ov9650: use write_array() for resolution sequences
>[media] ov9650: add multiple variant support
>[media] ov9650: add support of OV9655 variant
>[media] ov9650: add analog power supply and clock gating
> 
>   .../devicetree/bindings/media/i2c/ov965x.txt   |  45 ++
>   drivers/media/i2c/Kconfig  |   6 +-
>   drivers/media/i2c/ov9650.c | 816 
> +
>   3 files changed, 736 insertions(+), 131 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
>