Move hw configuration functions into ov5693_check_hwcfg. This is done to
separe the code that handle the hw cfg from probe in a clean way
Signed-off-by: Tommaso Merciai <[email protected]>
---
drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index d2adc5513a21..d5a934ace597 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
struct fwnode_handle *endpoint;
unsigned int i;
int ret;
+ u32 xvclk_rate;
+
+ ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");
+ if (IS_ERR(ov5693->xvclk))
+ return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
+ "failed to get xvclk: %ld\n",
+ PTR_ERR(ov5693->xvclk));
+
+ if (ov5693->xvclk) {
+ xvclk_rate = clk_get_rate(ov5693->xvclk);
+ } else {
+ ret = fwnode_property_read_u32(fwnode, "clock-frequency",
+ &xvclk_rate);
+
+ if (ret) {
+ dev_err(ov5693->dev, "can't get clock frequency");
+ return ret;
+ }
+ }
+
+ if (xvclk_rate != OV5693_XVCLK_FREQ)
+ dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
+ xvclk_rate, OV5693_XVCLK_FREQ);
+
+ ret = ov5693_configure_gpios(ov5693);
+ if (ret)
+ return ret;
+
+ ret = ov5693_get_regulators(ov5693);
+ if (ret)
+ return dev_err_probe(ov5693->dev, ret,
+ "Error fetching regulators\n");
endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
if (!endpoint)
@@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
static int ov5693_probe(struct i2c_client *client)
{
struct ov5693_device *ov5693;
- u32 xvclk_rate;
int ret = 0;
ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
@@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
- ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
- if (IS_ERR(ov5693->xvclk)) {
- dev_err(&client->dev, "Error getting clock\n");
- return PTR_ERR(ov5693->xvclk);
- }
-
- xvclk_rate = clk_get_rate(ov5693->xvclk);
- if (xvclk_rate != OV5693_XVCLK_FREQ)
- dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
- xvclk_rate, OV5693_XVCLK_FREQ);
-
- ret = ov5693_configure_gpios(ov5693);
- if (ret)
- return ret;
-
- ret = ov5693_get_regulators(ov5693);
- if (ret)
- return dev_err_probe(&client->dev, ret,
- "Error fetching regulators\n");
-
ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
--
2.25.1
Hi Tommaso,
On Mon, Jun 27, 2022 at 05:04:50PM +0200, Tommaso Merciai wrote:
> Move hw configuration functions into ov5693_check_hwcfg. This is done to
> separe the code that handle the hw cfg from probe in a clean way
s/separe/separate/
You also seem to change the logic of the clk handling, please mention
this in the commit message, otherwise one could be fooled into
thinking you're only moving code around with no functional changes...
>
> Signed-off-by: Tommaso Merciai <[email protected]>
> ---
> drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
> 1 file changed, 32 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index d2adc5513a21..d5a934ace597 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
> struct fwnode_handle *endpoint;
> unsigned int i;
> int ret;
> + u32 xvclk_rate;
nit: move it up to maintain reverse-xmas-tree order (I know, it's an
annoying comment, but since variables are already declared in this order..)
> +
> + ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");
Isn't this broken ?
if you use ov5693->xvclk to identify the ACPI vs OF use case shouldn't
you use the get_optionl() version ? Otherwise in the ACPI case you will have
-ENOENT if there's not 'xvclk' property and bail out.
Unless my understanding is wrong on ACPI we have "clock-frequency" and
on OF "xvclk" with an "assigned-clock-rates",
Dan you upstreamed this driver and I assume it was tested on ACPI ?
Can you clarify how this worked for you, as it seems the original code
wanted a mandatory "xvclk" ? Are there ACPI tables with an actual
'xvclk' property ?
> + if (IS_ERR(ov5693->xvclk))
> + return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
> + "failed to get xvclk: %ld\n",
> + PTR_ERR(ov5693->xvclk));
> +
> + if (ov5693->xvclk) {
> + xvclk_rate = clk_get_rate(ov5693->xvclk);
> + } else {
> + ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> + &xvclk_rate);
> +
> + if (ret) {
> + dev_err(ov5693->dev, "can't get clock frequency");
> + return ret;
> + }
> + }
> +
> + if (xvclk_rate != OV5693_XVCLK_FREQ)
> + dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
> + xvclk_rate, OV5693_XVCLK_FREQ);
> +
> + ret = ov5693_configure_gpios(ov5693);
> + if (ret)
> + return ret;
> +
> + ret = ov5693_get_regulators(ov5693);
> + if (ret)
> + return dev_err_probe(ov5693->dev, ret,
> + "Error fetching regulators\n");
>
> endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> if (!endpoint)
> @@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
> static int ov5693_probe(struct i2c_client *client)
> {
> struct ov5693_device *ov5693;
> - u32 xvclk_rate;
> int ret = 0;
>
> ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
> @@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
>
> v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
>
> - ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
> - if (IS_ERR(ov5693->xvclk)) {
> - dev_err(&client->dev, "Error getting clock\n");
> - return PTR_ERR(ov5693->xvclk);
> - }
> -
> - xvclk_rate = clk_get_rate(ov5693->xvclk);
> - if (xvclk_rate != OV5693_XVCLK_FREQ)
> - dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
> - xvclk_rate, OV5693_XVCLK_FREQ);
> -
> - ret = ov5693_configure_gpios(ov5693);
> - if (ret)
> - return ret;
> -
> - ret = ov5693_get_regulators(ov5693);
> - if (ret)
> - return dev_err_probe(&client->dev, ret,
> - "Error fetching regulators\n");
> -
> ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
> ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> --
> 2.25.1
>
Hi Jacopo,
On Wed, Jun 29, 2022 at 10:16:35AM +0200, Jacopo Mondi wrote:
> Hi Tommaso,
>
> On Mon, Jun 27, 2022 at 05:04:50PM +0200, Tommaso Merciai wrote:
> > Move hw configuration functions into ov5693_check_hwcfg. This is done to
> > separe the code that handle the hw cfg from probe in a clean way
>
> s/separe/separate/
>
> You also seem to change the logic of the clk handling, please mention
> this in the commit message, otherwise one could be fooled into
> thinking you're only moving code around with no functional changes...
Right. I'll add some comments on support to get clock-frequency using
fwnode_property_read_u32 in v3
>
> >
> > Signed-off-by: Tommaso Merciai <[email protected]>
> > ---
> > drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
> > 1 file changed, 32 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> > index d2adc5513a21..d5a934ace597 100644
> > --- a/drivers/media/i2c/ov5693.c
> > +++ b/drivers/media/i2c/ov5693.c
> > @@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
> > struct fwnode_handle *endpoint;
> > unsigned int i;
> > int ret;
> > + u32 xvclk_rate;
>
> nit: move it up to maintain reverse-xmas-tree order (I know, it's an
> annoying comment, but since variables are already declared in this order..)
No problem :)
I'll do it in v3.
>
> > +
> > + ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");
>
> Isn't this broken ?
>
> if you use ov5693->xvclk to identify the ACPI vs OF use case shouldn't
> you use the get_optionl() version ? Otherwise in the ACPI case you will have
> -ENOENT if there's not 'xvclk' property and bail out.
You are right, devm_clk_get_optional is the correct way.
Thanks,
Tommaso
>
> Unless my understanding is wrong on ACPI we have "clock-frequency" and
> on OF "xvclk" with an "assigned-clock-rates",
>
> Dan you upstreamed this driver and I assume it was tested on ACPI ?
> Can you clarify how this worked for you, as it seems the original code
> wanted a mandatory "xvclk" ? Are there ACPI tables with an actual
> 'xvclk' property ?
>
> > + if (IS_ERR(ov5693->xvclk))
> > + return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
> > + "failed to get xvclk: %ld\n",
> > + PTR_ERR(ov5693->xvclk));
> > +
> > + if (ov5693->xvclk) {
> > + xvclk_rate = clk_get_rate(ov5693->xvclk);
> > + } else {
> > + ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > + &xvclk_rate);
> > +
> > + if (ret) {
> > + dev_err(ov5693->dev, "can't get clock frequency");
> > + return ret;
> > + }
> > + }
> > +
> > + if (xvclk_rate != OV5693_XVCLK_FREQ)
> > + dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
> > + xvclk_rate, OV5693_XVCLK_FREQ);
> > +
> > + ret = ov5693_configure_gpios(ov5693);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ov5693_get_regulators(ov5693);
> > + if (ret)
> > + return dev_err_probe(ov5693->dev, ret,
> > + "Error fetching regulators\n");
> >
> > endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > if (!endpoint)
> > @@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
> > static int ov5693_probe(struct i2c_client *client)
> > {
> > struct ov5693_device *ov5693;
> > - u32 xvclk_rate;
> > int ret = 0;
> >
> > ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
> > @@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
> >
> > v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
> >
> > - ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
> > - if (IS_ERR(ov5693->xvclk)) {
> > - dev_err(&client->dev, "Error getting clock\n");
> > - return PTR_ERR(ov5693->xvclk);
> > - }
> > -
> > - xvclk_rate = clk_get_rate(ov5693->xvclk);
> > - if (xvclk_rate != OV5693_XVCLK_FREQ)
> > - dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
> > - xvclk_rate, OV5693_XVCLK_FREQ);
> > -
> > - ret = ov5693_configure_gpios(ov5693);
> > - if (ret)
> > - return ret;
> > -
> > - ret = ov5693_get_regulators(ov5693);
> > - if (ret)
> > - return dev_err_probe(&client->dev, ret,
> > - "Error fetching regulators\n");
> > -
> > ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
> > ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > --
> > 2.25.1
> >
--
Tommaso Merciai
Embedded Linux Engineer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
Hey
On 29/06/2022 09:16, Jacopo Mondi wrote:
> Hi Tommaso,
>
> On Mon, Jun 27, 2022 at 05:04:50PM +0200, Tommaso Merciai wrote:
>> Move hw configuration functions into ov5693_check_hwcfg. This is done to
>> separe the code that handle the hw cfg from probe in a clean way
> s/separe/separate/
>
> You also seem to change the logic of the clk handling, please mention
> this in the commit message, otherwise one could be fooled into
> thinking you're only moving code around with no functional changes...
>
>> Signed-off-by: Tommaso Merciai <[email protected]>
>> ---
>> drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
>> 1 file changed, 32 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
>> index d2adc5513a21..d5a934ace597 100644
>> --- a/drivers/media/i2c/ov5693.c
>> +++ b/drivers/media/i2c/ov5693.c
>> @@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>> struct fwnode_handle *endpoint;
>> unsigned int i;
>> int ret;
>> + u32 xvclk_rate;
> nit: move it up to maintain reverse-xmas-tree order (I know, it's an
> annoying comment, but since variables are already declared in this order..)
>
>> +
>> + ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");
> Isn't this broken ?
>
> if you use ov5693->xvclk to identify the ACPI vs OF use case shouldn't
> you use the get_optionl() version ? Otherwise in the ACPI case you will have
> -ENOENT if there's not 'xvclk' property and bail out.
>
> Unless my understanding is wrong on ACPI we have "clock-frequency" and
> on OF "xvclk" with an "assigned-clock-rates",
>
> Dan you upstreamed this driver and I assume it was tested on ACPI ?
> Can you clarify how this worked for you, as it seems the original code
> wanted a mandatory "xvclk" ? Are there ACPI tables with an actual
> 'xvclk' property ?
Sorry - late answer, but when I wrote this although it's ostensibly for
an ACPI platform, it's actually only tested with the IPU3 platforms
which work in a _weird_ way. The fix we eventually came to was to create
through the int3472-discrete driver clocks and regulators through the
normal frameworks that a dt platform would expect to consume, so even
though the devices are enumerated through ACPI, the clock/regulator
parts really work more like a dt platform.
You're right that it needs to be get_optional() here, but with that
added I think this is fine - I tested it last night and it works ok for me.
>
>> + if (IS_ERR(ov5693->xvclk))
>> + return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
>> + "failed to get xvclk: %ld\n",
>> + PTR_ERR(ov5693->xvclk));
>> +
>> + if (ov5693->xvclk) {
>> + xvclk_rate = clk_get_rate(ov5693->xvclk);
>> + } else {
>> + ret = fwnode_property_read_u32(fwnode, "clock-frequency",
>> + &xvclk_rate);
>> +
>> + if (ret) {
>> + dev_err(ov5693->dev, "can't get clock frequency");
>> + return ret;
>> + }
>> + }
>> +
>> + if (xvclk_rate != OV5693_XVCLK_FREQ)
>> + dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
>> + xvclk_rate, OV5693_XVCLK_FREQ);
>> +
>> + ret = ov5693_configure_gpios(ov5693);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ov5693_get_regulators(ov5693);
>> + if (ret)
>> + return dev_err_probe(ov5693->dev, ret,
>> + "Error fetching regulators\n");
>>
>> endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
>> if (!endpoint)
>> @@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>> static int ov5693_probe(struct i2c_client *client)
>> {
>> struct ov5693_device *ov5693;
>> - u32 xvclk_rate;
>> int ret = 0;
>>
>> ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
>> @@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
>>
>> v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
>>
>> - ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
>> - if (IS_ERR(ov5693->xvclk)) {
>> - dev_err(&client->dev, "Error getting clock\n");
>> - return PTR_ERR(ov5693->xvclk);
>> - }
>> -
>> - xvclk_rate = clk_get_rate(ov5693->xvclk);
>> - if (xvclk_rate != OV5693_XVCLK_FREQ)
>> - dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
>> - xvclk_rate, OV5693_XVCLK_FREQ);
>> -
>> - ret = ov5693_configure_gpios(ov5693);
>> - if (ret)
>> - return ret;
>> -
>> - ret = ov5693_get_regulators(ov5693);
>> - if (ret)
>> - return dev_err_probe(&client->dev, ret,
>> - "Error fetching regulators\n");
>> -
>> ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
>> ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>> --
>> 2.25.1
>>