* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-09-10 13:04 ` [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor dongchun.zhu
@ 2019-09-10 17:44 ` Andy Shevchenko
2019-10-30 13:04 ` Dongchun Zhu
2019-09-11 10:12 ` Tomasz Figa
2019-09-13 7:55 ` Sakari Ailus
2 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2019-09-10 17:44 UTC (permalink / raw)
To: dongchun.zhu
Cc: mchehab, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga,
matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
On Tue, Sep 10, 2019 at 09:04:46PM +0800, dongchun.zhu@mediatek.com wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch mainly adds two more sensor modes for OV8856 CMOS image sensor.
> That is, the resolution of 1632*1224 and 3264*2448, corresponding to the bayer order of BGGR.
> The sensor revision also differs in some OTP register.
> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> + int ret;
> +
> + ret = clk_prepare_enable(ov8856->xvclk);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to enable xvclk\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> +
> + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to enable regulators\n");
> + goto disable_clk;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> +
> + usleep_range(1400, 1500);
This should be commented why this is needed and from where the requirement
comes from. Also, not, that 100us, which is only ~7%, is small margin.
Recommended one is ~20%.
> +
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(ov8856->xvclk);
> +
> + return ret;
> +}
> + ov8856->is_1B_revision = (val == OV8856_1B_MODULE) ? 1 : 0;
!! will give same result without using ternary operator.
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> + if (IS_ERR(ov8856->xvclk)) {
> + dev_err(&client->dev, "failed to get xvclk\n");
> + return -EINVAL;
> + }
Previously it was optional.
How did it work before and why it's not optional?
> + ov8856->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(ov8856->n_shutdn_gpio)) {
> + dev_err(&client->dev, "failed to get reset-gpios\n");
> + return -EINVAL;
> + }
Ditto.
> +static const struct of_device_id ov8856_of_match[] = {
> + { .compatible = "ovti,ov8856" },
> + {},
No comma needed for terminator line.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-09-10 17:44 ` Andy Shevchenko
@ 2019-10-30 13:04 ` Dongchun Zhu
2019-10-30 13:55 ` Sakari Ailus
0 siblings, 1 reply; 16+ messages in thread
From: Dongchun Zhu @ 2019-10-30 13:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: mchehab, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga,
matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
Hi Andy,
On Tue, 2019-09-10 at 20:44 +0300, Andy Shevchenko wrote:
> On Tue, Sep 10, 2019 at 09:04:46PM +0800, dongchun.zhu@mediatek.com wrote:
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> >
> > This patch mainly adds two more sensor modes for OV8856 CMOS image sensor.
> > That is, the resolution of 1632*1224 and 3264*2448, corresponding to the bayer order of BGGR.
> > The sensor revision also differs in some OTP register.
>
> > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(ov8856->xvclk);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to enable xvclk\n");
> > + return ret;
> > + }
> > +
> > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > +
> > + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to enable regulators\n");
> > + goto disable_clk;
> > + }
> > +
> > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> > +
>
> > + usleep_range(1400, 1500);
>
> This should be commented why this is needed and from where the requirement
> comes from. Also, not, that 100us, which is only ~7%, is small margin.
> Recommended one is ~20%.
>
Thanks for reminder.
This would be refined in next release.
> > +
> > + return 0;
> > +
> > +disable_clk:
> > + clk_disable_unprepare(ov8856->xvclk);
> > +
> > + return ret;
> > +}
>
> > + ov8856->is_1B_revision = (val == OV8856_1B_MODULE) ? 1 : 0;
>
> !! will give same result without using ternary operator.
>
Fixed in next release.
> > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > + if (IS_ERR(ov8856->xvclk)) {
> > + dev_err(&client->dev, "failed to get xvclk\n");
> > + return -EINVAL;
> > + }
>
> Previously it was optional.
> How did it work before and why it's not optional?
>
Previous vision for this driver is for ACPI, not ARM.
> > + ov8856->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(ov8856->n_shutdn_gpio)) {
> > + dev_err(&client->dev, "failed to get reset-gpios\n");
> > + return -EINVAL;
> > + }
>
> Ditto.
>
> > +static const struct of_device_id ov8856_of_match[] = {
> > + { .compatible = "ovti,ov8856" },
>
> > + {},
>
> No comma needed for terminator line.
>
Fixed in next release.
> > +};
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-10-30 13:04 ` Dongchun Zhu
@ 2019-10-30 13:55 ` Sakari Ailus
0 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2019-10-30 13:55 UTC (permalink / raw)
To: Dongchun Zhu
Cc: Andy Shevchenko, mchehab, robh+dt, mark.rutland, drinkcat, tfiga,
matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
On Wed, Oct 30, 2019 at 09:04:13PM +0800, Dongchun Zhu wrote:
> > > + ov8856->is_1B_revision = (val == OV8856_1B_MODULE) ? 1 : 0;
> >
> > !! will give same result without using ternary operator.
> >
>
> Fixed in next release.
But casting to bool will have the same effect. Integers are also not
needed. I.e. this can be written as:
ov8856->is_1B_revision = val == OV8856_1B_MODULE;
--
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-09-10 13:04 ` [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor dongchun.zhu
2019-09-10 17:44 ` Andy Shevchenko
@ 2019-09-11 10:12 ` Tomasz Figa
2019-09-11 11:43 ` Sakari Ailus
2019-09-13 7:55 ` Sakari Ailus
2 siblings, 1 reply; 16+ messages in thread
From: Tomasz Figa @ 2019-09-11 10:12 UTC (permalink / raw)
To: Sakari Ailus
Cc: Dongchun Zhu, Mauro Carvalho Chehab, andriy.shevchenko,
Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger,
Cao Bing Bu, srv_heupstream,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS
<iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,,
Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo,
shengnan.wang
Hi Sakari,
On Tue, Sep 10, 2019 at 10:05 PM <dongchun.zhu@mediatek.com> wrote:
>
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch mainly adds two more sensor modes for OV8856 CMOS image sensor.
> That is, the resolution of 1632*1224 and 3264*2448, corresponding to the bayer order of BGGR.
> The sensor revision also differs in some OTP register.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> drivers/media/i2c/ov8856.c | 654 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 639 insertions(+), 15 deletions(-)
>
What do you think about the approach taken by this patch?
My understanding is that the register arrays being added by it can be
only used with 24MHz input clock, while the existing ones are for
19.2MHz. That means that this patch makes the driver expose completely
different modes (resolutions, mbus formats) depending on the input
clock. Are we okay with this?
Best regards,
Tomasz
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index cd347d6..9ad0b73 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 <linux/clk.h>
> #include <asm/unaligned.h>
> #include <linux/acpi.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -18,10 +21,15 @@
> #define OV8856_LINK_FREQ_360MHZ 360000000ULL
> #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> #define OV8856_SCLK 144000000ULL
> -#define OV8856_MCLK 19200000
> +#define OV8856_XVCLK 19200000
> +#define OV8856_XVCLK_TYP 24000000
> #define OV8856_DATA_LANES 4
> #define OV8856_RGB_DEPTH 10
>
> +#define REG_X_ADDR_START 0x3808
> +#define X_OUTPUT_FULL_SIZE 0x0cc0
> +#define X_OUTPUT_BINNING_SIZE 0x0660
> +
> #define OV8856_REG_CHIP_ID 0x300a
> #define OV8856_CHIP_ID 0x00885a
>
> @@ -29,6 +37,22 @@
> #define OV8856_MODE_STANDBY 0x00
> #define OV8856_MODE_STREAMING 0x01
>
> +/* define 1B module revision */
> +#define OV8856_1B_MODULE 0x02
> +
> +/* 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_MODULE_REVISION 0x700f
> +#define OV8856_OTP_MODE_CTRL 0x3d84
> +#define OV8856_OTP_LOAD_CTRL 0x3d81
> +#define OV8856_OTP_MODE_AUTO 0x00
> +#define OV8856_OTP_LOAD_CTRL_ENABLE BIT(0)
> +
> +/* Analog control register that decided by module revision */
> +#define OV8856_ANAL_MODE_CTRL 0x3614
> +#define OV8856_ANAL_1B_VAL 0x20
> +
> /* vertical-timings from sensor */
> #define OV8856_REG_VTS 0x380e
> #define OV8856_VTS_MAX 0x7fff
> @@ -64,6 +88,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,
> @@ -195,11 +227,11 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> {0x3800, 0x00},
> {0x3801, 0x00},
> {0x3802, 0x00},
> - {0x3803, 0x06},
> + {0x3803, 0x07},
> {0x3804, 0x0c},
> {0x3805, 0xdf},
> {0x3806, 0x09},
> - {0x3807, 0xa7},
> + {0x3807, 0xa6},
> {0x3808, 0x0c},
> {0x3809, 0xd0},
> {0x380a, 0x09},
> @@ -211,7 +243,7 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> {0x3810, 0x00},
> {0x3811, 0x00},
> {0x3812, 0x00},
> - {0x3813, 0x01},
> + {0x3813, 0x00},
> {0x3814, 0x01},
> {0x3815, 0x01},
> {0x3816, 0x00},
> @@ -316,6 +348,209 @@ 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, 0x20},
> + {0x3003, 0x08},
> + {0x300e, 0x20},
> + {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},
> + {0x380e, 0x09},
> + {0x380f, 0xb2},
> + {0x3810, 0x00},
> + {0x3811, 0x04},
> + {0x3812, 0x00},
> + {0x3813, 0x02},
> + {0x3814, 0x01},
> + {0x3815, 0x01},
> + {0x3816, 0x00},
> + {0x3817, 0x00},
> + {0x3818, 0x00},
> + {0x3819, 0x00},
> + {0x3820, 0x80},
> + {0x3821, 0x46},
> + {0x382a, 0x01},
> + {0x382b, 0x01},
> + {0x3830, 0x06},
> + {0x3836, 0x02},
> + {0x3862, 0x04},
> + {0x3863, 0x08},
> + {0x3cc0, 0x33},
> + {0x3d85, 0x17},
> + {0x3d8c, 0x73},
> + {0x3d8d, 0xde},
> + {0x4001, 0xe0},
> + {0x4003, 0x40},
> + {0x4008, 0x00},
> + {0x4009, 0x0b},
> + {0x400a, 0x00},
> + {0x400b, 0x84},
> + {0x400f, 0x80},
> + {0x4010, 0xf0},
> + {0x4011, 0xff},
> + {0x4012, 0x02},
> + {0x4013, 0x01},
> + {0x4014, 0x01},
> + {0x4015, 0x01},
> + {0x4042, 0x00},
> + {0x4043, 0x80},
> + {0x4044, 0x00},
> + {0x4045, 0x80},
> + {0x4046, 0x00},
> + {0x4047, 0x80},
> + {0x4048, 0x00},
> + {0x4049, 0x80},
> + {0x4041, 0x03},
> + {0x404c, 0x20},
> + {0x404d, 0x00},
> + {0x404e, 0x20},
> + {0x4203, 0x80},
> + {0x4307, 0x30},
> + {0x4317, 0x00},
> + {0x4502, 0x50},
> + {0x4503, 0x08},
> + {0x4601, 0x80},
> + {0x4800, 0x44},
> + {0x4816, 0x53},
> + {0x481b, 0x50},
> + {0x481f, 0x27},
> + {0x4823, 0x3c},
> + {0x482b, 0x00},
> + {0x4831, 0x66},
> + {0x4837, 0x16},
> + {0x483c, 0x0f},
> + {0x484b, 0x05},
> + {0x5000, 0x77},
> + {0x5001, 0x0a},
> + {0x5003, 0xc8},
> + {0x5004, 0x04},
> + {0x5006, 0x00},
> + {0x5007, 0x00},
> + {0x502e, 0x03},
> + {0x5030, 0x41},
> + {0x5780, 0x14},
> + {0x5781, 0x0f},
> + {0x5782, 0x44},
> + {0x5783, 0x02},
> + {0x5784, 0x01},
> + {0x5785, 0x01},
> + {0x5786, 0x00},
> + {0x5787, 0x04},
> + {0x5788, 0x02},
> + {0x5789, 0x0f},
> + {0x578a, 0xfd},
> + {0x578b, 0xf5},
> + {0x578c, 0xf5},
> + {0x578d, 0x03},
> + {0x578e, 0x08},
> + {0x578f, 0x0c},
> + {0x5790, 0x08},
> + {0x5791, 0x04},
> + {0x5792, 0x00},
> + {0x5793, 0x52},
> + {0x5794, 0xa3},
> + {0x5795, 0x02},
> + {0x5796, 0x20},
> + {0x5797, 0x20},
> + {0x5798, 0xd5},
> + {0x5799, 0xd5},
> + {0x579a, 0x00},
> + {0x579b, 0x50},
> + {0x579c, 0x00},
> + {0x579d, 0x2c},
> + {0x579e, 0x0c},
> + {0x579f, 0x40},
> + {0x57a0, 0x09},
> + {0x57a1, 0x40},
> + {0x59f8, 0x3d},
> + {0x5a08, 0x02},
> + {0x5b00, 0x02},
> + {0x5b01, 0x10},
> + {0x5b02, 0x03},
> + {0x5b03, 0xcf},
> + {0x5b05, 0x6c},
> + {0x5e00, 0x00},
> + {0x5e10, 0xfc}
> +};
> +
> static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x3000, 0x20},
> {0x3003, 0x08},
> @@ -385,11 +620,11 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x3800, 0x00},
> {0x3801, 0x00},
> {0x3802, 0x00},
> - {0x3803, 0x06},
> + {0x3803, 0x07},
> {0x3804, 0x0c},
> {0x3805, 0xdf},
> {0x3806, 0x09},
> - {0x3807, 0xa7},
> + {0x3807, 0xa6},
> {0x3808, 0x06},
> {0x3809, 0x68},
> {0x380a, 0x04},
> @@ -401,7 +636,7 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x3810, 0x00},
> {0x3811, 0x00},
> {0x3812, 0x00},
> - {0x3813, 0x01},
> + {0x3813, 0x00},
> {0x3814, 0x03},
> {0x3815, 0x01},
> {0x3816, 0x00},
> @@ -506,6 +741,209 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x5e00, 0x00}
> };
>
> +static const struct ov8856_reg mode_1632x1224_regs[] = {
> + {0x0103, 0x01},
> + {0x0302, 0x3c},
> + {0x0303, 0x01},
> + {0x031e, 0x0c},
> + {0x3000, 0x20},
> + {0x3003, 0x08},
> + {0x300e, 0x20},
> + {0x3010, 0x00},
> + {0x3015, 0x84},
> + {0x3018, 0x72},
> + {0x3021, 0x23},
> + {0x3033, 0x24},
> + {0x3500, 0x00},
> + {0x3501, 0x4c},
> + {0x3502, 0xe0},
> + {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, 0x08},
> + {0x3706, 0x86},
> + {0x370b, 0x7e},
> + {0x3714, 0x27},
> + {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, 0x14},
> + {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, 0x06},
> + {0x3809, 0x60},
> + {0x380a, 0x04},
> + {0x380b, 0xc8},
> + {0x380c, 0x07},
> + {0x380d, 0x8c},
> + {0x380e, 0x09},
> + {0x380f, 0xb2},
> + {0x3810, 0x00},
> + {0x3811, 0x02},
> + {0x3812, 0x00},
> + {0x3813, 0x02},
> + {0x3814, 0x03},
> + {0x3815, 0x01},
> + {0x3816, 0x00},
> + {0x3817, 0x00},
> + {0x3818, 0x00},
> + {0x3819, 0x00},
> + {0x3820, 0x80},
> + {0x3821, 0x47},
> + {0x382a, 0x03},
> + {0x382b, 0x01},
> + {0x3830, 0x06},
> + {0x3836, 0x02},
> + {0x3862, 0x04},
> + {0x3863, 0x08},
> + {0x3cc0, 0x33},
> + {0x3d85, 0x17},
> + {0x3d8c, 0x73},
> + {0x3d8d, 0xde},
> + {0x4001, 0xe0},
> + {0x4003, 0x40},
> + {0x4008, 0x00},
> + {0x4009, 0x05},
> + {0x400a, 0x00},
> + {0x400b, 0x84},
> + {0x400f, 0x80},
> + {0x4010, 0xf0},
> + {0x4011, 0xff},
> + {0x4012, 0x02},
> + {0x4013, 0x01},
> + {0x4014, 0x01},
> + {0x4015, 0x01},
> + {0x4042, 0x00},
> + {0x4043, 0x80},
> + {0x4044, 0x00},
> + {0x4045, 0x80},
> + {0x4046, 0x00},
> + {0x4047, 0x80},
> + {0x4048, 0x00},
> + {0x4049, 0x80},
> + {0x4041, 0x03},
> + {0x404c, 0x20},
> + {0x404d, 0x00},
> + {0x404e, 0x20},
> + {0x4203, 0x80},
> + {0x4307, 0x30},
> + {0x4317, 0x00},
> + {0x4502, 0x50},
> + {0x4503, 0x08},
> + {0x4601, 0x80},
> + {0x4800, 0x44},
> + {0x4816, 0x53},
> + {0x481b, 0x50},
> + {0x481f, 0x27},
> + {0x4823, 0x3c},
> + {0x482b, 0x00},
> + {0x4831, 0x66},
> + {0x4837, 0x16},
> + {0x483c, 0x0f},
> + {0x484b, 0x05},
> + {0x5000, 0x77},
> + {0x5001, 0x0a},
> + {0x5003, 0xc8},
> + {0x5004, 0x04},
> + {0x5006, 0x00},
> + {0x5007, 0x00},
> + {0x502e, 0x03},
> + {0x5030, 0x41},
> + {0x5795, 0x00},
> + {0x5796, 0x10},
> + {0x5797, 0x10},
> + {0x5798, 0x73},
> + {0x5799, 0x73},
> + {0x579a, 0x00},
> + {0x579b, 0x28},
> + {0x579c, 0x00},
> + {0x579d, 0x16},
> + {0x579e, 0x06},
> + {0x579f, 0x20},
> + {0x57a0, 0x04},
> + {0x57a1, 0xa0},
> + {0x5780, 0x14},
> + {0x5781, 0x0f},
> + {0x5782, 0x44},
> + {0x5783, 0x02},
> + {0x5784, 0x01},
> + {0x5785, 0x01},
> + {0x5786, 0x00},
> + {0x5787, 0x04},
> + {0x5788, 0x02},
> + {0x5789, 0x0f},
> + {0x578a, 0xfd},
> + {0x578b, 0xf5},
> + {0x578c, 0xf5},
> + {0x578d, 0x03},
> + {0x578e, 0x08},
> + {0x578f, 0x0c},
> + {0x5790, 0x08},
> + {0x5791, 0x04},
> + {0x5792, 0x00},
> + {0x5793, 0x52},
> + {0x5794, 0xa3},
> + {0x59f8, 0x3d},
> + {0x5a08, 0x02},
> + {0x5b00, 0x02},
> + {0x5b01, 0x10},
> + {0x5b02, 0x03},
> + {0x5b03, 0xcf},
> + {0x5b05, 0x6c},
> + {0x5e00, 0x00},
> + {0x5e10, 0xfc}
> +};
> +
> static const char * const ov8856_test_pattern_menu[] = {
> "Disabled",
> "Standard Color Bar",
> @@ -548,6 +986,18 @@ static const struct ov8856_mode supported_modes[] = {
> .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> },
> {
> + .width = 3264,
> + .height = 2448,
> + .hts = 1932,
> + .vts_def = 2482,
> + .vts_min = 2482,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_3264x2448_regs),
> + .regs = mode_3264x2448_regs,
> + },
> + .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> + },
> + {
> .width = 1640,
> .height = 1232,
> .hts = 3820,
> @@ -558,6 +1008,18 @@ static const struct ov8856_mode supported_modes[] = {
> .regs = mode_1640x1232_regs,
> },
> .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> + },
> + {
> + .width = 1632,
> + .height = 1224,
> + .hts = 1932,
> + .vts_def = 2482,
> + .vts_min = 2482,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_1632x1224_regs),
> + .regs = mode_1632x1224_regs,
> + },
> + .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> }
> };
>
> @@ -566,16 +1028,28 @@ struct ov8856 {
> struct media_pad pad;
> struct v4l2_ctrl_handler ctrl_handler;
>
> + struct clk *xvclk;
> + struct gpio_desc *n_shutdn_gpio;
> + struct regulator_bulk_data supplies[OV8856_NUM_SUPPLIES];
> +
> /* V4L2 Controls */
> struct v4l2_ctrl *link_freq;
> struct v4l2_ctrl *pixel_rate;
> struct v4l2_ctrl *vblank;
> struct v4l2_ctrl *hblank;
> struct v4l2_ctrl *exposure;
> + struct v4l2_mbus_framefmt fmt;
>
> /* Current mode */
> const struct ov8856_mode *cur_mode;
>
> + /* module hardware version that can be read out from register 0x700f
> + * the register value corresponds to different hardware version
> + * 01: 2A module revision
> + * 02: 1B module revision
> + */
> + bool is_1B_revision;
> +
> /* To serialize asynchronus callbacks */
> struct mutex mutex;
>
> @@ -696,6 +1170,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern)
> OV8856_REG_VALUE_08BIT, pattern);
> }
>
> +static int ov8856_check_revision(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_MODE_CTRL,
> + OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
> + if (ret)
> + return ret;
> +
> + return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
> + OV8856_REG_VALUE_08BIT,
> + OV8856_OTP_LOAD_CTRL_ENABLE);
> +}
> +
> static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov8856 *ov8856 = container_of(ctrl->handler,
> @@ -825,7 +1318,6 @@ static void ov8856_update_pad_format(const struct ov8856_mode *mode,
> {
> fmt->width = mode->width;
> fmt->height = mode->height;
> - fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> fmt->field = V4L2_FIELD_NONE;
> }
>
> @@ -834,6 +1326,7 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> const struct ov8856_reg_list *reg_list;
> int link_freq_index, ret;
> + u32 h_size;
>
> link_freq_index = ov8856->cur_mode->link_freq_index;
> reg_list = &link_freq_configs[link_freq_index].reg_list;
> @@ -850,6 +1343,29 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> return ret;
> }
>
> + /* Update R3614 if the revision is 1B module */
> + if (ov8856->is_1B_revision) {
> + ret = ov8856_write_reg(ov8856, OV8856_ANAL_MODE_CTRL,
> + OV8856_REG_VALUE_08BIT,
> + OV8856_ANAL_1B_VAL);
> + if (ret) {
> + dev_err(&client->dev, "failed to set R3614");
> + return ret;
> + }
> + }
> +
> + ret = ov8856_read_reg(ov8856, REG_X_ADDR_START,
> + OV8856_REG_VALUE_16BIT, &h_size);
> + if (ret) {
> + dev_err(&client->dev, "failed to read out R3614");
> + return ret;
> + }
> +
> + if (h_size == X_OUTPUT_FULL_SIZE || h_size == X_OUTPUT_BINNING_SIZE)
> + ov8856->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler);
> if (ret)
> return ret;
> @@ -878,6 +1394,7 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> struct ov8856 *ov8856 = to_ov8856(sd);
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> int ret = 0;
> + u32 val;
>
> if (ov8856->streaming == enable)
> return 0;
> @@ -908,6 +1425,44 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> return ret;
> }
>
> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> + int ret;
> +
> + ret = clk_prepare_enable(ov8856->xvclk);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to enable xvclk\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> +
> + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to enable regulators\n");
> + goto disable_clk;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> +
> + usleep_range(1400, 1500);
> +
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(ov8856->xvclk);
> +
> + return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 1);
> + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> + clk_disable_unprepare(ov8856->xvclk);
> +}
> +
> static int __maybe_unused ov8856_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -951,6 +1506,7 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *fmt)
> {
> struct ov8856 *ov8856 = to_ov8856(sd);
> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> const struct ov8856_mode *mode;
> s32 vblank_def, h_blank;
>
> @@ -960,7 +1516,9 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> fmt->format.height);
>
> mutex_lock(&ov8856->mutex);
> - ov8856_update_pad_format(mode, &fmt->format);
> + mbus_fmt->code = ov8856->fmt.code;
> + ov8856_update_pad_format(mode, mbus_fmt);
> + ov8856->fmt = fmt->format;
> if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> } else {
> @@ -992,13 +1550,17 @@ static int ov8856_get_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *fmt)
> {
> struct ov8856 *ov8856 = to_ov8856(sd);
> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
>
> mutex_lock(&ov8856->mutex);
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> fmt->format = *v4l2_subdev_get_try_format(&ov8856->sd, cfg,
> fmt->pad);
> - else
> - ov8856_update_pad_format(ov8856->cur_mode, &fmt->format);
> + } else {
> + fmt->format = ov8856->fmt;
> + ov8856_update_pad_format(ov8856->cur_mode, mbus_fmt);
> + mbus_fmt->code = ov8856->fmt.code;
> + }
>
> mutex_unlock(&ov8856->mutex);
>
> @@ -1009,11 +1571,12 @@ static int ov8856_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> - /* Only one bayer order GRBG is supported */
> + struct ov8856 *ov8856 = to_ov8856(sd);
> +
> if (code->index > 0)
> return -EINVAL;
>
> - code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> + code->code = ov8856->fmt.code;
>
> return 0;
> }
> @@ -1089,6 +1652,20 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> return -ENXIO;
> }
>
> + /* check sensor hardware revision */
> + ret = ov8856_check_revision(ov8856);
> + if (ret) {
> + dev_err(&client->dev, "failed to check sensor revision");
> + return ret;
> + }
> +
> + ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
> + OV8856_REG_VALUE_08BIT, &val);
> + if (ret)
> + return ret;
> +
> + ov8856->is_1B_revision = (val == OV8856_1B_MODULE) ? 1 : 0;
> +
> return 0;
> }
>
> @@ -1107,7 +1684,7 @@ static int ov8856_check_hwcfg(struct device *dev)
> return -ENXIO;
>
> fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> - if (mclk != OV8856_MCLK) {
> + if (mclk != OV8856_XVCLK) {
> dev_err(dev, "external clock %d is not supported", mclk);
> return -EINVAL;
> }
> @@ -1164,6 +1741,9 @@ static int ov8856_remove(struct i2c_client *client)
> media_entity_cleanup(&sd->entity);
> v4l2_ctrl_handler_free(sd->ctrl_handler);
> pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev))
> + __ov8856_power_off(ov8856);
> + pm_runtime_set_suspended(&client->dev);
> mutex_destroy(&ov8856->mutex);
>
> return 0;
> @@ -1172,6 +1752,7 @@ static int ov8856_remove(struct i2c_client *client)
> static int ov8856_probe(struct i2c_client *client)
> {
> struct ov8856 *ov8856;
> + unsigned int i;
> int ret;
>
> ret = ov8856_check_hwcfg(&client->dev);
> @@ -1186,6 +1767,42 @@ static int ov8856_probe(struct i2c_client *client)
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> + ov8856->fmt.code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> + if (IS_ERR(ov8856->xvclk)) {
> + dev_err(&client->dev, "failed to get xvclk\n");
> + return -EINVAL;
> + }
> +
> + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_TYP);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> + return ret;
> + }
> + if (clk_get_rate(ov8856->xvclk) != OV8856_XVCLK_TYP)
> + dev_warn(&client->dev,
> + "xvclk mismatched, modes are based on 24MHz\n");
> +
> + ov8856->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(ov8856->n_shutdn_gpio)) {
> + dev_err(&client->dev, "failed to get reset-gpios\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < OV8856_NUM_SUPPLIES; i++)
> + ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev, OV8856_NUM_SUPPLIES,
> + ov8856->supplies);
> + if (ret)
> + dev_warn(&client->dev, "failed to get regulators\n");
> +
> + ret = __ov8856_power_on(ov8856);
> + if (ret)
> + dev_warn(&client->dev, "failed to power on\n");
> +
> ret = ov8856_identify_module(ov8856);
> if (ret) {
> dev_err(&client->dev, "failed to find sensor: %d", ret);
> @@ -1251,11 +1868,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> #endif
>
> +static const struct of_device_id ov8856_of_match[] = {
> + { .compatible = "ovti,ov8856" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +
> static struct i2c_driver ov8856_i2c_driver = {
> .driver = {
> .name = "ov8856",
> .pm = &ov8856_pm_ops,
> .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> + .of_match_table = ov8856_of_match,
> },
> .probe_new = ov8856_probe,
> .remove = ov8856_remove,
> --
> 2.9.2
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-09-11 10:12 ` Tomasz Figa
@ 2019-09-11 11:43 ` Sakari Ailus
0 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2019-09-11 11:43 UTC (permalink / raw)
To: Tomasz Figa
Cc: Dongchun Zhu, Mauro Carvalho Chehab, andriy.shevchenko,
Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger,
Cao Bing Bu, srv_heupstream,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS
<iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,,
Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo,
shengnan.wang
Hi Tomasz,
On Wed, Sep 11, 2019 at 07:12:02PM +0900, Tomasz Figa wrote:
> Hi Sakari,
>
> On Tue, Sep 10, 2019 at 10:05 PM <dongchun.zhu@mediatek.com> wrote:
> >
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> >
> > This patch mainly adds two more sensor modes for OV8856 CMOS image sensor.
> > That is, the resolution of 1632*1224 and 3264*2448, corresponding to the bayer order of BGGR.
> > The sensor revision also differs in some OTP register.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> > drivers/media/i2c/ov8856.c | 654 +++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 639 insertions(+), 15 deletions(-)
> >
>
> What do you think about the approach taken by this patch?
>
> My understanding is that the register arrays being added by it can be
> only used with 24MHz input clock, while the existing ones are for
> 19.2MHz. That means that this patch makes the driver expose completely
> different modes (resolutions, mbus formats) depending on the input
> clock. Are we okay with this?
These register list based drivers only support a tiny subset of
configurations a sensor can support, and the number of those configurations
may be amended over time.
I don't see a problem in choosing a different set of available
configurations based on the external clock frequency; that may, after all,
cause that some of the configurations, at a particular frame rate, are not
even achievable --- albeit this is perhaps unlikely in this case.
In practice, it's often the case that the sensor vendor provides these
configurations and the vendor may provide different configurations
(including output resolutions etc.) to different parties. So it may well be
the submitter of the patch would also not have access to similar
configurations (output size, cropping etc.) that now exist in the driver.
I'll review the patch itself soonish.
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-09-10 13:04 ` [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor dongchun.zhu
2019-09-10 17:44 ` Andy Shevchenko
2019-09-11 10:12 ` Tomasz Figa
@ 2019-09-13 7:55 ` Sakari Ailus
2019-10-30 13:02 ` Dongchun Zhu
2 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2019-09-13 7:55 UTC (permalink / raw)
To: dongchun.zhu
Cc: mchehab, andriy.shevchenko, robh+dt, mark.rutland, drinkcat,
tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
Hi Dongchun,
On Tue, Sep 10, 2019 at 09:04:46PM +0800, dongchun.zhu@mediatek.com wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch mainly adds two more sensor modes for OV8856 CMOS image sensor.
> That is, the resolution of 1632*1224 and 3264*2448, corresponding to the bayer order of BGGR.
> The sensor revision also differs in some OTP register.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> drivers/media/i2c/ov8856.c | 654 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 639 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index cd347d6..9ad0b73 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 <linux/clk.h>
> #include <asm/unaligned.h>
> #include <linux/acpi.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -18,10 +21,15 @@
> #define OV8856_LINK_FREQ_360MHZ 360000000ULL
> #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> #define OV8856_SCLK 144000000ULL
> -#define OV8856_MCLK 19200000
> +#define OV8856_XVCLK 19200000
> +#define OV8856_XVCLK_TYP 24000000
How about calling these e.g. OV8856_XCLK_19_2 and OV8856_CLK_24? Or just
use the plain numbers; there's nothing really special about them.
Ideally you'd have both in a single location in the driver --- an array of
modes supported on each of them as they're different.
> #define OV8856_DATA_LANES 4
> #define OV8856_RGB_DEPTH 10
>
> +#define REG_X_ADDR_START 0x3808
> +#define X_OUTPUT_FULL_SIZE 0x0cc0
> +#define X_OUTPUT_BINNING_SIZE 0x0660
> +
> #define OV8856_REG_CHIP_ID 0x300a
> #define OV8856_CHIP_ID 0x00885a
>
> @@ -29,6 +37,22 @@
> #define OV8856_MODE_STANDBY 0x00
> #define OV8856_MODE_STREAMING 0x01
>
> +/* define 1B module revision */
> +#define OV8856_1B_MODULE 0x02
> +
> +/* 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_MODULE_REVISION 0x700f
> +#define OV8856_OTP_MODE_CTRL 0x3d84
> +#define OV8856_OTP_LOAD_CTRL 0x3d81
> +#define OV8856_OTP_MODE_AUTO 0x00
> +#define OV8856_OTP_LOAD_CTRL_ENABLE BIT(0)
> +
> +/* Analog control register that decided by module revision */
> +#define OV8856_ANAL_MODE_CTRL 0x3614
> +#define OV8856_ANAL_1B_VAL 0x20
> +
> /* vertical-timings from sensor */
> #define OV8856_REG_VTS 0x380e
> #define OV8856_VTS_MAX 0x7fff
> @@ -64,6 +88,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,
> @@ -195,11 +227,11 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> {0x3800, 0x00},
> {0x3801, 0x00},
> {0x3802, 0x00},
> - {0x3803, 0x06},
> + {0x3803, 0x07},
> {0x3804, 0x0c},
> {0x3805, 0xdf},
> {0x3806, 0x09},
> - {0x3807, 0xa7},
> + {0x3807, 0xa6},
This is changing an existing sensor mode. What are the effects of the
change?
Given that this register is not documented in the driver in any way, I'd
expect the commit message to carry some reasoning behind the change, also
suggesting it belongs to a separate patch. The same applies to changes to
the other existing modes.
> {0x3808, 0x0c},
> {0x3809, 0xd0},
> {0x380a, 0x09},
> @@ -211,7 +243,7 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> {0x3810, 0x00},
> {0x3811, 0x00},
> {0x3812, 0x00},
> - {0x3813, 0x01},
> + {0x3813, 0x00},
> {0x3814, 0x01},
> {0x3815, 0x01},
> {0x3816, 0x00},
> @@ -316,6 +348,209 @@ 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, 0x20},
> + {0x3003, 0x08},
> + {0x300e, 0x20},
> + {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},
> + {0x380e, 0x09},
> + {0x380f, 0xb2},
> + {0x3810, 0x00},
> + {0x3811, 0x04},
> + {0x3812, 0x00},
> + {0x3813, 0x02},
> + {0x3814, 0x01},
> + {0x3815, 0x01},
> + {0x3816, 0x00},
> + {0x3817, 0x00},
> + {0x3818, 0x00},
> + {0x3819, 0x00},
> + {0x3820, 0x80},
> + {0x3821, 0x46},
> + {0x382a, 0x01},
> + {0x382b, 0x01},
> + {0x3830, 0x06},
> + {0x3836, 0x02},
> + {0x3862, 0x04},
> + {0x3863, 0x08},
> + {0x3cc0, 0x33},
> + {0x3d85, 0x17},
> + {0x3d8c, 0x73},
> + {0x3d8d, 0xde},
> + {0x4001, 0xe0},
> + {0x4003, 0x40},
> + {0x4008, 0x00},
> + {0x4009, 0x0b},
> + {0x400a, 0x00},
> + {0x400b, 0x84},
> + {0x400f, 0x80},
> + {0x4010, 0xf0},
> + {0x4011, 0xff},
> + {0x4012, 0x02},
> + {0x4013, 0x01},
> + {0x4014, 0x01},
> + {0x4015, 0x01},
> + {0x4042, 0x00},
> + {0x4043, 0x80},
> + {0x4044, 0x00},
> + {0x4045, 0x80},
> + {0x4046, 0x00},
> + {0x4047, 0x80},
> + {0x4048, 0x00},
> + {0x4049, 0x80},
> + {0x4041, 0x03},
> + {0x404c, 0x20},
> + {0x404d, 0x00},
> + {0x404e, 0x20},
> + {0x4203, 0x80},
> + {0x4307, 0x30},
> + {0x4317, 0x00},
> + {0x4502, 0x50},
> + {0x4503, 0x08},
> + {0x4601, 0x80},
> + {0x4800, 0x44},
> + {0x4816, 0x53},
> + {0x481b, 0x50},
> + {0x481f, 0x27},
> + {0x4823, 0x3c},
> + {0x482b, 0x00},
> + {0x4831, 0x66},
> + {0x4837, 0x16},
> + {0x483c, 0x0f},
> + {0x484b, 0x05},
> + {0x5000, 0x77},
> + {0x5001, 0x0a},
> + {0x5003, 0xc8},
> + {0x5004, 0x04},
> + {0x5006, 0x00},
> + {0x5007, 0x00},
> + {0x502e, 0x03},
> + {0x5030, 0x41},
> + {0x5780, 0x14},
> + {0x5781, 0x0f},
> + {0x5782, 0x44},
> + {0x5783, 0x02},
> + {0x5784, 0x01},
> + {0x5785, 0x01},
> + {0x5786, 0x00},
> + {0x5787, 0x04},
> + {0x5788, 0x02},
> + {0x5789, 0x0f},
> + {0x578a, 0xfd},
> + {0x578b, 0xf5},
> + {0x578c, 0xf5},
> + {0x578d, 0x03},
> + {0x578e, 0x08},
> + {0x578f, 0x0c},
> + {0x5790, 0x08},
> + {0x5791, 0x04},
> + {0x5792, 0x00},
> + {0x5793, 0x52},
> + {0x5794, 0xa3},
> + {0x5795, 0x02},
> + {0x5796, 0x20},
> + {0x5797, 0x20},
> + {0x5798, 0xd5},
> + {0x5799, 0xd5},
> + {0x579a, 0x00},
> + {0x579b, 0x50},
> + {0x579c, 0x00},
> + {0x579d, 0x2c},
> + {0x579e, 0x0c},
> + {0x579f, 0x40},
> + {0x57a0, 0x09},
> + {0x57a1, 0x40},
> + {0x59f8, 0x3d},
> + {0x5a08, 0x02},
> + {0x5b00, 0x02},
> + {0x5b01, 0x10},
> + {0x5b02, 0x03},
> + {0x5b03, 0xcf},
> + {0x5b05, 0x6c},
> + {0x5e00, 0x00},
> + {0x5e10, 0xfc}
> +};
> +
> static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x3000, 0x20},
> {0x3003, 0x08},
> @@ -385,11 +620,11 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x3800, 0x00},
> {0x3801, 0x00},
> {0x3802, 0x00},
> - {0x3803, 0x06},
> + {0x3803, 0x07},
> {0x3804, 0x0c},
> {0x3805, 0xdf},
> {0x3806, 0x09},
> - {0x3807, 0xa7},
> + {0x3807, 0xa6},
> {0x3808, 0x06},
> {0x3809, 0x68},
> {0x380a, 0x04},
> @@ -401,7 +636,7 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x3810, 0x00},
> {0x3811, 0x00},
> {0x3812, 0x00},
> - {0x3813, 0x01},
> + {0x3813, 0x00},
> {0x3814, 0x03},
> {0x3815, 0x01},
> {0x3816, 0x00},
> @@ -506,6 +741,209 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x5e00, 0x00}
> };
>
> +static const struct ov8856_reg mode_1632x1224_regs[] = {
> + {0x0103, 0x01},
> + {0x0302, 0x3c},
> + {0x0303, 0x01},
> + {0x031e, 0x0c},
> + {0x3000, 0x20},
> + {0x3003, 0x08},
> + {0x300e, 0x20},
> + {0x3010, 0x00},
> + {0x3015, 0x84},
> + {0x3018, 0x72},
> + {0x3021, 0x23},
> + {0x3033, 0x24},
> + {0x3500, 0x00},
> + {0x3501, 0x4c},
> + {0x3502, 0xe0},
> + {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, 0x08},
> + {0x3706, 0x86},
> + {0x370b, 0x7e},
> + {0x3714, 0x27},
> + {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, 0x14},
> + {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, 0x06},
> + {0x3809, 0x60},
> + {0x380a, 0x04},
> + {0x380b, 0xc8},
> + {0x380c, 0x07},
> + {0x380d, 0x8c},
> + {0x380e, 0x09},
> + {0x380f, 0xb2},
> + {0x3810, 0x00},
> + {0x3811, 0x02},
> + {0x3812, 0x00},
> + {0x3813, 0x02},
> + {0x3814, 0x03},
> + {0x3815, 0x01},
> + {0x3816, 0x00},
> + {0x3817, 0x00},
> + {0x3818, 0x00},
> + {0x3819, 0x00},
> + {0x3820, 0x80},
> + {0x3821, 0x47},
> + {0x382a, 0x03},
> + {0x382b, 0x01},
> + {0x3830, 0x06},
> + {0x3836, 0x02},
> + {0x3862, 0x04},
> + {0x3863, 0x08},
> + {0x3cc0, 0x33},
> + {0x3d85, 0x17},
> + {0x3d8c, 0x73},
> + {0x3d8d, 0xde},
> + {0x4001, 0xe0},
> + {0x4003, 0x40},
> + {0x4008, 0x00},
> + {0x4009, 0x05},
> + {0x400a, 0x00},
> + {0x400b, 0x84},
> + {0x400f, 0x80},
> + {0x4010, 0xf0},
> + {0x4011, 0xff},
> + {0x4012, 0x02},
> + {0x4013, 0x01},
> + {0x4014, 0x01},
> + {0x4015, 0x01},
> + {0x4042, 0x00},
> + {0x4043, 0x80},
> + {0x4044, 0x00},
> + {0x4045, 0x80},
> + {0x4046, 0x00},
> + {0x4047, 0x80},
> + {0x4048, 0x00},
> + {0x4049, 0x80},
> + {0x4041, 0x03},
> + {0x404c, 0x20},
> + {0x404d, 0x00},
> + {0x404e, 0x20},
> + {0x4203, 0x80},
> + {0x4307, 0x30},
> + {0x4317, 0x00},
> + {0x4502, 0x50},
> + {0x4503, 0x08},
> + {0x4601, 0x80},
> + {0x4800, 0x44},
> + {0x4816, 0x53},
> + {0x481b, 0x50},
> + {0x481f, 0x27},
> + {0x4823, 0x3c},
> + {0x482b, 0x00},
> + {0x4831, 0x66},
> + {0x4837, 0x16},
> + {0x483c, 0x0f},
> + {0x484b, 0x05},
> + {0x5000, 0x77},
> + {0x5001, 0x0a},
> + {0x5003, 0xc8},
> + {0x5004, 0x04},
> + {0x5006, 0x00},
> + {0x5007, 0x00},
> + {0x502e, 0x03},
> + {0x5030, 0x41},
> + {0x5795, 0x00},
> + {0x5796, 0x10},
> + {0x5797, 0x10},
> + {0x5798, 0x73},
> + {0x5799, 0x73},
> + {0x579a, 0x00},
> + {0x579b, 0x28},
> + {0x579c, 0x00},
> + {0x579d, 0x16},
> + {0x579e, 0x06},
> + {0x579f, 0x20},
> + {0x57a0, 0x04},
> + {0x57a1, 0xa0},
> + {0x5780, 0x14},
> + {0x5781, 0x0f},
> + {0x5782, 0x44},
> + {0x5783, 0x02},
> + {0x5784, 0x01},
> + {0x5785, 0x01},
> + {0x5786, 0x00},
> + {0x5787, 0x04},
> + {0x5788, 0x02},
> + {0x5789, 0x0f},
> + {0x578a, 0xfd},
> + {0x578b, 0xf5},
> + {0x578c, 0xf5},
> + {0x578d, 0x03},
> + {0x578e, 0x08},
> + {0x578f, 0x0c},
> + {0x5790, 0x08},
> + {0x5791, 0x04},
> + {0x5792, 0x00},
> + {0x5793, 0x52},
> + {0x5794, 0xa3},
> + {0x59f8, 0x3d},
> + {0x5a08, 0x02},
> + {0x5b00, 0x02},
> + {0x5b01, 0x10},
> + {0x5b02, 0x03},
> + {0x5b03, 0xcf},
> + {0x5b05, 0x6c},
> + {0x5e00, 0x00},
> + {0x5e10, 0xfc}
> +};
> +
> static const char * const ov8856_test_pattern_menu[] = {
> "Disabled",
> "Standard Color Bar",
> @@ -548,6 +986,18 @@ static const struct ov8856_mode supported_modes[] = {
> .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> },
> {
> + .width = 3264,
> + .height = 2448,
> + .hts = 1932,
> + .vts_def = 2482,
> + .vts_min = 2482,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_3264x2448_regs),
> + .regs = mode_3264x2448_regs,
> + },
> + .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> + },
How do you guarantee that the modes are only used on the xvclk frequency
they're intended for?
I think it'd be best if you added the new modes in a separete patch.
> + {
> .width = 1640,
> .height = 1232,
> .hts = 3820,
> @@ -558,6 +1008,18 @@ static const struct ov8856_mode supported_modes[] = {
> .regs = mode_1640x1232_regs,
> },
> .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> + },
> + {
> + .width = 1632,
> + .height = 1224,
> + .hts = 1932,
> + .vts_def = 2482,
> + .vts_min = 2482,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_1632x1224_regs),
> + .regs = mode_1632x1224_regs,
> + },
> + .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> }
> };
>
> @@ -566,16 +1028,28 @@ struct ov8856 {
> struct media_pad pad;
> struct v4l2_ctrl_handler ctrl_handler;
>
> + struct clk *xvclk;
> + struct gpio_desc *n_shutdn_gpio;
> + struct regulator_bulk_data supplies[OV8856_NUM_SUPPLIES];
> +
> /* V4L2 Controls */
> struct v4l2_ctrl *link_freq;
> struct v4l2_ctrl *pixel_rate;
> struct v4l2_ctrl *vblank;
> struct v4l2_ctrl *hblank;
> struct v4l2_ctrl *exposure;
> + struct v4l2_mbus_framefmt fmt;
>
> /* Current mode */
> const struct ov8856_mode *cur_mode;
>
> + /* module hardware version that can be read out from register 0x700f
> + * the register value corresponds to different hardware version
> + * 01: 2A module revision
> + * 02: 1B module revision
> + */
> + bool is_1B_revision;
> +
> /* To serialize asynchronus callbacks */
> struct mutex mutex;
>
> @@ -696,6 +1170,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern)
> OV8856_REG_VALUE_08BIT, pattern);
> }
>
> +static int ov8856_check_revision(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_MODE_CTRL,
> + OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
> + if (ret)
> + return ret;
> +
> + return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
> + OV8856_REG_VALUE_08BIT,
> + OV8856_OTP_LOAD_CTRL_ENABLE);
Is it not necessary to disable streaming after reading EEPROM?
> +}
> +
> static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov8856 *ov8856 = container_of(ctrl->handler,
> @@ -825,7 +1318,6 @@ static void ov8856_update_pad_format(const struct ov8856_mode *mode,
> {
> fmt->width = mode->width;
> fmt->height = mode->height;
> - fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> fmt->field = V4L2_FIELD_NONE;
> }
>
> @@ -834,6 +1326,7 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> const struct ov8856_reg_list *reg_list;
> int link_freq_index, ret;
> + u32 h_size;
>
> link_freq_index = ov8856->cur_mode->link_freq_index;
> reg_list = &link_freq_configs[link_freq_index].reg_list;
> @@ -850,6 +1343,29 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> return ret;
> }
>
> + /* Update R3614 if the revision is 1B module */
> + if (ov8856->is_1B_revision) {
> + ret = ov8856_write_reg(ov8856, OV8856_ANAL_MODE_CTRL,
> + OV8856_REG_VALUE_08BIT,
> + OV8856_ANAL_1B_VAL);
> + if (ret) {
> + dev_err(&client->dev, "failed to set R3614");
> + return ret;
> + }
> + }
> +
> + ret = ov8856_read_reg(ov8856, REG_X_ADDR_START,
> + OV8856_REG_VALUE_16BIT, &h_size);
> + if (ret) {
> + dev_err(&client->dev, "failed to read out R3614");
> + return ret;
> + }
> +
> + if (h_size == X_OUTPUT_FULL_SIZE || h_size == X_OUTPUT_BINNING_SIZE)
> + ov8856->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
I wonder if this condition is right. The pixel order (and thus the media
bus code) is determined by cropping (and flipping and mirroring, but
they're not supported right now). See e.g. the smiapp driver for an
example.
The correct media bus code needs to be available to the user through the
regular API at all times, not only after streaming is started.
I'd suggest adding support for other pixel orders in a separate patch.
> +
> ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler);
> if (ret)
> return ret;
> @@ -878,6 +1394,7 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> struct ov8856 *ov8856 = to_ov8856(sd);
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> int ret = 0;
> + u32 val;
>
> if (ov8856->streaming == enable)
> return 0;
> @@ -908,6 +1425,44 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> return ret;
> }
>
> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> + int ret;
> +
> + ret = clk_prepare_enable(ov8856->xvclk);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to enable xvclk\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> +
> + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to enable regulators\n");
> + goto disable_clk;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
..., 1);
> +
> + usleep_range(1400, 1500);
> +
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(ov8856->xvclk);
How about the n_shutdn GPIO?
> +
> + return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 1);
Why to inactivate xshutdn here (it already was)?
> + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> + clk_disable_unprepare(ov8856->xvclk);
> +}
> +
> static int __maybe_unused ov8856_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -951,6 +1506,7 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *fmt)
> {
> struct ov8856 *ov8856 = to_ov8856(sd);
> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> const struct ov8856_mode *mode;
> s32 vblank_def, h_blank;
>
> @@ -960,7 +1516,9 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> fmt->format.height);
>
> mutex_lock(&ov8856->mutex);
> - ov8856_update_pad_format(mode, &fmt->format);
> + mbus_fmt->code = ov8856->fmt.code;
> + ov8856_update_pad_format(mode, mbus_fmt);
> + ov8856->fmt = fmt->format;
> if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> } else {
> @@ -992,13 +1550,17 @@ static int ov8856_get_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *fmt)
> {
> struct ov8856 *ov8856 = to_ov8856(sd);
> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
>
> mutex_lock(&ov8856->mutex);
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> fmt->format = *v4l2_subdev_get_try_format(&ov8856->sd, cfg,
> fmt->pad);
> - else
> - ov8856_update_pad_format(ov8856->cur_mode, &fmt->format);
> + } else {
> + fmt->format = ov8856->fmt;
> + ov8856_update_pad_format(ov8856->cur_mode, mbus_fmt);
> + mbus_fmt->code = ov8856->fmt.code;
> + }
>
> mutex_unlock(&ov8856->mutex);
>
> @@ -1009,11 +1571,12 @@ static int ov8856_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> - /* Only one bayer order GRBG is supported */
> + struct ov8856 *ov8856 = to_ov8856(sd);
> +
> if (code->index > 0)
> return -EINVAL;
>
> - code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> + code->code = ov8856->fmt.code;
>
> return 0;
> }
> @@ -1089,6 +1652,20 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> return -ENXIO;
> }
>
> + /* check sensor hardware revision */
> + ret = ov8856_check_revision(ov8856);
> + if (ret) {
> + dev_err(&client->dev, "failed to check sensor revision");
> + return ret;
> + }
> +
> + ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
> + OV8856_REG_VALUE_08BIT, &val);
> + if (ret)
> + return ret;
> +
> + ov8856->is_1B_revision = (val == OV8856_1B_MODULE) ? 1 : 0;
> +
> return 0;
> }
>
> @@ -1107,7 +1684,7 @@ static int ov8856_check_hwcfg(struct device *dev)
> return -ENXIO;
>
> fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> - if (mclk != OV8856_MCLK) {
> + if (mclk != OV8856_XVCLK) {
> dev_err(dev, "external clock %d is not supported", mclk);
> return -EINVAL;
> }
> @@ -1164,6 +1741,9 @@ static int ov8856_remove(struct i2c_client *client)
> media_entity_cleanup(&sd->entity);
> v4l2_ctrl_handler_free(sd->ctrl_handler);
> pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev))
> + __ov8856_power_off(ov8856);
> + pm_runtime_set_suspended(&client->dev);
> mutex_destroy(&ov8856->mutex);
>
> return 0;
> @@ -1172,6 +1752,7 @@ static int ov8856_remove(struct i2c_client *client)
> static int ov8856_probe(struct i2c_client *client)
> {
> struct ov8856 *ov8856;
> + unsigned int i;
> int ret;
>
> ret = ov8856_check_hwcfg(&client->dev);
> @@ -1186,6 +1767,42 @@ static int ov8856_probe(struct i2c_client *client)
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> + ov8856->fmt.code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
Does this work on ACPI?
See e.g. the smiapp driver for an example. The same applies to the rest of
the chunk.
> + if (IS_ERR(ov8856->xvclk)) {
> + dev_err(&client->dev, "failed to get xvclk\n");
> + return -EINVAL;
> + }
> +
> + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_TYP);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> + return ret;
> + }
> + if (clk_get_rate(ov8856->xvclk) != OV8856_XVCLK_TYP)
> + dev_warn(&client->dev,
> + "xvclk mismatched, modes are based on 24MHz\n");
> +
> + ov8856->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(ov8856->n_shutdn_gpio)) {
> + dev_err(&client->dev, "failed to get reset-gpios\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < OV8856_NUM_SUPPLIES; i++)
> + ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev, OV8856_NUM_SUPPLIES,
> + ov8856->supplies);
> + if (ret)
> + dev_warn(&client->dev, "failed to get regulators\n");
> +
> + ret = __ov8856_power_on(ov8856);
> + if (ret)
> + dev_warn(&client->dev, "failed to power on\n");
> +
> ret = ov8856_identify_module(ov8856);
> if (ret) {
> dev_err(&client->dev, "failed to find sensor: %d", ret);
> @@ -1251,11 +1868,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> #endif
>
> +static const struct of_device_id ov8856_of_match[] = {
> + { .compatible = "ovti,ov8856" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +
> static struct i2c_driver ov8856_i2c_driver = {
> .driver = {
> .name = "ov8856",
> .pm = &ov8856_pm_ops,
> .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> + .of_match_table = ov8856_of_match,
> },
> .probe_new = ov8856_probe,
> .remove = ov8856_remove,
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-09-13 7:55 ` Sakari Ailus
@ 2019-10-30 13:02 ` Dongchun Zhu
2019-11-06 14:22 ` Sakari Ailus
0 siblings, 1 reply; 16+ messages in thread
From: Dongchun Zhu @ 2019-10-30 13:02 UTC (permalink / raw)
To: Sakari Ailus
Cc: mchehab, andriy.shevchenko, robh+dt, mark.rutland, drinkcat,
tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
Hi Sakari,
On Fri, 2019-09-13 at 10:55 +0300, Sakari Ailus wrote:
> Hi Dongchun,
>
> On Tue, Sep 10, 2019 at 09:04:46PM +0800, dongchun.zhu@mediatek.com wrote:
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> >
> > This patch mainly adds two more sensor modes for OV8856 CMOS image sensor.
> > That is, the resolution of 1632*1224 and 3264*2448, corresponding to the bayer order of BGGR.
> > The sensor revision also differs in some OTP register.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> > drivers/media/i2c/ov8856.c | 654 +++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 639 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index cd347d6..9ad0b73 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 <linux/clk.h>
> > #include <asm/unaligned.h>
> > #include <linux/acpi.h>
> > #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/i2c.h>
> > #include <linux/module.h>
> > #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-fwnode.h>
> > @@ -18,10 +21,15 @@
> > #define OV8856_LINK_FREQ_360MHZ 360000000ULL
> > #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> > #define OV8856_SCLK 144000000ULL
> > -#define OV8856_MCLK 19200000
> > +#define OV8856_XVCLK 19200000
> > +#define OV8856_XVCLK_TYP 24000000
>
> How about calling these e.g. OV8856_XCLK_19_2 and OV8856_CLK_24? Or just
> use the plain numbers; there's nothing really special about them.
>
> Ideally you'd have both in a single location in the driver --- an array of
> modes supported on each of them as they're different.
>
Fixed in next release.
> > #define OV8856_DATA_LANES 4
> > #define OV8856_RGB_DEPTH 10
> >
> > +#define REG_X_ADDR_START 0x3808
> > +#define X_OUTPUT_FULL_SIZE 0x0cc0
> > +#define X_OUTPUT_BINNING_SIZE 0x0660
> > +
> > #define OV8856_REG_CHIP_ID 0x300a
> > #define OV8856_CHIP_ID 0x00885a
> >
> > @@ -29,6 +37,22 @@
> > #define OV8856_MODE_STANDBY 0x00
> > #define OV8856_MODE_STREAMING 0x01
> >
> > +/* define 1B module revision */
> > +#define OV8856_1B_MODULE 0x02
> > +
> > +/* 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_MODULE_REVISION 0x700f
> > +#define OV8856_OTP_MODE_CTRL 0x3d84
> > +#define OV8856_OTP_LOAD_CTRL 0x3d81
> > +#define OV8856_OTP_MODE_AUTO 0x00
> > +#define OV8856_OTP_LOAD_CTRL_ENABLE BIT(0)
> > +
> > +/* Analog control register that decided by module revision */
> > +#define OV8856_ANAL_MODE_CTRL 0x3614
> > +#define OV8856_ANAL_1B_VAL 0x20
> > +
> > /* vertical-timings from sensor */
> > #define OV8856_REG_VTS 0x380e
> > #define OV8856_VTS_MAX 0x7fff
> > @@ -64,6 +88,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,
> > @@ -195,11 +227,11 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> > {0x3800, 0x00},
> > {0x3801, 0x00},
> > {0x3802, 0x00},
> > - {0x3803, 0x06},
> > + {0x3803, 0x07},
> > {0x3804, 0x0c},
> > {0x3805, 0xdf},
> > {0x3806, 0x09},
> > - {0x3807, 0xa7},
> > + {0x3807, 0xa6},
>
> This is changing an existing sensor mode. What are the effects of the
> change?
>
> Given that this register is not documented in the driver in any way, I'd
> expect the commit message to carry some reasoning behind the change, also
> suggesting it belongs to a separate patch. The same applies to changes to
> the other existing modes.
>
Removed in next release.
> > {0x3808, 0x0c},
> > {0x3809, 0xd0},
> > {0x380a, 0x09},
> > @@ -211,7 +243,7 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> > {0x3810, 0x00},
> > {0x3811, 0x00},
> > {0x3812, 0x00},
> > - {0x3813, 0x01},
> > + {0x3813, 0x00},
> > {0x3814, 0x01},
> > {0x3815, 0x01},
> > {0x3816, 0x00},
> > @@ -316,6 +348,209 @@ 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, 0x20},
> > + {0x3003, 0x08},
> > + {0x300e, 0x20},
> > + {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},
> > + {0x380e, 0x09},
> > + {0x380f, 0xb2},
> > + {0x3810, 0x00},
> > + {0x3811, 0x04},
> > + {0x3812, 0x00},
> > + {0x3813, 0x02},
> > + {0x3814, 0x01},
> > + {0x3815, 0x01},
> > + {0x3816, 0x00},
> > + {0x3817, 0x00},
> > + {0x3818, 0x00},
> > + {0x3819, 0x00},
> > + {0x3820, 0x80},
> > + {0x3821, 0x46},
> > + {0x382a, 0x01},
> > + {0x382b, 0x01},
> > + {0x3830, 0x06},
> > + {0x3836, 0x02},
> > + {0x3862, 0x04},
> > + {0x3863, 0x08},
> > + {0x3cc0, 0x33},
> > + {0x3d85, 0x17},
> > + {0x3d8c, 0x73},
> > + {0x3d8d, 0xde},
> > + {0x4001, 0xe0},
> > + {0x4003, 0x40},
> > + {0x4008, 0x00},
> > + {0x4009, 0x0b},
> > + {0x400a, 0x00},
> > + {0x400b, 0x84},
> > + {0x400f, 0x80},
> > + {0x4010, 0xf0},
> > + {0x4011, 0xff},
> > + {0x4012, 0x02},
> > + {0x4013, 0x01},
> > + {0x4014, 0x01},
> > + {0x4015, 0x01},
> > + {0x4042, 0x00},
> > + {0x4043, 0x80},
> > + {0x4044, 0x00},
> > + {0x4045, 0x80},
> > + {0x4046, 0x00},
> > + {0x4047, 0x80},
> > + {0x4048, 0x00},
> > + {0x4049, 0x80},
> > + {0x4041, 0x03},
> > + {0x404c, 0x20},
> > + {0x404d, 0x00},
> > + {0x404e, 0x20},
> > + {0x4203, 0x80},
> > + {0x4307, 0x30},
> > + {0x4317, 0x00},
> > + {0x4502, 0x50},
> > + {0x4503, 0x08},
> > + {0x4601, 0x80},
> > + {0x4800, 0x44},
> > + {0x4816, 0x53},
> > + {0x481b, 0x50},
> > + {0x481f, 0x27},
> > + {0x4823, 0x3c},
> > + {0x482b, 0x00},
> > + {0x4831, 0x66},
> > + {0x4837, 0x16},
> > + {0x483c, 0x0f},
> > + {0x484b, 0x05},
> > + {0x5000, 0x77},
> > + {0x5001, 0x0a},
> > + {0x5003, 0xc8},
> > + {0x5004, 0x04},
> > + {0x5006, 0x00},
> > + {0x5007, 0x00},
> > + {0x502e, 0x03},
> > + {0x5030, 0x41},
> > + {0x5780, 0x14},
> > + {0x5781, 0x0f},
> > + {0x5782, 0x44},
> > + {0x5783, 0x02},
> > + {0x5784, 0x01},
> > + {0x5785, 0x01},
> > + {0x5786, 0x00},
> > + {0x5787, 0x04},
> > + {0x5788, 0x02},
> > + {0x5789, 0x0f},
> > + {0x578a, 0xfd},
> > + {0x578b, 0xf5},
> > + {0x578c, 0xf5},
> > + {0x578d, 0x03},
> > + {0x578e, 0x08},
> > + {0x578f, 0x0c},
> > + {0x5790, 0x08},
> > + {0x5791, 0x04},
> > + {0x5792, 0x00},
> > + {0x5793, 0x52},
> > + {0x5794, 0xa3},
> > + {0x5795, 0x02},
> > + {0x5796, 0x20},
> > + {0x5797, 0x20},
> > + {0x5798, 0xd5},
> > + {0x5799, 0xd5},
> > + {0x579a, 0x00},
> > + {0x579b, 0x50},
> > + {0x579c, 0x00},
> > + {0x579d, 0x2c},
> > + {0x579e, 0x0c},
> > + {0x579f, 0x40},
> > + {0x57a0, 0x09},
> > + {0x57a1, 0x40},
> > + {0x59f8, 0x3d},
> > + {0x5a08, 0x02},
> > + {0x5b00, 0x02},
> > + {0x5b01, 0x10},
> > + {0x5b02, 0x03},
> > + {0x5b03, 0xcf},
> > + {0x5b05, 0x6c},
> > + {0x5e00, 0x00},
> > + {0x5e10, 0xfc}
> > +};
> > +
> > static const struct ov8856_reg mode_1640x1232_regs[] = {
> > {0x3000, 0x20},
> > {0x3003, 0x08},
> > @@ -385,11 +620,11 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> > {0x3800, 0x00},
> > {0x3801, 0x00},
> > {0x3802, 0x00},
> > - {0x3803, 0x06},
> > + {0x3803, 0x07},
> > {0x3804, 0x0c},
> > {0x3805, 0xdf},
> > {0x3806, 0x09},
> > - {0x3807, 0xa7},
> > + {0x3807, 0xa6},
> > {0x3808, 0x06},
> > {0x3809, 0x68},
> > {0x380a, 0x04},
> > @@ -401,7 +636,7 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> > {0x3810, 0x00},
> > {0x3811, 0x00},
> > {0x3812, 0x00},
> > - {0x3813, 0x01},
> > + {0x3813, 0x00},
> > {0x3814, 0x03},
> > {0x3815, 0x01},
> > {0x3816, 0x00},
> > @@ -506,6 +741,209 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> > {0x5e00, 0x00}
> > };
> >
> > +static const struct ov8856_reg mode_1632x1224_regs[] = {
> > + {0x0103, 0x01},
> > + {0x0302, 0x3c},
> > + {0x0303, 0x01},
> > + {0x031e, 0x0c},
> > + {0x3000, 0x20},
> > + {0x3003, 0x08},
> > + {0x300e, 0x20},
> > + {0x3010, 0x00},
> > + {0x3015, 0x84},
> > + {0x3018, 0x72},
> > + {0x3021, 0x23},
> > + {0x3033, 0x24},
> > + {0x3500, 0x00},
> > + {0x3501, 0x4c},
> > + {0x3502, 0xe0},
> > + {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, 0x08},
> > + {0x3706, 0x86},
> > + {0x370b, 0x7e},
> > + {0x3714, 0x27},
> > + {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, 0x14},
> > + {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, 0x06},
> > + {0x3809, 0x60},
> > + {0x380a, 0x04},
> > + {0x380b, 0xc8},
> > + {0x380c, 0x07},
> > + {0x380d, 0x8c},
> > + {0x380e, 0x09},
> > + {0x380f, 0xb2},
> > + {0x3810, 0x00},
> > + {0x3811, 0x02},
> > + {0x3812, 0x00},
> > + {0x3813, 0x02},
> > + {0x3814, 0x03},
> > + {0x3815, 0x01},
> > + {0x3816, 0x00},
> > + {0x3817, 0x00},
> > + {0x3818, 0x00},
> > + {0x3819, 0x00},
> > + {0x3820, 0x80},
> > + {0x3821, 0x47},
> > + {0x382a, 0x03},
> > + {0x382b, 0x01},
> > + {0x3830, 0x06},
> > + {0x3836, 0x02},
> > + {0x3862, 0x04},
> > + {0x3863, 0x08},
> > + {0x3cc0, 0x33},
> > + {0x3d85, 0x17},
> > + {0x3d8c, 0x73},
> > + {0x3d8d, 0xde},
> > + {0x4001, 0xe0},
> > + {0x4003, 0x40},
> > + {0x4008, 0x00},
> > + {0x4009, 0x05},
> > + {0x400a, 0x00},
> > + {0x400b, 0x84},
> > + {0x400f, 0x80},
> > + {0x4010, 0xf0},
> > + {0x4011, 0xff},
> > + {0x4012, 0x02},
> > + {0x4013, 0x01},
> > + {0x4014, 0x01},
> > + {0x4015, 0x01},
> > + {0x4042, 0x00},
> > + {0x4043, 0x80},
> > + {0x4044, 0x00},
> > + {0x4045, 0x80},
> > + {0x4046, 0x00},
> > + {0x4047, 0x80},
> > + {0x4048, 0x00},
> > + {0x4049, 0x80},
> > + {0x4041, 0x03},
> > + {0x404c, 0x20},
> > + {0x404d, 0x00},
> > + {0x404e, 0x20},
> > + {0x4203, 0x80},
> > + {0x4307, 0x30},
> > + {0x4317, 0x00},
> > + {0x4502, 0x50},
> > + {0x4503, 0x08},
> > + {0x4601, 0x80},
> > + {0x4800, 0x44},
> > + {0x4816, 0x53},
> > + {0x481b, 0x50},
> > + {0x481f, 0x27},
> > + {0x4823, 0x3c},
> > + {0x482b, 0x00},
> > + {0x4831, 0x66},
> > + {0x4837, 0x16},
> > + {0x483c, 0x0f},
> > + {0x484b, 0x05},
> > + {0x5000, 0x77},
> > + {0x5001, 0x0a},
> > + {0x5003, 0xc8},
> > + {0x5004, 0x04},
> > + {0x5006, 0x00},
> > + {0x5007, 0x00},
> > + {0x502e, 0x03},
> > + {0x5030, 0x41},
> > + {0x5795, 0x00},
> > + {0x5796, 0x10},
> > + {0x5797, 0x10},
> > + {0x5798, 0x73},
> > + {0x5799, 0x73},
> > + {0x579a, 0x00},
> > + {0x579b, 0x28},
> > + {0x579c, 0x00},
> > + {0x579d, 0x16},
> > + {0x579e, 0x06},
> > + {0x579f, 0x20},
> > + {0x57a0, 0x04},
> > + {0x57a1, 0xa0},
> > + {0x5780, 0x14},
> > + {0x5781, 0x0f},
> > + {0x5782, 0x44},
> > + {0x5783, 0x02},
> > + {0x5784, 0x01},
> > + {0x5785, 0x01},
> > + {0x5786, 0x00},
> > + {0x5787, 0x04},
> > + {0x5788, 0x02},
> > + {0x5789, 0x0f},
> > + {0x578a, 0xfd},
> > + {0x578b, 0xf5},
> > + {0x578c, 0xf5},
> > + {0x578d, 0x03},
> > + {0x578e, 0x08},
> > + {0x578f, 0x0c},
> > + {0x5790, 0x08},
> > + {0x5791, 0x04},
> > + {0x5792, 0x00},
> > + {0x5793, 0x52},
> > + {0x5794, 0xa3},
> > + {0x59f8, 0x3d},
> > + {0x5a08, 0x02},
> > + {0x5b00, 0x02},
> > + {0x5b01, 0x10},
> > + {0x5b02, 0x03},
> > + {0x5b03, 0xcf},
> > + {0x5b05, 0x6c},
> > + {0x5e00, 0x00},
> > + {0x5e10, 0xfc}
> > +};
> > +
> > static const char * const ov8856_test_pattern_menu[] = {
> > "Disabled",
> > "Standard Color Bar",
> > @@ -548,6 +986,18 @@ static const struct ov8856_mode supported_modes[] = {
> > .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> > },
> > {
> > + .width = 3264,
> > + .height = 2448,
> > + .hts = 1932,
> > + .vts_def = 2482,
> > + .vts_min = 2482,
> > + .reg_list = {
> > + .num_of_regs = ARRAY_SIZE(mode_3264x2448_regs),
> > + .regs = mode_3264x2448_regs,
> > + },
> > + .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> > + },
>
> How do you guarantee that the modes are only used on the xvclk frequency
> they're intended for?
>
> I think it'd be best if you added the new modes in a separete patch.
>
In a separate patch?
> > + {
> > .width = 1640,
> > .height = 1232,
> > .hts = 3820,
> > @@ -558,6 +1008,18 @@ static const struct ov8856_mode supported_modes[] = {
> > .regs = mode_1640x1232_regs,
> > },
> > .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> > + },
> > + {
> > + .width = 1632,
> > + .height = 1224,
> > + .hts = 1932,
> > + .vts_def = 2482,
> > + .vts_min = 2482,
> > + .reg_list = {
> > + .num_of_regs = ARRAY_SIZE(mode_1632x1224_regs),
> > + .regs = mode_1632x1224_regs,
> > + },
> > + .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> > }
> > };
> >
> > @@ -566,16 +1028,28 @@ struct ov8856 {
> > struct media_pad pad;
> > struct v4l2_ctrl_handler ctrl_handler;
> >
> > + struct clk *xvclk;
> > + struct gpio_desc *n_shutdn_gpio;
> > + struct regulator_bulk_data supplies[OV8856_NUM_SUPPLIES];
> > +
> > /* V4L2 Controls */
> > struct v4l2_ctrl *link_freq;
> > struct v4l2_ctrl *pixel_rate;
> > struct v4l2_ctrl *vblank;
> > struct v4l2_ctrl *hblank;
> > struct v4l2_ctrl *exposure;
> > + struct v4l2_mbus_framefmt fmt;
> >
> > /* Current mode */
> > const struct ov8856_mode *cur_mode;
> >
> > + /* module hardware version that can be read out from register 0x700f
> > + * the register value corresponds to different hardware version
> > + * 01: 2A module revision
> > + * 02: 1B module revision
> > + */
> > + bool is_1B_revision;
> > +
> > /* To serialize asynchronus callbacks */
> > struct mutex mutex;
> >
> > @@ -696,6 +1170,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern)
> > OV8856_REG_VALUE_08BIT, pattern);
> > }
> >
> > +static int ov8856_check_revision(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_MODE_CTRL,
> > + OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
> > + if (ret)
> > + return ret;
> > +
> > + return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
> > + OV8856_REG_VALUE_08BIT,
> > + OV8856_OTP_LOAD_CTRL_ENABLE);
>
> Is it not necessary to disable streaming after reading EEPROM?
>
These registers are provided by OV vendor.
They need to be initialized before reading sensor revision.
> > +}
> > +
> > static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > struct ov8856 *ov8856 = container_of(ctrl->handler,
> > @@ -825,7 +1318,6 @@ static void ov8856_update_pad_format(const struct ov8856_mode *mode,
> > {
> > fmt->width = mode->width;
> > fmt->height = mode->height;
> > - fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > fmt->field = V4L2_FIELD_NONE;
> > }
> >
> > @@ -834,6 +1326,7 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> > struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > const struct ov8856_reg_list *reg_list;
> > int link_freq_index, ret;
> > + u32 h_size;
> >
> > link_freq_index = ov8856->cur_mode->link_freq_index;
> > reg_list = &link_freq_configs[link_freq_index].reg_list;
> > @@ -850,6 +1343,29 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> > return ret;
> > }
> >
> > + /* Update R3614 if the revision is 1B module */
> > + if (ov8856->is_1B_revision) {
> > + ret = ov8856_write_reg(ov8856, OV8856_ANAL_MODE_CTRL,
> > + OV8856_REG_VALUE_08BIT,
> > + OV8856_ANAL_1B_VAL);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to set R3614");
> > + return ret;
> > + }
> > + }
> > +
> > + ret = ov8856_read_reg(ov8856, REG_X_ADDR_START,
> > + OV8856_REG_VALUE_16BIT, &h_size);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to read out R3614");
> > + return ret;
> > + }
> > +
> > + if (h_size == X_OUTPUT_FULL_SIZE || h_size == X_OUTPUT_BINNING_SIZE)
> > + ov8856->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
>
> I wonder if this condition is right. The pixel order (and thus the media
> bus code) is determined by cropping (and flipping and mirroring, but
> they're not supported right now). See e.g. the smiapp driver for an
> example.
>
> The correct media bus code needs to be available to the user through the
> regular API at all times, not only after streaming is started.
>
> I'd suggest adding support for other pixel orders in a separate patch.
>
This may need time to think out.
Is there any recommended regular API?
> > +
> > ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler);
> > if (ret)
> > return ret;
> > @@ -878,6 +1394,7 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > struct ov8856 *ov8856 = to_ov8856(sd);
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > int ret = 0;
> > + u32 val;
> >
> > if (ov8856->streaming == enable)
> > return 0;
> > @@ -908,6 +1425,44 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > return ret;
> > }
> >
> > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(ov8856->xvclk);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to enable xvclk\n");
> > + return ret;
> > + }
> > +
> > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > +
> > + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to enable regulators\n");
> > + goto disable_clk;
> > + }
> > +
> > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
>
> ..., 1);
>
> > +
> > + usleep_range(1400, 1500);
> > +
> > + return 0;
> > +
> > +disable_clk:
> > + clk_disable_unprepare(ov8856->xvclk);
>
> How about the n_shutdn GPIO?
>
This is power up sequence function.
When enabling regulators failed, it is required to disable the enabled
power previously.
Thus here we need to disable xvclk.
But for n_shutdn GPIO, it was low state at that time.
So we don't need to do anything for it.
> > +
> > + return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > +{
> > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 1);
>
> Why to inactivate xshutdn here (it already was)?
>
> > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > + clk_disable_unprepare(ov8856->xvclk);
> > +}
> > +
> > static int __maybe_unused ov8856_suspend(struct device *dev)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > @@ -951,6 +1506,7 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> > struct v4l2_subdev_format *fmt)
> > {
> > struct ov8856 *ov8856 = to_ov8856(sd);
> > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > const struct ov8856_mode *mode;
> > s32 vblank_def, h_blank;
> >
> > @@ -960,7 +1516,9 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> > fmt->format.height);
> >
> > mutex_lock(&ov8856->mutex);
> > - ov8856_update_pad_format(mode, &fmt->format);
> > + mbus_fmt->code = ov8856->fmt.code;
> > + ov8856_update_pad_format(mode, mbus_fmt);
> > + ov8856->fmt = fmt->format;
> > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> > } else {
> > @@ -992,13 +1550,17 @@ static int ov8856_get_format(struct v4l2_subdev *sd,
> > struct v4l2_subdev_format *fmt)
> > {
> > struct ov8856 *ov8856 = to_ov8856(sd);
> > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> >
> > mutex_lock(&ov8856->mutex);
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > fmt->format = *v4l2_subdev_get_try_format(&ov8856->sd, cfg,
> > fmt->pad);
> > - else
> > - ov8856_update_pad_format(ov8856->cur_mode, &fmt->format);
> > + } else {
> > + fmt->format = ov8856->fmt;
> > + ov8856_update_pad_format(ov8856->cur_mode, mbus_fmt);
> > + mbus_fmt->code = ov8856->fmt.code;
> > + }
> >
> > mutex_unlock(&ov8856->mutex);
> >
> > @@ -1009,11 +1571,12 @@ static int ov8856_enum_mbus_code(struct v4l2_subdev *sd,
> > struct v4l2_subdev_pad_config *cfg,
> > struct v4l2_subdev_mbus_code_enum *code)
> > {
> > - /* Only one bayer order GRBG is supported */
> > + struct ov8856 *ov8856 = to_ov8856(sd);
> > +
> > if (code->index > 0)
> > return -EINVAL;
> >
> > - code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > + code->code = ov8856->fmt.code;
> >
> > return 0;
> > }
> > @@ -1089,6 +1652,20 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> > return -ENXIO;
> > }
> >
> > + /* check sensor hardware revision */
> > + ret = ov8856_check_revision(ov8856);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to check sensor revision");
> > + return ret;
> > + }
> > +
> > + ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
> > + OV8856_REG_VALUE_08BIT, &val);
> > + if (ret)
> > + return ret;
> > +
> > + ov8856->is_1B_revision = (val == OV8856_1B_MODULE) ? 1 : 0;
> > +
> > return 0;
> > }
> >
> > @@ -1107,7 +1684,7 @@ static int ov8856_check_hwcfg(struct device *dev)
> > return -ENXIO;
> >
> > fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > - if (mclk != OV8856_MCLK) {
> > + if (mclk != OV8856_XVCLK) {
> > dev_err(dev, "external clock %d is not supported", mclk);
> > return -EINVAL;
> > }
> > @@ -1164,6 +1741,9 @@ static int ov8856_remove(struct i2c_client *client)
> > media_entity_cleanup(&sd->entity);
> > v4l2_ctrl_handler_free(sd->ctrl_handler);
> > pm_runtime_disable(&client->dev);
> > + if (!pm_runtime_status_suspended(&client->dev))
> > + __ov8856_power_off(ov8856);
> > + pm_runtime_set_suspended(&client->dev);
> > mutex_destroy(&ov8856->mutex);
> >
> > return 0;
> > @@ -1172,6 +1752,7 @@ static int ov8856_remove(struct i2c_client *client)
> > static int ov8856_probe(struct i2c_client *client)
> > {
> > struct ov8856 *ov8856;
> > + unsigned int i;
> > int ret;
> >
> > ret = ov8856_check_hwcfg(&client->dev);
> > @@ -1186,6 +1767,42 @@ static int ov8856_probe(struct i2c_client *client)
> > return -ENOMEM;
> >
> > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > + ov8856->fmt.code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > +
> > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
>
> Does this work on ACPI?
>
> See e.g. the smiapp driver for an example. The same applies to the rest of
> the chunk.
For ACPI, this probe should not be executed,
as DT setting may not have compatible property.
>
> > + if (IS_ERR(ov8856->xvclk)) {
> > + dev_err(&client->dev, "failed to get xvclk\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_TYP);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> > + return ret;
> > + }
> > + if (clk_get_rate(ov8856->xvclk) != OV8856_XVCLK_TYP)
> > + dev_warn(&client->dev,
> > + "xvclk mismatched, modes are based on 24MHz\n");
> > +
> > + ov8856->n_shutdn_gpio = devm_gpiod_get(&client->dev, "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(ov8856->n_shutdn_gpio)) {
> > + dev_err(&client->dev, "failed to get reset-gpios\n");
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < OV8856_NUM_SUPPLIES; i++)
> > + ov8856->supplies[i].supply = ov8856_supply_names[i];
> > +
> > + ret = devm_regulator_bulk_get(&client->dev, OV8856_NUM_SUPPLIES,
> > + ov8856->supplies);
> > + if (ret)
> > + dev_warn(&client->dev, "failed to get regulators\n");
> > +
> > + ret = __ov8856_power_on(ov8856);
> > + if (ret)
> > + dev_warn(&client->dev, "failed to power on\n");
> > +
> > ret = ov8856_identify_module(ov8856);
> > if (ret) {
> > dev_err(&client->dev, "failed to find sensor: %d", ret);
> > @@ -1251,11 +1868,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> > MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> > #endif
> >
> > +static const struct of_device_id ov8856_of_match[] = {
> > + { .compatible = "ovti,ov8856" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > +
> > static struct i2c_driver ov8856_i2c_driver = {
> > .driver = {
> > .name = "ov8856",
> > .pm = &ov8856_pm_ops,
> > .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > + .of_match_table = ov8856_of_match,
> > },
> > .probe_new = ov8856_probe,
> > .remove = ov8856_remove,
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor
2019-10-30 13:02 ` Dongchun Zhu
@ 2019-11-06 14:22 ` Sakari Ailus
0 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2019-11-06 14:22 UTC (permalink / raw)
To: Dongchun Zhu
Cc: mchehab, andriy.shevchenko, robh+dt, mark.rutland, drinkcat,
tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
Hi Dongchun,
On Wed, Oct 30, 2019 at 09:02:08PM +0800, Dongchun Zhu wrote:
...
> > > @@ -548,6 +986,18 @@ static const struct ov8856_mode supported_modes[] = {
> > > .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> > > },
> > > {
> > > + .width = 3264,
> > > + .height = 2448,
> > > + .hts = 1932,
> > > + .vts_def = 2482,
> > > + .vts_min = 2482,
> > > + .reg_list = {
> > > + .num_of_regs = ARRAY_SIZE(mode_3264x2448_regs),
> > > + .regs = mode_3264x2448_regs,
> > > + },
> > > + .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> > > + },
> >
> > How do you guarantee that the modes are only used on the xvclk frequency
> > they're intended for?
> >
> > I think it'd be best if you added the new modes in a separete patch.
> >
>
> In a separate patch?
Yes, please create a new patch that only adds these modes.
...
> > > @@ -696,6 +1170,25 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern)
> > > OV8856_REG_VALUE_08BIT, pattern);
> > > }
> > >
> > > +static int ov8856_check_revision(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_MODE_CTRL,
> > > + OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
> > > + OV8856_REG_VALUE_08BIT,
> > > + OV8856_OTP_LOAD_CTRL_ENABLE);
> >
> > Is it not necessary to disable streaming after reading EEPROM?
> >
>
> These registers are provided by OV vendor.
> They need to be initialized before reading sensor revision.
I'd still expect streaming to be stopped once it's started. Most probably
the sensor will still be powered off before streaming is started the next
time, but that may not be the case.
>
> > > +}
> > > +
> > > static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
> > > {
> > > struct ov8856 *ov8856 = container_of(ctrl->handler,
> > > @@ -825,7 +1318,6 @@ static void ov8856_update_pad_format(const struct ov8856_mode *mode,
> > > {
> > > fmt->width = mode->width;
> > > fmt->height = mode->height;
> > > - fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > > fmt->field = V4L2_FIELD_NONE;
> > > }
> > >
> > > @@ -834,6 +1326,7 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> > > struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > const struct ov8856_reg_list *reg_list;
> > > int link_freq_index, ret;
> > > + u32 h_size;
> > >
> > > link_freq_index = ov8856->cur_mode->link_freq_index;
> > > reg_list = &link_freq_configs[link_freq_index].reg_list;
> > > @@ -850,6 +1343,29 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> > > return ret;
> > > }
> > >
> > > + /* Update R3614 if the revision is 1B module */
> > > + if (ov8856->is_1B_revision) {
> > > + ret = ov8856_write_reg(ov8856, OV8856_ANAL_MODE_CTRL,
> > > + OV8856_REG_VALUE_08BIT,
> > > + OV8856_ANAL_1B_VAL);
> > > + if (ret) {
> > > + dev_err(&client->dev, "failed to set R3614");
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + ret = ov8856_read_reg(ov8856, REG_X_ADDR_START,
> > > + OV8856_REG_VALUE_16BIT, &h_size);
> > > + if (ret) {
> > > + dev_err(&client->dev, "failed to read out R3614");
> > > + return ret;
> > > + }
> > > +
> > > + if (h_size == X_OUTPUT_FULL_SIZE || h_size == X_OUTPUT_BINNING_SIZE)
> > > + ov8856->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> >
> > I wonder if this condition is right. The pixel order (and thus the media
> > bus code) is determined by cropping (and flipping and mirroring, but
> > they're not supported right now). See e.g. the smiapp driver for an
> > example.
> >
> > The correct media bus code needs to be available to the user through the
> > regular API at all times, not only after streaming is started.
> >
> > I'd suggest adding support for other pixel orders in a separate patch.
> >
>
> This may need time to think out.
> Is there any recommended regular API?
Yes. Please see:
<URL:https://hverkuil.home.xs4all.nl/spec/uapi/v4l/dev-subdev.html>
Although that is more or less incompatible with the current driver
behaviour which is simply relying on SUBDEV_S_FMT.
>
> > > +
> > > ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler);
> > > if (ret)
> > > return ret;
> > > @@ -878,6 +1394,7 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > > struct ov8856 *ov8856 = to_ov8856(sd);
> > > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > int ret = 0;
> > > + u32 val;
> > >
> > > if (ov8856->streaming == enable)
> > > return 0;
> > > @@ -908,6 +1425,44 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > > return ret;
> > > }
> > >
> > > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > + int ret;
> > > +
> > > + ret = clk_prepare_enable(ov8856->xvclk);
> > > + if (ret < 0) {
> > > + dev_err(&client->dev, "failed to enable xvclk\n");
> > > + return ret;
> > > + }
> > > +
> > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_LOW);
> > > +
> > > + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > + if (ret < 0) {
> > > + dev_err(&client->dev, "failed to enable regulators\n");
> > > + goto disable_clk;
> > > + }
> > > +
> > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, GPIOD_OUT_HIGH);
> >
> > ..., 1);
> >
> > > +
> > > + usleep_range(1400, 1500);
> > > +
> > > + return 0;
> > > +
> > > +disable_clk:
> > > + clk_disable_unprepare(ov8856->xvclk);
> >
> > How about the n_shutdn GPIO?
> >
>
> This is power up sequence function.
> When enabling regulators failed, it is required to disable the enabled
> power previously.
> Thus here we need to disable xvclk.
> But for n_shutdn GPIO, it was low state at that time.
> So we don't need to do anything for it.
Ah, right. Please ignore the comment.
>
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > > +{
> > > + gpiod_set_value_cansleep(ov8856->n_shutdn_gpio, 1);
> >
> > Why to inactivate xshutdn here (it already was)?
^
> >
> > > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> > > + clk_disable_unprepare(ov8856->xvclk);
> > > +}
> > > +
> > > static int __maybe_unused ov8856_suspend(struct device *dev)
> > > {
> > > struct i2c_client *client = to_i2c_client(dev);
> > > @@ -951,6 +1506,7 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_format *fmt)
> > > {
> > > struct ov8856 *ov8856 = to_ov8856(sd);
> > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > > const struct ov8856_mode *mode;
> > > s32 vblank_def, h_blank;
> > >
> > > @@ -960,7 +1516,9 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> > > fmt->format.height);
> > >
> > > mutex_lock(&ov8856->mutex);
> > > - ov8856_update_pad_format(mode, &fmt->format);
> > > + mbus_fmt->code = ov8856->fmt.code;
> > > + ov8856_update_pad_format(mode, mbus_fmt);
> > > + ov8856->fmt = fmt->format;
> > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> > > } else {
> > > @@ -992,13 +1550,17 @@ static int ov8856_get_format(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_format *fmt)
> > > {
> > > struct ov8856 *ov8856 = to_ov8856(sd);
> > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > >
> > > mutex_lock(&ov8856->mutex);
> > > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > fmt->format = *v4l2_subdev_get_try_format(&ov8856->sd, cfg,
> > > fmt->pad);
> > > - else
> > > - ov8856_update_pad_format(ov8856->cur_mode, &fmt->format);
> > > + } else {
> > > + fmt->format = ov8856->fmt;
> > > + ov8856_update_pad_format(ov8856->cur_mode, mbus_fmt);
> > > + mbus_fmt->code = ov8856->fmt.code;
> > > + }
> > >
> > > mutex_unlock(&ov8856->mutex);
> > >
> > > @@ -1009,11 +1571,12 @@ static int ov8856_enum_mbus_code(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_pad_config *cfg,
> > > struct v4l2_subdev_mbus_code_enum *code)
> > > {
> > > - /* Only one bayer order GRBG is supported */
> > > + struct ov8856 *ov8856 = to_ov8856(sd);
> > > +
> > > if (code->index > 0)
> > > return -EINVAL;
> > >
> > > - code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > > + code->code = ov8856->fmt.code;
> > >
> > > return 0;
> > > }
> > > @@ -1089,6 +1652,20 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> > > return -ENXIO;
> > > }
> > >
> > > + /* check sensor hardware revision */
> > > + ret = ov8856_check_revision(ov8856);
> > > + if (ret) {
> > > + dev_err(&client->dev, "failed to check sensor revision");
> > > + return ret;
> > > + }
> > > +
> > > + ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
> > > + OV8856_REG_VALUE_08BIT, &val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ov8856->is_1B_revision = (val == OV8856_1B_MODULE) ? 1 : 0;
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -1107,7 +1684,7 @@ static int ov8856_check_hwcfg(struct device *dev)
> > > return -ENXIO;
> > >
> > > fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > - if (mclk != OV8856_MCLK) {
> > > + if (mclk != OV8856_XVCLK) {
> > > dev_err(dev, "external clock %d is not supported", mclk);
> > > return -EINVAL;
> > > }
> > > @@ -1164,6 +1741,9 @@ static int ov8856_remove(struct i2c_client *client)
> > > media_entity_cleanup(&sd->entity);
> > > v4l2_ctrl_handler_free(sd->ctrl_handler);
> > > pm_runtime_disable(&client->dev);
> > > + if (!pm_runtime_status_suspended(&client->dev))
> > > + __ov8856_power_off(ov8856);
> > > + pm_runtime_set_suspended(&client->dev);
> > > mutex_destroy(&ov8856->mutex);
> > >
> > > return 0;
> > > @@ -1172,6 +1752,7 @@ static int ov8856_remove(struct i2c_client *client)
> > > static int ov8856_probe(struct i2c_client *client)
> > > {
> > > struct ov8856 *ov8856;
> > > + unsigned int i;
> > > int ret;
> > >
> > > ret = ov8856_check_hwcfg(&client->dev);
> > > @@ -1186,6 +1767,42 @@ static int ov8856_probe(struct i2c_client *client)
> > > return -ENOMEM;
> > >
> > > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > > + ov8856->fmt.code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > > +
> > > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> >
> > Does this work on ACPI?
> >
> > See e.g. the smiapp driver for an example. The same applies to the rest of
> > the chunk.
>
> For ACPI, this probe should not be executed,
> as DT setting may not have compatible property.
This probe function will be executed on ACPI as well.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 16+ messages in thread