[PATCH v2 01/10] media: ipu3-cio2: Simplify cleanup code

[PATCH v2 01/10] media: ipu3-cio2: Simplify cleanup code

[Andy Shevchenko]

The code looks more nicer if we use:
	while (i--)
instead:
	for (i = i - 1; i >= 0; i--)

This would also allow making 'i' unsigned again.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: converted i to unsigned (Sakari)
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 92f5eadf2c99..cb74d49934f1 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -847,7 +847,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
 	struct sg_table *sg;
 	struct sg_dma_page_iter sg_iter;
-	int i, j;
+	unsigned int i, j;
 
 	if (lops <= 0 || lops > CIO2_MAX_LOPS) {
 		dev_err(dev, "%s: bad buffer size (%i)\n", __func__,
@@ -887,7 +887,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	b->lop[i][j] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
 	return 0;
 fail:
-	for (i--; i >= 0; i--)
+	while (i--)
 		dma_free_coherent(dev, CIO2_PAGE_SIZE,
 				  b->lop[i], b->lop_bus_addr[i]);
 	return -ENOMEM;
-- 
2.28.0

[PATCH v2 02/10] media: ipu3-cio2: Introduce CIO2_LOP_ENTRIES constant

[Andy Shevchenko]

This constant is used in several places in the code, define it
for better maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: renamed CIO2_MAX_ENTRIES -> CIO2_LOP_ENTRIES (Sakari)
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 13 +++++--------
 drivers/media/pci/intel/ipu3/ipu3-cio2.h |  3 +++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index cb74d49934f1..a89cb3c7e0dc 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -127,7 +127,7 @@ static int cio2_fbpt_init_dummy(struct cio2_device *cio2)
 	 * List of Pointers(LOP) contains 1024x32b pointers to 4KB page each
 	 * Initialize each entry to dummy_page bus base address.
 	 */
-	for (i = 0; i < CIO2_PAGE_SIZE / sizeof(*cio2->dummy_lop); i++)
+	for (i = 0; i < CIO2_LOP_ENTRIES; i++)
 		cio2->dummy_lop[i] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
 
 	return 0;
@@ -160,8 +160,7 @@ static void cio2_fbpt_entry_init_dummy(struct cio2_device *cio2,
 	unsigned int i;
 
 	entry[0].first_entry.first_page_offset = 0;
-	entry[1].second_entry.num_of_pages =
-		CIO2_PAGE_SIZE / sizeof(u32) * CIO2_MAX_LOPS;
+	entry[1].second_entry.num_of_pages = CIO2_LOP_ENTRIES * CIO2_MAX_LOPS;
 	entry[1].second_entry.last_page_available_bytes = CIO2_PAGE_SIZE - 1;
 
 	for (i = 0; i < CIO2_MAX_LOPS; i++)
@@ -201,7 +200,7 @@ static void cio2_fbpt_entry_init_buf(struct cio2_device *cio2,
 	i = 0;
 	while (remaining > 0) {
 		entry->lop_page_addr = b->lop_bus_addr[i] >> PAGE_SHIFT;
-		remaining -= CIO2_PAGE_SIZE / sizeof(u32) * CIO2_PAGE_SIZE;
+		remaining -= CIO2_LOP_ENTRIES * CIO2_PAGE_SIZE;
 		entry++;
 		i++;
 	}
@@ -841,10 +840,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	struct device *dev = &cio2->pci_dev->dev;
 	struct cio2_buffer *b =
 		container_of(vb, struct cio2_buffer, vbb.vb2_buf);
-	static const unsigned int entries_per_page =
-		CIO2_PAGE_SIZE / sizeof(u32);
 	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE);
-	unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
+	unsigned int lops = DIV_ROUND_UP(pages + 1, CIO2_LOP_ENTRIES);
 	struct sg_table *sg;
 	struct sg_dma_page_iter sg_iter;
 	unsigned int i, j;
@@ -878,7 +875,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 			break;
 		b->lop[i][j] = sg_page_iter_dma_address(&sg_iter) >> PAGE_SHIFT;
 		j++;
-		if (j == entries_per_page) {
+		if (j == CIO2_LOP_ENTRIES) {
 			i++;
 			j = 0;
 		}
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index 7caab9b8c2b9..a64a829acc34 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -4,6 +4,8 @@
 #ifndef __IPU3_CIO2_H
 #define __IPU3_CIO2_H
 
+#include <linux/types.h>
+
 #define CIO2_NAME					"ipu3-cio2"
 #define CIO2_DEVICE_NAME				"Intel IPU3 CIO2"
 #define CIO2_ENTITY_NAME				"ipu3-csi2"
@@ -17,6 +19,7 @@
 /* 32MB = 8xFBPT_entry */
 #define CIO2_MAX_LOPS					8
 #define CIO2_MAX_BUFFERS			(PAGE_SIZE / 16 / CIO2_MAX_LOPS)
+#define CIO2_LOP_ENTRIES			(PAGE_SIZE / sizeof(u32))
 
 #define CIO2_PAD_SINK					0
 #define CIO2_PAD_SOURCE					1
-- 
2.28.0

Re: [PATCH v2 02/10] media: ipu3-cio2: Introduce CIO2_LOP_ENTRIES constant

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Mon, Aug 17, 2020 at 07:07:25PM +0300, Andy Shevchenko wrote:
> This constant is used in several places in the code, define it
> for better maintenance.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: renamed CIO2_MAX_ENTRIES -> CIO2_LOP_ENTRIES (Sakari)
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 13 +++++--------
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h |  3 +++
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index cb74d49934f1..a89cb3c7e0dc 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -127,7 +127,7 @@ static int cio2_fbpt_init_dummy(struct cio2_device *cio2)
>  	 * List of Pointers(LOP) contains 1024x32b pointers to 4KB page each
>  	 * Initialize each entry to dummy_page bus base address.
>  	 */
> -	for (i = 0; i < CIO2_PAGE_SIZE / sizeof(*cio2->dummy_lop); i++)
> +	for (i = 0; i < CIO2_LOP_ENTRIES; i++)
>  		cio2->dummy_lop[i] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
>  
>  	return 0;
> @@ -160,8 +160,7 @@ static void cio2_fbpt_entry_init_dummy(struct cio2_device *cio2,
>  	unsigned int i;
>  
>  	entry[0].first_entry.first_page_offset = 0;
> -	entry[1].second_entry.num_of_pages =
> -		CIO2_PAGE_SIZE / sizeof(u32) * CIO2_MAX_LOPS;
> +	entry[1].second_entry.num_of_pages = CIO2_LOP_ENTRIES * CIO2_MAX_LOPS;
>  	entry[1].second_entry.last_page_available_bytes = CIO2_PAGE_SIZE - 1;
>  
>  	for (i = 0; i < CIO2_MAX_LOPS; i++)
> @@ -201,7 +200,7 @@ static void cio2_fbpt_entry_init_buf(struct cio2_device *cio2,
>  	i = 0;
>  	while (remaining > 0) {
>  		entry->lop_page_addr = b->lop_bus_addr[i] >> PAGE_SHIFT;
> -		remaining -= CIO2_PAGE_SIZE / sizeof(u32) * CIO2_PAGE_SIZE;
> +		remaining -= CIO2_LOP_ENTRIES * CIO2_PAGE_SIZE;
>  		entry++;
>  		i++;
>  	}
> @@ -841,10 +840,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  	struct device *dev = &cio2->pci_dev->dev;
>  	struct cio2_buffer *b =
>  		container_of(vb, struct cio2_buffer, vbb.vb2_buf);
> -	static const unsigned int entries_per_page =
> -		CIO2_PAGE_SIZE / sizeof(u32);
>  	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE);
> -	unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> +	unsigned int lops = DIV_ROUND_UP(pages + 1, CIO2_LOP_ENTRIES);
>  	struct sg_table *sg;
>  	struct sg_dma_page_iter sg_iter;
>  	unsigned int i, j;
> @@ -878,7 +875,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  			break;
>  		b->lop[i][j] = sg_page_iter_dma_address(&sg_iter) >> PAGE_SHIFT;
>  		j++;
> -		if (j == entries_per_page) {
> +		if (j == CIO2_LOP_ENTRIES) {
>  			i++;
>  			j = 0;
>  		}
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index 7caab9b8c2b9..a64a829acc34 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -4,6 +4,8 @@
>  #ifndef __IPU3_CIO2_H
>  #define __IPU3_CIO2_H
>  
> +#include <linux/types.h>
> +
>  #define CIO2_NAME					"ipu3-cio2"
>  #define CIO2_DEVICE_NAME				"Intel IPU3 CIO2"
>  #define CIO2_ENTITY_NAME				"ipu3-csi2"
> @@ -17,6 +19,7 @@
>  /* 32MB = 8xFBPT_entry */
>  #define CIO2_MAX_LOPS					8
>  #define CIO2_MAX_BUFFERS			(PAGE_SIZE / 16 / CIO2_MAX_LOPS)
> +#define CIO2_LOP_ENTRIES			(PAGE_SIZE / sizeof(u32))

Shouldn't this be CIO2_PAGE_SIZE instead of PAGE_SIZE ?

>  
>  #define CIO2_PAD_SINK					0
>  #define CIO2_PAD_SOURCE					1

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 02/10] media: ipu3-cio2: Introduce CIO2_LOP_ENTRIES constant

[Andy Shevchenko]

On Fri, Oct 9, 2020 at 4:10 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Aug 17, 2020 at 07:07:25PM +0300, Andy Shevchenko wrote:
> > This constant is used in several places in the code, define it
> > for better maintenance.


> >  #define CIO2_MAX_BUFFERS                     (PAGE_SIZE / 16 / CIO2_MAX_LOPS)
> > +#define CIO2_LOP_ENTRIES                     (PAGE_SIZE / sizeof(u32))
>
> Shouldn't this be CIO2_PAGE_SIZE instead of PAGE_SIZE ?

I don't think it makes sense to define this. Then you need to define
all others as well and they will be the same as for the CPU/MMU. As I
explained in another patch it's quite unlikely that these two will be
different.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 03/10] media: ipu2-cio2: Replace custom definition with PAGE_SIZE

[Andy Shevchenko]

It's quite unlikely that another page size will be supported,
but in any case there is still an inconsistency between custom
page size definition and generic macros used in the driver.

Switch over to the generic PAGE_SIZE for sake of the consistency.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no change
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 30 ++++++++++--------------
 drivers/media/pci/intel/ipu3/ipu3-cio2.h |  1 -
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index a89cb3c7e0dc..0cb5461bfb1e 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -96,12 +96,12 @@ static inline u32 cio2_bytesperline(const unsigned int width)
 static void cio2_fbpt_exit_dummy(struct cio2_device *cio2)
 {
 	if (cio2->dummy_lop) {
-		dma_free_coherent(&cio2->pci_dev->dev, CIO2_PAGE_SIZE,
+		dma_free_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
 				  cio2->dummy_lop, cio2->dummy_lop_bus_addr);
 		cio2->dummy_lop = NULL;
 	}
 	if (cio2->dummy_page) {
-		dma_free_coherent(&cio2->pci_dev->dev, CIO2_PAGE_SIZE,
+		dma_free_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
 				  cio2->dummy_page, cio2->dummy_page_bus_addr);
 		cio2->dummy_page = NULL;
 	}
@@ -111,12 +111,10 @@ static int cio2_fbpt_init_dummy(struct cio2_device *cio2)
 {
 	unsigned int i;
 
-	cio2->dummy_page = dma_alloc_coherent(&cio2->pci_dev->dev,
-					      CIO2_PAGE_SIZE,
+	cio2->dummy_page = dma_alloc_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
 					      &cio2->dummy_page_bus_addr,
 					      GFP_KERNEL);
-	cio2->dummy_lop = dma_alloc_coherent(&cio2->pci_dev->dev,
-					     CIO2_PAGE_SIZE,
+	cio2->dummy_lop = dma_alloc_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
 					     &cio2->dummy_lop_bus_addr,
 					     GFP_KERNEL);
 	if (!cio2->dummy_page || !cio2->dummy_lop) {
@@ -161,7 +159,7 @@ static void cio2_fbpt_entry_init_dummy(struct cio2_device *cio2,
 
 	entry[0].first_entry.first_page_offset = 0;
 	entry[1].second_entry.num_of_pages = CIO2_LOP_ENTRIES * CIO2_MAX_LOPS;
-	entry[1].second_entry.last_page_available_bytes = CIO2_PAGE_SIZE - 1;
+	entry[1].second_entry.last_page_available_bytes = PAGE_SIZE - 1;
 
 	for (i = 0; i < CIO2_MAX_LOPS; i++)
 		entry[i].lop_page_addr = cio2->dummy_lop_bus_addr >> PAGE_SHIFT;
@@ -182,25 +180,24 @@ static void cio2_fbpt_entry_init_buf(struct cio2_device *cio2,
 	entry[0].first_entry.first_page_offset = b->offset;
 	remaining = length + entry[0].first_entry.first_page_offset;
 	entry[1].second_entry.num_of_pages =
-		DIV_ROUND_UP(remaining, CIO2_PAGE_SIZE);
+		DIV_ROUND_UP(remaining, PAGE_SIZE);
 	/*
 	 * last_page_available_bytes has the offset of the last byte in the
 	 * last page which is still accessible by DMA. DMA cannot access
 	 * beyond this point. Valid range for this is from 0 to 4095.
 	 * 0 indicates 1st byte in the page is DMA accessible.
-	 * 4095 (CIO2_PAGE_SIZE - 1) means every single byte in the last page
+	 * 4095 (PAGE_SIZE - 1) means every single byte in the last page
 	 * is available for DMA transfer.
 	 */
 	entry[1].second_entry.last_page_available_bytes =
 			(remaining & ~PAGE_MASK) ?
-				(remaining & ~PAGE_MASK) - 1 :
-				CIO2_PAGE_SIZE - 1;
+				(remaining & ~PAGE_MASK) - 1 : PAGE_SIZE - 1;
 	/* Fill FBPT */
 	remaining = length;
 	i = 0;
 	while (remaining > 0) {
 		entry->lop_page_addr = b->lop_bus_addr[i] >> PAGE_SHIFT;
-		remaining -= CIO2_LOP_ENTRIES * CIO2_PAGE_SIZE;
+		remaining -= CIO2_LOP_ENTRIES * PAGE_SIZE;
 		entry++;
 		i++;
 	}
@@ -840,7 +837,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	struct device *dev = &cio2->pci_dev->dev;
 	struct cio2_buffer *b =
 		container_of(vb, struct cio2_buffer, vbb.vb2_buf);
-	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE);
+	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, PAGE_SIZE);
 	unsigned int lops = DIV_ROUND_UP(pages + 1, CIO2_LOP_ENTRIES);
 	struct sg_table *sg;
 	struct sg_dma_page_iter sg_iter;
@@ -855,7 +852,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	memset(b->lop, 0, sizeof(b->lop));
 	/* Allocate LOP table */
 	for (i = 0; i < lops; i++) {
-		b->lop[i] = dma_alloc_coherent(dev, CIO2_PAGE_SIZE,
+		b->lop[i] = dma_alloc_coherent(dev, PAGE_SIZE,
 					       &b->lop_bus_addr[i], GFP_KERNEL);
 		if (!b->lop[i])
 			goto fail;
@@ -885,8 +882,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	return 0;
 fail:
 	while (i--)
-		dma_free_coherent(dev, CIO2_PAGE_SIZE,
-				  b->lop[i], b->lop_bus_addr[i]);
+		dma_free_coherent(dev, PAGE_SIZE, b->lop[i], b->lop_bus_addr[i]);
 	return -ENOMEM;
 }
 
@@ -976,7 +972,7 @@ static void cio2_vb2_buf_cleanup(struct vb2_buffer *vb)
 	/* Free LOP table */
 	for (i = 0; i < CIO2_MAX_LOPS; i++) {
 		if (b->lop[i])
-			dma_free_coherent(&cio2->pci_dev->dev, CIO2_PAGE_SIZE,
+			dma_free_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
 					  b->lop[i], b->lop_bus_addr[i]);
 	}
 }
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
index a64a829acc34..549b08f88f0c 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
@@ -392,7 +392,6 @@ struct cio2_device {
 					 sizeof(struct cio2_fbpt_entry))
 
 #define CIO2_FBPT_SUBENTRY_UNIT		4
-#define CIO2_PAGE_SIZE			4096
 
 /* cio2 fbpt first_entry ctrl status */
 #define CIO2_FBPT_CTRL_VALID		BIT(0)
-- 
2.28.0

Re: [PATCH v2 03/10] media: ipu2-cio2: Replace custom definition with PAGE_SIZE

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Mon, Aug 17, 2020 at 07:07:26PM +0300, Andy Shevchenko wrote:
> It's quite unlikely that another page size will be supported,
> but in any case there is still an inconsistency between custom
> page size definition and generic macros used in the driver.
> 
> Switch over to the generic PAGE_SIZE for sake of the consistency.

Is this conceptually correct though ? Does the CIO2 have an intrinsic
page size, or do pages here always refer to system memory pages ? In the
latter case the change is good, otherwise a separate macro seems best.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: no change
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 30 ++++++++++--------------
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h |  1 -
>  2 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index a89cb3c7e0dc..0cb5461bfb1e 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -96,12 +96,12 @@ static inline u32 cio2_bytesperline(const unsigned int width)
>  static void cio2_fbpt_exit_dummy(struct cio2_device *cio2)
>  {
>  	if (cio2->dummy_lop) {
> -		dma_free_coherent(&cio2->pci_dev->dev, CIO2_PAGE_SIZE,
> +		dma_free_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
>  				  cio2->dummy_lop, cio2->dummy_lop_bus_addr);
>  		cio2->dummy_lop = NULL;
>  	}
>  	if (cio2->dummy_page) {
> -		dma_free_coherent(&cio2->pci_dev->dev, CIO2_PAGE_SIZE,
> +		dma_free_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
>  				  cio2->dummy_page, cio2->dummy_page_bus_addr);
>  		cio2->dummy_page = NULL;
>  	}
> @@ -111,12 +111,10 @@ static int cio2_fbpt_init_dummy(struct cio2_device *cio2)
>  {
>  	unsigned int i;
>  
> -	cio2->dummy_page = dma_alloc_coherent(&cio2->pci_dev->dev,
> -					      CIO2_PAGE_SIZE,
> +	cio2->dummy_page = dma_alloc_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
>  					      &cio2->dummy_page_bus_addr,
>  					      GFP_KERNEL);
> -	cio2->dummy_lop = dma_alloc_coherent(&cio2->pci_dev->dev,
> -					     CIO2_PAGE_SIZE,
> +	cio2->dummy_lop = dma_alloc_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
>  					     &cio2->dummy_lop_bus_addr,
>  					     GFP_KERNEL);
>  	if (!cio2->dummy_page || !cio2->dummy_lop) {
> @@ -161,7 +159,7 @@ static void cio2_fbpt_entry_init_dummy(struct cio2_device *cio2,
>  
>  	entry[0].first_entry.first_page_offset = 0;
>  	entry[1].second_entry.num_of_pages = CIO2_LOP_ENTRIES * CIO2_MAX_LOPS;
> -	entry[1].second_entry.last_page_available_bytes = CIO2_PAGE_SIZE - 1;
> +	entry[1].second_entry.last_page_available_bytes = PAGE_SIZE - 1;
>  
>  	for (i = 0; i < CIO2_MAX_LOPS; i++)
>  		entry[i].lop_page_addr = cio2->dummy_lop_bus_addr >> PAGE_SHIFT;
> @@ -182,25 +180,24 @@ static void cio2_fbpt_entry_init_buf(struct cio2_device *cio2,
>  	entry[0].first_entry.first_page_offset = b->offset;
>  	remaining = length + entry[0].first_entry.first_page_offset;
>  	entry[1].second_entry.num_of_pages =
> -		DIV_ROUND_UP(remaining, CIO2_PAGE_SIZE);
> +		DIV_ROUND_UP(remaining, PAGE_SIZE);
>  	/*
>  	 * last_page_available_bytes has the offset of the last byte in the
>  	 * last page which is still accessible by DMA. DMA cannot access
>  	 * beyond this point. Valid range for this is from 0 to 4095.
>  	 * 0 indicates 1st byte in the page is DMA accessible.
> -	 * 4095 (CIO2_PAGE_SIZE - 1) means every single byte in the last page
> +	 * 4095 (PAGE_SIZE - 1) means every single byte in the last page
>  	 * is available for DMA transfer.
>  	 */
>  	entry[1].second_entry.last_page_available_bytes =
>  			(remaining & ~PAGE_MASK) ?
> -				(remaining & ~PAGE_MASK) - 1 :
> -				CIO2_PAGE_SIZE - 1;
> +				(remaining & ~PAGE_MASK) - 1 : PAGE_SIZE - 1;
>  	/* Fill FBPT */
>  	remaining = length;
>  	i = 0;
>  	while (remaining > 0) {
>  		entry->lop_page_addr = b->lop_bus_addr[i] >> PAGE_SHIFT;
> -		remaining -= CIO2_LOP_ENTRIES * CIO2_PAGE_SIZE;
> +		remaining -= CIO2_LOP_ENTRIES * PAGE_SIZE;
>  		entry++;
>  		i++;
>  	}
> @@ -840,7 +837,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  	struct device *dev = &cio2->pci_dev->dev;
>  	struct cio2_buffer *b =
>  		container_of(vb, struct cio2_buffer, vbb.vb2_buf);
> -	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE);
> +	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, PAGE_SIZE);
>  	unsigned int lops = DIV_ROUND_UP(pages + 1, CIO2_LOP_ENTRIES);
>  	struct sg_table *sg;
>  	struct sg_dma_page_iter sg_iter;
> @@ -855,7 +852,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  	memset(b->lop, 0, sizeof(b->lop));
>  	/* Allocate LOP table */
>  	for (i = 0; i < lops; i++) {
> -		b->lop[i] = dma_alloc_coherent(dev, CIO2_PAGE_SIZE,
> +		b->lop[i] = dma_alloc_coherent(dev, PAGE_SIZE,
>  					       &b->lop_bus_addr[i], GFP_KERNEL);
>  		if (!b->lop[i])
>  			goto fail;
> @@ -885,8 +882,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  	return 0;
>  fail:
>  	while (i--)
> -		dma_free_coherent(dev, CIO2_PAGE_SIZE,
> -				  b->lop[i], b->lop_bus_addr[i]);
> +		dma_free_coherent(dev, PAGE_SIZE, b->lop[i], b->lop_bus_addr[i]);
>  	return -ENOMEM;
>  }
>  
> @@ -976,7 +972,7 @@ static void cio2_vb2_buf_cleanup(struct vb2_buffer *vb)
>  	/* Free LOP table */
>  	for (i = 0; i < CIO2_MAX_LOPS; i++) {
>  		if (b->lop[i])
> -			dma_free_coherent(&cio2->pci_dev->dev, CIO2_PAGE_SIZE,
> +			dma_free_coherent(&cio2->pci_dev->dev, PAGE_SIZE,
>  					  b->lop[i], b->lop_bus_addr[i]);
>  	}
>  }
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index a64a829acc34..549b08f88f0c 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -392,7 +392,6 @@ struct cio2_device {
>  					 sizeof(struct cio2_fbpt_entry))
>  
>  #define CIO2_FBPT_SUBENTRY_UNIT		4
> -#define CIO2_PAGE_SIZE			4096
>  
>  /* cio2 fbpt first_entry ctrl status */
>  #define CIO2_FBPT_CTRL_VALID		BIT(0)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/10] media: ipu2-cio2: Replace custom definition with PAGE_SIZE

[Andy Shevchenko]

On Fri, Oct 09, 2020 at 04:04:56AM +0300, Laurent Pinchart wrote:
> On Mon, Aug 17, 2020 at 07:07:26PM +0300, Andy Shevchenko wrote:
> > It's quite unlikely that another page size will be supported,
> > but in any case there is still an inconsistency between custom
> > page size definition and generic macros used in the driver.
> > 
> > Switch over to the generic PAGE_SIZE for sake of the consistency.
> 
> Is this conceptually correct though ? Does the CIO2 have an intrinsic
> page size, or do pages here always refer to system memory pages ? In the
> latter case the change is good, otherwise a separate macro seems best.

I don't think the hardware is going to change these defaults.
But of course we may repeat all macros and constants over the code, which makes
a little sense to me, because hardware is using same settings as CPU MMU.

In principle we may do that, but in reality the page size change will bring a
hell out of it with all code being at least rechecked and I believe partially
rewritten (you can imagine how to map for example CIO2 MMU with 11-bit per page
to CPU page which is 12-bit).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 04/10] media: ipu3-cio2: Use macros from pfn.h

[Andy Shevchenko]

There are few nice macros in pfn.h, some of which we may use here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no change
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 0cb5461bfb1e..35bf05de5d5d 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -16,6 +16,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/pfn.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/vmalloc.h>
@@ -126,7 +127,7 @@ static int cio2_fbpt_init_dummy(struct cio2_device *cio2)
 	 * Initialize each entry to dummy_page bus base address.
 	 */
 	for (i = 0; i < CIO2_LOP_ENTRIES; i++)
-		cio2->dummy_lop[i] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
+		cio2->dummy_lop[i] = PFN_DOWN(cio2->dummy_page_bus_addr);
 
 	return 0;
 }
@@ -162,7 +163,7 @@ static void cio2_fbpt_entry_init_dummy(struct cio2_device *cio2,
 	entry[1].second_entry.last_page_available_bytes = PAGE_SIZE - 1;
 
 	for (i = 0; i < CIO2_MAX_LOPS; i++)
-		entry[i].lop_page_addr = cio2->dummy_lop_bus_addr >> PAGE_SHIFT;
+		entry[i].lop_page_addr = PFN_DOWN(cio2->dummy_lop_bus_addr);
 
 	cio2_fbpt_entry_enable(cio2, entry);
 }
@@ -179,8 +180,7 @@ static void cio2_fbpt_entry_init_buf(struct cio2_device *cio2,
 
 	entry[0].first_entry.first_page_offset = b->offset;
 	remaining = length + entry[0].first_entry.first_page_offset;
-	entry[1].second_entry.num_of_pages =
-		DIV_ROUND_UP(remaining, PAGE_SIZE);
+	entry[1].second_entry.num_of_pages = PFN_UP(remaining);
 	/*
 	 * last_page_available_bytes has the offset of the last byte in the
 	 * last page which is still accessible by DMA. DMA cannot access
@@ -196,7 +196,7 @@ static void cio2_fbpt_entry_init_buf(struct cio2_device *cio2,
 	remaining = length;
 	i = 0;
 	while (remaining > 0) {
-		entry->lop_page_addr = b->lop_bus_addr[i] >> PAGE_SHIFT;
+		entry->lop_page_addr = PFN_DOWN(b->lop_bus_addr[i]);
 		remaining -= CIO2_LOP_ENTRIES * PAGE_SIZE;
 		entry++;
 		i++;
@@ -205,7 +205,7 @@ static void cio2_fbpt_entry_init_buf(struct cio2_device *cio2,
 	/*
 	 * The first not meaningful FBPT entry should point to a valid LOP
 	 */
-	entry->lop_page_addr = cio2->dummy_lop_bus_addr >> PAGE_SHIFT;
+	entry->lop_page_addr = PFN_DOWN(cio2->dummy_lop_bus_addr);
 
 	cio2_fbpt_entry_enable(cio2, entry);
 }
@@ -471,8 +471,7 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q)
 	}
 
 	/* Enable DMA */
-	writel(q->fbpt_bus_addr >> PAGE_SHIFT,
-	       base + CIO2_REG_CDMABA(CIO2_DMA_CHAN));
+	writel(PFN_DOWN(q->fbpt_bus_addr), base + CIO2_REG_CDMABA(CIO2_DMA_CHAN));
 
 	writel(num_buffers1 << CIO2_CDMAC0_FBPT_LEN_SHIFT |
 	       FBPT_WIDTH << CIO2_CDMAC0_FBPT_WIDTH_SHIFT |
@@ -837,7 +836,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	struct device *dev = &cio2->pci_dev->dev;
 	struct cio2_buffer *b =
 		container_of(vb, struct cio2_buffer, vbb.vb2_buf);
-	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, PAGE_SIZE);
+	unsigned int pages = PFN_UP(vb->planes[0].length);
 	unsigned int lops = DIV_ROUND_UP(pages + 1, CIO2_LOP_ENTRIES);
 	struct sg_table *sg;
 	struct sg_dma_page_iter sg_iter;
@@ -870,7 +869,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 	for_each_sg_dma_page (sg->sgl, &sg_iter, sg->nents, 0) {
 		if (!pages--)
 			break;
-		b->lop[i][j] = sg_page_iter_dma_address(&sg_iter) >> PAGE_SHIFT;
+		b->lop[i][j] = PFN_DOWN(sg_page_iter_dma_address(&sg_iter));
 		j++;
 		if (j == CIO2_LOP_ENTRIES) {
 			i++;
@@ -878,7 +877,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 		}
 	}
 
-	b->lop[i][j] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
+	b->lop[i][j] = PFN_DOWN(cio2->dummy_page_bus_addr);
 	return 0;
 fail:
 	while (i--)
-- 
2.28.0

[PATCH v2 05/10] media: ipu3-cio2: Replace infinite loop by one with clear exit condition

[Andy Shevchenko]

Refactor cio2_buffer_done() to get rid of infinite loop by replacing it by one
with clear exit condition. This change also allows to check for an error ahead.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: Dropped const (Bingbu)
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 35bf05de5d5d..36b4c7730f43 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -540,7 +540,7 @@ static void cio2_buffer_done(struct cio2_device *cio2, unsigned int dma_chan)
 {
 	struct device *dev = &cio2->pci_dev->dev;
 	struct cio2_queue *q = cio2->cur_queue;
-	int buffers_found = 0;
+	struct cio2_fbpt_entry *entry;
 	u64 ns = ktime_get_ns();
 
 	if (dma_chan >= CIO2_QUEUES) {
@@ -548,15 +548,18 @@ static void cio2_buffer_done(struct cio2_device *cio2, unsigned int dma_chan)
 		return;
 	}
 
+	entry = &q->fbpt[q->bufs_first * CIO2_MAX_LOPS];
+	if (entry->first_entry.ctrl & CIO2_FBPT_CTRL_VALID) {
+		dev_warn(&cio2->pci_dev->dev,
+			 "no ready buffers found on DMA channel %u\n",
+			 dma_chan);
+		return;
+	}
+
 	/* Find out which buffer(s) are ready */
 	do {
-		struct cio2_fbpt_entry *const entry =
-			&q->fbpt[q->bufs_first * CIO2_MAX_LOPS];
 		struct cio2_buffer *b;
 
-		if (entry->first_entry.ctrl & CIO2_FBPT_CTRL_VALID)
-			break;
-
 		b = q->bufs[q->bufs_first];
 		if (b) {
 			unsigned int bytes = entry[1].second_entry.num_of_bytes;
@@ -578,13 +581,8 @@ static void cio2_buffer_done(struct cio2_device *cio2, unsigned int dma_chan)
 		atomic_inc(&q->frame_sequence);
 		cio2_fbpt_entry_init_dummy(cio2, entry);
 		q->bufs_first = (q->bufs_first + 1) % CIO2_MAX_BUFFERS;
-		buffers_found++;
-	} while (1);
-
-	if (buffers_found == 0)
-		dev_warn(&cio2->pci_dev->dev,
-			 "no ready buffers found on DMA channel %u\n",
-			 dma_chan);
+		entry = &q->fbpt[q->bufs_first * CIO2_MAX_LOPS];
+	} while (!(entry->first_entry.ctrl & CIO2_FBPT_CTRL_VALID));
 }
 
 static void cio2_queue_event_sof(struct cio2_device *cio2, struct cio2_queue *q)
-- 
2.28.0

[PATCH v2 06/10] media: ipu3-cio2: Use readl_poll_timeout() helper

[Andy Shevchenko]

We may use special helper macro to poll IO till condition or timeout occurs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: wrapped long line (Bingbu)
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 36b4c7730f43..7bcde3ba8f6e 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -14,6 +14,7 @@
 
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pfn.h>
@@ -507,8 +508,10 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q)
 
 static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q)
 {
-	void __iomem *base = cio2->base;
-	unsigned int i, maxloops = 1000;
+	void __iomem *const base = cio2->base;
+	unsigned int i;
+	u32 value;
+	int ret;
 
 	/* Disable CSI receiver and MIPI backend devices */
 	writel(0, q->csi_rx_base + CIO2_REG_IRQCTRL_MASK);
@@ -518,13 +521,10 @@ static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q)
 
 	/* Halt DMA */
 	writel(0, base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN));
-	do {
-		if (readl(base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN)) &
-		    CIO2_CDMAC0_DMA_HALTED)
-			break;
-		usleep_range(1000, 2000);
-	} while (--maxloops);
-	if (!maxloops)
+	ret = readl_poll_timeout(base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN),
+				 value, value & CIO2_CDMAC0_DMA_HALTED,
+				 4000, 2000000);
+	if (ret)
 		dev_err(&cio2->pci_dev->dev,
 			"DMA %i can not be halted\n", CIO2_DMA_CHAN);
 
-- 
2.28.0

Re: [PATCH v2 06/10] media: ipu3-cio2: Use readl_poll_timeout() helper

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Mon, Aug 17, 2020 at 07:07:29PM +0300, Andy Shevchenko wrote:
> We may use special helper macro to poll IO till condition or timeout occurs.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: wrapped long line (Bingbu)
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 36b4c7730f43..7bcde3ba8f6e 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/pfn.h>
> @@ -507,8 +508,10 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q)
>  
>  static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q)
>  {
> -	void __iomem *base = cio2->base;
> -	unsigned int i, maxloops = 1000;
> +	void __iomem *const base = cio2->base;
> +	unsigned int i;
> +	u32 value;
> +	int ret;
>  
>  	/* Disable CSI receiver and MIPI backend devices */
>  	writel(0, q->csi_rx_base + CIO2_REG_IRQCTRL_MASK);
> @@ -518,13 +521,10 @@ static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q)
>  
>  	/* Halt DMA */
>  	writel(0, base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN));
> -	do {
> -		if (readl(base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN)) &
> -		    CIO2_CDMAC0_DMA_HALTED)
> -			break;
> -		usleep_range(1000, 2000);
> -	} while (--maxloops);
> -	if (!maxloops)
> +	ret = readl_poll_timeout(base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN),
> +				 value, value & CIO2_CDMAC0_DMA_HALTED,
> +				 4000, 2000000);

