Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Todor Tomov <todor.tomov@linaro.org>]

And using Mark Brown's correct address...

On 10/26/2016 02:15 PM, Todor Tomov wrote:
> Hi,
> 
> Adding Mark Brown in --to list.
> 
> My reply on comments below.
> The question on regulator bulk API to Mark Brown still holds.
> 
> 
> On 10/19/2016 11:44 AM, Laurent Pinchart wrote:
>> Hi Todor,
>>
>> (CC'ing Mark Brown for a question on regulators)
>>
>> On Friday 14 Oct 2016 14:57:01 Todor Tomov wrote:
>>> Hi Laurent,
>>>
>>> Thank you for the time spent to do this thorough review of the patch!
>>>
>>> Below I have removed some of the comments where I agree and I'll fix.
>>> I have left the places where I have something relevant to say or ask.
>>>
>>> On 09/08/2016 03:22 PM, Laurent Pinchart wrote:
>>>> On Thursday 08 Sep 2016 12:13:55 Todor Tomov wrote:
>>>>> The ov5645 sensor from Omnivision supports up to 2592x1944
>>>>> and CSI2 interface.
>>>>>
>>>>> The driver adds support for the following modes:
>>>>> - 1280x960
>>>>> - 1920x1080
>>>>> - 2592x1944
>>>>>
>>>>> Output format is packed 8bit UYVY.
>>>>>
>>>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
>>>>> ---
>>>>>
>>>>>  drivers/media/i2c/Kconfig  |   12 +
>>>>>  drivers/media/i2c/Makefile |    1 +
>>>>>  drivers/media/i2c/ov5645.c | 1372 ++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 1385 insertions(+)
>>>>>  create mode 100644 drivers/media/i2c/ov5645.c
>>>>
>>>> [snip]
>>>>
>>>>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
>>>>> new file mode 100644
>>>>> index 0000000..5e5c37e
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/i2c/ov5645.c
>>>>> @@ -0,0 +1,1372 @@
>>
>> [snip]
>>
>>>>> +	{ 0x3103, 0x11 },
>>>>> +	{ 0x3008, 0x82 },
>>>>> +	{ 0x3008, 0x42 },
>>>>> +	{ 0x3103, 0x03 },
>>>>> +	{ 0x3503, 0x07 },
>>>>
>>>> [snip]
>>>>
>>>>> +	{ 0x3503, 0x00 },
>>>>
>>>> Can't you get rid of the first write to 0x3503 ?
>>>
>>> No, this is a startup sequence from the vendor so I'm following it as it is.
>>
>> 0x3503 controls the AEC/AGC mode, I wonder if that's really needed. I'm OK 
>> keeping it as-is for now.
> 
> I agree that there is a reason to wonder if it is really needed, but I'd still
> prefer not to touch it.
> 
>>
>>> [snip]
>>>
>>>>> +static int ov5645_regulators_enable(struct ov5645 *ov5645)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = regulator_enable(ov5645->io_regulator);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(ov5645->dev, "set io voltage failed\n");
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	ret = regulator_enable(ov5645->core_regulator);
>>>>> +	if (ret) {
>>>>> +		dev_err(ov5645->dev, "set core voltage failed\n");
>>>>> +		goto err_disable_io;
>>>>> +	}
>>>>> +
>>>>> +	ret = regulator_enable(ov5645->analog_regulator);
>>>>> +	if (ret) {
>>>>> +		dev_err(ov5645->dev, "set analog voltage failed\n");
>>>>> +		goto err_disable_core;
>>>>> +	}
>>>>
>>>> How about using the regulator bulk API ? This would simplify the enable
>>>> and disable functions.
>>>
>>> The driver must enable the regulators in this order. I can see in the
>>> implementation of the bulk api that they are enabled again in order
>>> but I don't see stated anywhere that it is guaranteed to follow the
>>> same order in future. I'd prefer to keep it explicit as it is now.
>>
>> I believe it should be an API guarantee, otherwise many drivers using the bulk 
>> API would break. Mark, could you please comment on that ?
> 
> Ok, let's wait for a response from Mark.
> 
>>
>>> [snip]
>>>
>>>>> +static int ov5645_set_power_on(struct ov5645 *ov5645)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	clk_set_rate(ov5645->xclk, ov5645->xclk_freq);
>>>>
>>>> Is this needed every time you power the sensor on or could you do it just
>>>> once at probe time ?
>>>
>>> I'll move it at probe time.
>>>
>>>>> +	ret = clk_prepare_enable(ov5645->xclk);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(ov5645->dev, "clk prepare enable failed\n");
>>>>> +		return ret;
>>>>> +	}
>>>>
>>>> Is it safe to start the clock before the regulators ? Driving an input of
>>>> an unpowered chip can lead to latch-up issues.
>>>
>>> Correct, power should be enabled first. I'll fix this.
>>>
>>>>> +	ret = ov5645_regulators_enable(ov5645);
>>>>> +	if (ret < 0) {
>>>>> +		clk_disable_unprepare(ov5645->xclk);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	usleep_range(5000, 15000);
>>>>> +	gpiod_set_value_cansleep(ov5645->enable_gpio, 1);
>>>>> +
>>>>> +	usleep_range(1000, 2000);
>>>>> +	gpiod_set_value_cansleep(ov5645->rst_gpio, 0);
>>>>> +
>>>>> +	msleep(20);
>>>>> +
>>>>> +	return ret;
>>>>
>>>> You can return 0.
>>>
>>> Ok.
>>>
>>> [snip]
>>>
>>>>> +static int ov5645_set_hflip(struct ov5645 *ov5645, s32 value)
>>>>> +{
>>>>> +	u8 val;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, &val);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (value == 0)
>>>>> +		val &= ~(OV5645_SENSOR_MIRROR);
>>>>> +	else
>>>>> +		val |= (OV5645_SENSOR_MIRROR);
>>>>> +
>>>>> +	return ov5645_write_reg(ov5645, OV5645_TIMING_TC_REG21, val);
>>>>
>>>> You could cache this register too.
>>>
>>> Ok.
>>>
>>>>> +}
>>>>> +
>>>>> +static int ov5645_set_vflip(struct ov5645 *ov5645, s32 value)
>>>>> +{
>>>>> +	u8 val;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, &val);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (value == 0)
>>>>> +		val |= (OV5645_SENSOR_VFLIP | OV5645_ISP_VFLIP);
>>>>> +	else
>>>>> +		val &= ~(OV5645_SENSOR_VFLIP | OV5645_ISP_VFLIP);
>>>>> +
>>>>> +	return ov5645_write_reg(ov5645, OV5645_TIMING_TC_REG20, val);
>>>>
>>>> And this one as well.
>>>
>>> Yes.
>>>
>>>> How about using regmap by the way ?
>>>
>>> I'd prefer to keep it as is for now.
>>>
>>>>> +}
>>>>> +
>>>>> +static int ov5645_set_test_pattern(struct ov5645 *ov5645, s32 value)
>>>>> +{
>>>>> +	u8 val;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = ov5645_read_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, &val);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (value) {
>>>>> +		val &= ~OV5645_SET_TEST_PATTERN(OV5645_TEST_PATTERN_MASK);
>>>>> +		val |= OV5645_SET_TEST_PATTERN(value - 1);
>>>>> +		val |= OV5645_TEST_PATTERN_ENABLE;
>>>>> +	} else {
>>>>> +		val &= ~OV5645_TEST_PATTERN_ENABLE;
>>>>> +	}
>>>>> +
>>>>> +	return ov5645_write_reg(ov5645, OV5645_PRE_ISP_TEST_SETTING_1, val);
>>>>
>>>> Are there other bits that need to be preserved in this register ?
>>>
>>> This driver is based on the driver for OV5645 from QC and the driver for
>>> OV5640 that was sent to linux-media. I cannot add additional functionality
>>> so I preserve the rest of the bits. But I'll add caching in a variable here
>>> too.
>>
>> As far as I know, based on the documentation I've seen, all bits in this 
>> register control the test pattern and none need to be preserved. The default 
>> reset value of the register is 0x00 and the initialization sequence sets it to 
>> 0x00 as well, so it should be safe not caching it.
>>
> 
> Ok, then I'll remove any reading or caching for this one.
> 
>>>>> +}
>>>>> +
>>>>> +static const char * const ov5645_test_pattern_menu[] = {
>>>>> +	"Disabled",
>>>>> +	"Vertical Color Bars",
>>>>> +	"Pseudo-Random Data",
>>>>> +	"Color Square",
>>>>> +	"Black Image",
>>>>> +};
>>>>> +
>>>>> +static int ov5645_set_awb(struct ov5645 *ov5645, s32 enable_auto)
>>>>> +{
>>>>> +	u8 val;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = ov5645_read_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, &val);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (enable_auto)
>>>>> +		val &= ~OV5645_AWB_MANUAL_ENABLE;
>>>>> +	else
>>>>> +		val |= OV5645_AWB_MANUAL_ENABLE;
>>>>> +
>>>>> +	return ov5645_write_reg(ov5645, OV5645_AWB_MANUAL_CONTROL, val);
>>>>
>>>> Same here, are there other bits that need to be preserved ?
>>>
>>> Same as above.
>>
>> Bits 7:1 are documented as "debug mode" and are set to 0 at reset time. It 
>> should be fine not caching this register.
>>
> 
> I will remove the reading and caching here too.
> 
>>>>> +}
>>
>> [snip]
>>
>>>>> +static int ov5645_entity_init_cfg(struct v4l2_subdev *subdev,
>>>>> +				  struct v4l2_subdev_pad_config *cfg)
>>>>> +{
>>>>> +	struct v4l2_subdev_format fmt = { 0 };
>>>>> +
>>>>> +	fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>>>>
>>>> The function will always be called with cfg != NULL.
>>>
>>> I intend to call this function from probe to init the active format. Will
>>> this be ok?
>>
>> If you plan to call it with cfg == NULL then yes this has to be handled.
>>
>>>>> +	fmt.format.width = 1920;
>>>>> +	fmt.format.height = 1080;
>>>>> +
>>>>> +	v4l2_subdev_call(subdev, pad, set_fmt, cfg, &fmt);
>>>>
>>>> You can call ov5645_set_format directly.
>>>
>>> Ok.
>>>
>>>>> +	return 0;
>>>>> +}
>>
>> [snip]
>>
> 

-- 
Best regards,
Todor Tomov