Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1686011 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera Sylwester Nawrocki <snawrocki@kernel.org> - 2017-07-12 22:10 +0200
    Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera Hans Verkuil <hverkuil@xs4all.nl> - 2017-07-18 14:10 +0200
      Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-07-18 14:20 +0200
        Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera Hugues FRUCHET <hugues.fruchet@st.com> - 2017-07-18 15:00 +0200
          Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera Sakari Ailus <sakari.ailus@iki.fi> - 2017-07-18 22:00 +0200
            Re: [PATCH v2 0/7] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-07-20 10:40 +0200
              Re: [PATCH v2 0/7] Add support of OV9655 camera Hugues FRUCHET <hugues.fruchet@st.com> - 2017-07-20 11:20 +0200

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

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

[toc] | [next] | [standalone]

#1690208

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
> 

[toc] | [prev] | [next] | [standalone]

#1690213

Hi,

> Am 18.07.2017 um 13:59 schrieb Hans Verkuil <hverkuil@xs4all.nl>:
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1690244

On 07/18/2017 02:17 PM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 18.07.2017 um 13:59 schrieb Hans Verkuil <hverkuil@xs4all.nl>:
>>
>> 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.

[toc] | [prev] | [next] | [standalone]

#1690605

On Tue, Jul 18, 2017 at 12:53:12PM +0000, 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 <hverkuil@xs4all.nl>:
> >>
> >> 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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

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

Hi,

> Am 18.07.2017 um 21:52 schrieb Sakari Ailus <sakari.ailus@iki.fi>:
> 
> On Tue, Jul 18, 2017 at 12:53:12PM +0000, 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 <hverkuil@xs4all.nl>:
>>>> 
>>>> 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.

I finally managed to find the bug why mplayer did select-timeout on the GTA04.
Was a bug in pinmux setup of the GTA04 for the omap3isp.

And I have resurrected our years old 3.12 camera driver, which was based on the
MT9P031 code. It was already separate from ov9650/52.

I have extended it to support DT by including some parts of Hugues' work.

It still needs some cleanup and discussion but will be a simple patch (one
for ov9655.c + Kconfig + Makefile) and one for bindings (I hope it includes
all your comments).

I will post v1 in the next days.

BR,
Nikolaus

[toc] | [prev] | [next] | [standalone]

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

On 07/20/2017 10:37 AM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 18.07.2017 um 21:52 schrieb Sakari Ailus <sakari.ailus@iki.fi>:
>>
>> On Tue, Jul 18, 2017 at 12:53:12PM +0000, 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 <hverkuil@xs4all.nl>:
>>>>>
>>>>> 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.
> 
> I finally managed to find the bug why mplayer did select-timeout on the GTA04.
> Was a bug in pinmux setup of the GTA04 for the omap3isp.
> 
> And I have resurrected our years old 3.12 camera driver, which was based on the
> MT9P031 code. It was already separate from ov9650/52.
> 
> I have extended it to support DT by including some parts of Hugues' work.
> 
> It still needs some cleanup and discussion but will be a simple patch (one
> for ov9655.c + Kconfig + Makefile) and one for bindings (I hope it includes
> all your comments).
> 
> I will post v1 in the next days.
> 
> BR,
> Nikolaus
> 

Thanks Nikolaus,

I was ready to push the new version in new file ov9655.c with all 
comments included, but as my version is very minimal and I suspect that 
yours is more complete, let's merge things together.
Can I consider that you now take ownership of this driver upstream ?
If so I'll send to you my current patchset so you can compare, 
double-check review comments and add missing support on your side 
(RGB565 and VGA/QVGA resolution matter on my side).

Thanks again Nikolaus for this work,

BR,
Hugues.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web