I had to read the implementation of readl_poll_timeout() to see why you
turned [1000, 2000] to 4000. This effectively becomes [1000, 4000] which
should be fine.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (ret)
>  		dev_err(&cio2->pci_dev->dev,
>  			"DMA %i can not be halted\n", CIO2_DMA_CHAN);
>  
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 07/10] media: ipu3-cio2: Get rid of pci_set_master() duplication

[Andy Shevchenko]

It's unclear why driver repeats the code from PCI core.
Drop it for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 26 +++++-------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 7bcde3ba8f6e..57310d7874ce 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1711,24 +1711,6 @@ static void cio2_queues_exit(struct cio2_device *cio2)
 
 /**************** PCI interface ****************/
 
-static int cio2_pci_config_setup(struct pci_dev *dev)
-{
-	u16 pci_command;
-	int r = pci_enable_msi(dev);
-
-	if (r) {
-		dev_err(&dev->dev, "failed to enable MSI (%d)\n", r);
-		return r;
-	}
-
-	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
-	pci_command |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
-		PCI_COMMAND_INTX_DISABLE;
-	pci_write_config_word(dev, PCI_COMMAND, pci_command);
-
-	return 0;
-}
-
 static int cio2_pci_probe(struct pci_dev *pci_dev,
 			  const struct pci_device_id *id)
 {
@@ -1774,9 +1756,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 		return -ENODEV;
 	}
 
