Re: [PATCH] Add tw5864 driver

[Leon Romanovsky]

On Mon, Mar 14, 2016 at 03:55:14AM +0200, Andrey Utkin wrote:
> From: Andrey Utkin 
> 
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).
> 
> Signed-off-by: Andrey Utkin 
> Tested-by: Andrey Utkin 
> ---
>  MAINTAINERS  |7 +
>  drivers/staging/media/Kconfig|2 +
>  drivers/staging/media/Makefile   |1 +
>  drivers/staging/media/tw5864/Kconfig |   11 +
>  drivers/staging/media/tw5864/Makefile|3 +
>  drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
>  drivers/staging/media/tw5864/tw5864-config.c |  359 +
>  drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
>  drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
>  drivers/staging/media/tw5864/tw5864-reg.h| 2200 
> ++
>  drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
>  drivers/staging/media/tw5864/tw5864-video.c  | 1364 
>  drivers/staging/media/tw5864/tw5864.h|  280 
>  include/linux/pci_ids.h  |1 +
>  14 files changed, 5255 insertions(+)
>  create mode 100644 drivers/staging/media/tw5864/Kconfig
>  create mode 100644 drivers/staging/media/tw5864/Makefile
>  create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 409509d..7bb1fa9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11195,6 +11195,13 @@ T:   git git://linuxtv.org/media_tree.git
>  S:   Odd fixes
>  F:   drivers/media/usb/tm6000/
>  
> +TW5864 VIDEO4LINUX DRIVER
> +M:   Bluecherry Maintainers 

I wonder if this the right thing to do. Generally speaking a maintainer is a
person and not a corporate.

> +M:   Andrey Utkin 
> +L:   linux-me...@vger.kernel.org
> +S:   Supported
> +F:   drivers/staging/media/tw5864/



