Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 4388 bytes --]

Hello Mita-san,

On Fri, Apr 27, 2018 at 01:17:55AM +0900, Akinobu Mita wrote:
> 2018-04-26 7:40 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > Hi Mita-san,
> >
> > On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
> >> 2018-04-24 0:54 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
> >> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
> >> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
> >> >>> This adds a device tree binding documentation for OV7720/OV7725 sensor.
> >> >>>
> >> >>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> >>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> >> >>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> >>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> >> >>> Cc: Rob Herring <robh+dt@kernel.org>
> >> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >> >>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> >>> ---
> >> >>> * v3
> >> >>> - Add Reviewed-by: lines
> >> >>>
> >> >>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 +++++++++++++++
> >> >>>  MAINTAINERS                                        |  1 +
> >> >>>  2 files changed, 43 insertions(+)
> >> >>>  create mode 100644
> >> >>>  Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>>
> >> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> b/Documentation/devicetree/bindings/media/i2c/ov772x.txt new file mode
> >> >>> 100644
> >> >>> index 0000000..b045503
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> @@ -0,0 +1,42 @@
> >> >>> +* Omnivision OV7720/OV7725 CMOS sensor
> >> >>> +
> >> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
> >> >>> output,
> >> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also
> >> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
> >> >>> formats. +
> >> >>> +Required Properties:
> >> >>> +- compatible: shall be one of
> >> >>> +     "ovti,ov7720"
> >> >>> +     "ovti,ov7725"
> >> >>> +- clocks: reference to the xclk input clock.
> >> >>> +- clock-names: shall be "xclk".
> >> >>
> >> >> As there's a single clock we could omit clock-names, couldn't we ?
> >> >
> >> > Sounds good.
> >> >
> >> > I'll prepare another patch that replaces the clock consumer ID argument
> >> > of clk_get() from "xclk" to NULL, and remove the above line in this
> >> > bindings.
> >>
> >> I thought it's easy to do.  However, there is a non-DT user
> >> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
> >>
> >> This can be resolved by retrying clk_get() with NULL if no entry
> >> with "xclk".  But should we do so or leave as is?
> >
> > How about patching the board code to register the clock alias with
> >
> >         clk_add_alias(NULL, "0-0021", "video_clk", NULL);
>
> Sounds good.
>
> But I'm a bit worried about whether clk_add_alias() can be called with
> alias == NULL.  I couldn't find such use case.
>
> Probably Jacopo can verify whether it works or not with v4 patchset.

Yes, you can use NULL to register a clock alias. Just make sure to drop the
clock name in ov772x driver (I have just verified the following works :)

diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
index 3d7d004..2deee53 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -592,7 +592,7 @@ static int __init migor_devices_setup(void)
        }

        /* Add a clock alias for ov7725 xclk source. */
-       clk_add_alias("xclk", "0-0021", "video_clk", NULL);
+       clk_add_alias(NULL, "0-0021", "video_clk", NULL);

        /* Register GPIOs for video sources. */
        gpiod_add_lookup_table(&ov7725_gpios);
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..e1f4076 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1281,7 +1281,7 @@ static int ov772x_probe(struct i2c_client *client,
        if (priv->hdl.error)
                return priv->hdl.error;

-       priv->clk = clk_get(&client->dev, "xclk");
+       priv->clk = clk_get(&client->dev, NULL);
        if (IS_ERR(priv->clk)) {
                dev_err(&client->dev, "Unable
Thanks
   j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]