Hi Jacopo,
Many thanks for the explanations on commit message, I'm currently
writing the v2, but if you already have comments on this v1 -appart from
the power off section- feel free to post.
BR,
Hugues.
On 10/12/20 4:27 PM, Jacopo Mondi wrote:
> Hi Hugues
>
> On Mon, Oct 12, 2020 at 02:12:09PM +0000, Hugues FRUCHET wrote:
>> Hi Jacopo,
>>
>> Thanks for reviewing, comments below
>>
>> On 10/12/20 3:35 PM, Jacopo Mondi wrote:
>>> Hi Hugues,
>>>
>>> On Wed, Oct 07, 2020 at 04:24:52PM +0200, Hugues Fruchet wrote:
>
>>>
>>> As per the comment from Sakari in another patch, 'Fixes' (capital 'F')
>>> usually comes before Signed-off/Reviewed-by tags
>> I understand the "F" upper case, but I don't understand the "comes
>> before Signed-off", my signed-off is inserted after this line.
>>
>
> So, this is not stated explicitly in submitting-patches.rst, but as
> per all the other tags attached to a patch, they're usually placed
> after the commit message
>
> in this case:
>
> Fix PCLK polarity not being taken into account.
> Fix ov5640_write_reg()return value unchecked at power off.
> Reformat code to keep register access below the register description.
> Remove useless ov5640_set_stream_bt656() function.
>
> Fixes: 4039b03720f7 ("media: i2c: ov5640: Add support for BT656 mode")
> Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxx>
>
>>>
>>>>
>>>> Fix PCLK polarity not being taken into account.
>>
>> This one fixes a real bug.
>>
>
> I don't think I've contested this
>
>>>> Fix ov5640_write_reg()return value unchecked at power off.
>>>
>>> Am I wrong or you broke out this part to a separate patch ?
>>> As commented there, I don't think it's a good idea.
>>>
>>> Are you planning to send a v2 of this patch ?
>> The patches was sent at same time, so it's normal that they are designed
>> in same way, I'll send a v2 with this "reset to default at power off
>> without check" strategy.
>>
>
> I was wondering if you were planning to send a v2 with the part that
> changed the power-off path removed as I would have reviewed that one
> instead of this, that's all.
>
>
>>>
>>> Thanks
>>> j
>>>
>>>> Reformat code to keep register access below the register description.
>>>> Remove useless ov5640_set_stream_bt656() function.
>>>>
>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxx>
>>>> ---
>>>> drivers/media/i2c/ov5640.c | 95 +++++++++++++++++++++++++---------------------
>>>> 1 file changed, 51 insertions(+), 44 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>> index 8d0254d..1b20db7 100644
>>>> --- a/drivers/media/i2c/ov5640.c
>>>> +++ b/drivers/media/i2c/ov5640.c
>>>> @@ -1216,20 +1216,6 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>>>> BIT(1), on ? 0 : BIT(1));
>>>> }
>>>>
>>>> -static int ov5640_set_stream_bt656(struct ov5640_dev *sensor, bool on)
>>>> -{
>>>> - int ret;
>>>> -
>>>> - ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00,
>>>> - on ? 0x1 : 0x00);
>>>> - if (ret)
>>>> - return ret;
>>>> -
>>>> - return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>>> - OV5640_REG_SYS_CTRL0_SW_PWUP :
>>>> - OV5640_REG_SYS_CTRL0_SW_PWDN);
>>>> -}
>>>> -
>>>> static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>>> {
>>>> return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>>> @@ -1994,20 +1980,12 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>>>> static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
>>>> {
>>>> unsigned int flags = sensor->ep.bus.parallel.flags;
>>>> + bool bt656 = sensor->ep.bus_type == V4L2_MBUS_BT656;
>>>> u8 pclk_pol = 0;
>>>> u8 hsync_pol = 0;
>>>> u8 vsync_pol = 0;
>>>> int ret;
>>>>
>>>> - if (!on) {
>>>> - /* Reset settings to their default values. */
>>>> - ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
>>>> - ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
>>>> - ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
>>>> - ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
>>>> - return 0;
>>>> - }
>>>> -
>>>> /*
>>>> * Note about parallel port configuration.
>>>> *
>>>> @@ -2024,27 +2002,57 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
>>>> * - VSYNC: active high
>>>> * - HREF: active low
>>>> * - PCLK: active low
>>>> + *
>>>> + * VSYNC & HREF are not configured if BT656 bus mode is selected
>>>> */
>>>> +
>>>> /*
>>>> - * configure parallel port control lines polarity
>>>> + * BT656 embedded synchronization configuration
>>>> + *
>>>> + * CCIR656 CTRL00
>>>> + * - [7]: SYNC code selection (0: auto generate sync code,
>>>> + * 1: sync code from regs 0x4732-0x4735)
>>>> + * - [6]: f value in CCIR656 SYNC code when fixed f value
>>>> + * - [5]: Fixed f value
>>>> + * - [4:3]: Blank toggle data options (00: data=1'h040/1'h200,
>>>> + * 01: data from regs 0x4736-0x4738, 10: always keep 0)
>>>> + * - [1]: Clip data disable
>>>> + * - [0]: CCIR656 mode enable
>>>> *
>>>> - * POLARITY CTRL0
>>>> - * - [5]: PCLK polarity (0: active low, 1: active high)
>>>> - * - [1]: HREF polarity (0: active low, 1: active high)
>>>> - * - [0]: VSYNC polarity (mismatch here between
>>>> - * datasheet and hardware, 0 is active high
>>>> - * and 1 is active low...)
>>>> + * Default CCIR656 SAV/EAV mode with default codes
>>>> + * SAV=0xff000080 & EAV=0xff00009d is enabled here with settings:
>>>> + * - CCIR656 mode enable
>>>> + * - auto generation of sync codes
>>>> + * - blank toggle data 1'h040/1'h200
>>>> + * - clip reserved data (0x00 & 0xff changed to 0x01 & 0xfe)
>>>> */
>>>> - if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) {
>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00,
>>>> + on && bt656 ? 0x01 : 0x00);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (on) {
>>>> + /*
>>>> + * configure parallel port control lines polarity
>>>> + *
>>>> + * POLARITY CTRL0
>>>> + * - [5]: PCLK polarity (0: active low, 1: active high)
>>>> + * - [1]: HREF polarity (0: active low, 1: active high)
>>>> + * - [0]: VSYNC polarity (mismatch here between
>>>> + * datasheet and hardware, 0 is active high
>>>> + * and 1 is active low...)
>>>> + */
>>>> if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
>>>> pclk_pol = 1;
>>>> - if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
>>>> + if (!bt656 && flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
>>>> hsync_pol = 1;
>>>> - if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
>>>> + if (!bt656 && flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
>>>> vsync_pol = 1;
>>>>
>>>> - ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
>>>> - (pclk_pol << 5) | (hsync_pol << 1) |
>>>> + ret = ov5640_write_reg(sensor,
>>>> + OV5640_REG_POLARITY_CTRL00,
>>>> + (pclk_pol << 5) |
>>>> + (hsync_pol << 1) |
>>>> vsync_pol);
>>>>
>>>> if (ret)
>>>> @@ -2052,12 +2060,12 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
>>>> }
>>>>
>>>> /*
>>>> - * powerdown MIPI TX/RX PHY & disable MIPI
>>>> + * powerdown MIPI TX/RX PHY & enable DVP
>>>> *
>>>> * MIPI CONTROL 00
>>>> - * 4: PWDN PHY TX
>>>> - * 3: PWDN PHY RX
>>>> - * 2: MIPI enable
>>>> + * [4] = 1 : Power down MIPI HS Tx
>>>> + * [3] = 1 : Power down MIPI LS Rx
>>>> + * [2] = 0 : DVP enable (MIPI disable)
>>>> */
>>>> ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
>>>> if (ret)
>>>> @@ -2074,8 +2082,7 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
>>>> * - [3:0]: D[9:6] output enable
>>>> */
>>>> ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01,
>>>> - sensor->ep.bus_type == V4L2_MBUS_PARALLEL ?
>>>> - 0x7f : 0x1f);
>>>> + on ? (bt656 ? 0x1f : 0x7f) : 0x00);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> @@ -2085,7 +2092,9 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
>>>> * PAD OUTPUT ENABLE 02
>>>> * - [7:2]: D[5:0] output enable
>>>> */
>>>> - return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
>>>> + return ov5640_write_reg(sensor,
>>>> + OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>> + on ? 0xfc : 0);
>>>> }
>>>>
>>>> static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>>>> @@ -2925,8 +2934,6 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>>>>
>>>> if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
>>>> ret = ov5640_set_stream_mipi(sensor, enable);
>>>> - else if (sensor->ep.bus_type == V4L2_MBUS_BT656)
>>>> - ret = ov5640_set_stream_bt656(sensor, enable);
>>>> else
>>>> ret = ov5640_set_stream_dvp(sensor, enable);
>>>>
>>>> --
>>>> 2.7.4
>>>>
