| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPybu_0FQRA+5wnKo_m_4gxZv0AS9fQuyp3OXwaGA+2t+QdyaA@mail.gmail.com>
Date: Tue, 11 Mar 2025 22:22:36 +0100
From: Ricardo Ribalda Delgado <ribalda@...nel.org>
To: git@...tzsch.eu
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, ~postmarketos/upstreaming@...ts.sr.ht,
phone-devel@...r.kernel.org, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 3/4] media: i2c: imx214: Read clock frequency from
device tree
On Sat, Mar 8, 2025 at 10:48 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@...nel.org> wrote:
>
> From: André Apitzsch <git@...tzsch.eu>
>
> Replace the hard coded external clock frequency by the one read from
> device tree.
>
> Signed-off-by: André Apitzsch <git@...tzsch.eu>
> ---
> drivers/media/i2c/imx214.c | 99 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 79 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 53b6b427f263a8ad7e3a0d1f711ece234601100e..c3d55259d6fd1c4ca96f52833864bdfe6bedf13a 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -30,11 +30,10 @@
>
> #define IMX214_REG_FAST_STANDBY_CTRL CCI_REG8(0x0106)
>
> -#define IMX214_DEFAULT_CLK_FREQ 24000000
> -#define IMX214_DEFAULT_LINK_FREQ 600000000
> +#define IMX214_CLK_FREQ_24000KHZ 24000000
> +#define IMX214_LINK_FREQ_600MHZ 600000000
> /* Keep wrong link frequency for backward compatibility */
> #define IMX214_DEFAULT_LINK_FREQ_LEGACY 480000000
> -#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
> #define IMX214_FPS 30
>
> /* V-TIMING internal */
> @@ -233,6 +232,22 @@ static const char * const imx214_supply_name[] = {
>
> #define IMX214_NUM_SUPPLIES ARRAY_SIZE(imx214_supply_name)
>
> +static const s64 link_freq[] = {
> + IMX214_LINK_FREQ_600MHZ,
> +};
Do we need to move this?
> +
> +struct imx214_clk_params {
> + u32 clk_freq;
> + u64 link_freq;
> +};
> +
> +static const struct imx214_clk_params imx214_clk_params[] = {
> + {
> + .clk_freq = IMX214_CLK_FREQ_24000KHZ,
> + .link_freq = IMX214_LINK_FREQ_600MHZ,
> + },
> +};
> +
> /*
> * The supported formats.
> * This table MUST contain 4 entries per format, to cover the various flip
> @@ -270,6 +285,8 @@ struct imx214 {
> struct clk *xclk;
> struct regmap *regmap;
>
> + const struct imx214_clk_params *clk_params;
> +
> struct v4l2_subdev sd;
> struct media_pad pad;
>
> @@ -794,7 +811,7 @@ static int imx214_set_clock(struct imx214 *imx214)
> cci_write(imx214->regmap, IMX214_REG_PLL_MULT_DRIV,
> IMX214_PLL_SINGLE, &ret);
> cci_write(imx214->regmap, IMX214_REG_EXCK_FREQ,
> - IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000), &ret);
> + IMX214_EXCK_FREQ(imx214->clk_params->clk_freq / 1000000), &ret);
>
> return ret;
> }
> @@ -899,9 +916,6 @@ static const struct v4l2_ctrl_ops imx214_ctrl_ops = {
>
> static int imx214_ctrls_init(struct imx214 *imx214)
> {
> - static const s64 link_freq[] = {
> - IMX214_DEFAULT_LINK_FREQ
> - };
> static const struct v4l2_area unit_size = {
> .width = 1120,
> .height = 1120,
> @@ -910,6 +924,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> struct v4l2_fwnode_device_properties props;
> struct v4l2_ctrl_handler *ctrl_hdlr;
> int exposure_max, exposure_def;
> + int pixel_rate;
> int hblank;
> int i, ret;
>
> @@ -922,15 +937,25 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> if (ret)
> return ret;
>
> + pixel_rate = imx214->clk_params->link_freq * 8 / 10;
> imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
> V4L2_CID_PIXEL_RATE, 0,
> - IMX214_DEFAULT_PIXEL_RATE, 1,
> - IMX214_DEFAULT_PIXEL_RATE);
> + pixel_rate, 1, pixel_rate);
> +
> + for (i = 0; i < ARRAY_SIZE(link_freq); ++i) {
> + if (imx214->clk_params->link_freq == link_freq[i])
> + break;
> + }
> + if (i == ARRAY_SIZE(link_freq)) {
> + dev_err(imx214->dev, "link frequency %lld not supported\n",
> + imx214->clk_params->link_freq);
> + return -EINVAL;
> + }
>
> imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL,
> V4L2_CID_LINK_FREQ,
> ARRAY_SIZE(link_freq) - 1,
> - 0, link_freq);
> + i, link_freq);
> if (imx214->link_freq)
> imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> @@ -1047,7 +1072,7 @@ static int imx214_start_streaming(struct imx214 *imx214)
> return ret;
> }
>
> - link_bit_rate = IMX214_LINK_BIT_RATE(IMX214_DEFAULT_CLK_FREQ);
> + link_bit_rate = IMX214_LINK_BIT_RATE(imx214->clk_params->clk_freq);
> ret = cci_write(imx214->regmap, IMX214_REG_REQ_LINK_BIT_RATE,
> IMX214_LINK_BIT_RATE_MBPS(link_bit_rate), NULL);
> if (ret) {
> @@ -1238,7 +1263,20 @@ static int imx214_identify_module(struct imx214 *imx214)
> return 0;
> }
>
> -static int imx214_parse_fwnode(struct device *dev)
> +static int imx214_has_link_freq(struct imx214 *imx214, u64 link_frequency)
static bool imx214_set_clk_params(struct imx214 *imx214, u64 link_frequency)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(imx214_clk_params); ++i) {
> + if (imx214_clk_params[i].link_freq == link_frequency) {
> + imx214->clk_params = &imx214_clk_params[i];
> + break;
return true;
> + }
> + }
> + return (i < ARRAY_SIZE(imx214_clk_params));
return false;
> +}
> +
> +static int imx214_parse_fwnode(struct device *dev, struct imx214 *imx214)
> {
> struct fwnode_handle *endpoint;
> struct v4l2_fwnode_endpoint bus_cfg = {
> @@ -1268,13 +1306,14 @@ static int imx214_parse_fwnode(struct device *dev)
> dev_warn(dev, "Only one link-frequency supported, please review your DT. Continuing anyway\n");
>
> for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> - if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
> + if (imx214_has_link_freq(imx214, bus_cfg.link_frequencies[i]))
> break;
> if (bus_cfg.link_frequencies[i] ==
> IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> dev_warn(dev,
> "link-frequencies %d not supported, please review your DT. Continuing anyway\n",
> - IMX214_DEFAULT_LINK_FREQ);
> + IMX214_LINK_FREQ_600MHZ);
> + imx214->clk_params = &imx214_clk_params[1];
I guess you mean:
imx214->clk_params = &imx214_clk_params[0];
> break;
> }
> }
> @@ -1282,7 +1321,7 @@ static int imx214_parse_fwnode(struct device *dev)
> if (i == bus_cfg.nr_of_link_frequencies)
> ret = dev_err_probe(dev, -EINVAL,
> "link-frequencies %d not supported, please review your DT\n",
> - IMX214_DEFAULT_LINK_FREQ);
> + IMX214_LINK_FREQ_600MHZ);
I think this was wrong on the original driver.
it should be
bus_cfg.nr_of_link_frequencies ? bus_cfg.link_frequencies[0] : 0;
>
> done:
> v4l2_fwnode_endpoint_free(&bus_cfg);
> @@ -1294,16 +1333,17 @@ static int imx214_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> struct imx214 *imx214;
> + u32 xclk_freq;
> int ret;
>
> - ret = imx214_parse_fwnode(dev);
> - if (ret)
> - return ret;
> -
> imx214 = devm_kzalloc(dev, sizeof(*imx214), GFP_KERNEL);
> if (!imx214)
> return -ENOMEM;
>
> + ret = imx214_parse_fwnode(dev, imx214);
> + if (ret)
> + return ret;
> +
> imx214->dev = dev;
>
> imx214->xclk = devm_clk_get(dev, NULL);
> @@ -1311,7 +1351,26 @@ static int imx214_probe(struct i2c_client *client)
> return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> "failed to get xclk\n");
>
> - ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> + ret = device_property_read_u32(dev, "clock-frequency", &xclk_freq);
> + if (ret) {
> + dev_warn(dev,
> + "clock-frequency not set, please review your DT. Fallback to default\n");
> + xclk_freq = IMX214_CLK_FREQ_24000KHZ;
> + }
> +
> + switch (xclk_freq) {
> + case IMX214_CLK_FREQ_24000KHZ:
> + if (imx214->clk_params->clk_freq != xclk_freq)
> + return dev_err_probe(imx214->dev, -EINVAL,
> + "combination of clock and link frequency is not supported\n");
> + break;
> + default:
> + return dev_err_probe(imx214->dev, -EINVAL,
> + "external clock frequency %u is not supported\n",
> + xclk_freq);
> + }
> +
> + ret = clk_set_rate(imx214->xclk, xclk_freq);
> if (ret)
> return dev_err_probe(dev, ret,
> "failed to set xclk frequency\n");
>
> --
> 2.48.1
>
>
Powered by blists - more mailing lists