| Message ID | 20220627150453.220292-5-tommaso.merciai@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5693: cleanup code and add dts support | expand |
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 <tommaso.merciai@amarulasolutions.com> > --- > 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 <tommaso.merciai@amarulasolutions.com> > > --- > > 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 > >
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 <tommaso.merciai@amarulasolutions.com> >> --- >> 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 >>
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;
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 <tommaso.merciai@amarulasolutions.com> --- drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 21 deletions(-)