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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f782d747-f0c8-ed95-ed61-26f1557ca046@ideasonboard.com>
Date:   Wed, 2 Aug 2023 12:45:43 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Jai Luthra <j-luthra@...com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        niklas.soderlund+renesas@...natech.se,
        Benoit Parrot <bparrot@...com>,
        Vaishnav Achath <vaishnav.a@...com>,
        Vignesh Raghavendra <vigneshr@...com>, nm@...com,
        devarsht@...com
Subject: Re: [PATCH v8 10/16] media: cadence: csi2rx: Set the STOP bit when
 stopping a stream

On 31/07/2023 11:29, Jai Luthra wrote:
> From: Pratyush Yadav <p.yadav@...com>
> 
> The stream stop procedure says that the STOP bit should be set when the
> stream is to be stopped, and then the ready bit in stream status
> register polled to make sure the STOP operation is finished.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@...com>
> Signed-off-by: Jai Luthra <j-luthra@...com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> ---
> v7->v8:
> - Fix bug where intention was to wait till stream status is idle, i.e.
>    STREAM_STATUS[31] -> 0 - but we were instead checking the opposite
> 
>   drivers/media/platform/cadence/cdns-csi2rx.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 30cdc260b46a..a17ef88dff82 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -8,6 +8,7 @@
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/io.h>
> +#include <linux/iopoll.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_graph.h>
> @@ -41,8 +42,12 @@
>   
>   #define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
>   #define CSI2RX_STREAM_CTRL_SOFT_RST			BIT(4)
> +#define CSI2RX_STREAM_CTRL_STOP				BIT(1)
>   #define CSI2RX_STREAM_CTRL_START			BIT(0)
>   
> +#define CSI2RX_STREAM_STATUS_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x004)
> +#define CSI2RX_STREAM_STATUS_RDY			BIT(31)
> +
>   #define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
>   #define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
>   #define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
> @@ -314,13 +319,24 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>   static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>   {
>   	unsigned int i;
> +	u32 val;
> +	int ret;
>   
>   	clk_prepare_enable(csi2rx->p_clk);
>   	reset_control_assert(csi2rx->sys_rst);
>   	clk_disable_unprepare(csi2rx->sys_clk);
>   
>   	for (i = 0; i < csi2rx->max_streams; i++) {
> -		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +		writel(CSI2RX_STREAM_CTRL_STOP,
> +		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +
> +		ret = readl_relaxed_poll_timeout(csi2rx->base +
> +						 CSI2RX_STREAM_STATUS_REG(i),
> +						 val,
> +						 !(val & CSI2RX_STREAM_STATUS_RDY),
> +						 10, 10000);
> +		if (ret)
> +			dev_warn(csi2rx->dev, "Failed to stop stream%u\n", i);

When adding streams support, I think the driver might need some 
adjustments. E.g. above says that e.g. stopping stream2 failed, which 
will be quite confusing as the reader probably thinks it refers to the 
"logical" streams. It would be helpful if it was always clear which of 
the streams the driver refers to (in prints, but also in code).

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>

  Tomi

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