Re: [PATCH] BLACKFIN MEDIA DRIVER: rewrite the blackfin style of read/write into common style

2015-01-20 Thread Steven Miao 
Hi Hans,

On Mon, Jan 19, 2015 at 5:45 PM, Hans Verkuil hverk...@xs4all.nl wrote:
 On 01/19/2015 04:13 AM, Hao Liang wrote:
 Hi Hans,

 Thank you for your reply.
 This change comes largely from a non-blackfin architecture dsp processor of 
 ADI want to reuse this driver.
 And i have tested common read/write function on blackfin board to ensure 
 usability and stability.

 Well, looking at arch/blackfin/include/asm/def_LPBlackfin.h it seems that for 
 certain
 blackfin variants the bfin_read/write functions insert an extra nop, so 
 unless you
 test on one of those variants you wouldn't see any problems relating to this 
 change
 (and possibly not even then if this is a rare race condition).

 You will have to check this with the blackfin architecture maintainer, Steven 
 Miao
 (added to the CC list). Without the OK of someone who actually understands 
 that
 architecture and the 'ANOMALY_05000198' issues I am not going to merge this.
the ANOMALY_05000198 only exists in blackfin bf533 and bf561 old
silicon revision, and this ppi driver mostly will be used on bf609 and
later platform.

 If the bfin_read/write functions are really needed for the blackfin 
 architecture,
we will sync the __raw_read/write macros with bfin_read/write, and
test it on most of the blackfin bf5xx and bf6xx platform.
 then you can always make ppi_read/write functions that check the architecture 
 and use
 bfin_read/write if it is a blackfin and readw/writew otherwise.

 That should be safe.

 Regards,

 Hans


 BR
 Hao

 2015-01-16 19:34 GMT+08:00 Hans Verkuil hverk...@xs4all.nl 
 mailto:hverk...@xs4all.nl:

 Hi Hao,

 Why would you do this? read/writew() is AFAICT not the same as 
 bfin_read/write16
 (defined in arch/blackfin/include/asm/def_LPBlackfin.h). And all other 
 blackfin
 sources I've seen all use bfin_read/write.

 So unless there is a good reason for this change I am not going to 
 accept this.

 Regards,

 Hans

 On 01/14/2015 07:57 AM, Hao Liang wrote:
  Signed-off-by: Hao Liang hliang1...@gmail.com 
 mailto:hliang1...@gmail.com
  ---
   drivers/media/platform/blackfin/ppi.c |   72 
 -
   1 file changed, 35 insertions(+), 37 deletions(-)
 
  diff --git a/drivers/media/platform/blackfin/ppi.c 
 b/drivers/media/platform/blackfin/ppi.c
  index cff63e5..de4b5c7 100644
  --- a/drivers/media/platform/blackfin/ppi.c
  +++ b/drivers/media/platform/blackfin/ppi.c
  @@ -20,6 +20,7 @@
   #include linux/module.h
   #include linux/slab.h
   #include linux/platform_device.h
  +#include linux/io.h
 
   #include asm/bfin_ppi.h
   #include asm/blackfin.h
  @@ -59,10 +60,10 @@ static irqreturn_t ppi_irq_err(int irq, void 
 *dev_id)
/* register on bf561 is cleared when read
 * others are W1C
 */
  - status = bfin_read16(reg-status);
  + status = readw(reg-status);
if (status  0x3000)
ppi-err = true;
  - bfin_write16(reg-status, 0xff00);
  + writew(0xff00, reg-status);
break;
}
case PPI_TYPE_EPPI:
  @@ -70,10 +71,10 @@ static irqreturn_t ppi_irq_err(int irq, void 
 *dev_id)
struct bfin_eppi_regs *reg = info-base;
unsigned short status;
 
  - status = bfin_read16(reg-status);
  + status = readw(reg-status);
if (status  0x2)
ppi-err = true;
  - bfin_write16(reg-status, 0x);
  + writew(0x, reg-status);
