[PATCH 1/3] [media] tw68: make tw68_pci_tbl static and constify

[PATCH 1/3] [media] tw68: make tw68_pci_tbl static and constify

[Mauro Carvalho Chehab]

drivers/media/pci/tw68/tw68-core.c:72:22: warning: symbol 'tw68_pci_tbl' was not declared. Should it be static?

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

diff --git a/drivers/media/pci/tw68/tw68-core.c b/drivers/media/pci/tw68/tw68-core.c
index baf93af1d764..a6fb48cf7aae 100644
--- a/drivers/media/pci/tw68/tw68-core.c
+++ b/drivers/media/pci/tw68/tw68-core.c
@@ -69,7 +69,7 @@ static atomic_t tw68_instance = ATOMIC_INIT(0);
  * the PCI ID database up to date.  Note that the entries must be
  * added under vendor 0x1797 (Techwell Inc.) as subsystem IDs.
  */
-struct pci_device_id tw68_pci_tbl[] = {
+static const struct pci_device_id tw68_pci_tbl[] = {
 	{PCI_DEVICE(PCI_VENDOR_ID_TECHWELL, PCI_DEVICE_ID_6800)},
 	{PCI_DEVICE(PCI_VENDOR_ID_TECHWELL, PCI_DEVICE_ID_6801)},
 	{PCI_DEVICE(PCI_VENDOR_ID_TECHWELL, PCI_DEVICE_ID_6804)},
-- 
1.9.3

[PATCH 2/3] [media] tw68: Remove a sparse warning

[Mauro Carvalho Chehab]

drivers/media/pci/tw68/tw68-video.c:351:9: warning: incorrect type in argument 1 (different base types)
drivers/media/pci/tw68/tw68-video.c:351:9:    expected unsigned int [unsigned] val
drivers/media/pci/tw68/tw68-video.c:351:9:    got restricted __le32 [usertype] <noident>

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c
index 66fae2345fdd..4dd38578cf1b 100644
--- a/drivers/media/pci/tw68/tw68-video.c
+++ b/drivers/media/pci/tw68/tw68-video.c
@@ -348,7 +348,7 @@ int tw68_video_start_dma(struct tw68_dev *dev, struct tw68_buf *buf)
 	 *  a new address can be set.
 	 */
 	tw_clearl(TW68_DMAC, TW68_DMAP_EN);
-	tw_writel(TW68_DMAP_SA, cpu_to_le32(buf->dma));
+	tw_writel(TW68_DMAP_SA, (__force u32)cpu_to_le32(buf->dma));
 	/* Clear any pending interrupts */
 	tw_writel(TW68_INTSTAT, dev->board_virqmask);
 	/* Enable the risc engine and the fifo */
-- 
1.9.3

[PATCH 3/3] [media] tw68: don't assume that pagesize is always 4096

[Mauro Carvalho Chehab]

This code implicitly assumes that pagesize is 4096, but this is
not true on all archs, as the PAGE_SHIFT can be different than
12 on all those architectures: alpha, arc, arm64, cris, frv,
hexagon,ia64, m68k, metag, microblaze, mips, openrisc, parisc,
powerpc, sh, sparc and tile.

The real constrant here seems to be to limit the buffer size to
4MB.

So, fix the code to reflect that, in a way that it will keep
working with differnt values for PAGE_SIZE.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c
index 4dd38578cf1b..66658accdca9 100644
--- a/drivers/media/pci/tw68/tw68-video.c
+++ b/drivers/media/pci/tw68/tw68-video.c
@@ -366,7 +366,7 @@ static int tw68_buffer_pages(int size)
 {
 	size  = PAGE_ALIGN(size);
 	size += PAGE_SIZE; /* for non-page-aligned buffers */
-	size /= 4096;
+	size /= PAGE_SIZE;
 	return size;
 }
 
@@ -376,7 +376,7 @@ static int tw68_buffer_count(unsigned int size, unsigned int count)
 {
 	unsigned int maxcount;
 
-	maxcount = 1024 / tw68_buffer_pages(size);
+	maxcount = (4096 * 1024 / PAGE_SIZE) / tw68_buffer_pages(size);
 	if (count > maxcount)
 		count = maxcount;
 	return count;
-- 
1.9.3

Re: [PATCH 1/3] [media] tw68: make tw68_pci_tbl static and constify

[Hans Verkuil]

Acked-by: Hans Verkuil <hans.verkuil@xs4all.nl>

Thanks!

	Hans

On 09/04/14 16:46, Mauro Carvalho Chehab wrote:
> drivers/media/pci/tw68/tw68-core.c:72:22: warning: symbol 'tw68_pci_tbl' was not declared. Should it be static?
> 
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> diff --git a/drivers/media/pci/tw68/tw68-core.c b/drivers/media/pci/tw68/tw68-core.c
> index baf93af1d764..a6fb48cf7aae 100644
> --- a/drivers/media/pci/tw68/tw68-core.c
> +++ b/drivers/media/pci/tw68/tw68-core.c
> @@ -69,7 +69,7 @@ static atomic_t tw68_instance = ATOMIC_INIT(0);
>   * the PCI ID database up to date.  Note that the entries must be
>   * added under vendor 0x1797 (Techwell Inc.) as subsystem IDs.
>   */
> -struct pci_device_id tw68_pci_tbl[] = {
> +static const struct pci_device_id tw68_pci_tbl[] = {
>  	{PCI_DEVICE(PCI_VENDOR_ID_TECHWELL, PCI_DEVICE_ID_6800)},
>  	{PCI_DEVICE(PCI_VENDOR_ID_TECHWELL, PCI_DEVICE_ID_6801)},
>  	{PCI_DEVICE(PCI_VENDOR_ID_TECHWELL, PCI_DEVICE_ID_6804)},
> 

Re: [PATCH 3/3] [media] tw68: don't assume that pagesize is always 4096

[Hans Verkuil]

I need to review this more closely tomorrow.

	Hans

On 09/04/14 16:46, Mauro Carvalho Chehab wrote:
> This code implicitly assumes that pagesize is 4096, but this is
> not true on all archs, as the PAGE_SHIFT can be different than
> 12 on all those architectures: alpha, arc, arm64, cris, frv,
> hexagon,ia64, m68k, metag, microblaze, mips, openrisc, parisc,
> powerpc, sh, sparc and tile.
> 
> The real constrant here seems to be to limit the buffer size to
> 4MB.
> 
> So, fix the code to reflect that, in a way that it will keep
> working with differnt values for PAGE_SIZE.
> 
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c
> index 4dd38578cf1b..66658accdca9 100644
> --- a/drivers/media/pci/tw68/tw68-video.c
> +++ b/drivers/media/pci/tw68/tw68-video.c
> @@ -366,7 +366,7 @@ static int tw68_buffer_pages(int size)
>  {
>  	size  = PAGE_ALIGN(size);
>  	size += PAGE_SIZE; /* for non-page-aligned buffers */
> -	size /= 4096;
> +	size /= PAGE_SIZE;
>  	return size;
>  }
>  
> @@ -376,7 +376,7 @@ static int tw68_buffer_count(unsigned int size, unsigned int count)
>  {
>  	unsigned int maxcount;
>  
> -	maxcount = 1024 / tw68_buffer_pages(size);
> +	maxcount = (4096 * 1024 / PAGE_SIZE) / tw68_buffer_pages(size);
>  	if (count > maxcount)
>  		count = maxcount;
>  	return count;
> 

Re: [PATCH 2/3] [media] tw68: Remove a sparse warning

[Hans Verkuil]

I'll need to review this as well. Perhaps tw_writel should expect a __le32?

	Hans

On 09/04/14 16:46, Mauro Carvalho Chehab wrote:
> drivers/media/pci/tw68/tw68-video.c:351:9: warning: incorrect type in argument 1 (different base types)
> drivers/media/pci/tw68/tw68-video.c:351:9:    expected unsigned int [unsigned] val
> drivers/media/pci/tw68/tw68-video.c:351:9:    got restricted __le32 [usertype] <noident>
> 
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c
> index 66fae2345fdd..4dd38578cf1b 100644
> --- a/drivers/media/pci/tw68/tw68-video.c
> +++ b/drivers/media/pci/tw68/tw68-video.c
> @@ -348,7 +348,7 @@ int tw68_video_start_dma(struct tw68_dev *dev, struct tw68_buf *buf)
>  	 *  a new address can be set.
>  	 */
>  	tw_clearl(TW68_DMAC, TW68_DMAP_EN);
> -	tw_writel(TW68_DMAP_SA, cpu_to_le32(buf->dma));
> +	tw_writel(TW68_DMAP_SA, (__force u32)cpu_to_le32(buf->dma));
>  	/* Clear any pending interrupts */
>  	tw_writel(TW68_INTSTAT, dev->board_virqmask);
>  	/* Enable the risc engine and the fifo */
> 

Re: [PATCH 2/3] [media] tw68: Remove a sparse warning

[Mauro Carvalho Chehab]

Em Thu, 04 Sep 2014 16:54:38 +0200
Hans Verkuil <hansverk@cisco.com> escreveu:

> I'll need to review this as well. Perhaps tw_writel should expect a __le32?

There are several other parts where it is using the address as CPU endian.

Anyway, if you have some BE system, then you can test the board there and
see what would work.

Regards,
Mauro

> 
> 	Hans
> 
> On 09/04/14 16:46, Mauro Carvalho Chehab wrote:
> > drivers/media/pci/tw68/tw68-video.c:351:9: warning: incorrect type in argument 1 (different base types)
> > drivers/media/pci/tw68/tw68-video.c:351:9:    expected unsigned int [unsigned] val
> > drivers/media/pci/tw68/tw68-video.c:351:9:    got restricted __le32 [usertype] <noident>
> > 
> > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > 
> > diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c
> > index 66fae2345fdd..4dd38578cf1b 100644
> > --- a/drivers/media/pci/tw68/tw68-video.c
> > +++ b/drivers/media/pci/tw68/tw68-video.c
> > @@ -348,7 +348,7 @@ int tw68_video_start_dma(struct tw68_dev *dev, struct tw68_buf *buf)
> >  	 *  a new address can be set.
> >  	 */
> >  	tw_clearl(TW68_DMAC, TW68_DMAP_EN);
> > -	tw_writel(TW68_DMAP_SA, cpu_to_le32(buf->dma));
> > +	tw_writel(TW68_DMAP_SA, (__force u32)cpu_to_le32(buf->dma));
> >  	/* Clear any pending interrupts */
> >  	tw_writel(TW68_INTSTAT, dev->board_virqmask);
> >  	/* Enable the risc engine and the fifo */
> > 

Re: [PATCH 2/3] [media] tw68: Remove a sparse warning

[Hans Verkuil]

On 09/04/2014 04:46 PM, Mauro Carvalho Chehab wrote:
> drivers/media/pci/tw68/tw68-video.c:351:9: warning: incorrect type in argument 1 (different base types)
> drivers/media/pci/tw68/tw68-video.c:351:9:    expected unsigned int [unsigned] val
> drivers/media/pci/tw68/tw68-video.c:351:9:    got restricted __le32 [usertype] <noident>
> 
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>

Nacked-by: Hans Verkuil <hverkuil@xs4all.nl>

tw_writel maps to writel which already does cpu_to_le32(), so doing it again is once
too many. I'll post a patch that removes the bogus cpu_to_le32().

	Hans

> 
> diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c
> index 66fae2345fdd..4dd38578cf1b 100644
> --- a/drivers/media/pci/tw68/tw68-video.c
> +++ b/drivers/media/pci/tw68/tw68-video.c
> @@ -348,7 +348,7 @@ int tw68_video_start_dma(struct tw68_dev *dev, struct tw68_buf *buf)
>  	 *  a new address can be set.
>  	 */
>  	tw_clearl(TW68_DMAC, TW68_DMAP_EN);
> -	tw_writel(TW68_DMAP_SA, cpu_to_le32(buf->dma));
> +	tw_writel(TW68_DMAP_SA, (__force u32)cpu_to_le32(buf->dma));
>  	/* Clear any pending interrupts */
>  	tw_writel(TW68_INTSTAT, dev->board_virqmask);
>  	/* Enable the risc engine and the fifo */
> 

Re: [PATCH 3/3] [media] tw68: don't assume that pagesize is always 4096

[Hans Verkuil]

On 09/04/2014 04:46 PM, Mauro Carvalho Chehab wrote:
> This code implicitly assumes that pagesize is 4096, but this is
> not true on all archs, as the PAGE_SHIFT can be different than
> 12 on all those architectures: alpha, arc, arm64, cris, frv,
> hexagon,ia64, m68k, metag, microblaze, mips, openrisc, parisc,
> powerpc, sh, sparc and tile.
> 
> The real constrant here seems to be to limit the buffer size to
> 4MB.
> 
> So, fix the code to reflect that, in a way that it will keep
> working with differnt values for PAGE_SIZE.

While your fix is OK, I have a better patch that makes the code a lot more
readable. I'll post that soon.

Regards,

	Hans

> 
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> 
> diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c
> index 4dd38578cf1b..66658accdca9 100644
> --- a/drivers/media/pci/tw68/tw68-video.c
> +++ b/drivers/media/pci/tw68/tw68-video.c
> @@ -366,7 +366,7 @@ static int tw68_buffer_pages(int size)
>  {
>  	size  = PAGE_ALIGN(size);
>  	size += PAGE_SIZE; /* for non-page-aligned buffers */
> -	size /= 4096;
> +	size /= PAGE_SIZE;
>  	return size;
>  }
>  
> @@ -376,7 +376,7 @@ static int tw68_buffer_count(unsigned int size, unsigned int count)
>  {
>  	unsigned int maxcount;
>  
> -	maxcount = 1024 / tw68_buffer_pages(size);
> +	maxcount = (4096 * 1024 / PAGE_SIZE) / tw68_buffer_pages(size);
>  	if (count > maxcount)
>  		count = maxcount;
>  	return count;
>