RE: [PATCH v11] media: imx258: Add imx258 camera sensor driver

["Chen, JasonX Z" <jasonx.z.chen@intel.com>]

Hello Tomasz

>> +/* Test Pattern Control */
>> +#define IMX258_REG_TEST_PATTERN                0x0600
>> +#define IMX258_TEST_PATTERN_DISABLE    0
>> +#define IMX258_TEST_PATTERN_SOLID_COLOR        1
>> +#define IMX258_TEST_PATTERN_COLOR_BARS 2 #define 
>> +IMX258_TEST_PATTERN_GREY_COLOR 3
>> +#define IMX258_TEST_PATTERN_PN9                4
>> +
>> +/* Orientation */
>> +#define REG_MIRROR_FLIP_CONTROL                0x0101
>> +#define REG_CONFIG_MIRROR_FLIP         0x03
>> +#define REG_CONFIG_FLIP_TEST_PATTERN   0x02
>
>The names are inconsistent here. All other register addresses start with IMX258_REG and values with IMX258_<field name> (no REG).
>
>[snip]

We will update at next patch.

>> +static const char * const imx258_test_pattern_menu[] = {
>> +       "Disabled",
>> +       "Color Bars",
>> +       "Solid Color",
>> +       "Grey Color Bars",
>> +       "PN9"
>> +};
>> +
>> +static const int imx258_test_pattern_val[] = {
>> +       IMX258_TEST_PATTERN_DISABLE,
>> +       IMX258_TEST_PATTERN_COLOR_BARS,
>> +       IMX258_TEST_PATTERN_SOLID_COLOR,
>> +       IMX258_TEST_PATTERN_GREY_COLOR,
>> +       IMX258_TEST_PATTERN_PN9,
>> +};
>
>By reordering imx258_test_pattern_menu[], this array can be removed and
>ctrl->val can be used directly. It is validated by control framework to 
>ctrl->be
>within menu range and so safe to be used for programming hardware.
>
>[snip]

IPU3 HAL has a handler to bind test_pattern mode.
The COLOR BAR MODE in HAL has been configured to 1 when APP requests to output color bar image. 
However Sony sensor's COLOR BAR MODE is designed as 2 in register table. (grey color bars as 1).
When HAL sends handler to driver to switch test pattern mode (to COLOR BAR - val: 1), it will be grey color, since driver still set TEST_PATTERN_MODE reg value to 1, those it is not what we expected.

That is why we have to make an array with index to arrange the order of the test pattern items, so driver will choose COLOR BAR correctly when HAL send test_pattern message (with 1).
The concept is the test_pattern_menu could be listed in driver per real requirement, no matter how the sensor register is designed.

>> +       case V4L2_CID_TEST_PATTERN:
>> +               ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
>> +                               IMX258_REG_VALUE_16BIT,
>> +                               imx258_test_pattern_val[ctrl->val]);
>> +
>> +               ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
>> +                               IMX258_REG_VALUE_08BIT,
>> +                               ctrl->val == imx258_test_pattern_val
>> +                               [IMX258_TEST_PATTERN_DISABLE] ?
>> +                               REG_CONFIG_MIRROR_FLIP :
>> +                               REG_CONFIG_FLIP_TEST_PATTERN);
>
>The comparison above doesn't make any sense. ctrl->val is an index into imx258_test_pattern_val[], but imx258_test_pattern_val[IMX258_TEST_PATTERN_DISABLE] is a register value.
>Moreover, IMX258_TEST_PATTERN_DISABLE is also a register value, so it doesn't make sense to use it for indexing the array. I'd suggest simply checking for (!ctrl->val).
>

We will update at next patch.


B.R.,
Jason