[PATCH v1 1/7] media: ipu3-cio2: Simplify cleanup code

[PATCH v1 1/7] 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--)

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

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 92f5eadf2c99..c7c3692a6e72 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -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 v1 2/7] media: ipu3-cio2: Introduce CIO2_MAX_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>
---
 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 c7c3692a6e72..79641c79df25 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_MAX_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_MAX_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_MAX_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_MAX_ENTRIES);
 	struct sg_table *sg;
 	struct sg_dma_page_iter sg_iter;
 	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_MAX_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..6ee5919d913b 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_MAX_ENTRIES			(PAGE_SIZE / sizeof(u32))
 
 #define CIO2_PAD_SINK					0
 #define CIO2_PAD_SOURCE					1
-- 
2.28.0

Re: [PATCH v1 2/7] media: ipu3-cio2: Introduce CIO2_MAX_ENTRIES constant

[Sakari Ailus]

Hi Andy,

On Fri, Aug 14, 2020 at 07:30:12PM +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>
> ---
>  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 c7c3692a6e72..79641c79df25 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_MAX_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_MAX_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_MAX_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_MAX_ENTRIES);
>  	struct sg_table *sg;
>  	struct sg_dma_page_iter sg_iter;
>  	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_MAX_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..6ee5919d913b 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_MAX_ENTRIES			(PAGE_SIZE / sizeof(u32))

How about calling this CIO2_ENTRIES_PER_LOP (or CIO2_LOP_ENTRIES) instead?

>  
>  #define CIO2_PAD_SINK					0
>  #define CIO2_PAD_SOURCE					1

-- 
Sakari Ailus

Re: [PATCH v1 2/7] media: ipu3-cio2: Introduce CIO2_MAX_ENTRIES constant

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 11:18:36PM +0300, Sakari Ailus wrote:
> On Fri, Aug 14, 2020 at 07:30:12PM +0300, Andy Shevchenko wrote:
> > This constant is used in several places in the code, define it
> > for better maintenance.

...

> > +#define CIO2_MAX_ENTRIES			(PAGE_SIZE / sizeof(u32))
> 
> How about calling this CIO2_ENTRIES_PER_LOP (or CIO2_LOP_ENTRIES) instead?

Sure. (I like the latter).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/7] media: ipu2-cio2: Replace custom definition with PAGE_SIZE

[Andy Shevchenko]

It's quite unlikely the other 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 generic PAGE_SIZE for sake of the consistency.

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

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 79641c79df25..2b3a865fa2da 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_MAX_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,25 @@ 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;
+				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_MAX_ENTRIES * CIO2_PAGE_SIZE;
+		remaining -= CIO2_MAX_ENTRIES * PAGE_SIZE;
 		entry++;
 		i++;
 	}
@@ -840,7 +838,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_MAX_ENTRIES);
 	struct sg_table *sg;
 	struct sg_dma_page_iter sg_iter;
@@ -855,7 +853,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 +883,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 +973,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 6ee5919d913b..48da6f7eb218 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

[PATCH v1 4/7] 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>
---
 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 2b3a865fa2da..eee7f841050d 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_MAX_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
@@ -197,7 +197,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_MAX_ENTRIES * PAGE_SIZE;
 		entry++;
 		i++;
@@ -206,7 +206,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);
 }
@@ -472,8 +472,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 |
@@ -838,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, PAGE_SIZE);
+	unsigned int pages = PFN_UP(vb->planes[0].length);
 	unsigned int lops = DIV_ROUND_UP(pages + 1, CIO2_MAX_ENTRIES);
 	struct sg_table *sg;
 	struct sg_dma_page_iter sg_iter;
@@ -871,7 +870,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_MAX_ENTRIES) {
 			i++;
@@ -879,7 +878,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 v1 5/7] 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>
---
 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 eee7f841050d..f4175dc1501a 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -541,7 +541,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 *const entry;
 	u64 ns = ktime_get_ns();
 
 	if (dma_chan >= CIO2_QUEUES) {
@@ -549,15 +549,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;
@@ -579,13 +582,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

Re: [PATCH v1 5/7] media: ipu3-cio2: Replace infinite loop by one with clear exit condition

[Bingbu Cao]

On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> 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>
> ---
>  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 eee7f841050d..f4175dc1501a 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -541,7 +541,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 *const entry;
No 'const' here.

>  	u64 ns = ktime_get_ns();
>  
>  	if (dma_chan >= CIO2_QUEUES) {
> @@ -549,15 +549,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;
> @@ -579,13 +582,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)
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v1 5/7] media: ipu3-cio2: Replace infinite loop by one with clear exit condition

