Re: [PATCH v8 2/2] Add support for OV5647 sensor.

[Vladimir Zapolskiy]

Hi Ramiro,

On 02/13/2017 09:14 PM, Ramiro Oliveira wrote:
> Hi Vladimir,
> 
> Thank you for your feedback.
> 
> On 2/13/2017 12:21 PM, Vladimir Zapolskiy wrote:
>> Hello Ramiro,
>>
>> On 02/13/2017 01:25 PM, Ramiro Oliveira wrote:
>>> Modes supported:
>>>  - 640x480 RAW 8
>>>

[snip]

>>> +static int ov5647_probe(struct i2c_client *client,
>>> +			const struct i2c_device_id *id)
>>> +{
>>> +	struct device *dev = &client->dev;
>>> +	struct ov5647 *sensor;
>>> +	int ret;
>>> +	struct v4l2_subdev *sd;
>>> +
>>> +	dev_info(&client->dev, "Installing OmniVision OV5647 camera driver\n");
>>
>> Please remove the informational line, it will pollute the kernel log for no
>> good reason.
>>
> 
> Is it okay if I change it to debug?
> 

Most probably here it is okay to change it to dev_dbg(), however

1) please note that ftrace functionality should provide all the magic for you,
   and by the way this makes dev_dbg() calls from ov5647_write(), ov5647_read(),
   ov5647_stream_on(), ov5647_stream_off() and __sensor_init() all redundant,

2) please move the informational message to the end of the .probe function,
   right before returning success to avoid duplicates on deferred re-probing:

	dev_dbg(&client->dev, "OmniVision OV5647 camera driver probed\n");
	                                                      ^^^^^^^^
[snip]

>>> +
>>> +static const struct i2c_device_id ov5647_id[] = {
>>> +	{ "ov5647", 0 },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ov5647_id);
>>> +
>>> +#if IS_ENABLED(CONFIG_OF)
>>
>> From Kconfig the driver depends on OF.
>>
> 
> You're right. Do you think I should remove the dependency in Kconfig or the
> check here?
> 

Let see...

I've been able to locate only one place where OF dependency is utterly needed:

	ret = of_property_read_u32(dev->of_node, "clock-frequency",
				    &sensor->xclk_freq);
	if (ret) {
		dev_err(dev, "could not get xclk frequency\n");
		return ret;
	}

It might be preferred to change this snippet of code into something like one
below:

	sensor->xclk_freq = clk_get_rate(sensor->xclk);
	if (sensor->xclk_freq != 30000000) {
		dev_err(dev, "Unsupported clock frequency: %lu\n",
			sensor->xclk_freq);
		return -EINVAL;
	}

Then

1) in case of an OF platform "xclk" clock frequency can be set directly
   in a board DTS file, please reference to clock-bindings.txt,
2) next you can drop 'clock-frequency' property from the sensor bindings,
3) you can drop 'depends on OF' from drivers/media/i2c/Kconfig,
4) you can drop 'clk_set_rate()' from sensor_power().

The proposed code is slightly more flexible, at least the change gives
a little chance to run the driver successfully on a non-OF platform,
otherwise it is known in advance that the driver is unusable on any
non-OF platforms and thus an explicit CONFIG_OF build dependency makes
sense.

>>> +static const struct of_device_id ov5647_of_match[] = {
>>> +	{ .compatible = "ovti,ov5647" },
>>> +	{ /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ov5647_of_match);
>>> +#endif
>>> +
>>> +/**
>>> + * @short i2c driver structure
>>> + */
>>> +static struct i2c_driver ov5647_driver = {
>>> +	.driver = {
>>> +		.of_match_table = of_match_ptr(ov5647_of_match),
>>
>> Same comment as above, from Kconfig the driver depends on OF.
>>
> 
> I'm sorry but I'm not understanding what you're trying to say.
> 

You may take a look at of_match_ptr() macro definition from include/linux/of.h:

#ifdef CONFIG_OF
...
#define of_match_ptr(_ptr)      (_ptr)
...
#else
...
#define of_match_ptr(_ptr)      NULL
...
#endif

Hence if the code compilation always depends on the enabled CONFIG_OF option,
then of_match_ptr() macro should be dropped:

	.of_match_table = ov5647_of_match,

>>> +		.owner	= THIS_MODULE,
>>
>> .owner is set by the core, please remove it.
>>
> 
> Ok.
> 
>>> +		.name	= "ov5647",
>>
>> May be .name = SENSOR_NAME, ?
>>
>> Otherwise SENSOR_NAME macro is unused and it should be removed.
>>
> 
> I'll change it to .name = SENSOR_NAME,
> 
>>> +	},
>>> +	.probe		= ov5647_probe,
>>> +	.remove		= ov5647_remove,
>>> +	.id_table	= ov5647_id,
>>> +};
>>> +
>>> +module_i2c_driver(ov5647_driver);
>>> +
>>> +MODULE_AUTHOR("Ramiro Oliveira <roliveir@xxxxxxxxxxxx>");
>>> +MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>

--
With best wishes,
Vladimir