Re: [PATCH v2 5.10.y-cip 11/28] media: platform: Add Renesas RZ/G2L CRU driver

[Pavel Machek <pavel@denx.de>]

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

Hi!

> commit 07fc05bd0a797dcc34620869933993e1cbf56b98 upstream.
> 
> Add v4l driver for Renesas RZ/G2L Camera data Receiving Unit.
> 
> Based on a patch in the BSP by Hien Huynh
> <hien.huynh.px@renesas.com>

> +/**
> + * struct rzg2l_cru_dev - Renesas CRU device structure
> + * @dev:		(OF) device
> + * @base:		device I/O register space remapped to virtual memory
> + * @info:		info about CRU instance
> + *
...
> + * @num_buf:		Holds the current number of buffers enabled
> + * @ip:			Image processing subdev info

Holds-> holds, Image->image, for consistency with the rest.

> +void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
> +{
> +	u32 amnfifopntr, amnfifopntr_w, amnfifopntr_r_y;
> +	unsigned int retries = 0;
> +	unsigned long flags;
> +	u32 icnms;
> +
> +	spin_lock_irqsave(&cru->qlock, flags);
> +
> +	/* Disable and clear the interrupt */
> +	rzg2l_cru_write(cru, CRUnIE, 0);
> +	rzg2l_cru_write(cru, CRUnINTS, 0x001F0F0F);
...
> +	/* Wait until the FIFO becomes empty */
> +	for (retries = 5; retries > 0; retries--) {
> +		amnfifopntr = rzg2l_cru_read(cru, AMnFIFOPNTR);
> +
> +		amnfifopntr_w = amnfifopntr & AMnFIFOPNTR_FIFOWPNTR;
> +		amnfifopntr_r_y =
> +			(amnfifopntr & AMnFIFOPNTR_FIFORPNTR_Y) >> 16;
> +		if (amnfifopntr_w == amnfifopntr_r_y)
> +			break;
> +
> +		usleep_range(10, 20);

We are running with interrupts disabled here, we should use udelay(),
not sleep.

> +	}
> +
> +	/* Notify that FIFO is not empty here */
> +	if (!retries)
> +		dev_err(cru->dev, "Failed to empty FIFO\n");
> +
> +	/* Stop AXI bus */
> +	rzg2l_cru_write(cru, AMnAXISTP, AMnAXISTP_AXI_STOP);
> +
> +	/* Wait until the AXI bus stop */
> +	for (retries = 5; retries > 0; retries--) {
> +		if (rzg2l_cru_read(cru, AMnAXISTPACK) &
> +			AMnAXISTPACK_AXI_STOP_ACK)
> +			break;
> +
> +		usleep_range(10, 20);

Same here.

> +static int rzg2l_cru_open(struct file *file)
> +{
> +	struct rzg2l_cru_dev *cru = video_drvdata(file);
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&cru->lock);
> +	if (ret)
> +		return ret;
> +
> +	file->private_data = cru;
> +	ret = v4l2_fh_open(file);
> +	if (ret)
> +		goto err_unlock;
> +
> +	mutex_unlock(&cru->lock);
> +
> +	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&cru->lock);
> +
> +	return ret;
> +}

This can be simplified. You already have "ret = 0" in the normal path,
so this can be deleted:

> +	if (ret)
> +		goto err_unlock;
> +
> +	mutex_unlock(&cru->lock);
> +
> +	return 0;

Best regards,
									Pavel
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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