[Andy Shevchenko]

On Mon, Aug 17, 2020 at 06:21:23PM +0800, Bingbu Cao wrote:
> On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > 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.

...

> > -	int buffers_found = 0;
> > +	struct cio2_fbpt_entry *const entry;
> No 'const' here.

Sure, will not be in v2.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 6/7] 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>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index f4175dc1501a..5f8ff91dbf09 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>
@@ -509,7 +510,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 *dma = base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN);
+	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,14 +522,9 @@ static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q)
 	writel(0, q->csi_rx_base + CIO2_REG_MIPIBE_ENABLE);
 
 	/* 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)
+	writel(0, dma);
+	ret = readl_poll_timeout(dma, 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 v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Bingbu Cao]

On 8/15/20 12:30 AM, 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>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index f4175dc1501a..5f8ff91dbf09 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>
> @@ -509,7 +510,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 *dma = base + CIO2_REG_CDMAC0(CIO2_DMA_CHAN);
> +	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,14 +522,9 @@ static void cio2_hw_exit(struct cio2_device *cio2, struct cio2_queue *q)
>  	writel(0, q->csi_rx_base + CIO2_REG_MIPIBE_ENABLE);
>  
>  	/* 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)
> +	writel(0, dma);
> +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);

This line is too long, need a break, others look good for me.

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

-- 
Best regards,
Bingbu Cao

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Andy Shevchenko]

On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > We may use special helper macro to poll IO till condition or timeout occurs.

> > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> 
> This line is too long, need a break, others look good for me.

checkpatch doesn't complain, but if you insist, I'll split it in v2.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Sakari Ailus]

On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> > On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > > We may use special helper macro to poll IO till condition or timeout occurs.
> 
> > > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> > 
> > This line is too long, need a break, others look good for me.
> 
> checkpatch doesn't complain, but if you insist, I'll split it in v2.

The coding style hasn't changed, it's just the checkpatch.pl warning that
has.

-- 
Sakari Ailus

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Andy Shevchenko]

On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote:
> On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> > > On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > > > We may use special helper macro to poll IO till condition or timeout occurs.
> > 
> > > > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> > > 
> > > This line is too long, need a break, others look good for me.
> > 
> > checkpatch doesn't complain, but if you insist, I'll split it in v2.
> 
> The coding style hasn't changed, it's just the checkpatch.pl warning that
> has.

Joe, it seems we have inconsistency now between checkpatch and coding style.
Shouldn't we revert 100 limit warning to 80?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Sakari Ailus]

On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote:
> > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > > > > We may use special helper macro to poll IO till condition or timeout occurs.
> > > 
> > > > > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> > > > 
> > > > This line is too long, need a break, others look good for me.
> > > 
> > > checkpatch doesn't complain, but if you insist, I'll split it in v2.
> > 
> > The coding style hasn't changed, it's just the checkpatch.pl warning that
> > has.
> 
> Joe, it seems we have inconsistency now between checkpatch and coding style.
> Shouldn't we revert 100 limit warning to 80?

There are sometimes genuine reasons for having longer lines than 80, and
depending on the code, that happens more often in some places than
elsewhere. This tended to generate lots of checkpatch.pl warnings in the
past.

While I didn't see the patch removing the 80 chars per line limit until it
made the news, I think it was a quite reasonable compromise.

-- 
Sakari Ailus

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Andy Shevchenko]

On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote:
> On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote:
> > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > > > > > We may use special helper macro to poll IO till condition or timeout occurs.
> > > > 
> > > > > > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> > > > > 
> > > > > This line is too long, need a break, others look good for me.
> > > > 
> > > > checkpatch doesn't complain, but if you insist, I'll split it in v2.
> > > 
> > > The coding style hasn't changed, it's just the checkpatch.pl warning that
> > > has.
> > 
> > Joe, it seems we have inconsistency now between checkpatch and coding style.
> > Shouldn't we revert 100 limit warning to 80?
> 
> There are sometimes genuine reasons for having longer lines than 80, and
> depending on the code, that happens more often in some places than
> elsewhere. This tended to generate lots of checkpatch.pl warnings in the
> past.
> 
> While I didn't see the patch removing the 80 chars per line limit until it
> made the news, I think it was a quite reasonable compromise.

But doesn't it make harder life for reviewers like you? You have to keep in
mind all these inconsistencies and rule either way.

That said, we either would fix the doc, or revert the checkpatch change.

Jonathan, what is your opinion?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Sakari Ailus]

On Mon, Aug 17, 2020 at 02:33:22PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote:
> > On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote:
> > > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> > > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > > > > > > We may use special helper macro to poll IO till condition or timeout occurs.
> > > > > 
> > > > > > > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> > > > > > 
> > > > > > This line is too long, need a break, others look good for me.
> > > > > 
> > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2.
> > > > 
> > > > The coding style hasn't changed, it's just the checkpatch.pl warning that
> > > > has.
> > > 
> > > Joe, it seems we have inconsistency now between checkpatch and coding style.
> > > Shouldn't we revert 100 limit warning to 80?
> > 
> > There are sometimes genuine reasons for having longer lines than 80, and
> > depending on the code, that happens more often in some places than
> > elsewhere. This tended to generate lots of checkpatch.pl warnings in the
> > past.
> > 
> > While I didn't see the patch removing the 80 chars per line limit until it
> > made the news, I think it was a quite reasonable compromise.
> 
> But doesn't it make harder life for reviewers like you? You have to keep in
> mind all these inconsistencies and rule either way.
> 
> That said, we either would fix the doc, or revert the checkpatch change.

I don't think the documentation should be changed. There *are* reasons for
the 80-column limit. Although the exact number can be connected to the
width of traditional text terminals, using very long lines makes reading
text harder. IOW, there's a connection to the same reason why wide
newspaper articles are split into several columns.

Would it be reasonable if checkpatch.pl warned about exceeding 80-column
limits if such lines are rare or nonexistent in the same file?

-- 
Sakari Ailus

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Andy Shevchenko]

On Mon, Aug 17, 2020 at 02:55:38PM +0300, Sakari Ailus wrote:
> On Mon, Aug 17, 2020 at 02:33:22PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote:
> > > On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote:
> > > > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote:
> > > > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> > > > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > > > > > > > We may use special helper macro to poll IO till condition or timeout occurs.
> > > > > > 
> > > > > > > > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> > > > > > > 
> > > > > > > This line is too long, need a break, others look good for me.
> > > > > > 
> > > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2.
> > > > > 
> > > > > The coding style hasn't changed, it's just the checkpatch.pl warning that
> > > > > has.
> > > > 
> > > > Joe, it seems we have inconsistency now between checkpatch and coding style.
> > > > Shouldn't we revert 100 limit warning to 80?
> > > 
> > > There are sometimes genuine reasons for having longer lines than 80, and
> > > depending on the code, that happens more often in some places than
> > > elsewhere. This tended to generate lots of checkpatch.pl warnings in the
> > > past.
> > > 
> > > While I didn't see the patch removing the 80 chars per line limit until it
> > > made the news, I think it was a quite reasonable compromise.
> > 
> > But doesn't it make harder life for reviewers like you? You have to keep in
> > mind all these inconsistencies and rule either way.
> > 
> > That said, we either would fix the doc, or revert the checkpatch change.
> 
> I don't think the documentation should be changed. There *are* reasons for
> the 80-column limit. Although the exact number can be connected to the
> width of traditional text terminals, using very long lines makes reading
> text harder. IOW, there's a connection to the same reason why wide
> newspaper articles are split into several columns.

> Would it be reasonable if checkpatch.pl warned about exceeding 80-column
> limits if such lines are rare or nonexistent in the same file?

I don't think so. If somebody decides to *gradually* move to 100 limit in the
driver, it will be painful with all these new old warnings.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/7] media: ipu3-cio2: Use readl_poll_timeout() helper

[Sakari Ailus]

On Mon, Aug 17, 2020 at 03:29:11PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 17, 2020 at 02:55:38PM +0300, Sakari Ailus wrote:
> > On Mon, Aug 17, 2020 at 02:33:22PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 17, 2020 at 02:27:47PM +0300, Sakari Ailus wrote:
> > > > On Mon, Aug 17, 2020 at 02:20:06PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Aug 17, 2020 at 02:13:16PM +0300, Sakari Ailus wrote:
> > > > > > On Mon, Aug 17, 2020 at 12:44:36PM +0300, Andy Shevchenko wrote:
> > > > > > > On Mon, Aug 17, 2020 at 05:27:33PM +0800, Bingbu Cao wrote:
> > > > > > > > On 8/15/20 12:30 AM, Andy Shevchenko wrote:
> > > > > > > > > We may use special helper macro to poll IO till condition or timeout occurs.
> > > > > > > 
> > > > > > > > > +	ret = readl_poll_timeout(dma, value, value & CIO2_CDMAC0_DMA_HALTED, 4000, 2000000);
> > > > > > > > 
> > > > > > > > This line is too long, need a break, others look good for me.
> > > > > > > 
> > > > > > > checkpatch doesn't complain, but if you insist, I'll split it in v2.
> > > > > > 
> > > > > > The coding style hasn't changed, it's just the checkpatch.pl warning that
> > > > > > has.
> > > > > 
> > > > > Joe, it seems we have inconsistency now between checkpatch and coding style.
> > > > > Shouldn't we revert 100 limit warning to 80?
> > > > 
> > > > There are sometimes genuine reasons for having longer lines than 80, and
> > > > depending on the code, that happens more often in some places than
> > > > elsewhere. This tended to generate lots of checkpatch.pl warnings in the
> > > > past.
> > > > 
> > > > While I didn't see the patch removing the 80 chars per line limit until it
> > > > made the news, I think it was a quite reasonable compromise.
> > > 
> > > But doesn't it make harder life for reviewers like you? You have to keep in
> > > mind all these inconsistencies and rule either way.
> > > 
> > > That said, we either would fix the doc, or revert the checkpatch change.
> > 
> > I don't think the documentation should be changed. There *are* reasons for
> > the 80-column limit. Although the exact number can be connected to the
> > width of traditional text terminals, using very long lines makes reading
> > text harder. IOW, there's a connection to the same reason why wide
> > newspaper articles are split into several columns.
> 
> > Would it be reasonable if checkpatch.pl warned about exceeding 80-column
> > limits if such lines are rare or nonexistent in the same file?
> 
> I don't think so. If somebody decides to *gradually* move to 100 limit in the
> driver, it will be painful with all these new old warnings.

There would be no more such warnings than with the 80-column limit.

I'd also question whether this is a realistic example. If a file was fine
without long lines in the past, it's unlikely a sudden and sustained need
for having them arises.

I think common reasons for those long lines are e.g. referring to definitions
from standards as-is or IOCTL command definitions. Or flags used on various
interfaces to keep them descriptive. All either exist in given files or do
not. That generally doesn't change over the lifetime of the said files.

-- 
Sakari Ailus

[PATCH v1 7/7] 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>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 5f8ff91dbf09..8925bab8ba06 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>
@@ -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 v1 1/7] media: ipu3-cio2: Simplify cleanup code

[Sakari Ailus]

Hi Andy,

Thanks for the patchset.

On Fri, Aug 14, 2020 at 07:30:11PM +0300, Andy Shevchenko wrote:
> The code looks more nicer if we use:
> 	while (i--)
> instead:
> 	for (i = i - 1; i >= 0; i--)
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 92f5eadf2c99..c7c3692a6e72 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -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--)

Nice! This would also allow making i unsigned again.

>  		dma_free_coherent(dev, CIO2_PAGE_SIZE,
>  				  b->lop[i], b->lop_bus_addr[i]);
>  	return -ENOMEM;

-- 
Sakari Ailus

Re: [PATCH v1 1/7] media: ipu3-cio2: Simplify cleanup code

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 11:16:17PM +0300, Sakari Ailus wrote:
> On Fri, Aug 14, 2020 at 07:30:11PM +0300, Andy Shevchenko wrote:
> > The code looks more nicer if we use:
> > 	while (i--)
> > instead:
> > 	for (i = i - 1; i >= 0; i--)

...

> > -	for (i--; i >= 0; i--)
> > +	while (i--)
> 
> Nice! This would also allow making i unsigned again.

Yes. I will add this to v2.

-- 
With Best Regards,
Andy Shevchenko