break;
}
case PPI_TYPE_EPPI3:
  @@ -81,10 +82,10 @@ static irqreturn_t ppi_irq_err(int irq, void 
 *dev_id)
struct bfin_eppi3_regs *reg = info-base;
unsigned long stat;
 
  - stat = bfin_read32(reg-stat);
  + stat = readl(reg-stat);
if (stat  0x2)
ppi-err = true;
  - bfin_write32(reg-stat, 0xc0ff);
  + writel(0xc0ff, reg-stat);
break;
}
default:
  @@ -139,26 +140,25 @@ static int ppi_start(struct ppi_if *ppi)
case PPI_TYPE_PPI:
{
struct bfin_ppi_regs *reg = info-base;
  - bfin_write16(reg-control, ppi-ppi_control);
  + writew(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI:
{
struct bfin_eppi_regs *reg = info-base;
  - bfin_write32(reg-control, ppi-ppi_control);
  + writel(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI3:

Re: [PATCH] BLACKFIN MEDIA DRIVER: rewrite the blackfin style of read/write into common style

2015-01-19 Thread Hans Verkuil 
On 01/19/2015 04:13 AM, Hao Liang wrote:
 Hi Hans,
 
 Thank you for your reply.
 This change comes largely from a non-blackfin architecture dsp processor of 
 ADI want to reuse this driver.
 And i have tested common read/write function on blackfin board to ensure 
 usability and stability.

Well, looking at arch/blackfin/include/asm/def_LPBlackfin.h it seems that for 
certain
blackfin variants the bfin_read/write functions insert an extra nop, so unless 
you
test on one of those variants you wouldn't see any problems relating to this 
change
(and possibly not even then if this is a rare race condition).

You will have to check this with the blackfin architecture maintainer, Steven 
Miao
(added to the CC list). Without the OK of someone who actually understands that
architecture and the 'ANOMALY_05000198' issues I am not going to merge this.

If the bfin_read/write functions are really needed for the blackfin 
architecture,
then you can always make ppi_read/write functions that check the architecture 
and use
bfin_read/write if it is a blackfin and readw/writew otherwise.

That should be safe.

Regards,

Hans

 
 BR
 Hao
 
 2015-01-16 19:34 GMT+08:00 Hans Verkuil hverk...@xs4all.nl 
 mailto:hverk...@xs4all.nl:
 
 Hi Hao,
 
 Why would you do this? read/writew() is AFAICT not the same as 
 bfin_read/write16
 (defined in arch/blackfin/include/asm/def_LPBlackfin.h). And all other 
 blackfin
 sources I've seen all use bfin_read/write.
 
 So unless there is a good reason for this change I am not going to accept 
 this.
 
 Regards,
 
 Hans
 
 On 01/14/2015 07:57 AM, Hao Liang wrote:
  Signed-off-by: Hao Liang hliang1...@gmail.com 
 mailto:hliang1...@gmail.com
  ---
   drivers/media/platform/blackfin/ppi.c |   72 
 -
   1 file changed, 35 insertions(+), 37 deletions(-)
 
  diff --git a/drivers/media/platform/blackfin/ppi.c 
 b/drivers/media/platform/blackfin/ppi.c
  index cff63e5..de4b5c7 100644
  --- a/drivers/media/platform/blackfin/ppi.c
  +++ b/drivers/media/platform/blackfin/ppi.c
  @@ -20,6 +20,7 @@
   #include linux/module.h
   #include linux/slab.h
   #include linux/platform_device.h
  +#include linux/io.h
 
   #include asm/bfin_ppi.h
   #include asm/blackfin.h
  @@ -59,10 +60,10 @@ static irqreturn_t ppi_irq_err(int irq, void 
 *dev_id)
/* register on bf561 is cleared when read
 * others are W1C
 */
  - status = bfin_read16(reg-status);
  + status = readw(reg-status);
if (status  0x3000)
ppi-err = true;
  - bfin_write16(reg-status, 0xff00);
  + writew(0xff00, reg-status);
break;
}
case PPI_TYPE_EPPI:
  @@ -70,10 +71,10 @@ static irqreturn_t ppi_irq_err(int irq, void 
 *dev_id)
struct bfin_eppi_regs *reg = info-base;
unsigned short status;
 
  - status = bfin_read16(reg-status);
  + status = readw(reg-status);
if (status  0x2)
ppi-err = true;
  - bfin_write16(reg-status, 0x);
  + writew(0x, reg-status);
break;
}
case PPI_TYPE_EPPI3:
  @@ -81,10 +82,10 @@ static irqreturn_t ppi_irq_err(int irq, void 
 *dev_id)
struct bfin_eppi3_regs *reg = info-base;
unsigned long stat;
 
  - stat = bfin_read32(reg-stat);
  + stat = readl(reg-stat);
if (stat  0x2)
ppi-err = true;
  - bfin_write32(reg-stat, 0xc0ff);
  + writel(0xc0ff, reg-stat);
break;
}
default:
  @@ -139,26 +140,25 @@ static int ppi_start(struct ppi_if *ppi)
case PPI_TYPE_PPI:
{
struct bfin_ppi_regs *reg = info-base;
  - bfin_write16(reg-control, ppi-ppi_control);
  + writew(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI:
{
struct bfin_eppi_regs *reg = info-base;
  - bfin_write32(reg-control, ppi-ppi_control);
  + writel(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI3:
{
struct bfin_eppi3_regs *reg = info-base;
  - bfin_write32(reg-ctl, ppi-ppi_control);
  + writel(ppi-ppi_control, reg-ctl);
break;
}
default:
return -EINVAL;
}
 
  - SSYNC();
return 0;
   }
 
  @@

Re: [PATCH] BLACKFIN MEDIA DRIVER: rewrite the blackfin style of read/write into common style

2015-01-16 Thread Hans Verkuil 
Hi Hao,

Why would you do this? read/writew() is AFAICT not the same as bfin_read/write16
(defined in arch/blackfin/include/asm/def_LPBlackfin.h). And all other blackfin
sources I've seen all use bfin_read/write.

So unless there is a good reason for this change I am not going to accept this.

Regards,

Hans

On 01/14/2015 07:57 AM, Hao Liang wrote:
 Signed-off-by: Hao Liang hliang1...@gmail.com
 ---
  drivers/media/platform/blackfin/ppi.c |   72 
 -
  1 file changed, 35 insertions(+), 37 deletions(-)
 
 diff --git a/drivers/media/platform/blackfin/ppi.c 
 b/drivers/media/platform/blackfin/ppi.c
 index cff63e5..de4b5c7 100644
 --- a/drivers/media/platform/blackfin/ppi.c
 +++ b/drivers/media/platform/blackfin/ppi.c
 @@ -20,6 +20,7 @@
  #include linux/module.h
  #include linux/slab.h
  #include linux/platform_device.h
 +#include linux/io.h
  
  #include asm/bfin_ppi.h
  #include asm/blackfin.h
 @@ -59,10 +60,10 @@ static irqreturn_t ppi_irq_err(int irq, void *dev_id)
   /* register on bf561 is cleared when read 
* others are W1C
*/
 - status = bfin_read16(reg-status);
 + status = readw(reg-status);
   if (status  0x3000)
   ppi-err = true;
 - bfin_write16(reg-status, 0xff00);
 + writew(0xff00, reg-status);
   break;
   }
   case PPI_TYPE_EPPI:
 @@ -70,10 +71,10 @@ static irqreturn_t ppi_irq_err(int irq, void *dev_id)
   struct bfin_eppi_regs *reg = info-base;
   unsigned short status;
  
 - status = bfin_read16(reg-status);
 + status = readw(reg-status);
   if (status  0x2)
   ppi-err = true;
 - bfin_write16(reg-status, 0x);
 + writew(0x, reg-status);
   break;
   }
   case PPI_TYPE_EPPI3:
 @@ -81,10 +82,10 @@ static irqreturn_t ppi_irq_err(int irq, void *dev_id)
   struct bfin_eppi3_regs *reg = info-base;
   unsigned long stat;
  
 - stat = bfin_read32(reg-stat);
 + stat = readl(reg-stat);
   if (stat  0x2)
   ppi-err = true;
 - bfin_write32(reg-stat, 0xc0ff);
 + writel(0xc0ff, reg-stat);
   break;
   }
   default:
 @@ -139,26 +140,25 @@ static int ppi_start(struct ppi_if *ppi)
   case PPI_TYPE_PPI:
   {
   struct bfin_ppi_regs *reg = info-base;
 - bfin_write16(reg-control, ppi-ppi_control);
 + writew(ppi-ppi_control, reg-control);
   break;
   }
   case PPI_TYPE_EPPI:
   {
   struct bfin_eppi_regs *reg = info-base;
 - bfin_write32(reg-control, ppi-ppi_control);
 + writel(ppi-ppi_control, reg-control);
   break;
   }
   case PPI_TYPE_EPPI3:
   {
   struct bfin_eppi3_regs *reg = info-base;
 - bfin_write32(reg-ctl, ppi-ppi_control);
 + writel(ppi-ppi_control, reg-ctl);
   break;
   }
   default:
   return -EINVAL;
   }
  
 - SSYNC();
   return 0;
  }
  
 @@ -172,19 +172,19 @@ static int ppi_stop(struct ppi_if *ppi)
   case PPI_TYPE_PPI:
   {
   struct bfin_ppi_regs *reg = info-base;
 - bfin_write16(reg-control, ppi-ppi_control);
 + writew(ppi-ppi_control, reg-control);
   break;
   }
   case PPI_TYPE_EPPI:
   {
   struct bfin_eppi_regs *reg = info-base;
 - bfin_write32(reg-control, ppi-ppi_control);
 + writel(ppi-ppi_control, reg-control);
   break;
   }
   case PPI_TYPE_EPPI3:
   {
   struct bfin_eppi3_regs *reg = info-base;
 - bfin_write32(reg-ctl, ppi-ppi_control);
 + writel(ppi-ppi_control, reg-ctl);
   break;
   }
   default:
 @@ -195,7 +195,6 @@ static int ppi_stop(struct ppi_if *ppi)
   clear_dma_irqstat(info-dma_ch);
   disable_dma(info-dma_ch);
  
 - SSYNC();
   return 0;
  }
  
 @@ -242,9 +241,9 @@ static int ppi_set_params(struct ppi_if *ppi, struct 
 ppi_params *params)
   if (params-ppi_control  DMA32)
   dma32 = 1;
  
 - bfin_write16(reg-control, ppi-ppi_control);
 - bfin_write16(reg-count, samples_per_line - 1);
 - bfin_write16(reg-frame, params-frame);
 + writew(ppi-ppi_control, reg-control);
 + writew(samples_per_line - 1, reg-count);
 + writew(params-frame, reg-frame);
   break;
   }
   case PPI_TYPE_EPPI:
 @@ -255,13 +254,13 @@ static int ppi_set_params(struct ppi_if *ppi, struct 
 ppi_params *params)
   || (params-ppi_control  0x38000)  DLEN_16)
   dma32 = 1;
  
 -