-	r = cio2_pci_config_setup(pci_dev);
-	if (r)
-		return -ENODEV;
+	r = pci_enable_msi(pci_dev);
+	if (r) {
+		dev_err(&pci_dev->dev, "failed to enable MSI (%d)\n", r);
+		return r;
+	}
 
 	r = cio2_fbpt_init_dummy(cio2);
 	if (r)
-- 
2.28.0

Re: [PATCH v2 07/10] media: ipu3-cio2: Get rid of pci_set_master() duplication

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Mon, Aug 17, 2020 at 07:07:30PM +0300, Andy Shevchenko wrote:
> It's unclear why driver repeats the code from PCI core.
> Drop it for good.

You may want to mention that pci_set_master() is already called by the
cio2_pci_probe().

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 26 +++++-------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 7bcde3ba8f6e..57310d7874ce 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1711,24 +1711,6 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>  
>  /**************** PCI interface ****************/
>  
> -static int cio2_pci_config_setup(struct pci_dev *dev)
> -{
> -	u16 pci_command;
> -	int r = pci_enable_msi(dev);
> -
> -	if (r) {
> -		dev_err(&dev->dev, "failed to enable MSI (%d)\n", r);
> -		return r;
> -	}
> -
> -	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> -	pci_command |= PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
> -		PCI_COMMAND_INTX_DISABLE;
> -	pci_write_config_word(dev, PCI_COMMAND, pci_command);
> -
> -	return 0;
> -}
> -
>  static int cio2_pci_probe(struct pci_dev *pci_dev,
>  			  const struct pci_device_id *id)
>  {
> @@ -1774,9 +1756,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  		return -ENODEV;
>  	}
>  
> -	r = cio2_pci_config_setup(pci_dev);
> -	if (r)
> -		return -ENODEV;
> +	r = pci_enable_msi(pci_dev);
> +	if (r) {
> +		dev_err(&pci_dev->dev, "failed to enable MSI (%d)\n", r);
> +		return r;
> +	}
>  
>  	r = cio2_fbpt_init_dummy(cio2);
>  	if (r)

