Re: [PATCH] Add tw5864 driver
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-media@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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add tw5864 driver
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);
> + f
Re: [PATCH] Add tw5864 driver
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add tw5864 driver
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add tw5864 driver
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add tw5864 driver - cover letter
From: Andrey Utkin This is a driver for multimedia devices based on Techwell/Intersil TW5864 chip. It is basically written from scratch. There was an awful reference driver for 2.6 kernel, which is nearly million lines of code and requires half a dozen special userspace libraries, and still doesn't quite work. So currently submitted driver is a product of reverse-engineering and heuristics. tw68 driver was used as code skeleton. The device advertises many capabilities, but this version of driver only supports H.264 encoding of captured video channels. There is one known issue, which reproduces on two of five setups of which I know: P-frames are distorted, but I-frames are fine. Changing quality and framerate settings does not affect this. Currently such workaround is used: v4l2-ctl -d /dev/video$n -c video_gop_size=1 GOP size is set to 1, so that every output frame is I-frame. We are regularly contacting manufacturer regarding such issues, but unfortunately they can do little to help us. Andrey Utkin (1): Add tw5864 driver 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 -- 2.7.1.380.g0fea050.dirty -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add tw5864 driver - cover letter
This is a driver for multimedia devices based on Techwell/Intersil TW5864 chip. It is basically written from scratch. There was an awful reference driver for 2.6 kernel, which is nearly million lines of code and requires half a dozen special userspace libraries, and still doesn't quite work. So currently submitted driver is a product of reverse-engineering and heuristics. tw68 driver was used as code skeleton. The device advertises many capabilities, but this version of driver only supports H.264 encoding of captured video channels. There is one known issue, which reproduces on two of five setups of which I know: P-frames are distorted, but I-frames are fine. Changing quality and framerate settings does not affect this. Currently such workaround is used: v4l2-ctl -d /dev/video$n -c video_gop_size=1 GOP size is set to 1, so that every output frame is I-frame. We are regularly contacting manufacturer regarding such issues, but unfortunately they can do little to help us. Andrey Utkin (1): Add tw5864 driver 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 -- 2.7.1.380.g0fea050.dirty -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