> +
> --- /dev/null
> +++ b/drivers/staging/media/tw5864/tw5864-bs.h
> @@ -0,0 +1,154 @@
> +/*
> + *  TW5864 driver - Exp-Golomb code functions
> + *
> + *  Copyright (C) 2015 Bluecherry, LLC 
> + *  Author: Andrey Utkin 

You don't need to state your name here. It is written in git log.

Re: [PATCH] Add tw5864 driver

[Leon Romanovsky]

On Mon, Mar 14, 2016 at 03:55:14AM +0200, Andrey Utkin wrote:
> From: Andrey Utkin 
> 
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).
> 
> Signed-off-by: Andrey Utkin 
> Tested-by: Andrey Utkin 
> ---
>  MAINTAINERS  |7 +
>  drivers/staging/media/Kconfig|2 +
>  drivers/staging/media/Makefile   |1 +
>  drivers/staging/media/tw5864/Kconfig |   11 +
>  drivers/staging/media/tw5864/Makefile|3 +
>  drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
>  drivers/staging/media/tw5864/tw5864-config.c |  359 +
>  drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
>  drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
>  drivers/staging/media/tw5864/tw5864-reg.h| 2200 
> ++
>  drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
>  drivers/staging/media/tw5864/tw5864-video.c  | 1364 
>  drivers/staging/media/tw5864/tw5864.h|  280 
>  include/linux/pci_ids.h  |1 +
>  14 files changed, 5255 insertions(+)
>  create mode 100644 drivers/staging/media/tw5864/Kconfig
>  create mode 100644 drivers/staging/media/tw5864/Makefile
>  create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 409509d..7bb1fa9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11195,6 +11195,13 @@ T:   git git://linuxtv.org/media_tree.git
>  S:   Odd fixes
>  F:   drivers/media/usb/tm6000/
>  
> +TW5864 VIDEO4LINUX DRIVER
> +M:   Bluecherry Maintainers 

I wonder if this the right thing to do. Generally speaking a maintainer is a
person and not a corporate.

> +M:   Andrey Utkin 
> +L:   linux-me...@vger.kernel.org
> +S:   Supported
> +F:   drivers/staging/media/tw5864/



> +
> --- /dev/null
> +++ b/drivers/staging/media/tw5864/tw5864-bs.h
> @@ -0,0 +1,154 @@
> +/*
> + *  TW5864 driver - Exp-Golomb code functions
> + *
> + *  Copyright (C) 2015 Bluecherry, LLC 
> + *  Author: Andrey Utkin 

You don't need to state your name here. It is written in git log.

Re: [PATCH] Add tw5864 driver

[Hans Verkuil]

Hi Andrey,

See below for a quick review of the code.

I agree with Greg's comment why this is added to staging instead of 
drivers/media/pci?

When you post the v2 patch, can you add the output of 'v4l2-compliance -s' to 
the
cover letter? Please use the latest v4l2-compliance version from the 
v4l-utils.git
repository when testing.

On 03/14/2016 02:59 AM, Andrey Utkin wrote:
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).
> 
> Signed-off-by: Andrey Utkin 
> Tested-by: Andrey Utkin 
> ---
>  MAINTAINERS  |7 +
>  drivers/staging/media/Kconfig|2 +
>  drivers/staging/media/Makefile   |1 +
>  drivers/staging/media/tw5864/Kconfig |   11 +
>  drivers/staging/media/tw5864/Makefile|3 +
>  drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
>  drivers/staging/media/tw5864/tw5864-config.c |  359 +
>  drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
>  drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
>  drivers/staging/media/tw5864/tw5864-reg.h| 2200 
> ++
>  drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
>  drivers/staging/media/tw5864/tw5864-video.c  | 1364 
>  drivers/staging/media/tw5864/tw5864.h|  280 
>  include/linux/pci_ids.h  |1 +
>  14 files changed, 5255 insertions(+)
>  create mode 100644 drivers/staging/media/tw5864/Kconfig
>  create mode 100644 drivers/staging/media/tw5864/Makefile
>  create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864.h
> 



> diff --git a/drivers/staging/media/tw5864/tw5864-bs.h 
> b/drivers/staging/media/tw5864/tw5864-bs.h
> new file mode 100644
> index 000..8c1df7a
> --- /dev/null
> +++ b/drivers/staging/media/tw5864/tw5864-bs.h
> @@ -0,0 +1,154 @@



> +static inline void bs_direct_write(struct bs *s, u8 value)
> +{
> + *s->p = value;
> + s->p++;
> + s->i_left = 8;
> +}
> +
> +static inline void bs_write(struct bs *s, int i_count, u32 i_bits)

This one is a bit too big to be an inline IMHO.

> +{
> + if (s->p >= s->p_end - 4)
> + return;
> + while (i_count > 0) {
> + if (i_count < 32)
> + i_bits &= (1 << i_count) - 1;
> + if (i_count < s->i_left) {
> + *s->p = (*s->p << i_count) | i_bits;
> + s->i_left -= i_count;
> + break;
> + }
> + *s->p = (*s->p << s->i_left) | (i_bits >> (i_count -
> +s->i_left));
> + i_count -= s->i_left;
> + s->p++;
> + s->i_left = 8;
> + }
> +}
> +



> diff --git a/drivers/staging/media/tw5864/tw5864-config.c 
> b/drivers/staging/media/tw5864/tw5864-config.c
> new file mode 100644
> index 000..ff3e308
> --- /dev/null
> +++ b/drivers/staging/media/tw5864/tw5864-config.c
> @@ -0,0 +1,359 @@
> +/*
> + *  TW5864 driver - analog decoders configuration functions
> + *
> + *  Copyright (C) 2015 Bluecherry, LLC 
> + *  Author: Andrey Utkin 
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#include "tw5864.h"
> +#include "tw5864-reg.h"
> +
> +#define TW5864_IIC_TIMEOUT  (3)
> +
> +static unsigned char tbl_pal_tw2864_common[] __used = {

Just use static const and remove the __used.

It would be nice to have a short comment before each array that gives a bit
more information.

> + 0x00, 0x00, 0x64, 0x11,
> + 0x80, 0x80, 0x00, 0x12,
> + 0x12, 0x20, 0x0a, 0xD0,
> + 0x00, 0x00, 0x07, 0x7F,
> +};
> +



> +static int __used i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn,
> +  u8 *buf, u32 count)

Again, what's up with the __used? Just remove it.

> +{
> + int i = 0;
> 

Re: [PATCH] Add tw5864 driver

[Hans Verkuil]

Hi Andrey,

See below for a quick review of the code.

I agree with Greg's comment why this is added to staging instead of 
drivers/media/pci?

When you post the v2 patch, can you add the output of 'v4l2-compliance -s' to 
the
cover letter? Please use the latest v4l2-compliance version from the 
v4l-utils.git
repository when testing.

On 03/14/2016 02:59 AM, Andrey Utkin wrote:
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).
> 
> Signed-off-by: Andrey Utkin 
> Tested-by: Andrey Utkin 
> ---
>  MAINTAINERS  |7 +
>  drivers/staging/media/Kconfig|2 +
>  drivers/staging/media/Makefile   |1 +
>  drivers/staging/media/tw5864/Kconfig |   11 +
>  drivers/staging/media/tw5864/Makefile|3 +
>  drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
>  drivers/staging/media/tw5864/tw5864-config.c |  359 +
>  drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
>  drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
>  drivers/staging/media/tw5864/tw5864-reg.h| 2200 
> ++
>  drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
>  drivers/staging/media/tw5864/tw5864-video.c  | 1364 
>  drivers/staging/media/tw5864/tw5864.h|  280 
>  include/linux/pci_ids.h  |1 +
>  14 files changed, 5255 insertions(+)
>  create mode 100644 drivers/staging/media/tw5864/Kconfig
>  create mode 100644 drivers/staging/media/tw5864/Makefile
>  create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
>  create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
>  create mode 100644 drivers/staging/media/tw5864/tw5864.h
> 



> diff --git a/drivers/staging/media/tw5864/tw5864-bs.h 
> b/drivers/staging/media/tw5864/tw5864-bs.h
> new file mode 100644
> index 000..8c1df7a
> --- /dev/null
> +++ b/drivers/staging/media/tw5864/tw5864-bs.h
> @@ -0,0 +1,154 @@



> +static inline void bs_direct_write(struct bs *s, u8 value)
> +{
> + *s->p = value;
> + s->p++;
> + s->i_left = 8;
> +}
> +
> +static inline void bs_write(struct bs *s, int i_count, u32 i_bits)

This one is a bit too big to be an inline IMHO.

> +{
> + if (s->p >= s->p_end - 4)
> + return;
> + while (i_count > 0) {
> + if (i_count < 32)
> + i_bits &= (1 << i_count) - 1;
> + if (i_count < s->i_left) {
> + *s->p = (*s->p << i_count) | i_bits;
> + s->i_left -= i_count;
> + break;
> + }
> + *s->p = (*s->p << s->i_left) | (i_bits >> (i_count -
> +s->i_left));
> + i_count -= s->i_left;
> + s->p++;
> + s->i_left = 8;
> + }
> +}
> +



> diff --git a/drivers/staging/media/tw5864/tw5864-config.c 
> b/drivers/staging/media/tw5864/tw5864-config.c
> new file mode 100644
> index 000..ff3e308
> --- /dev/null
> +++ b/drivers/staging/media/tw5864/tw5864-config.c
> @@ -0,0 +1,359 @@
> +/*
> + *  TW5864 driver - analog decoders configuration functions
> + *
> + *  Copyright (C) 2015 Bluecherry, LLC 
> + *  Author: Andrey Utkin 
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#include "tw5864.h"
> +#include "tw5864-reg.h"
> +
> +#define TW5864_IIC_TIMEOUT  (3)
> +
> +static unsigned char tbl_pal_tw2864_common[] __used = {

Just use static const and remove the __used.

It would be nice to have a short comment before each array that gives a bit
more information.

> + 0x00, 0x00, 0x64, 0x11,
> + 0x80, 0x80, 0x00, 0x12,
> + 0x12, 0x20, 0x0a, 0xD0,
> + 0x00, 0x00, 0x07, 0x7F,
> +};
> +



> +static int __used i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn,
> +  u8 *buf, u32 count)

Again, what's up with the __used? Just remove it.

> +{
> + int i = 0;
> + u32 val = 0;
> + int timeout = TW5864_IIC_TIMEOUT;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + 

Re: [PATCH] Add tw5864 driver

[Greg Kroah-Hartman]

On Mon, Mar 14, 2016 at 03:55:14AM +0200, Andrey Utkin wrote:
> From: Andrey Utkin 
> 
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).
> 
> Signed-off-by: Andrey Utkin 
> Tested-by: Andrey Utkin 

Meta-conmment, why add this to drivers/staging/media?  Why can't it just
go into drivers/media/ ?

thanks,

greg k-h

Re: [PATCH] Add tw5864 driver

[Greg Kroah-Hartman]

On Mon, Mar 14, 2016 at 03:55:14AM +0200, Andrey Utkin wrote:
> From: Andrey Utkin 
> 
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).
> 
> Signed-off-by: Andrey Utkin 
> Tested-by: Andrey Utkin 

Meta-conmment, why add this to drivers/staging/media?  Why can't it just
go into drivers/media/ ?

thanks,

greg k-h

Re: [PATCH] Add tw5864 driver

[Greg Kroah-Hartman]

On Mon, Mar 14, 2016 at 03:55:14AM +0200, Andrey Utkin wrote:
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2333,6 +2333,7 @@
>  #define PCI_VENDOR_ID_CAVIUM 0x177d
>  
>  #define PCI_VENDOR_ID_TECHWELL   0x1797
> +#define PCI_DEVICE_ID_TECHWELL_5864  0x5864
>  #define PCI_DEVICE_ID_TECHWELL_6800  0x6800
>  #define PCI_DEVICE_ID_TECHWELL_6801  0x6801
>  #define PCI_DEVICE_ID_TECHWELL_6804  0x6804

Please read the comments at the top of this file for why you don't need
to put any new ids into it.

thanks,

greg k-h

Re: [PATCH] Add tw5864 driver

[Greg Kroah-Hartman]

On Mon, Mar 14, 2016 at 03:55:14AM +0200, Andrey Utkin wrote:
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2333,6 +2333,7 @@
>  #define PCI_VENDOR_ID_CAVIUM 0x177d
>  
>  #define PCI_VENDOR_ID_TECHWELL   0x1797
> +#define PCI_DEVICE_ID_TECHWELL_5864  0x5864
>  #define PCI_DEVICE_ID_TECHWELL_6800  0x6800
>  #define PCI_DEVICE_ID_TECHWELL_6801  0x6801
>  #define PCI_DEVICE_ID_TECHWELL_6804  0x6804

Please read the comments at the top of this file for why you don't need
to put any new ids into it.

thanks,

greg k-h

Re: [PATCH] Add tw5864 driver

[Joe Perches]

On Mon, 2016-03-14 at 03:59 +0200, Andrey Utkin wrote:
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).

trivia:

Perhaps all the __used arrays could be const

Re: [PATCH] Add tw5864 driver

[Joe Perches]

On Mon, 2016-03-14 at 03:59 +0200, Andrey Utkin wrote:
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).

trivia:

Perhaps all the __used arrays could be const