Hi Andy, Sakari,
On Fri, Mar 9, 2018 at 5:54 PM, Sakari Ailus
<sakari.ai...@linux.intel.com> wrote:
> Hi Andy,
>
> Thanks for the update. Please see my comments below.
>
> On Fri, Mar 09, 2018 at 12:15:54AM +0800, Andy Yeh wrote:
>> Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
>> This is a camera sensor using the I2C bus for control and the
>> CSI-2 bus for data.
>>
>> Signed-off-by: Jason Chen <jasonx.z.c...@intel.com>
>> Signed-off-by: Alan Chiang <alanx.chi...@intel.com>
>> ---
>> since v2:
>> -- Update the streaming function to remove SW_STANDBY in the beginning.
>> -- Adjust the delay time from 1ms to 12ms before set stream-on register.
>> since v3:
>> -- fix the sd.entity to make code be compiled on the mainline kernel.
>> since v4:
>> -- Enabled AG, DG, and Exposure time control correctly.
>> since v5:
>> -- Sensor vendor provided a new setting to fix different CLK issue
>> -- Add one more resolution for 1048x780, used for VGA streaming
>> since v6:
>> -- improve i2c write function to support writing 2 registers
>> -- Not Orring ret in update_digital_gain which lead to unintended error
>> since v7:
>> -- modified imx258_reg read / write function with a more portable way
>> -- arranged some format per suggestions
>>
>>
>> MAINTAINERS | 7 +
>> drivers/media/i2c/Kconfig | 11 +
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/imx258.c | 1324
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 1343 insertions(+)
>> create mode 100644 drivers/media/i2c/imx258.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a339bb5..9f75510 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12646,6 +12646,13 @@ S: Maintained
>> F: drivers/ssb/
>> F: include/linux/ssb/
>>
>> +SONY IMX258 SENSOR DRIVER
>> +M: Sakari Ailus <sakari.ai...@linux.intel.com>
>> +L: linux-media@vger.kernel.org
>> +T: git git://linuxtv.org/media_tree.git
>> +S: Maintained
>> +F: drivers/media/i2c/imx258.c
>> +
>> SONY IMX274 SENSOR DRIVER
>> M: Leon Luo <le...@leopardimaging.com>
>> L: linux-media@vger.kernel.org
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index fd01842..bcd4bf1 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -565,6 +565,17 @@ config VIDEO_APTINA_PLL
>> config VIDEO_SMIAPP_PLL
>> tristate
>>
>> +config VIDEO_IMX258
>> + tristate "Sony IMX258 sensor support"
>> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> + depends on MEDIA_CAMERA_SUPPORT
>> + ---help---
>> + This is a Video4Linux2 sensor-level driver for the Sony
>> + IMX258 camera.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called imx258.
>> +
>> config VIDEO_IMX274
>> tristate "Sony IMX274 sensor support"
>> depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 1b62639..4bf7d00 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -94,6 +94,7 @@ obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
>> obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o
>> obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
>> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
>> +obj-$(CONFIG_VIDEO_IMX258) += imx258.o
>> obj-$(CONFIG_VIDEO_IMX274) += imx274.o
>>
>> obj-$(CONFIG_SDR_MAX2175) += max2175.o
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> new file mode 100644
>> index 0000000..814520f
>> --- /dev/null
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -0,0 +1,1324 @@
>> +// Copyright (C) 2018 Intel Corporation
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/pm_runtime.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-device.h>
>> +#include <asm/unaligned.h>
>> +
>> +#define IMX258_REG_VALUE_08BIT 1
>> +#define IMX258_REG_VALUE_16BIT 2
>> +#define IMX258_REG_VALUE_24BIT 3
>
> The last one is not used. Perhaps because the sensor does not have any
> 24-bit registers? :-) How about removing it?
I pointed in my earlier review comments that we don't even need these
macros anymore. They add as much value as defining ONE = 1 and TWO =
2.
Andy, this is already a second re-spin of this patch, where my
previous comments are left unaddressed.
>
>> +
>> +#define IMX258_REG_MODE_SELECT 0x0100
>> +#define IMX258_MODE_STANDBY 0x00
>> +#define IMX258_MODE_STREAMING 0x01
>> +
>> +/* Chip ID */
>> +#define IMX258_REG_CHIP_ID 0x0016
>> +#define IMX258_CHIP_ID 0x0258
>> +
>> +/* V_TIMING internal */
>> +#define IMX258_REG_VTS 0x0340
>> +#define IMX258_VTS_30FPS 0x0c98
>> +#define IMX258_VTS_MAX 0xffff
>> +
>> +/*Frame Length Line*/
>> +#define IMX258_FLL_MIN 0x08a6
>> +#define IMX258_FLL_MAX 0xffff
>> +#define IMX258_FLL_STEP 1
>> +#define IMX258_FLL_DEFAULT 0x0c98
>> +
>> +/* HBLANK control - read only */
>> +#define IMX258_PPL_DEFAULT 5352
>> +
>> +/* Exposure control */
>> +#define IMX258_REG_EXPOSURE 0x0202
>> +#define IMX258_EXPOSURE_MIN 4
>> +#define IMX258_EXPOSURE_STEP 1
>> +#define IMX258_EXPOSURE_DEFAULT 0x640
>> +#define IMX258_EXPOSURE_MAX 65535
>> +
>> +/* Analog gain control */
>> +#define IMX258_REG_ANALOG_GAIN 0x0204
>> +#define IMX258_ANA_GAIN_MIN 0
>> +#define IMX258_ANA_GAIN_MAX 0x1fff
>> +#define IMX258_ANA_GAIN_STEP 1
>> +#define IMX258_ANA_GAIN_DEFAULT 0x0
>> +
>> +/* Digital gain control */
>> +#define IMX258_REG_GR_DIGITAL_GAIN 0x020e
>> +#define IMX258_REG_R_DIGITAL_GAIN 0x0210
>> +#define IMX258_REG_B_DIGITAL_GAIN 0x0212
>> +#define IMX258_REG_GB_DIGITAL_GAIN 0x0214
>> +#define IMX258_DGTL_GAIN_MIN 0
>> +#define IMX258_DGTL_GAIN_MAX 4096 /* Max = 0xFFF */
>> +#define IMX258_DGTL_GAIN_DEFAULT 1024
>> +#define IMX258_DGTL_GAIN_STEP 1
>> +
>> +/* Orientation */
>> +#define REG_MIRROR_FLIP_CONTROL 0x0101
>> +#define REG_CONFIG_MIRROR_FLIP 0x03
>> +
>> +struct imx258_reg {
>> + u16 address;
>> + u8 val;
>> +};
>> +
>> +struct imx258_reg_list {
>> + u32 num_of_regs;
>> + const struct imx258_reg *regs;
>> +};
>> +
>> +/* Link frequency config */
>> +struct imx258_link_freq_config {
>> + u32 pixels_per_line;
>> +
>> + /* PLL registers for this link frequency */
>> + struct imx258_reg_list reg_list;
>> +};
>> +
>> +/* Mode : resolution and related config&values */
>> +struct imx258_mode {
>> + /* Frame width */
>> + u32 width;
>> + /* Frame height */
>> + u32 height;
>> +
>> + /* V-timing */
>> + u32 vts_def;
>> + u32 vts_min;
>> +
>> + /* Index of Link frequency config to be used */
>> + u32 link_freq_index;
>> + /* Default register values */
>> + struct imx258_reg_list reg_list;
>> +};
>> +
>> +/* 4208x3118 needs 1267Mbps/lane, 4 lanes */
>> +static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>> + { 0x0301, 0x05},
>
> If you have a space after the opening brace (which I'd recommend), you
> should have one before the closing one as well.
>
>> + { 0x0303, 0x02},
>> + { 0x0305, 0x03},
>> + { 0x0306, 0x00},
>> + { 0x0307, 0xC6},
>> + { 0x0309, 0x0A},
>> + { 0x030B, 0x01},
>> + { 0x030D, 0x02},
>> + { 0x030E, 0x00},
>> + { 0x030F, 0xD8},
>> + { 0x0310, 0x00},
>> + { 0x0820, 0x13},
>> + { 0x0821, 0x4C},
>> + { 0x0822, 0xCC},
>> + { 0x0823, 0xCC},
>> +};
>> +
>> +static const struct imx258_reg mipi_data_rate_640mbps[] = {
>> + { 0x0301, 0x05},
>> + { 0x0303, 0x02},
>> + { 0x0305, 0x03},
>> + { 0x0306, 0x00},
>> + { 0x0307, 0x64},
>> + { 0x0309, 0x0A},
>> + { 0x030B, 0x01},
>> + { 0x030D, 0x02},
>> + { 0x030E, 0x00},
>> + { 0x030F, 0xD8},
>> + { 0x0310, 0x00},
>> + { 0x0820, 0x0A},
>> + { 0x0821, 0x00},
>> + { 0x0822, 0x00},
>> + { 0x0823, 0x00},
>> +};
>> +
>> +static const struct imx258_reg mode_4208x3118_regs[] = {
>> + { 0x0136, 0x13},
>> + { 0x0137, 0x33},
>> + { 0x3051, 0x00},
>> + { 0x3052, 0x00},
>> + { 0x4E21, 0x14},
>> + { 0x6B11, 0xCF},
>> + { 0x7FF0, 0x08},
>> + { 0x7FF1, 0x0F},
>> + { 0x7FF2, 0x08},
>> + { 0x7FF3, 0x1B},
>> + { 0x7FF4, 0x23},
>> + { 0x7FF5, 0x60},
>> + { 0x7FF6, 0x00},
>> + { 0x7FF7, 0x01},
>> + { 0x7FF8, 0x00},
>> + { 0x7FF9, 0x78},
>> + { 0x7FFA, 0x00},
>> + { 0x7FFB, 0x00},
>> + { 0x7FFC, 0x00},
>> + { 0x7FFD, 0x00},
>> + { 0x7FFE, 0x00},
>> + { 0x7FFF, 0x03},
>> + { 0x7F76, 0x03},
>> + { 0x7F77, 0xFE},
>> + { 0x7FA8, 0x03},
>> + { 0x7FA9, 0xFE},
>> + { 0x7B24, 0x81},
>> + { 0x7B25, 0x00},
>> + { 0x6564, 0x07},
>> + { 0x6B0D, 0x41},
>> + { 0x653D, 0x04},
>> + { 0x6B05, 0x8C},
>> + { 0x6B06, 0xF9},
>> + { 0x6B08, 0x65},
>> + { 0x6B09, 0xFC},
>> + { 0x6B0A, 0xCF},
>> + { 0x6B0B, 0xD2},
>> + { 0x6700, 0x0E},
>> + { 0x6707, 0x0E},
>> + { 0x9104, 0x00},
>> + { 0x4648, 0x7F},
>> + { 0x7420, 0x00},
>> + { 0x7421, 0x1C},
>> + { 0x7422, 0x00},
>> + { 0x7423, 0xD7},
>> + { 0x5F04, 0x00},
>> + { 0x5F05, 0xED},
>> + { 0x0112, 0x0A},
>> + { 0x0113, 0x0A},
>> + { 0x0114, 0x03},
>> + { 0x0342, 0x14},
>> + { 0x0343, 0xE8},
>> + { 0x0340, 0x0C},
>> + { 0x0341, 0x50},
>> + { 0x0344, 0x00},
>> + { 0x0345, 0x00},
>> + { 0x0346, 0x00},
>> + { 0x0347, 0x00},
>> + { 0x0348, 0x10},
>> + { 0x0349, 0x6F},
>> + { 0x034A, 0x0C},
>> + { 0x034B, 0x2E},
>> + { 0x0381, 0x01},
>> + { 0x0383, 0x01},
>> + { 0x0385, 0x01},
>> + { 0x0387, 0x01},
>> + { 0x0900, 0x00},
>> + { 0x0901, 0x11},
>> + { 0x0401, 0x00},
>> + { 0x0404, 0x00},
>> + { 0x0405, 0x10},
>> + { 0x0408, 0x00},
>> + { 0x0409, 0x00},
>> + { 0x040A, 0x00},
>> + { 0x040B, 0x00},
>> + { 0x040C, 0x10},
>> + { 0x040D, 0x70},
>> + { 0x040E, 0x0C},
>> + { 0x040F, 0x30},
>> + { 0x3038, 0x00},
>> + { 0x303A, 0x00},
>> + { 0x303B, 0x10},
>> + { 0x300D, 0x00},
>> + { 0x034C, 0x10},
>> + { 0x034D, 0x70},
>> + { 0x034E, 0x0C},
>> + { 0x034F, 0x30},
>> + { 0x0202, 0x0C},
>> + { 0x0203, 0x46},
>> + { 0x0204, 0x00},
>> + { 0x0205, 0x00},
>> + { 0x020E, 0x01},
>> + { 0x020F, 0x00},
>> + { 0x0210, 0x01},
>> + { 0x0211, 0x00},
>> + { 0x0212, 0x01},
>> + { 0x0213, 0x00},
>> + { 0x0214, 0x01},
>> + { 0x0215, 0x00},
>> + { 0x7BCD, 0x00},
>> + { 0x94DC, 0x20},
>> + { 0x94DD, 0x20},
>> + { 0x94DE, 0x20},
>> + { 0x95DC, 0x20},
>> + { 0x95DD, 0x20},
>> + { 0x95DE, 0x20},
>> + { 0x7FB0, 0x00},
>> + { 0x9010, 0x3E},
>> + { 0x9419, 0x50},
>> + { 0x941B, 0x50},
>> + { 0x9519, 0x50},
>> + { 0x951B, 0x50},
>> + { 0x3030, 0x00},
>> + { 0x3032, 0x00},
>> + { 0x0220, 0x00},
>> +};
>> +
>> +static const struct imx258_reg mode_2104_1560_regs[] = {
>> + { 0x0136, 0x13},
>> + { 0x0137, 0x33},
>> + { 0x3051, 0x00},
>> + { 0x3052, 0x00},
>> + { 0x4E21, 0x14},
>> + { 0x6B11, 0xCF},
>> + { 0x7FF0, 0x08},
>> + { 0x7FF1, 0x0F},
>> + { 0x7FF2, 0x08},
>> + { 0x7FF3, 0x1B},
>> + { 0x7FF4, 0x23},
>> + { 0x7FF5, 0x60},
>> + { 0x7FF6, 0x00},
>> + { 0x7FF7, 0x01},
>> + { 0x7FF8, 0x00},
>> + { 0x7FF9, 0x78},
>> + { 0x7FFA, 0x00},
>> + { 0x7FFB, 0x00},
>> + { 0x7FFC, 0x00},
>> + { 0x7FFD, 0x00},
>> + { 0x7FFE, 0x00},
>> + { 0x7FFF, 0x03},
>> + { 0x7F76, 0x03},
>> + { 0x7F77, 0xFE},
>> + { 0x7FA8, 0x03},
>> + { 0x7FA9, 0xFE},
>> + { 0x7B24, 0x81},
>> + { 0x7B25, 0x00},
>> + { 0x6564, 0x07},
>> + { 0x6B0D, 0x41},
>> + { 0x653D, 0x04},
>> + { 0x6B05, 0x8C},
>> + { 0x6B06, 0xF9},
>> + { 0x6B08, 0x65},
>> + { 0x6B09, 0xFC},
>> + { 0x6B0A, 0xCF},
>> + { 0x6B0B, 0xD2},
>> + { 0x6700, 0x0E},
>> + { 0x6707, 0x0E},
>> + { 0x9104, 0x00},
>> + { 0x4648, 0x7F},
>> + { 0x7420, 0x00},
>> + { 0x7421, 0x1C},
>> + { 0x7422, 0x00},
>> + { 0x7423, 0xD7},
>> + { 0x5F04, 0x00},
>> + { 0x5F05, 0xED},
>> + { 0x0112, 0x0A},
>> + { 0x0113, 0x0A},
>> + { 0x0114, 0x03},
>> + { 0x0342, 0x14},
>> + { 0x0343, 0xE8},
>> + { 0x0340, 0x06},
>> + { 0x0341, 0x38},
>> + { 0x0344, 0x00},
>> + { 0x0345, 0x00},
>> + { 0x0346, 0x00},
>> + { 0x0347, 0x00},
>> + { 0x0348, 0x10},
>> + { 0x0349, 0x6F},
>> + { 0x034A, 0x0C},
>> + { 0x034B, 0x2E},
>> + { 0x0381, 0x01},
>> + { 0x0383, 0x01},
>> + { 0x0385, 0x01},
>> + { 0x0387, 0x01},
>> + { 0x0900, 0x01},
>> + { 0x0901, 0x12},
>> + { 0x0401, 0x01},
>> + { 0x0404, 0x00},
>> + { 0x0405, 0x20},
>> + { 0x0408, 0x00},
>> + { 0x0409, 0x02},
>> + { 0x040A, 0x00},
>> + { 0x040B, 0x00},
>> + { 0x040C, 0x10},
>> + { 0x040D, 0x6A},
>> + { 0x040E, 0x06},
>> + { 0x040F, 0x18},
>> + { 0x3038, 0x00},
>> + { 0x303A, 0x00},
>> + { 0x303B, 0x10},
>> + { 0x300D, 0x00},
>> + { 0x034C, 0x08},
>> + { 0x034D, 0x38},
>> + { 0x034E, 0x06},
>> + { 0x034F, 0x18},
>> + { 0x0202, 0x06},
>> + { 0x0203, 0x2E},
>> + { 0x0204, 0x00},
>> + { 0x0205, 0x00},
>> + { 0x020E, 0x01},
>> + { 0x020F, 0x00},
>> + { 0x0210, 0x01},
>> + { 0x0211, 0x00},
>> + { 0x0212, 0x01},
>> + { 0x0213, 0x00},
>> + { 0x0214, 0x01},
>> + { 0x0215, 0x00},
>> + { 0x7BCD, 0x01},
>> + { 0x94DC, 0x20},
>> + { 0x94DD, 0x20},
>> + { 0x94DE, 0x20},
>> + { 0x95DC, 0x20},
>> + { 0x95DD, 0x20},
>> + { 0x95DE, 0x20},
>> + { 0x7FB0, 0x00},
>> + { 0x9010, 0x3E},
>> + { 0x9419, 0x50},
>> + { 0x941B, 0x50},
>> + { 0x9519, 0x50},
>> + { 0x951B, 0x50},
>> + { 0x3030, 0x00},
>> + { 0x3032, 0x00},
>> + { 0x0220, 0x00},
>> +};
>> +
>> +static const struct imx258_reg mode_1048_780_regs[] = {
>> + { 0x0136, 0x13},
>> + { 0x0137, 0x33},
>> + { 0x3051, 0x00},
>> + { 0x3052, 0x00},
>> + { 0x4E21, 0x14},
>> + { 0x6B11, 0xCF},
>> + { 0x7FF0, 0x08},
>> + { 0x7FF1, 0x0F},
>> + { 0x7FF2, 0x08},
>> + { 0x7FF3, 0x1B},
>> + { 0x7FF4, 0x23},
>> + { 0x7FF5, 0x60},
>> + { 0x7FF6, 0x00},
>> + { 0x7FF7, 0x01},
>> + { 0x7FF8, 0x00},
>> + { 0x7FF9, 0x78},
>> + { 0x7FFA, 0x00},
>> + { 0x7FFB, 0x00},
>> + { 0x7FFC, 0x00},
>> + { 0x7FFD, 0x00},
>> + { 0x7FFE, 0x00},
>> + { 0x7FFF, 0x03},
>> + { 0x7F76, 0x03},
>> + { 0x7F77, 0xFE},
>> + { 0x7FA8, 0x03},
>> + { 0x7FA9, 0xFE},
>> + { 0x7B24, 0x81},
>> + { 0x7B25, 0x00},
>> + { 0x6564, 0x07},
>> + { 0x6B0D, 0x41},
>> + { 0x653D, 0x04},
>> + { 0x6B05, 0x8C},
>> + { 0x6B06, 0xF9},
>> + { 0x6B08, 0x65},
>> + { 0x6B09, 0xFC},
>> + { 0x6B0A, 0xCF},
>> + { 0x6B0B, 0xD2},
>> + { 0x6700, 0x0E},
>> + { 0x6707, 0x0E},
>> + { 0x9104, 0x00},
>> + { 0x4648, 0x7F},
>> + { 0x7420, 0x00},
>> + { 0x7421, 0x1C},
>> + { 0x7422, 0x00},
>> + { 0x7423, 0xD7},
>> + { 0x5F04, 0x00},
>> + { 0x5F05, 0xED},
>> + { 0x0112, 0x0A},
>> + { 0x0113, 0x0A},
>> + { 0x0114, 0x03},
>> + { 0x0342, 0x14},
>> + { 0x0343, 0xE8},
>> + { 0x0340, 0x03},
>> + { 0x0341, 0x4C},
>> + { 0x0344, 0x00},
>> + { 0x0345, 0x00},
>> + { 0x0346, 0x00},
>> + { 0x0347, 0x00},
>> + { 0x0348, 0x10},
>> + { 0x0349, 0x6F},
>> + { 0x034A, 0x0C},
>> + { 0x034B, 0x2E},
>> + { 0x0381, 0x01},
>> + { 0x0383, 0x01},
>> + { 0x0385, 0x01},
>> + { 0x0387, 0x01},
>> + { 0x0900, 0x01},
>> + { 0x0901, 0x14},
>> + { 0x0401, 0x01},
>> + { 0x0404, 0x00},
>> + { 0x0405, 0x40},
>> + { 0x0408, 0x00},
>> + { 0x0409, 0x06},
>> + { 0x040A, 0x00},
>> + { 0x040B, 0x00},
>> + { 0x040C, 0x10},
>> + { 0x040D, 0x64},
>> + { 0x040E, 0x03},
>> + { 0x040F, 0x0C},
>> + { 0x3038, 0x00},
>> + { 0x303A, 0x00},
>> + { 0x303B, 0x10},
>> + { 0x300D, 0x00},
>> + { 0x034C, 0x04},
>> + { 0x034D, 0x18},
>> + { 0x034E, 0x03},
>> + { 0x034F, 0x0C},
>> + { 0x0202, 0x03},
>> + { 0x0203, 0x42},
>> + { 0x0204, 0x00},
>> + { 0x0205, 0x00},
>> + { 0x020E, 0x01},
>> + { 0x020F, 0x00},
>> + { 0x0210, 0x01},
>> + { 0x0211, 0x00},
>> + { 0x0212, 0x01},
>> + { 0x0213, 0x00},
>> + { 0x0214, 0x01},
>> + { 0x0215, 0x00},
>> + { 0x7BCD, 0x00},
>> + { 0x94DC, 0x20},
>> + { 0x94DD, 0x20},
>> + { 0x94DE, 0x20},
>> + { 0x95DC, 0x20},
>> + { 0x95DD, 0x20},
>> + { 0x95DE, 0x20},
>> + { 0x7FB0, 0x00},
>> + { 0x9010, 0x3E},
>> + { 0x9419, 0x50},
>> + { 0x941B, 0x50},
>> + { 0x9519, 0x50},
>> + { 0x951B, 0x50},
>> + { 0x3030, 0x00},
>> + { 0x3032, 0x00},
>> + { 0x0220, 0x00},
>> +};
>> +
>> +static const char * const imx258_test_pattern_menu[] = {
>> + "Disabled",
>> + "Vertical Color Bar Type 1",
>> + "Vertical Color Bar Type 2",
>> + "Vertical Color Bar Type 3",
>> + "Vertical Color Bar Type 4"
>> +};
>> +
>> +/* Configurations for supported link frequencies */
>> +#define IMX258_LINK_FREQ_634MHZ 633600000ULL
>> +#define IMX258_LINK_FREQ_320MHZ 320000000ULL
>> +
>> +/*
>> + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample
>> + * data rate => double data rate; number of lanes => 4; bits per pixel => 10
>> + */
>> +static u64 link_freq_to_pixel_rate(u64 f)
>> +{
>> + f *= 2 * 4;
>> + do_div(f, 10);
>> +
>> + return f;
>> +}
>> +
>> +/* Menu items for LINK_FREQ V4L2 control */
>> +static const s64 link_freq_menu_items[] = {
>> + IMX258_LINK_FREQ_634MHZ,
>> + IMX258_LINK_FREQ_320MHZ,
>> +};
>> +
>> +/* Link frequency configs */
>> +static const struct imx258_link_freq_config link_freq_configs[] = {
>> + {
>> + .pixels_per_line = IMX258_PPL_DEFAULT,
>> + .reg_list = {
>> + .num_of_regs = ARRAY_SIZE(mipi_data_rate_1267mbps),
>> + .regs = mipi_data_rate_1267mbps,
>> + }
>> + },
>> + {
>> + .pixels_per_line = IMX258_PPL_DEFAULT,
>> + .reg_list = {
>> + .num_of_regs = ARRAY_SIZE(mipi_data_rate_640mbps),
>> + .regs = mipi_data_rate_640mbps,
>> + }
>> + },
>> +};
I also had comments for using explicit indices in this array, to avoid
mistakes in supported_modes[] below.
>> +
>> +/* Mode configs */
>> +static const struct imx258_mode supported_modes[] = {
>> + {
>> + .width = 4208,
>> + .height = 3118,
>> + .vts_def = IMX258_VTS_30FPS,
>> + .vts_min = IMX258_VTS_30FPS,
>> + .reg_list = {
>> + .num_of_regs = ARRAY_SIZE(mode_4208x3118_regs),
>> + .regs = mode_4208x3118_regs,
>> + },
>> + .link_freq_index = 0,
>> + },
>> + {
>> + .width = 2104,
>> + .height = 1560,
>> + .vts_def = IMX258_VTS_30FPS,
>> + .vts_min = 1608,
>> + .reg_list = {
>> + .num_of_regs = ARRAY_SIZE(mode_2104_1560_regs),
>> + .regs = mode_2104_1560_regs,
>> + },
>> + .link_freq_index = 1,
>> + },
>> + {
>> + .width = 1048,
>> + .height = 780,
>> + .vts_def = IMX258_VTS_30FPS,
>> + .vts_min = 804,
>> + .reg_list = {
>> + .num_of_regs = ARRAY_SIZE(mode_1048_780_regs),
>> + .regs = mode_1048_780_regs,
>> + },
>> + .link_freq_index = 1,
>> + },
>> +};
>> +
>> +struct imx258 {
>> + struct v4l2_subdev sd;
>> + struct media_pad pad;
>> +
>> + struct v4l2_ctrl_handler ctrl_handler;
>> + /* V4L2 Controls */
>> + struct v4l2_ctrl *link_freq;
>> + struct v4l2_ctrl *pixel_rate;
>> + struct v4l2_ctrl *vblank;
>> + struct v4l2_ctrl *hblank;
>> + struct v4l2_ctrl *exposure;
>> +
>> + /* Current mode */
>> + const struct imx258_mode *cur_mode;
>> +
>> + /* Mutex for serialized access */
>> + struct mutex mutex;
>> + /*
>> + * Protect sensor module set pad format and start streaming normally.
>> + */
>> +
>> + /* Streaming on/off */
>> + bool streaming;
>> +};
>> +
>> +#define to_imx258(_sd) container_of(_sd, struct imx258, sd)
>> +
>> +/* Read registers up to 2 at a time */
>> +static int imx258_read_reg(struct imx258 *imx258, u16 reg, u32 len, u32
>> *val)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>> + struct i2c_msg msgs[2];
>> + u8 addr_buf[2] = { reg >> 8, reg & 0xff };
>> + u8 data_buf[4] = { 0, };
>> + int ret;
>> +
>> + if (len > 4)
>> + return -EINVAL;
>> +
>> + /* Write register address */
>> + msgs[0].addr = client->addr;
>> + msgs[0].flags = 0;
>> + msgs[0].len = ARRAY_SIZE(addr_buf);
>> + msgs[0].buf = addr_buf;
>> +
>> + /* Read data from register */
>> + msgs[1].addr = client->addr;
>> + msgs[1].flags = I2C_M_RD;
>> + msgs[1].len = len;
>> + msgs[1].buf = &data_buf[4 - len];
>> +
>> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>> + if (ret != ARRAY_SIZE(msgs))
>> + return -EIO;
>> +
>> + *val = get_unaligned_be32(data_buf);
>> +
>> + return 0;
>> +}
>> +
>> +/* Write registers up to 2 at a time */
>> +static int imx258_write_reg(struct imx258 *imx258, u16 reg, u32 len, u32
>> val)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>> + u8 __buf[6], *buf = __buf;
>> +
>> + if (len > 4)
>> + return -EINVAL;
>> +
>> + *buf++ = reg >> 8;
>> + *buf++ = reg & 0xff;
>
> You assign reg in variable declaration in imx258_read_reg(). Could you do
> the same here? Or even better, use put_unaligned_be16().
>
> I wasn't aware of these functions, thanks for introducing them to me. :-)
>
> You can then remove buf and rename __buf as buf.
I believe I gave an exact implementation, without that problem, in my
previous comments actually.
Andy, please, really please, go through all the comments in my reply
to v6 on the list and make sure that they are all addressed. Perhaps
reply to it, with "Okay" next to each comment, to make sure some of
the message was not lost due to some weird mail client settings.
Best regards,
Tomasz
