Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: michael.srba@seznam.cz
Cc: devicetree@vger.kernel.org,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	Shawn Guo <shawnguo@kernel.org>,
	Magnus Damm <magnus.damm@gmail.com>,
	linux-renesas-soc@vger.kernel.org,
	Rob Herring <robh+dt@kernel.org>,
	NXP Linux Team <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Fabio Estevam <festevam@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v2 1/3] media: i2c: imx219: add support for specifying clock-frequencies
Date: Mon, 7 Dec 2020 06:59:52 +0100
Message-ID: <20201207055952.GB14307@pengutronix.de> (raw)
In-Reply-To: <20201206172720.9406-1-michael.srba@seznam.cz>

Hi Michael,

On Sun, Dec 06, 2020 at 06:27:18PM +0100, michael.srba@seznam.cz wrote:
> From: Michael Srba <Michael.Srba@seznam.cz>
> 
> This patch adds 1% tolerance on input clock, similar to other camera sensor
> drivers. It also allows for specifying the actual clock in the device tree,
> instead of relying on it being already set to the right frequency (which is
> often not the case).
> 
> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
> 
> ---
> 
> changes since v1: default to exactly 24MHz when `clock-frequency` is not present
> 
> ---
>  drivers/media/i2c/imx219.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f64c0ef7a897..b6500e2ab19e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1443,13 +1443,28 @@ static int imx219_probe(struct i2c_client *client)
>  		return PTR_ERR(imx219->xclk);
>  	}
>  
> -	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> -	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &imx219->xclk_freq);
> +	if (ret) {
> +		dev_warn(dev, "could not get xclk frequency\n");
> +
> +		/* default to 24MHz */
> +		imx219->xclk_freq = 24000000;
> +	}
> +
> +	/* this driver currently expects 24MHz; allow 1% tolerance */
> +	if (imx219->xclk_freq < 23760000 || imx219->xclk_freq > 24240000) {
>  		dev_err(dev, "xclk frequency not supported: %d Hz\n",
>  			imx219->xclk_freq);
>  		return -EINVAL;
>  	}
>  
> +	ret = clk_set_rate(imx219->xclk, imx219->xclk_freq);
> +	if (ret) {
> +		dev_err(dev, "could not set xclk frequency\n");
> +		return ret;
> +	}

clk_set_rate() returns successfully when the rate change has succeeded.
It tells you nothing about the actual rate that has been set. The rate
could be very different from what you want to get, depending on what the
hardware is able to archieve. There's clk_round_rate() that tells you
which rate you'll get when you call clk_set_rate() with that value.
You would have to call clk_round_rate() first and see if you are happy
with the result, afterwards set the rate. From that view it doesn't make
much sense to check the device tree if a number between 23760000 and
24240000 is specified there, the clk api will do rounding anyway.

Also there's the assigned-clocks device tree binding, see
Documentation/devicetree/bindings/clock/clock-bindings.txt. This allows
you to set the desired clock rate directly in the device tree. All
that's left to do in the driver is to replace the check for the exact
rate with a check which allows a certain tolerance.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-06 17:27 michael.srba
2020-12-06 17:27 ` [PATCH v2 2/3] media: dt-bindings: media: i2c: imx219: document clock-frequency property michael.srba
2020-12-06 17:27 ` [PATCH v2 3/3] arm64: dts: update device trees to specify clock-frequency in imx219 node michael.srba
2020-12-07  5:59 ` Sascha Hauer [this message]
2020-12-10 20:51   ` [PATCH v2 1/3] media: i2c: imx219: add support for specifying clock-frequencies Michael Srba
2020-12-18  9:59     ` Krzysztof Kozlowski
2020-12-18 10:48 ` Dave Stevenson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201207055952.GB14307@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=dave.stevenson@raspberrypi.com \
    --cc=devicetree@vger.kernel.org \
    --cc=festevam@gmail.com \
    --cc=geert+renesas@glider.be \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=magnus.damm@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=michael.srba@seznam.cz \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git