Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor

2019-09-09 Thread Sakari Ailus
Hi Dongchun,

On Mon, Sep 09, 2019 at 04:46:15PM +0800, Dongchun Zhu wrote:
> Hi Sakari,
> 
> On Thu, 2019-08-08 at 16:53 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> > 
> > Thanks for the patch.
> > 
> > On Thu, Aug 08, 2019 at 05:22:15PM +0800, [email protected] wrote:
> > > From: Dongchun Zhu 
> > > 
> > > This patch mainly adds two more sensor modes for OV8856 image sensor.
> > > The OV8856 driver currently supports output format: 10-bit Raw,
> > > the resolution of 1632*1224 and 3264*2448, and the bayer order of BGGR.
> > > The hardware version also differs in some OTP regiser,
> > > as well as PLL register setting.
> > > 
> > > Signed-off-by: Dongchun Zhu 
> > > ---
> > >  drivers/media/i2c/ov8856.c | 624 
> > > -
> > >  1 file changed, 621 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > index cd347d6..e0610b6 100644
> > > --- a/drivers/media/i2c/ov8856.c
> > > +++ b/drivers/media/i2c/ov8856.c
> > > @@ -1,12 +1,15 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  // Copyright (c) 2019 Intel Corporation.
> > >  
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -19,6 +22,7 @@
> > >  #define OV8856_LINK_FREQ_180MHZ  18000ULL
> > >  #define OV8856_SCLK  14400ULL
> > >  #define OV8856_MCLK  1920
> > > +#define OV8856_XVCLK_FREQ2400
> > 
> > The driver currenctly uses, perhaps misleadingly, OV8856_MCLK for this
> > purpose. You could rename the existing MCLK as XVCLK.
> > 
> 
> OV8856_MCLK would be replaced by OV8856_XVCLK in next release.
> 
> > This also means the driver needs to differentiate configurations for 24 and
> > 19,2 MHz which it currently does not do. I think it may make sense to make
> > this a separate patch from the rest.
> > 
> 
> From datasheet, OV8856 could support input clock XVCLK range 6-27MHz.
> But the typical frequency is 24MHz.
> That's the value we now provide.

You'll still need to check for it in the driver, to avoid breaking existing
systems that use another frequency.

...