-- 
Regards,

Laurent Pinchart

[PATCH v2 08/10] media: ipu3-cio2: Drop bogus check and error message

[Andy Shevchenko]

pcim_iomap_table() won't fail if previous pcim_iomap_regions() hasn't.
Since we check pcim_iomap_regions() for failure the check close to
pcim_iomap_table() is bogus and not needed.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 57310d7874ce..f5c27c1aa9a2 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1715,7 +1715,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 			  const struct pci_device_id *id)
 {
 	struct cio2_device *cio2;
-	void __iomem *const *iomap;
 	int r;
 
 	cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);
@@ -1738,13 +1737,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 		return -ENODEV;
 	}
 
-	iomap = pcim_iomap_table(pci_dev);
-	if (!iomap) {
-		dev_err(&pci_dev->dev, "failed to iomap table\n");
-		return -ENODEV;
-	}
-
-	cio2->base = iomap[CIO2_PCI_BAR];
+	cio2->base = pcim_iomap_table(pci_dev)[CIO2_PCI_BAR];
 
 	pci_set_drvdata(pci_dev, cio2);
 
-- 
2.28.0

Re: [PATCH v2 08/10] media: ipu3-cio2: Drop bogus check and error message

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Mon, Aug 17, 2020 at 07:07:31PM +0300, Andy Shevchenko wrote:
> pcim_iomap_table() won't fail if previous pcim_iomap_regions() hasn't.
> Since we check pcim_iomap_regions() for failure the check close to
> pcim_iomap_table() is bogus and not needed.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 57310d7874ce..f5c27c1aa9a2 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1715,7 +1715,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  			  const struct pci_device_id *id)
>  {
>  	struct cio2_device *cio2;
> -	void __iomem *const *iomap;
>  	int r;
>  
>  	cio2 = devm_kzalloc(&pci_dev->dev, sizeof(*cio2), GFP_KERNEL);
> @@ -1738,13 +1737,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  		return -ENODEV;
>  	}
>  
> -	iomap = pcim_iomap_table(pci_dev);
> -	if (!iomap) {
> -		dev_err(&pci_dev->dev, "failed to iomap table\n");
> -		return -ENODEV;
> -	}
> -
> -	cio2->base = iomap[CIO2_PCI_BAR];
> +	cio2->base = pcim_iomap_table(pci_dev)[CIO2_PCI_BAR];

