On 15/03/2022 09:46, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch.
>>
>> On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote:
>>> Add support for probing the main system clock using the common clock
>>> framework and its OF bindings.
>>>
>>> Maintain ACPI compatibility by falling back to parse 'clock-frequency'
>>> if the no clock device reference is available.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>> ---
>>> drivers/media/i2c/ov5670.c | 21 +++++++++++++++++----
>>> 1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
>>> index 721441024598..25d792794fc7 100644
>>> --- a/drivers/media/i2c/ov5670.c
>>> +++ b/drivers/media/i2c/ov5670.c
>>> @@ -2,6 +2,7 @@
>>> // Copyright (c) 2017 Intel Corporation.
>>>
>>> #include <linux/acpi.h>
>>> +#include <linux/clk.h>
>>> #include <linux/i2c.h>
>>> #include <linux/mod_devicetable.h>
>>> #include <linux/module.h>
>>> @@ -1819,6 +1820,8 @@ struct ov5670 {
>>> struct v4l2_subdev sd;
>>> struct media_pad pad;
>>>
>>> + struct clk *clk;
>>> +
>>> struct v4l2_ctrl_handler ctrl_handler;
>>> /* V4L2 Controls */
>>> struct v4l2_ctrl *link_freq;
>>> @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client)
>>> bool full_power;
>>> int ret;
>>>
>>> - device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
>>> - if (input_clk != 19200000)
>>> - return -EINVAL;
>>> -
>>> ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
>>> if (!ov5670) {
>>> ret = -ENOMEM;
>>> @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client)
>>> goto error_print;
>>> }
>>>
>>> + /* OF uses the common clock framework, ACPI uses "clock-frequency". */
>>> + ov5670->clk = devm_clk_get_optional(&client->dev, NULL);
>>> + if (IS_ERR(ov5670->clk))
>>> + return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk),
>>> + "error getting clock\n");
>>> +
>>> + if (ov5670->clk)
>>> + input_clk = clk_get_rate(ov5670->clk);
>>> + else
>>> + device_property_read_u32(&client->dev, "clock-frequency",
>>> + &input_clk);
>>
>> This will try to use the clock-frequency property on OF-based systems if
>> no clock is specified. Could we instead have
>>
>
> 'clocks' is listed as a required property in the OF bindings and my
> understanding is that driver assume DTS are correct.
>
>> if (probed through OF) {
>
> Otherwise yes, I can check for dev->of_node
> But again, I don't think drivers should have to work-around broken DTS
>
Driver should expect broken DTS and handle it. You're introducing new
feature (OF support) so you can be strict about DTS and reject the ones
which do not provide clock.
Best regards,
Krzysztof