[PATCH] BLACKFIN MEDIA DRIVER: rewrite the blackfin style of read/write into common style

2015-01-13 Thread Hao Liang 
Signed-off-by: Hao Liang hliang1...@gmail.com
---
 drivers/media/platform/blackfin/ppi.c |   72 -
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/media/platform/blackfin/ppi.c 
b/drivers/media/platform/blackfin/ppi.c
index cff63e5..de4b5c7 100644
--- a/drivers/media/platform/blackfin/ppi.c
+++ b/drivers/media/platform/blackfin/ppi.c
@@ -20,6 +20,7 @@
 #include linux/module.h
 #include linux/slab.h
 #include linux/platform_device.h
+#include linux/io.h
 
 #include asm/bfin_ppi.h
 #include asm/blackfin.h
@@ -59,10 +60,10 @@ static irqreturn_t ppi_irq_err(int irq, void *dev_id)
/* register on bf561 is cleared when read 
 * others are W1C
 */
-   status = bfin_read16(reg-status);
+   status = readw(reg-status);
if (status  0x3000)
ppi-err = true;
-   bfin_write16(reg-status, 0xff00);
+   writew(0xff00, reg-status);
break;
}
case PPI_TYPE_EPPI:
@@ -70,10 +71,10 @@ static irqreturn_t ppi_irq_err(int irq, void *dev_id)
struct bfin_eppi_regs *reg = info-base;
unsigned short status;
 