pcim_iomap_table() can return NULL if devres_alloc() runs out of memory.

>  
>  	pci_set_drvdata(pci_dev, cio2);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/10] media: ipu3-cio2: Drop bogus check and error message

[Andy Shevchenko]

On Fri, Oct 9, 2020 at 4:22 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Aug 17, 2020 at 07:07:31PM +0300, Andy Shevchenko wrote:
> > pcim_iomap_table() won't fail if previous pcim_iomap_regions() hasn't.
> > Since we check pcim_iomap_regions() for failure the check close to
> > pcim_iomap_table() is bogus and not needed.

> > +     cio2->base = pcim_iomap_table(pci_dev)[CIO2_PCI_BAR];
>
> pcim_iomap_table() can return NULL if devres_alloc() runs out of memory.

True. And this is checked by pcim_iomap_regions(). So, dup check is
not necessary.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 09/10] media: ipu3-cio2: Drop useless assignments

[Andy Shevchenko]

There are assignments inside the functions which are useless.
Drop them for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index f5c27c1aa9a2..f3ec2d62cace 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1979,8 +1979,8 @@ static int __maybe_unused cio2_suspend(struct device *dev)
 static int __maybe_unused cio2_resume(struct device *dev)
 {
 	struct cio2_device *cio2 = dev_get_drvdata(dev);
-	int r = 0;
 	struct cio2_queue *q = cio2->cur_queue;
+	int r;
 
 	dev_dbg(dev, "cio2 resume\n");
 	if (!cio2->streaming)
@@ -2007,7 +2007,7 @@ static const struct dev_pm_ops cio2_pm_ops = {
 
 static const struct pci_device_id cio2_pci_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, CIO2_PCI_ID) },
