Hi All,
This series cleanup code on ov5693 driver and bring up dts support, also add
documentation for ov5693 camera sensor
Inspired by recently Quentin series:
- https://patchwork.kernel.org/project/linux-media/list/?series=64807
Tommaso Merciai (6):
media: ov5693: count num_supplies using array_size
media: ov5693: add dvdd into ov5693_supply_names array
media: ov5693: rename clk into xvclk
media: ov5693: move hw cfg functions into ov5693_hwcfg
media: dt-bindings: ov5693: document YAML binding
media: ov5693: add ov5693_of_match, dts support
.../bindings/media/i2c/ovti,ov5693.yaml | 106 ++++++++++++++++++
MAINTAINERS | 1 +
drivers/media/i2c/ov5693.c | 86 ++++++++------
3 files changed, 159 insertions(+), 34 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
--
2.25.1
Add documentation of device tree in YAML schema for the OV5693
CMOS image sensor from Omnivision
Signed-off-by: Tommaso Merciai <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
---
Changes since v1:
- Fix allOf position as suggested by Krzysztof
- Remove port description as suggested by Krzysztof
- Fix EOF as suggested by Krzysztof
Changes since v2:
- Fix commit body as suggested by Krzysztof
Changes since v3:
- Add reviewed-by tags, ssuggested by Jacopo, Krzysztof
.../bindings/media/i2c/ovti,ov5693.yaml | 106 ++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 107 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
new file mode 100644
index 000000000000..b83c9fc04023
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2022 Amarulasolutions
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5693.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5693 CMOS Sensor
+
+maintainers:
+ - Tommaso Merciai <[email protected]>
+
+description: |
+ The Omnivision OV5693 is a high performance, 1/4-inch, 5 megapixel, CMOS
+ image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
+ sub-sampled, and windowed 10-bit MIPI images in various formats via the
+ Serial Camera Control Bus (SCCB) interface.
+
+ OV5693 is controlled via I2C and two-wire Serial Camera Control Bus (SCCB).
+ The sensor output is available via CSI-2 serial data output (up to 2-lane).
+
+allOf:
+ - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+ compatible:
+ const: ovti,ov5693
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ description:
+ System input clock (aka XVCLK). From 6 to 27 MHz.
+ maxItems: 1
+
+ dovdd-supply:
+ description:
+ Digital I/O voltage supply, 1.8V.
+
+ avdd-supply:
+ description:
+ Analog voltage supply, 2.8V.
+
+ dvdd-supply:
+ description:
+ Digital core voltage supply, 1.2V.
+
+ reset-gpios:
+ description:
+ The phandle and specifier for the GPIO that controls sensor reset.
+ This corresponds to the hardware pin XSHUTDN which is physically
+ active low.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - dovdd-supply
+ - avdd-supply
+ - dvdd-supply
+ - port
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/px30-cru.h>
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/pinctrl/rockchip.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ov5693: camera@36 {
+ compatible = "ovti,ov5693";
+ reg = <0x36>;
+
+ reset-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_LOW>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&cif_clkout_m0>;
+
+ clocks = <&cru SCLK_CIF_OUT>;
+ assigned-clocks = <&cru SCLK_CIF_OUT>;
+ assigned-clock-rates = <19200000>;
+
+ avdd-supply = <&vcc_1v8>;
+ dvdd-supply = <&vcc_1v2>;
+ dovdd-supply = <&vcc_2v8>;
+
+ rotation = <90>;
+ orientation = <0>;
+
+ port {
+ ucam_out: endpoint {
+ remote-endpoint = <&mipi_in_ucam>;
+ data-lanes = <1 2>;
+ link-frequencies = /bits/ 64 <450000000>;
+ };
+ };
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 1fc9ead83d2a..844307cb20c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14719,6 +14719,7 @@ M: Daniel Scally <[email protected]>
L: [email protected]
S: Maintained
T: git git://linuxtv.org/media_tree.git
+F: Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
F: drivers/media/i2c/ov5693.c
OMNIVISION OV5695 SENSOR DRIVER
--
2.25.1
Instead of hardcode OV5693_NUM_SUPPLIES in a define is better use
ARRAY_SIZE function to count the number of supplies from
ov5693_supply_names array
Signed-off-by: Tommaso Merciai <[email protected]>
Reviewed-by: Jacopo Mondi <[email protected]>
---
Changes since v3:
- Add reviewed-by tag, suggested by Jacopo, Krzysztof
drivers/media/i2c/ov5693.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 117ff5403312..f410333c4c67 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -127,11 +127,15 @@
#define OV5693_LINK_FREQ_419_2MHZ 419200000
#define OV5693_PIXEL_RATE 167680000
-/* Miscellaneous */
-#define OV5693_NUM_SUPPLIES 2
-
#define to_ov5693_sensor(x) container_of(x, struct ov5693_device, sd)
+static const char * const ov5693_supply_names[] = {
+ "avdd", /* Analog power */
+ "dovdd", /* Digital I/O power */
+};
+
+#define OV5693_NUM_SUPPLIES ARRAY_SIZE(ov5693_supply_names)
+
struct ov5693_reg {
u32 reg;
u8 val;
@@ -352,11 +356,6 @@ static const s64 link_freq_menu_items[] = {
OV5693_LINK_FREQ_419_2MHZ
};
-static const char * const ov5693_supply_names[] = {
- "avdd",
- "dovdd",
-};
-
static const char * const ov5693_test_pattern_menu[] = {
"Disabled",
"Random Data",
--
2.25.1
Rename clk pdata pointer into xvclk (system clock input).
Same for clk_rate into xvclk_rate. This is more explicit
Signed-off-by: Tommaso Merciai <[email protected]>
Reviewed-by: Jacopo Mondi <[email protected]>
---
Changes since v3:
- Add reviewed-by tag, suggested by Jacopo, Krzysztof
drivers/media/i2c/ov5693.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 9e84468d920e..d2adc5513a21 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -157,7 +157,7 @@ struct ov5693_device {
struct gpio_desc *reset;
struct gpio_desc *powerdown;
struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
- struct clk *clk;
+ struct clk *xvclk;
struct ov5693_mode {
struct v4l2_rect crop;
@@ -794,7 +794,7 @@ static void ov5693_sensor_powerdown(struct ov5693_device *ov5693)
regulator_bulk_disable(OV5693_NUM_SUPPLIES, ov5693->supplies);
- clk_disable_unprepare(ov5693->clk);
+ clk_disable_unprepare(ov5693->xvclk);
}
static int ov5693_sensor_powerup(struct ov5693_device *ov5693)
@@ -804,7 +804,7 @@ static int ov5693_sensor_powerup(struct ov5693_device *ov5693)
gpiod_set_value_cansleep(ov5693->reset, 1);
gpiod_set_value_cansleep(ov5693->powerdown, 1);
- ret = clk_prepare_enable(ov5693->clk);
+ ret = clk_prepare_enable(ov5693->xvclk);
if (ret) {
dev_err(ov5693->dev, "Failed to enable clk\n");
goto fail_power;
@@ -1390,7 +1390,7 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
static int ov5693_probe(struct i2c_client *client)
{
struct ov5693_device *ov5693;
- u32 clk_rate;
+ u32 xvclk_rate;
int ret = 0;
ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
@@ -1408,16 +1408,16 @@ static int ov5693_probe(struct i2c_client *client)
v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
- ov5693->clk = devm_clk_get(&client->dev, "xvclk");
- if (IS_ERR(ov5693->clk)) {
+ 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->clk);
+ return PTR_ERR(ov5693->xvclk);
}
- clk_rate = clk_get_rate(ov5693->clk);
- if (clk_rate != OV5693_XVCLK_FREQ)
+ xvclk_rate = clk_get_rate(ov5693->xvclk);
+ if (xvclk_rate != OV5693_XVCLK_FREQ)
dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
- clk_rate, OV5693_XVCLK_FREQ);
+ xvclk_rate, OV5693_XVCLK_FREQ);
ret = ov5693_configure_gpios(ov5693);
if (ret)
--
2.25.1
On 30/06/2022 09:45, Tommaso Merciai wrote:
> Add documentation of device tree in YAML schema for the OV5693
> CMOS image sensor from Omnivision
>
> Signed-off-by: Tommaso Merciai <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> Reviewed-by: Sakari Ailus <[email protected]>
How Sakari's tag appeared here? There was no email from him.
Best regards,
Krzysztof
On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2022 09:45, Tommaso Merciai wrote:
> > Add documentation of device tree in YAML schema for the OV5693
> > CMOS image sensor from Omnivision
> >
> > Signed-off-by: Tommaso Merciai <[email protected]>
> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > Reviewed-by: Sakari Ailus <[email protected]>
>
> How Sakari's tag appeared here? There was no email from him.
Sakari made me some review on v2, but I think he forgot to add the mailing
list in cc. ( I suppose :) )
Let me know if I need to remove this.
Regards,
Tommaso
>
> Best regards,
> Krzysztof
--
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
On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > Add documentation of device tree in YAML schema for the OV5693
> > > CMOS image sensor from Omnivision
> > >
> > > Signed-off-by: Tommaso Merciai <[email protected]>
> > > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > > Reviewed-by: Sakari Ailus <[email protected]>
> >
> > How Sakari's tag appeared here? There was no email from him.
>
> Sakari made me some review on v2, but I think he forgot to add the mailing
> list in cc. ( I suppose :) )
>
> Let me know if I need to remove this.
You're only supposed to put these tags into patches if you get them in
written form as part of the review, signalling acceptance of the patch in
various forms. Just commenting a patch does not imply this.
Please also see Documentation/process/submitting-patches.rst for more
information on how to use the tags.
--
Sakari Ailus
Hi Sakari,
On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > > Add documentation of device tree in YAML schema for the OV5693
> > > > CMOS image sensor from Omnivision
> > > >
> > > > Signed-off-by: Tommaso Merciai <[email protected]>
> > > > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > > > Reviewed-by: Sakari Ailus <[email protected]>
> > >
> > > How Sakari's tag appeared here? There was no email from him.
> >
> > Sakari made me some review on v2, but I think he forgot to add the mailing
> > list in cc. ( I suppose :) )
> >
> > Let me know if I need to remove this.
>
> You're only supposed to put these tags into patches if you get them in
> written form as part of the review, signalling acceptance of the patch in
> various forms. Just commenting a patch does not imply this.
>
> Please also see Documentation/process/submitting-patches.rst for more
> information on how to use the tags.
Thanks for sharing this. My bad.
I remove your tags.
Regards,
Tommaso
>
> --
> Sakari Ailus
--
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
Hi Sakari,
On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
> Hi Tommaso,
>
> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> > Hi Sakari,
> >
> > On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> > > On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > > > On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > > > > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > > > > Add documentation of device tree in YAML schema for the OV5693
> > > > > > CMOS image sensor from Omnivision
> > > > > >
> > > > > > Signed-off-by: Tommaso Merciai <[email protected]>
> > > > > > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > > > > > Reviewed-by: Sakari Ailus <[email protected]>
> > > > >
> > > > > How Sakari's tag appeared here? There was no email from him.
> > > >
> > > > Sakari made me some review on v2, but I think he forgot to add the mailing
> > > > list in cc. ( I suppose :) )
> > > >
> > > > Let me know if I need to remove this.
> > >
> > > You're only supposed to put these tags into patches if you get them in
> > > written form as part of the review, signalling acceptance of the patch in
> > > various forms. Just commenting a patch does not imply this.
> > >
> > > Please also see Documentation/process/submitting-patches.rst for more
> > > information on how to use the tags.
> >
> > Thanks for sharing this. My bad.
> > I remove your tags.
>
> The patches themselves seem fine. I'd just drop the 4th patch or at least
> come up with a better name for ov5693_hwcfg() --- you're acquiring
> resources there, and that generally fits well for probe. The code is fine
> already.
Then we don't need v5 with your reviewed tags removed?
I think the patch4 is needed to add dts support properly.
Also this contains devm_clk_get_optional fix suggested by Jacopo and
support for ACPI-based platforms that specify the clock frequency by
using the "clock-frequency" property instead of specifying a clock
provider reference.
Some suggestion on the function name?
Thanks,
Tommaso
>
> --
> Sakari Ailus
--
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
Hi Tommaso,
On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> Hi Sakari,
>
> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> > On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > > On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > > > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > > > Add documentation of device tree in YAML schema for the OV5693
> > > > > CMOS image sensor from Omnivision
> > > > >
> > > > > Signed-off-by: Tommaso Merciai <[email protected]>
> > > > > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > > > > Reviewed-by: Sakari Ailus <[email protected]>
> > > >
> > > > How Sakari's tag appeared here? There was no email from him.
> > >
> > > Sakari made me some review on v2, but I think he forgot to add the mailing
> > > list in cc. ( I suppose :) )
> > >
> > > Let me know if I need to remove this.
> >
> > You're only supposed to put these tags into patches if you get them in
> > written form as part of the review, signalling acceptance of the patch in
> > various forms. Just commenting a patch does not imply this.
> >
> > Please also see Documentation/process/submitting-patches.rst for more
> > information on how to use the tags.
>
> Thanks for sharing this. My bad.
> I remove your tags.
The patches themselves seem fine. I'd just drop the 4th patch or at least
come up with a better name for ov5693_hwcfg() --- you're acquiring
resources there, and that generally fits well for probe. The code is fine
already.
--
Sakari Ailus
Hello - sorry, hadn't had much time to look at this series.
On 30/06/2022 08:45, Tommaso Merciai wrote:
> Instead of hardcode OV5693_NUM_SUPPLIES in a define is better use
> ARRAY_SIZE function to count the number of supplies from
> ov5693_supply_names array
>
> Signed-off-by: Tommaso Merciai <[email protected]>
> Reviewed-by: Jacopo Mondi <[email protected]>
Reviewed-by: Daniel Scally <[email protected]>
> ---
> Changes since v3:
> - Add reviewed-by tag, suggested by Jacopo, Krzysztof
>
> drivers/media/i2c/ov5693.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 117ff5403312..f410333c4c67 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -127,11 +127,15 @@
> #define OV5693_LINK_FREQ_419_2MHZ 419200000
> #define OV5693_PIXEL_RATE 167680000
>
> -/* Miscellaneous */
> -#define OV5693_NUM_SUPPLIES 2
> -
> #define to_ov5693_sensor(x) container_of(x, struct ov5693_device, sd)
>
> +static const char * const ov5693_supply_names[] = {
> + "avdd", /* Analog power */
> + "dovdd", /* Digital I/O power */
> +};
> +
> +#define OV5693_NUM_SUPPLIES ARRAY_SIZE(ov5693_supply_names)
> +
> struct ov5693_reg {
> u32 reg;
> u8 val;
> @@ -352,11 +356,6 @@ static const s64 link_freq_menu_items[] = {
> OV5693_LINK_FREQ_419_2MHZ
> };
>
> -static const char * const ov5693_supply_names[] = {
> - "avdd",
> - "dovdd",
> -};
> -
> static const char * const ov5693_test_pattern_menu[] = {
> "Disabled",
> "Random Data",
Hello
On 30/06/2022 11:09, Tommaso Merciai wrote:
> Hi Sakari,
>
> On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
>> Hi Tommaso,
>>
>> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
>>> Hi Sakari,
>>>
>>> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
>>>> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
>>>>> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 30/06/2022 09:45, Tommaso Merciai wrote:
>>>>>>> Add documentation of device tree in YAML schema for the OV5693
>>>>>>> CMOS image sensor from Omnivision
>>>>>>>
>>>>>>> Signed-off-by: Tommaso Merciai <[email protected]>
>>>>>>> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>>>>>>> Reviewed-by: Sakari Ailus <[email protected]>
>>>>>> How Sakari's tag appeared here? There was no email from him.
>>>>> Sakari made me some review on v2, but I think he forgot to add the mailing
>>>>> list in cc. ( I suppose :) )
>>>>>
>>>>> Let me know if I need to remove this.
>>>> You're only supposed to put these tags into patches if you get them in
>>>> written form as part of the review, signalling acceptance of the patch in
>>>> various forms. Just commenting a patch does not imply this.
>>>>
>>>> Please also see Documentation/process/submitting-patches.rst for more
>>>> information on how to use the tags.
>>> Thanks for sharing this. My bad.
>>> I remove your tags.
>> The patches themselves seem fine. I'd just drop the 4th patch or at least
>> come up with a better name for ov5693_hwcfg() --- you're acquiring
>> resources there, and that generally fits well for probe. The code is fine
>> already.
> Then we don't need v5 with your reviewed tags removed?
>
> I think the patch4 is needed to add dts support properly.
> Also this contains devm_clk_get_optional fix suggested by Jacopo and
> support for ACPI-based platforms that specify the clock frequency by
> using the "clock-frequency" property instead of specifying a clock
> provider reference.
I agree patch 4 in some form is needed - I didn't do the clock handling
particularly well in this driver, and though it's ostensibly an ACPI
driver it wouldn't actually work with a "normal" ACPI, but just with the
cio2-bridge-repaired style. So the changes to the clock handling logic
are welcome and needed I think. whether it needs to go into a separate
function I don't particularly mind either way.
>
> Some suggestion on the function name?
>
> Thanks,
> Tommaso
>
>> --
>> Sakari Ailus
On Thu, Jun 30, 2022 at 11:15:40AM +0100, Daniel Scally wrote:
> Hello
>
> On 30/06/2022 11:09, Tommaso Merciai wrote:
> > Hi Sakari,
> >
> > On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
> >> Hi Tommaso,
> >>
> >> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> >>> Hi Sakari,
> >>>
> >>> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> >>>> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> >>>>> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On 30/06/2022 09:45, Tommaso Merciai wrote:
> >>>>>>> Add documentation of device tree in YAML schema for the OV5693
> >>>>>>> CMOS image sensor from Omnivision
> >>>>>>>
> >>>>>>> Signed-off-by: Tommaso Merciai <[email protected]>
> >>>>>>> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >>>>>>> Reviewed-by: Sakari Ailus <[email protected]>
> >>>>>> How Sakari's tag appeared here? There was no email from him.
> >>>>> Sakari made me some review on v2, but I think he forgot to add the mailing
> >>>>> list in cc. ( I suppose :) )
> >>>>>
> >>>>> Let me know if I need to remove this.
> >>>> You're only supposed to put these tags into patches if you get them in
> >>>> written form as part of the review, signalling acceptance of the patch in
> >>>> various forms. Just commenting a patch does not imply this.
> >>>>
> >>>> Please also see Documentation/process/submitting-patches.rst for more
> >>>> information on how to use the tags.
> >>> Thanks for sharing this. My bad.
> >>> I remove your tags.
> >> The patches themselves seem fine. I'd just drop the 4th patch or at least
> >> come up with a better name for ov5693_hwcfg() --- you're acquiring
> >> resources there, and that generally fits well for probe. The code is fine
> >> already.
> > Then we don't need v5 with your reviewed tags removed?
> >
> > I think the patch4 is needed to add dts support properly.
> > Also this contains devm_clk_get_optional fix suggested by Jacopo and
> > support for ACPI-based platforms that specify the clock frequency by
> > using the "clock-frequency" property instead of specifying a clock
> > provider reference.
>
>
> I agree patch 4 in some form is needed - I didn't do the clock handling
> particularly well in this driver, and though it's ostensibly an ACPI
> driver it wouldn't actually work with a "normal" ACPI, but just with the
> cio2-bridge-repaired style. So the changes to the clock handling logic
> are welcome and needed I think. whether it needs to go into a separate
> function I don't particularly mind either way.
Yes, the clock handling needs to be changed. But I'd keep it in probe.
--
Sakari Ailus
Hi Sakari,
On Thu, Jun 30, 2022 at 02:21:04PM +0300, Sakari Ailus wrote:
> On Thu, Jun 30, 2022 at 11:15:40AM +0100, Daniel Scally wrote:
> > Hello
> >
> > On 30/06/2022 11:09, Tommaso Merciai wrote:
> > > Hi Sakari,
> > >
> > > On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
> > >> Hi Tommaso,
> > >>
> > >> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> > >>> Hi Sakari,
> > >>>
> > >>> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> > >>>> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > >>>>> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > >>>>>> On 30/06/2022 09:45, Tommaso Merciai wrote:
> > >>>>>>> Add documentation of device tree in YAML schema for the OV5693
> > >>>>>>> CMOS image sensor from Omnivision
> > >>>>>>>
> > >>>>>>> Signed-off-by: Tommaso Merciai <[email protected]>
> > >>>>>>> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > >>>>>>> Reviewed-by: Sakari Ailus <[email protected]>
> > >>>>>> How Sakari's tag appeared here? There was no email from him.
> > >>>>> Sakari made me some review on v2, but I think he forgot to add the mailing
> > >>>>> list in cc. ( I suppose :) )
> > >>>>>
> > >>>>> Let me know if I need to remove this.
> > >>>> You're only supposed to put these tags into patches if you get them in
> > >>>> written form as part of the review, signalling acceptance of the patch in
> > >>>> various forms. Just commenting a patch does not imply this.
> > >>>>
> > >>>> Please also see Documentation/process/submitting-patches.rst for more
> > >>>> information on how to use the tags.
> > >>> Thanks for sharing this. My bad.
> > >>> I remove your tags.
> > >> The patches themselves seem fine. I'd just drop the 4th patch or at least
> > >> come up with a better name for ov5693_hwcfg() --- you're acquiring
> > >> resources there, and that generally fits well for probe. The code is fine
> > >> already.
> > > Then we don't need v5 with your reviewed tags removed?
> > >
> > > I think the patch4 is needed to add dts support properly.
> > > Also this contains devm_clk_get_optional fix suggested by Jacopo and
> > > support for ACPI-based platforms that specify the clock frequency by
> > > using the "clock-frequency" property instead of specifying a clock
> > > provider reference.
> >
> >
> > I agree patch 4 in some form is needed - I didn't do the clock handling
> > particularly well in this driver, and though it's ostensibly an ACPI
> > driver it wouldn't actually work with a "normal" ACPI, but just with the
> > cio2-bridge-repaired style. So the changes to the clock handling logic
> > are welcome and needed I think. whether it needs to go into a separate
> > function I don't particularly mind either way.
>
> Yes, the clock handling needs to be changed. But I'd keep it in probe.
Fixed in v5, as you suggest.
Thanks all for your time.
Regards,
Tommaso
>
> --
> Sakari Ailus
--
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