> > > @@ -566,6 +1018,10 @@ struct ov8856 {
> > >   struct media_pad pad;
> > >   struct v4l2_ctrl_handler ctrl_handler;
> > >  
> > > + struct clk  *xvclk;
> > > + struct gpio_desc*reset_gpio;
> > > + struct regulator_bulk_data supplies[OV8856_NUM_SUPPLIES];
> > > +
> > >   /* V4L2 Controls */
> > >   struct v4l2_ctrl *link_freq;
> > >   struct v4l2_ctrl *pixel_rate;
> > > @@ -576,6 +1032,9 @@ struct ov8856 {
> > >   /* Current mode */
> > >   const struct ov8856_mode *cur_mode;
> > >  
> > > + /* module hardware version */
> > > + bool is_1B_module;
> > 
> > What other hardware versions are there, and what are the differences?
> > 
> 
> According to OV, there are two MP hardware versions (i.e., 1B and 2A)
> for OV8856.
> There is one difference between 1B and 2A module - which is 0x3614
> register.

Could you document the difference in a comment, and also how the B1 variant
is told apart from the 2A one?

> 
> > > +
> > >   /* To serialize asynchronus callbacks */
> > >   struct mutex mutex;
> > >  
> > > @@ -696,6 +1155,24 @@ static int ov8856_test_pattern(struct ov8856 
> > > *ov8856, u32 pattern)
> > >   OV8856_REG_VALUE_08BIT, pattern);
> > >  }
> > >  
> > > +static int ov8856_update_otp_reg(struct ov8856 *ov8856)
> > > +{
> > > + int ret;
> > > +
> > > + ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
> > > +OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = ov8856_write_reg(ov8856, OV8856_OTP_REG_ONE,
> > > +OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return ov8856_write_reg(ov8856, OV8856_OTP_REG_TWO,
> > > + OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
> > > +}
> > 
> > What does this do?
> > 
> 
> R700f is OTP register, the register value corresponds different hardware
> version.
> Mainly including:
> 01: 2A: EMI improvement
> 02: 1B: remove some mask for cost; initial setting cannot share with 1A

If you're accessing EEPROM here, please tell that.

Do you really need to start streaming to do that? Please add comments to
what do these steps actually achieve.

> 
> To identify sensor chip version, the following steps needs to be done
> before reading out R700f.
> R0100 write 0x01.
> R3d84 write 0x00
> R3d81 write 0x01
> 
> To make more clear, ov8856_update_otp_reg() would be renamed to
> ov8856_check_revision() in next release.

Sounds good to me.

> 
> > > +
> > >  static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  {
> > >   struct ov8856 *ov8856 = container_of(ctrl->handler,

Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor

2019-09-09 Thread Dongchun Zhu
Hi Tomasz,

On Fri, 2019-08-23 at 19:01 +0900, Tomasz Figa wrote:
> Hi Dongchun,
> 
> On Thu, Aug 08, 2019 at 05:22:15PM +0800, [email protected] wrote:
> > From: Dongchun Zhu 
> > 
> > This patch mainly adds two more sensor modes for OV8856 image sensor.
> > The OV8856 driver currently supports output format: 10-bit Raw,
> > the resolution of 1632*1224 and 3264*2448, and the bayer order of BGGR.
> > The hardware version also differs in some OTP regiser,
> > as well as PLL register setting.
> > 
> > Signed-off-by: Dongchun Zhu 
> > ---
> >  drivers/media/i2c/ov8856.c | 624 
> > -
> >  1 file changed, 621 insertions(+), 3 deletions(-)
> > 
> 
> Thanks for the patch! Please see my comments inline.
> 
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index cd347d6..e0610b6 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -1,12 +1,15 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  // Copyright (c) 2019 Intel Corporation.
> >  
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -19,6 +22,7 @@
> >  #define OV8856_LINK_FREQ_180MHZ18000ULL
> >  #define OV8856_SCLK14400ULL
> >  #define OV8856_MCLK1920
> > +#define OV8856_XVCLK_FREQ  2400
> >  #define OV8856_DATA_LANES  4
> >  #define OV8856_RGB_DEPTH   10
> >  
> > @@ -29,6 +33,18 @@
> >  #define OV8856_MODE_STANDBY0x00
> >  #define OV8856_MODE_STREAMING  0x01
> >  
> > +/* define 1B module */
> > +#define OV8856_1B_MODULE   0x02
> > +
> > +/* otp sram register */
> > +#define OV8856_OTP_REG 0x700f
> 
> This isn't a register. I believe the OTP read-out buffer is at 0x7000 and
> 0xf is the offset of the byte in the OTP that means the module revision.
> 

Agreed.
I would rename this macro.

> > +#define OV8856_OTP_REG_ONE 0x3d84
> > +#define OV8856_OTP_REG_TWO 0x3d81
> 
> These registers are definitely not "ONE" and "TWO". Please use proper
> names as per the datasheet.
> 

Fixed in next release.

> > +
> > +/* clock register */
> > +#define OV8856_CLK_REG 0x3614
> > +#define OV8856_CLK_REG_1B_VAL  0x20
> 
> Same here. These don't look like the real names of the register and bit
> field.
> 

Fixed in next release.

> > +
> >  /* vertical-timings from sensor */
> >  #define OV8856_REG_VTS 0x380e
> >  #define OV8856_VTS_MAX 0x7fff
> > @@ -64,6 +80,14 @@
> >  
> >  #define to_ov8856(_sd) container_of(_sd, struct 
> > ov8856, sd)
> >  
> > +static const char * const ov8856_supply_names[] = {
> > +   "dovdd",/* Digital I/O power */
> > +   "avdd", /* Analog power */
> > +   "dvdd", /* Digital core power */
> > +};
> > +
> > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> > +
> >  enum {
> > OV8856_LINK_FREQ_720MBPS,
> > OV8856_LINK_FREQ_360MBPS,
> > @@ -316,6 +340,208 @@ static const struct ov8856_reg mode_3280x2464_regs[] 
> > = {
> > {0x5e00, 0x00}
> >  };
> >  
> > +static const struct ov8856_reg mode_3264x2448_regs[] = {
> > +   {0x0103, 0x01},
> > +   {0x0302, 0x3c},
> > +   {0x0303, 0x01},
> > +   {0x031e, 0x0c},
> > +   {0x3000, 0x00},
> > +   {0x300e, 0x00},
> > +   {0x3010, 0x00},
> > +   {0x3015, 0x84},
> > +   {0x3018, 0x72},
> > +   {0x3021, 0x23},
> > +   {0x3033, 0x24},
> > +   {0x3500, 0x00},
> > +   {0x3501, 0x9a},
> > +   {0x3502, 0x20},
> > +   {0x3503, 0x08},
> > +   {0x3505, 0x83},
> > +   {0x3508, 0x01},
> > +   {0x3509, 0x80},
> > +   {0x350c, 0x00},
> > +   {0x350d, 0x80},
> > +   {0x350e, 0x04},
> > +   {0x350f, 0x00},
> > +   {0x3510, 0x00},
> > +   {0x3511, 0x02},
> > +   {0x3512, 0x00},
> > +   {0x3600, 0x72},
> > +   {0x3601, 0x40},
> > +   {0x3602, 0x30},
> > +   {0x3610, 0xc5},
> > +   {0x3611, 0x58},
> > +   {0x3612, 0x5c},
> > +   {0x3613, 0xca},
> > +   {0x3614, 0x60},
> > +   {0x3628, 0xff},
> > +   {0x3629, 0xff},
> > +   {0x362a, 0xff},
> > +   {0x3633, 0x10},
> > +   {0x3634, 0x10},
> > +   {0x3635, 0x10},
> > +   {0x3636, 0x10},
> > +   {0x3663, 0x08},
> > +   {0x3669, 0x34},
> > +   {0x366d, 0x00},
> > +   {0x366e, 0x10},
> > +   {0x3706, 0x86},
> > +   {0x370b, 0x7e},
> > +   {0x3714, 0x23},
> > +   {0x3730, 0x12},
> > +   {0x3733, 0x10},
> > +   {0x3764, 0x00},
> > +   {0x3765, 0x00},
> > +   {0x3769, 0x62},
> > +   {0x376a, 0x2a},
> > +   {0x376b, 0x30},
> > +   {0x3780, 0x00},
> > +   {0x3781, 0x24},
> > +   {0x3782, 0x00},
> > +   {0x3783, 0x23},
> > +   {0x3798, 0x2f},
> > +   {0x37a1, 0x60},
> > +   {0x37a8, 0x6a},
> > +   {0x37ab, 0x3f},
> > +   {0x37c2, 0x04},
> > +   {0x37c3, 0xf1},
> > +   {0x37c9, 0x80},
> > +   {0x37cb, 0x16},
> > +   {0x37cc, 0x16},
> > +   {0x

Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor

2019-09-09 Thread Dongchun Zhu
Hi Sakari,

On Thu, 2019-08-08 at 16:53 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> Thanks for the patch.
> 
> On Thu, Aug 08, 2019 at 05:22:15PM +0800, [email protected] wrote:
> > From: Dongchun Zhu 
> > 
> > This patch mainly adds two more sensor modes for OV8856 image sensor.
> > The OV8856 driver currently supports output format: 10-bit Raw,
> > the resolution of 1632*1224 and 3264*2448, and the bayer order of BGGR.
> > The hardware version also differs in some OTP regiser,
> > as well as PLL register setting.
> > 
> > Signed-off-by: Dongchun Zhu 
> > ---
> >  drivers/media/i2c/ov8856.c | 624 
> > -
> >  1 file changed, 621 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index cd347d6..e0610b6 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -1,12 +1,15 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  // Copyright (c) 2019 Intel Corporation.
> >  
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -19,6 +22,7 @@
> >  #define OV8856_LINK_FREQ_180MHZ18000ULL
> >  #define OV8856_SCLK14400ULL
> >  #define OV8856_MCLK1920
> > +#define OV8856_XVCLK_FREQ  2400
> 
> The driver currenctly uses, perhaps misleadingly, OV8856_MCLK for this
> purpose. You could rename the existing MCLK as XVCLK.
> 

OV8856_MCLK would be replaced by OV8856_XVCLK in next release.

> This also means the driver needs to differentiate configurations for 24 and
> 19,2 MHz which it currently does not do. I think it may make sense to make
> this a separate patch from the rest.
> 

>From datasheet, OV8856 could support input clock XVCLK range 6-27MHz.
But the typical frequency is 24MHz.
That's the value we now provide.
+Cc: Tomasz.
Tomasz, do you have any ideas about Sakari's propose?

> >  #define OV8856_DATA_LANES  4
> >  #define OV8856_RGB_DEPTH   10
> >  
> > @@ -29,6 +33,18 @@
> >  #define OV8856_MODE_STANDBY0x00
> >  #define OV8856_MODE_STREAMING  0x01
> >  
> > +/* define 1B module */
> > +#define OV8856_1B_MODULE   0x02
> > +
> > +/* otp sram register */
> > +#define OV8856_OTP_REG 0x700f
> > +#define OV8856_OTP_REG_ONE 0x3d84
> > +#define OV8856_OTP_REG_TWO 0x3d81
> > +
> > +/* clock register */
> > +#define OV8856_CLK_REG 0x3614
> > +#define OV8856_CLK_REG_1B_VAL  0x20
> > +
> >  /* vertical-timings from sensor */
> >  #define OV8856_REG_VTS 0x380e
> >  #define OV8856_VTS_MAX 0x7fff
> > @@ -64,6 +80,14 @@
> >  
> >  #define to_ov8856(_sd) container_of(_sd, struct 
> > ov8856, sd)
> >  
> > +static const char * const ov8856_supply_names[] = {
> > +   "dovdd",/* Digital I/O power */
> > +   "avdd", /* Analog power */
> > +   "dvdd", /* Digital core power */
> > +};
> > +
> > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> > +
> >  enum {
> > OV8856_LINK_FREQ_720MBPS,
> > OV8856_LINK_FREQ_360MBPS,
> > @@ -316,6 +340,208 @@ static const struct ov8856_reg mode_3280x2464_regs[] 
> > = {
> > {0x5e00, 0x00}
> >  };
> >  
> > +static const struct ov8856_reg mode_3264x2448_regs[] = {
> > +   {0x0103, 0x01},
> > +   {0x0302, 0x3c},
> > +   {0x0303, 0x01},
> > +   {0x031e, 0x0c},
> > +   {0x3000, 0x00},
> > +   {0x300e, 0x00},
> > +   {0x3010, 0x00},
> > +   {0x3015, 0x84},
> > +   {0x3018, 0x72},
> > +   {0x3021, 0x23},
> > +   {0x3033, 0x24},
> > +   {0x3500, 0x00},
> > +   {0x3501, 0x9a},
> > +   {0x3502, 0x20},
> > +   {0x3503, 0x08},
> > +   {0x3505, 0x83},
> > +   {0x3508, 0x01},
> > +   {0x3509, 0x80},
> > +   {0x350c, 0x00},
> > +   {0x350d, 0x80},
> > +   {0x350e, 0x04},
> > +   {0x350f, 0x00},
> > +   {0x3510, 0x00},
> > +   {0x3511, 0x02},
> > +   {0x3512, 0x00},
> > +   {0x3600, 0x72},
> > +   {0x3601, 0x40},
> > +   {0x3602, 0x30},
> > +   {0x3610, 0xc5},
> > +   {0x3611, 0x58},
> > +   {0x3612, 0x5c},
> > +   {0x3613, 0xca},
> > +   {0x3614, 0x60},
> > +   {0x3628, 0xff},
> > +   {0x3629, 0xff},
> > +   {0x362a, 0xff},
> > +   {0x3633, 0x10},
> > +   {0x3634, 0x10},
> > +   {0x3635, 0x10},
> > +   {0x3636, 0x10},
> > +   {0x3663, 0x08},
> > +   {0x3669, 0x34},
> > +   {0x366d, 0x00},
> > +   {0x366e, 0x10},
> > +   {0x3706, 0x86},
> > +   {0x370b, 0x7e},
> > +   {0x3714, 0x23},
> > +   {0x3730, 0x12},
> > +   {0x3733, 0x10},
> > +   {0x3764, 0x00},
> > +   {0x3765, 0x00},
> > +   {0x3769, 0x62},
> > +   {0x376a, 0x2a},
> > +   {0x376b, 0x30},
> > +   {0x3780, 0x00},
> > +   {0x3781, 0x24},
> > +   {0x3782, 0x00},
> > +   {0x3783, 0x23},
> > +   {0x3798, 0x2f},
> > +   {0x37a1, 0x60},
> > +   {0x37a8, 0x6a},
> > +   {0x37ab, 0x3f},

Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor

2019-08-23 Thread Tomasz Figa
Hi Dongchun,

On Thu, Aug 08, 2019 at 05:22:15PM +0800, [email protected] wrote:
> From: Dongchun Zhu 
> 
> This patch mainly adds two more sensor modes for OV8856 image sensor.
> The OV8856 driver currently supports output format: 10-bit Raw,
> the resolution of 1632*1224 and 3264*2448, and the bayer order of BGGR.
> The hardware version also differs in some OTP regiser,
> as well as PLL register setting.
> 
> Signed-off-by: Dongchun Zhu 
> ---
>  drivers/media/i2c/ov8856.c | 624 
> -
>  1 file changed, 621 insertions(+), 3 deletions(-)
> 

Thanks for the patch! Please see my comments inline.

> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index cd347d6..e0610b6 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -1,12 +1,15 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (c) 2019 Intel Corporation.
>  
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -19,6 +22,7 @@
>  #define OV8856_LINK_FREQ_180MHZ  18000ULL
>  #define OV8856_SCLK  14400ULL
>  #define OV8856_MCLK  1920
> +#define OV8856_XVCLK_FREQ2400
>  #define OV8856_DATA_LANES4
>  #define OV8856_RGB_DEPTH 10
>  
> @@ -29,6 +33,18 @@
>  #define OV8856_MODE_STANDBY  0x00
>  #define OV8856_MODE_STREAMING0x01
>  
> +/* define 1B module */
> +#define OV8856_1B_MODULE 0x02
> +
> +/* otp sram register */
> +#define OV8856_OTP_REG   0x700f

This isn't a register. I believe the OTP read-out buffer is at 0x7000 and
0xf is the offset of the byte in the OTP that means the module revision.

> +#define OV8856_OTP_REG_ONE   0x3d84
> +#define OV8856_OTP_REG_TWO   0x3d81

These registers are definitely not "ONE" and "TWO". Please use proper
names as per the datasheet.

> +
> +/* clock register */
> +#define OV8856_CLK_REG   0x3614
> +#define OV8856_CLK_REG_1B_VAL0x20

Same here. These don't look like the real names of the register and bit
field.

> +
>  /* vertical-timings from sensor */
>  #define OV8856_REG_VTS   0x380e
>  #define OV8856_VTS_MAX   0x7fff
> @@ -64,6 +80,14 @@
>  
>  #define to_ov8856(_sd)   container_of(_sd, struct 
> ov8856, sd)
>  
> +static const char * const ov8856_supply_names[] = {
> + "dovdd",/* Digital I/O power */
> + "avdd", /* Analog power */
> + "dvdd", /* Digital core power */
> +};
> +
> +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> +
>  enum {
>   OV8856_LINK_FREQ_720MBPS,
>   OV8856_LINK_FREQ_360MBPS,
> @@ -316,6 +340,208 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
>   {0x5e00, 0x00}
>  };
>  
> +static const struct ov8856_reg mode_3264x2448_regs[] = {
> + {0x0103, 0x01},
> + {0x0302, 0x3c},
> + {0x0303, 0x01},
> + {0x031e, 0x0c},
> + {0x3000, 0x00},
> + {0x300e, 0x00},
> + {0x3010, 0x00},
> + {0x3015, 0x84},
> + {0x3018, 0x72},
> + {0x3021, 0x23},
> + {0x3033, 0x24},
> + {0x3500, 0x00},
> + {0x3501, 0x9a},
> + {0x3502, 0x20},
> + {0x3503, 0x08},
> + {0x3505, 0x83},
> + {0x3508, 0x01},
> + {0x3509, 0x80},
> + {0x350c, 0x00},
> + {0x350d, 0x80},
> + {0x350e, 0x04},
> + {0x350f, 0x00},
> + {0x3510, 0x00},
> + {0x3511, 0x02},
> + {0x3512, 0x00},
> + {0x3600, 0x72},
> + {0x3601, 0x40},
> + {0x3602, 0x30},
> + {0x3610, 0xc5},
> + {0x3611, 0x58},
> + {0x3612, 0x5c},
> + {0x3613, 0xca},
> + {0x3614, 0x60},
> + {0x3628, 0xff},
> + {0x3629, 0xff},
> + {0x362a, 0xff},
> + {0x3633, 0x10},
> + {0x3634, 0x10},
> + {0x3635, 0x10},
> + {0x3636, 0x10},
> + {0x3663, 0x08},
> + {0x3669, 0x34},
> + {0x366d, 0x00},
> + {0x366e, 0x10},
> + {0x3706, 0x86},
> + {0x370b, 0x7e},
> + {0x3714, 0x23},
> + {0x3730, 0x12},
> + {0x3733, 0x10},
> + {0x3764, 0x00},
> + {0x3765, 0x00},
> + {0x3769, 0x62},
> + {0x376a, 0x2a},
> + {0x376b, 0x30},
> + {0x3780, 0x00},
> + {0x3781, 0x24},
> + {0x3782, 0x00},
> + {0x3783, 0x23},
> + {0x3798, 0x2f},
> + {0x37a1, 0x60},
> + {0x37a8, 0x6a},
> + {0x37ab, 0x3f},
> + {0x37c2, 0x04},
> + {0x37c3, 0xf1},
> + {0x37c9, 0x80},
> + {0x37cb, 0x16},
> + {0x37cc, 0x16},
> + {0x37cd, 0x16},
> + {0x37ce, 0x16},
> + {0x3800, 0x00},
> + {0x3801, 0x00},
> + {0x3802, 0x00},
> + {0x3803, 0x0c},
> + {0x3804, 0x0c},
> + {0x3805, 0xdf},
> + {0x3806, 0x09},
> + {0x3807, 0xa3},
> + {0x3808, 0x0c},
> + {0x3809, 0xc0},
> + {0x380a, 0x09},
> + {0x380b, 0x90},
> + {0x

Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor

2019-08-08 Thread Sakari Ailus
Hi Dongchun,

Thanks for the patch.

On Thu, Aug 08, 2019 at 05:22:15PM +0800, [email protected] wrote:
> From: Dongchun Zhu 
> 
> This patch mainly adds two more sensor modes for OV8856 image sensor.
> The OV8856 driver currently supports output format: 10-bit Raw,
> the resolution of 1632*1224 and 3264*2448, and the bayer order of BGGR.
> The hardware version also differs in some OTP regiser,
> as well as PLL register setting.
> 
> Signed-off-by: Dongchun Zhu 
> ---
>  drivers/media/i2c/ov8856.c | 624 
> -
>  1 file changed, 621 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index cd347d6..e0610b6 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -1,12 +1,15 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (c) 2019 Intel Corporation.
>  
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -19,6 +22,7 @@
>  #define OV8856_LINK_FREQ_180MHZ  18000ULL
>  #define OV8856_SCLK  14400ULL
>  #define OV8856_MCLK  1920
> +#define OV8856_XVCLK_FREQ2400

The driver currenctly uses, perhaps misleadingly, OV8856_MCLK for this
purpose. You could rename the existing MCLK as XVCLK.

This also means the driver needs to differentiate configurations for 24 and
19,2 MHz which it currently does not do. I think it may make sense to make
this a separate patch from the rest.

>  #define OV8856_DATA_LANES4
>  #define OV8856_RGB_DEPTH 10
>  
> @@ -29,6 +33,18 @@
>  #define OV8856_MODE_STANDBY  0x00
>  #define OV8856_MODE_STREAMING0x01
>  
> +/* define 1B module */
> +#define OV8856_1B_MODULE 0x02
> +
> +/* otp sram register */
> +#define OV8856_OTP_REG   0x700f
> +#define OV8856_OTP_REG_ONE   0x3d84
> +#define OV8856_OTP_REG_TWO   0x3d81
> +
> +/* clock register */
> +#define OV8856_CLK_REG   0x3614
> +#define OV8856_CLK_REG_1B_VAL0x20
> +
>  /* vertical-timings from sensor */
>  #define OV8856_REG_VTS   0x380e
>  #define OV8856_VTS_MAX   0x7fff
> @@ -64,6 +80,14 @@
>  
>  #define to_ov8856(_sd)   container_of(_sd, struct 
> ov8856, sd)
>  
> +static const char * const ov8856_supply_names[] = {
> + "dovdd",/* Digital I/O power */
> + "avdd", /* Analog power */
> + "dvdd", /* Digital core power */
> +};
> +
> +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> +
>  enum {
>   OV8856_LINK_FREQ_720MBPS,
>   OV8856_LINK_FREQ_360MBPS,
> @@ -316,6 +340,208 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
>   {0x5e00, 0x00}
>  };
>  
> +static const struct ov8856_reg mode_3264x2448_regs[] = {
> + {0x0103, 0x01},
> + {0x0302, 0x3c},
> + {0x0303, 0x01},
> + {0x031e, 0x0c},
> + {0x3000, 0x00},
> + {0x300e, 0x00},
> + {0x3010, 0x00},
> + {0x3015, 0x84},
> + {0x3018, 0x72},
> + {0x3021, 0x23},
> + {0x3033, 0x24},
> + {0x3500, 0x00},
> + {0x3501, 0x9a},
> + {0x3502, 0x20},
> + {0x3503, 0x08},
> + {0x3505, 0x83},
> + {0x3508, 0x01},
> + {0x3509, 0x80},
> + {0x350c, 0x00},
> + {0x350d, 0x80},
> + {0x350e, 0x04},
> + {0x350f, 0x00},
> + {0x3510, 0x00},
> + {0x3511, 0x02},
> + {0x3512, 0x00},
> + {0x3600, 0x72},
> + {0x3601, 0x40},
> + {0x3602, 0x30},
> + {0x3610, 0xc5},
> + {0x3611, 0x58},
> + {0x3612, 0x5c},
> + {0x3613, 0xca},
> + {0x3614, 0x60},
> + {0x3628, 0xff},
> + {0x3629, 0xff},
> + {0x362a, 0xff},
> + {0x3633, 0x10},
> + {0x3634, 0x10},
> + {0x3635, 0x10},
> + {0x3636, 0x10},
> + {0x3663, 0x08},
> + {0x3669, 0x34},
> + {0x366d, 0x00},
> + {0x366e, 0x10},
> + {0x3706, 0x86},
> + {0x370b, 0x7e},
> + {0x3714, 0x23},
> + {0x3730, 0x12},
> + {0x3733, 0x10},
> + {0x3764, 0x00},
> + {0x3765, 0x00},
> + {0x3769, 0x62},
> + {0x376a, 0x2a},
> + {0x376b, 0x30},
> + {0x3780, 0x00},
> + {0x3781, 0x24},
> + {0x3782, 0x00},
> + {0x3783, 0x23},
> + {0x3798, 0x2f},
> + {0x37a1, 0x60},
> + {0x37a8, 0x6a},
> + {0x37ab, 0x3f},
> + {0x37c2, 0x04},
> + {0x37c3, 0xf1},
> + {0x37c9, 0x80},
> + {0x37cb, 0x16},
> + {0x37cc, 0x16},
> + {0x37cd, 0x16},
> + {0x37ce, 0x16},
> + {0x3800, 0x00},
> + {0x3801, 0x00},
> + {0x3802, 0x00},
> + {0x3803, 0x0c},
> + {0x3804, 0x0c},
> + {0x3805, 0xdf},
> + {0x3806, 0x09},
> + {0x3807, 0xa3},
> + {0x3808, 0x0c},
> + {0x3809, 0xc0},
> + {0x380a, 0x09},
> + {0x380b, 0x90},
> + {0x380c, 0x07},
> + {0x380d, 0x8c},
> + {