-	{ 0 }
+	{ }
 };
 
 MODULE_DEVICE_TABLE(pci, cio2_pci_id_table);
-- 
2.28.0

Re: [PATCH v2 09/10] media: ipu3-cio2: Drop useless assignments

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Mon, Aug 17, 2020 at 07:07:32PM +0300, Andy Shevchenko wrote:
> There are assignments inside the functions which are useless.
> Drop them for good.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index f5c27c1aa9a2..f3ec2d62cace 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1979,8 +1979,8 @@ static int __maybe_unused cio2_suspend(struct device *dev)
>  static int __maybe_unused cio2_resume(struct device *dev)
>  {
>  	struct cio2_device *cio2 = dev_get_drvdata(dev);
> -	int r = 0;
>  	struct cio2_queue *q = cio2->cur_queue;
> +	int r;
>  
>  	dev_dbg(dev, "cio2 resume\n");
>  	if (!cio2->streaming)
> @@ -2007,7 +2007,7 @@ static const struct dev_pm_ops cio2_pm_ops = {
>  
>  static const struct pci_device_id cio2_pci_id_table[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, CIO2_PCI_ID) },
> -	{ 0 }
> +	{ }

This change is good but doesn't really match the commit message. You may
want to update it. With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  };
>  
>  MODULE_DEVICE_TABLE(pci, cio2_pci_id_table);