-   status = bfin_read16(reg-status);
+   status = readw(reg-status);
if (status  0x2)
ppi-err = true;
-   bfin_write16(reg-status, 0x);
+   writew(0x, reg-status);
break;
}
case PPI_TYPE_EPPI3:
@@ -81,10 +82,10 @@ static irqreturn_t ppi_irq_err(int irq, void *dev_id)
struct bfin_eppi3_regs *reg = info-base;
unsigned long stat;
 
-   stat = bfin_read32(reg-stat);
+   stat = readl(reg-stat);
if (stat  0x2)
ppi-err = true;
-   bfin_write32(reg-stat, 0xc0ff);
+   writel(0xc0ff, reg-stat);
break;
}
default:
@@ -139,26 +140,25 @@ static int ppi_start(struct ppi_if *ppi)
case PPI_TYPE_PPI:
{
struct bfin_ppi_regs *reg = info-base;
-   bfin_write16(reg-control, ppi-ppi_control);
+   writew(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI:
{
struct bfin_eppi_regs *reg = info-base;
-   bfin_write32(reg-control, ppi-ppi_control);
+   writel(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI3:
{
struct bfin_eppi3_regs *reg = info-base;
-   bfin_write32(reg-ctl, ppi-ppi_control);
+   writel(ppi-ppi_control, reg-ctl);
break;
}
default:
return -EINVAL;
}
 
-   SSYNC();
return 0;
 }
 
@@ -172,19 +172,19 @@ static int ppi_stop(struct ppi_if *ppi)
case PPI_TYPE_PPI:
{
struct bfin_ppi_regs *reg = info-base;
-   bfin_write16(reg-control, ppi-ppi_control);
+   writew(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI:
{
struct bfin_eppi_regs *reg = info-base;
-   bfin_write32(reg-control, ppi-ppi_control);
+   writel(ppi-ppi_control, reg-control);
break;
}
case PPI_TYPE_EPPI3:
{
struct bfin_eppi3_regs *reg = info-base;
-   bfin_write32(reg-ctl, ppi-ppi_control);
+   writel(ppi-ppi_control, reg-ctl);
break;
}
default:
@@ -195,7 +195,6 @@ static int ppi_stop(struct ppi_if *ppi)
clear_dma_irqstat(info-dma_ch);
disable_dma(info-dma_ch);
 
-   SSYNC();
return 0;
 }
 
@@ -242,9 +241,9 @@ static int ppi_set_params(struct ppi_if *ppi, struct 
ppi_params *params)
if (params-ppi_control  DMA32)
dma32 = 1;
 
-   bfin_write16(reg-control, ppi-ppi_control);
-   bfin_write16(reg-count, samples_per_line - 1);
-   bfin_write16(reg-frame, params-frame);
+   writew(ppi-ppi_control, reg-control);
+   writew(samples_per_line - 1, reg-count);
+   writew(params-frame, reg-frame);
break;
}
case PPI_TYPE_EPPI:
@@ -255,13 +254,13 @@ static int ppi_set_params(struct ppi_if *ppi, struct 
ppi_params *params)
|| (params-ppi_control  0x38000)  DLEN_16)
dma32 = 1;
 
-   bfin_write32(reg-control, ppi-ppi_control);
-   bfin_write16(reg-line, samples_per_line);
-   bfin_write16(reg-frame, params-frame);
-   bfin_write16(reg-hdelay, hdelay);
-   bfin_write16(reg-vdelay, params-vdelay);
-   bfin_write16(reg-hcount, hcount);
-