-- 
Regards,

Laurent Pinchart

[PATCH v2 10/10] media: ipu3-cio2: Update Copyright year and fix indentation issues

[Andy Shevchenko]

Update Copyright year to cover the previous changes and at the same time
address indentation issues.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: one more indentation fix
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index f3ec2d62cace..9a8c6e99d3ac 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (C) 2017 Intel Corporation
+ * Copyright (C) 2017,2020 Intel Corporation
  *
  * Based partially on Intel IPU4 driver written by
  *  Sakari Ailus <sakari.ailus@linux.intel.com>
@@ -9,7 +9,6 @@
  *  Jouni Ukkonen <jouni.ukkonen@intel.com>
  *  Antti Laakso <antti.laakso@intel.com>
  * et al.
- *
  */
 
 #include <linux/delay.h>
@@ -292,7 +291,7 @@ static int cio2_csi2_calc_timing(struct cio2_device *cio2, struct cio2_queue *q,
 				 struct cio2_csi2_timing *timing)
 {
 	struct device *dev = &cio2->pci_dev->dev;
-	struct v4l2_querymenu qm = {.id = V4L2_CID_LINK_FREQ, };
+	struct v4l2_querymenu qm = { .id = V4L2_CID_LINK_FREQ };
 	struct v4l2_ctrl *link_freq;
 	s64 freq;
 	int r;
@@ -864,7 +863,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 		b->offset = sg->sgl->offset;
 
 	i = j = 0;
-	for_each_sg_dma_page (sg->sgl, &sg_iter, sg->nents, 0) {
+	for_each_sg_dma_page(sg->sgl, &sg_iter, sg->nents, 0) {
 		if (!pages--)
 			break;
 		b->lop[i][j] = PFN_DOWN(sg_page_iter_dma_address(&sg_iter));
-- 
2.28.0

Re: [PATCH v2 10/10] media: ipu3-cio2: Update Copyright year and fix indentation issues

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Mon, Aug 17, 2020 at 07:07:33PM +0300, Andy Shevchenko wrote:
> Update Copyright year to cover the previous changes and at the same time
> address indentation issues.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v2: one more indentation fix
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index f3ec2d62cace..9a8c6e99d3ac 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * Copyright (C) 2017 Intel Corporation
> + * Copyright (C) 2017,2020 Intel Corporation
>   *
>   * Based partially on Intel IPU4 driver written by
>   *  Sakari Ailus <sakari.ailus@linux.intel.com>
> @@ -9,7 +9,6 @@
>   *  Jouni Ukkonen <jouni.ukkonen@intel.com>
>   *  Antti Laakso <antti.laakso@intel.com>
>   * et al.
> - *
>   */
>  
>  #include <linux/delay.h>
> @@ -292,7 +291,7 @@ static int cio2_csi2_calc_timing(struct cio2_device *cio2, struct cio2_queue *q,
>  				 struct cio2_csi2_timing *timing)
>  {
>  	struct device *dev = &cio2->pci_dev->dev;
> -	struct v4l2_querymenu qm = {.id = V4L2_CID_LINK_FREQ, };
> +	struct v4l2_querymenu qm = { .id = V4L2_CID_LINK_FREQ };
>  	struct v4l2_ctrl *link_freq;
>  	s64 freq;
>  	int r;
> @@ -864,7 +863,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  		b->offset = sg->sgl->offset;
>  
>  	i = j = 0;
> -	for_each_sg_dma_page (sg->sgl, &sg_iter, sg->nents, 0) {
> +	for_each_sg_dma_page(sg->sgl, &sg_iter, sg->nents, 0) {
>  		if (!pages--)
>  			break;
>  		b->lop[i][j] = PFN_DOWN(sg_page_iter_dma_address(&sg_iter));

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: ipu3-cio2: Simplify cleanup code

[Laurent Pinchart]

Hi Andy,

Thank you for the patch. Glad to see interest in the CIO2 driver :-)

On Mon, Aug 17, 2020 at 07:07:24PM +0300, Andy Shevchenko wrote:
> The code looks more nicer if we use:
> 	while (i--)
> instead:
> 	for (i = i - 1; i >= 0; i--)
> 
> This would also allow making 'i' unsigned again.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: converted i to unsigned (Sakari)
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 92f5eadf2c99..cb74d49934f1 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -847,7 +847,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  	unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
>  	struct sg_table *sg;
>  	struct sg_dma_page_iter sg_iter;
> -	int i, j;
> +	unsigned int i, j;
>  
>  	if (lops <= 0 || lops > CIO2_MAX_LOPS) {
>  		dev_err(dev, "%s: bad buffer size (%i)\n", __func__,
> @@ -887,7 +887,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  	b->lop[i][j] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
>  	return 0;
>  fail:
> -	for (i--; i >= 0; i--)
> +	while (i--)
>  		dma_free_coherent(dev, CIO2_PAGE_SIZE,
>  				  b->lop[i], b->lop_bus_addr[i]);

Looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

There's an additional issue though, if vb2_dma_sg_plane_desc() fails, we
should free all the allocated memory.

>  	return -ENOMEM;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: ipu3-cio2: Simplify cleanup code

[Sakari Ailus]

Hi Laurent and Andy,

On Fri, Oct 09, 2020 at 03:57:16AM +0300, Laurent Pinchart wrote:
> Hi Andy,
> 
> Thank you for the patch. Glad to see interest in the CIO2 driver :-)
> 
> On Mon, Aug 17, 2020 at 07:07:24PM +0300, Andy Shevchenko wrote:
> > The code looks more nicer if we use:
> > 	while (i--)
> > instead:
> > 	for (i = i - 1; i >= 0; i--)
> > 
> > This would also allow making 'i' unsigned again.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: converted i to unsigned (Sakari)
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 92f5eadf2c99..cb74d49934f1 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -847,7 +847,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
> >  	unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> >  	struct sg_table *sg;
> >  	struct sg_dma_page_iter sg_iter;
> > -	int i, j;
> > +	unsigned int i, j;
> >  
> >  	if (lops <= 0 || lops > CIO2_MAX_LOPS) {
> >  		dev_err(dev, "%s: bad buffer size (%i)\n", __func__,
> > @@ -887,7 +887,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
> >  	b->lop[i][j] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
> >  	return 0;
> >  fail:
> > -	for (i--; i >= 0; i--)
> > +	while (i--)
> >  		dma_free_coherent(dev, CIO2_PAGE_SIZE,
> >  				  b->lop[i], b->lop_bus_addr[i]);
> 
> Looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> There's an additional issue though, if vb2_dma_sg_plane_desc() fails, we
> should free all the allocated memory.

These patches have been merged some time ago. Further changes should be on
top of this set.

-- 
Sakari Ailus