* [GIT PULL] SDR stuff
@ 2014-07-18 1:14 Antti Palosaari
2014-07-20 16:41 ` Antti Palosaari
2014-07-21 23:50 ` Mauro Carvalho Chehab
0 siblings, 2 replies; 11+ messages in thread
From: Antti Palosaari @ 2014-07-18 1:14 UTC (permalink / raw)
To: LMML, Hans Verkuil
* AirSpy SDR driver
* all SDR drivers moved out of staging
* few new SDR stream formats
regards
Antti
The following changes since commit 3445857b22eafb70a6ac258979e955b116bfd2c6:
[media] hdpvr: fix two audio bugs (2014-07-04 15:13:02 -0300)
are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git sdr_pull
for you to fetch changes up to 1c3378e1c17d6acd9b6d392ff75addad4c63cc6c:
MAINTAINERS: add airspy driver (2014-07-18 04:12:27 +0300)
----------------------------------------------------------------
Antti Palosaari (23):
v4l: uapi: add SDR format RU12LE
DocBook: V4L: add V4L2_SDR_FMT_RU12LE - 'RU12'
airspy: AirSpy SDR driver
v4l: uapi: add SDR format CS8
DocBook: V4L: add V4L2_SDR_FMT_CS8 - 'CS08'
v4l: uapi: add SDR format CS14
DocBook: V4L: add V4L2_SDR_FMT_CS14LE - 'CS14'
msi001: move out of staging
MAINTAINERS: update MSI001 driver location
Kconfig: add SDR support
Kconfig: sub-driver auto-select SPI bus
msi2500: move msi3101 out of staging and rename
MAINTAINERS: update MSI3101 / MSI2500 driver location
msi2500: change supported formats
msi2500: print notice to point SDR API is not 100% stable yet
rtl2832_sdr: move from staging to media
rtl2832_sdr: put complex U16 format behind module parameter
rtl2832_sdr: print notice to point SDR API is not 100% stable yet
MAINTAINERS: update RTL2832_SDR location
airspy: remove v4l2-compliance workaround
airspy: move out of staging into drivers/media/usb
airspy: print notice to point SDR API is not 100% stable yet
MAINTAINERS: add airspy driver
Documentation/DocBook/media/v4l/pixfmt-sdr-cs08.xml
| 44 ++++
Documentation/DocBook/media/v4l/pixfmt-sdr-cs14le.xml
| 47 +++++
Documentation/DocBook/media/v4l/pixfmt-sdr-ru12le.xml
| 40 ++++
Documentation/DocBook/media/v4l/pixfmt.xml
| 3 +
MAINTAINERS
| 18 +-
drivers/media/Kconfig
| 12 +-
drivers/media/dvb-frontends/Kconfig
| 9 +
drivers/media/dvb-frontends/Makefile
| 6 +
drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.c | 21 +-
drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.h | 0
drivers/media/tuners/Kconfig
| 6 +
drivers/media/tuners/Makefile
| 1 +
drivers/{staging/media/msi3101 => media/tuners}/msi001.c
| 0
drivers/media/usb/Kconfig
| 6 +
drivers/media/usb/Makefile
| 2 +
drivers/media/usb/airspy/Kconfig
| 10 +
drivers/media/usb/airspy/Makefile
| 1 +
drivers/media/usb/airspy/airspy.c
| 1122
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/media/usb/dvb-usb-v2/Kconfig
| 1 +
drivers/media/usb/msi2500/Kconfig
| 5 +
drivers/media/usb/msi2500/Makefile
| 1 +
drivers/{staging/media/msi3101/sdr-msi3101.c =>
media/usb/msi2500/msi2500.c} | 47 +++--
drivers/staging/media/Kconfig
| 4 -
drivers/staging/media/Makefile
| 2 -
drivers/staging/media/msi3101/Kconfig
| 10 -
drivers/staging/media/msi3101/Makefile
| 2 -
drivers/staging/media/rtl2832u_sdr/Kconfig
| 7 -
drivers/staging/media/rtl2832u_sdr/Makefile
| 6 -
include/uapi/linux/videodev2.h
| 3 +
29 files changed, 1375 insertions(+), 61 deletions(-)
create mode 100644 Documentation/DocBook/media/v4l/pixfmt-sdr-cs08.xml
create mode 100644 Documentation/DocBook/media/v4l/pixfmt-sdr-cs14le.xml
create mode 100644 Documentation/DocBook/media/v4l/pixfmt-sdr-ru12le.xml
rename drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.c (98%)
rename drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.h (100%)
rename drivers/{staging/media/msi3101 => media/tuners}/msi001.c (100%)
create mode 100644 drivers/media/usb/airspy/Kconfig
create mode 100644 drivers/media/usb/airspy/Makefile
create mode 100644 drivers/media/usb/airspy/airspy.c
create mode 100644 drivers/media/usb/msi2500/Kconfig
create mode 100644 drivers/media/usb/msi2500/Makefile
rename drivers/{staging/media/msi3101/sdr-msi3101.c =>
media/usb/msi2500/msi2500.c} (98%)
delete mode 100644 drivers/staging/media/msi3101/Kconfig
delete mode 100644 drivers/staging/media/msi3101/Makefile
delete mode 100644 drivers/staging/media/rtl2832u_sdr/Kconfig
delete mode 100644 drivers/staging/media/rtl2832u_sdr/Makefile
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-18 1:14 [GIT PULL] SDR stuff Antti Palosaari
@ 2014-07-20 16:41 ` Antti Palosaari
2014-07-21 23:50 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 11+ messages in thread
From: Antti Palosaari @ 2014-07-20 16:41 UTC (permalink / raw)
To: LMML, Hans Verkuil
On 07/18/2014 04:14 AM, Antti Palosaari wrote:
> * AirSpy SDR driver
> * all SDR drivers moved out of staging
> * few new SDR stream formats
Added few patches more.
Antti
The following changes since commit 3445857b22eafb70a6ac258979e955b116bfd2c6:
[media] hdpvr: fix two audio bugs (2014-07-04 15:13:02 -0300)
are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git sdr_pull
for you to fetch changes up to 57c6d1bcea459f50bfe1b8a47f575655deca888a:
airspy: fill FMT buffer size (2014-07-20 19:37:34 +0300)
----------------------------------------------------------------
Antti Palosaari (28):
v4l: uapi: add SDR format RU12LE
DocBook: V4L: add V4L2_SDR_FMT_RU12LE - 'RU12'
airspy: AirSpy SDR driver
v4l: uapi: add SDR format CS8
DocBook: V4L: add V4L2_SDR_FMT_CS8 - 'CS08'
v4l: uapi: add SDR format CS14
DocBook: V4L: add V4L2_SDR_FMT_CS14LE - 'CS14'
msi001: move out of staging
MAINTAINERS: update MSI001 driver location
Kconfig: add SDR support
Kconfig: sub-driver auto-select SPI bus
msi2500: move msi3101 out of staging and rename
MAINTAINERS: update MSI3101 / MSI2500 driver location
msi2500: change supported formats
msi2500: print notice to point SDR API is not 100% stable yet
rtl2832_sdr: move from staging to media
rtl2832_sdr: put complex U16 format behind module parameter
rtl2832_sdr: print notice to point SDR API is not 100% stable yet
MAINTAINERS: update RTL2832_SDR location
airspy: remove v4l2-compliance workaround
airspy: move out of staging into drivers/media/usb
airspy: print notice to point SDR API is not 100% stable yet
MAINTAINERS: add airspy driver
v4l: videodev2: add buffer size to SDR format
rtl2832_sdr: fill FMT buffer size
DocBook media: v4l2_sdr_format buffersize field
msi2500: fill FMT buffer size
airspy: fill FMT buffer size
Documentation/DocBook/media/v4l/dev-sdr.xml
| 18 +-
Documentation/DocBook/media/v4l/pixfmt-sdr-cs08.xml
| 44 ++++
Documentation/DocBook/media/v4l/pixfmt-sdr-cs14le.xml
| 47 +++++
Documentation/DocBook/media/v4l/pixfmt-sdr-ru12le.xml
| 40 ++++
Documentation/DocBook/media/v4l/pixfmt.xml
| 3 +
MAINTAINERS
| 18 +-
drivers/media/Kconfig
| 12 +-
drivers/media/dvb-frontends/Kconfig
| 9 +
drivers/media/dvb-frontends/Makefile
| 6 +
drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.c | 48 +++--
drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.h | 0
drivers/media/tuners/Kconfig
| 6 +
drivers/media/tuners/Makefile
| 1 +
drivers/{staging/media/msi3101 => media/tuners}/msi001.c
| 0
drivers/media/usb/Kconfig
| 6 +
drivers/media/usb/Makefile
| 2 +
drivers/media/usb/airspy/Kconfig
| 10 +
drivers/media/usb/airspy/Makefile
| 1 +
drivers/media/usb/airspy/airspy.c
| 1134
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/media/usb/dvb-usb-v2/Kconfig
| 1 +
drivers/media/usb/msi2500/Kconfig
| 5 +
drivers/media/usb/msi2500/Makefile
| 1 +
drivers/{staging/media/msi3101/sdr-msi3101.c =>
media/usb/msi2500/msi2500.c} | 78 ++++---
drivers/staging/media/Kconfig
| 4 -
drivers/staging/media/Makefile
| 2 -
drivers/staging/media/msi3101/Kconfig
| 10 -
drivers/staging/media/msi3101/Makefile
| 2 -
drivers/staging/media/rtl2832u_sdr/Kconfig
| 7 -
drivers/staging/media/rtl2832u_sdr/Makefile
| 6 -
include/uapi/linux/videodev2.h
| 7 +-
30 files changed, 1444 insertions(+), 84 deletions(-)
create mode 100644 Documentation/DocBook/media/v4l/pixfmt-sdr-cs08.xml
create mode 100644 Documentation/DocBook/media/v4l/pixfmt-sdr-cs14le.xml
create mode 100644 Documentation/DocBook/media/v4l/pixfmt-sdr-ru12le.xml
rename drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.c (96%)
rename drivers/{staging/media/rtl2832u_sdr =>
media/dvb-frontends}/rtl2832_sdr.h (100%)
rename drivers/{staging/media/msi3101 => media/tuners}/msi001.c (100%)
create mode 100644 drivers/media/usb/airspy/Kconfig
create mode 100644 drivers/media/usb/airspy/Makefile
create mode 100644 drivers/media/usb/airspy/airspy.c
create mode 100644 drivers/media/usb/msi2500/Kconfig
create mode 100644 drivers/media/usb/msi2500/Makefile
rename drivers/{staging/media/msi3101/sdr-msi3101.c =>
media/usb/msi2500/msi2500.c} (96%)
delete mode 100644 drivers/staging/media/msi3101/Kconfig
delete mode 100644 drivers/staging/media/msi3101/Makefile
delete mode 100644 drivers/staging/media/rtl2832u_sdr/Kconfig
delete mode 100644 drivers/staging/media/rtl2832u_sdr/Makefile
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-18 1:14 [GIT PULL] SDR stuff Antti Palosaari
2014-07-20 16:41 ` Antti Palosaari
@ 2014-07-21 23:50 ` Mauro Carvalho Chehab
2014-07-22 0:05 ` Mauro Carvalho Chehab
2014-07-22 0:08 ` Antti Palosaari
1 sibling, 2 replies; 11+ messages in thread
From: Mauro Carvalho Chehab @ 2014-07-21 23:50 UTC (permalink / raw)
To: Antti Palosaari; +Cc: LMML, Hans Verkuil
Em Fri, 18 Jul 2014 04:14:32 +0300
Antti Palosaari <crope@iki.fi> escreveu:
> * AirSpy SDR driver
> * all SDR drivers moved out of staging
> * few new SDR stream formats
>
> regards
> Antti
>
>
> The following changes since commit 3445857b22eafb70a6ac258979e955b116bfd2c6:
>
> [media] hdpvr: fix two audio bugs (2014-07-04 15:13:02 -0300)
>
> are available in the git repository at:
>
> git://linuxtv.org/anttip/media_tree.git sdr_pull
>
> for you to fetch changes up to 1c3378e1c17d6acd9b6d392ff75addad4c63cc6c:
>
> MAINTAINERS: add airspy driver (2014-07-18 04:12:27 +0300)
>
> ----------------------------------------------------------------
> Antti Palosaari (23):
> v4l: uapi: add SDR format RU12LE
> DocBook: V4L: add V4L2_SDR_FMT_RU12LE - 'RU12'
> airspy: AirSpy SDR driver
> v4l: uapi: add SDR format CS8
> DocBook: V4L: add V4L2_SDR_FMT_CS8 - 'CS08'
> v4l: uapi: add SDR format CS14
> DocBook: V4L: add V4L2_SDR_FMT_CS14LE - 'CS14'
> msi001: move out of staging
> MAINTAINERS: update MSI001 driver location
> Kconfig: add SDR support
> Kconfig: sub-driver auto-select SPI bus
> msi2500: move msi3101 out of staging and rename
There are several issues pointed by checkpath on this driver:
WARNING: line over 80 characters
#55: FILE: drivers/media/usb/msi2500/msi2500.c:55:
+#define V4L2_PIX_FMT_SDR_S8 v4l2_fourcc('D', 'S', '0', '8') /* signed 8-bit */
WARNING: line over 80 characters
#56: FILE: drivers/media/usb/msi2500/msi2500.c:56:
+#define V4L2_PIX_FMT_SDR_S12 v4l2_fourcc('D', 'S', '1', '2') /* signed 12-bit */
WARNING: line over 80 characters
#57: FILE: drivers/media/usb/msi2500/msi2500.c:57:
+#define V4L2_PIX_FMT_SDR_S14 v4l2_fourcc('D', 'S', '1', '4') /* signed 14-bit */
WARNING: line over 80 characters
#58: FILE: drivers/media/usb/msi2500/msi2500.c:58:
+#define V4L2_PIX_FMT_SDR_MSI2500_384 v4l2_fourcc('M', '3', '8', '4') /* Mirics MSi2500 format 384 */
The above are OK, however those formats should be moved to videodev2.h,
where those API bits belong.
There are several warnings, not all are mandatory for moving it out of
staging. I'll point the critical ones below:
WARNING: Missing a blank line after declarations
#135: FILE: drivers/media/usb/msi2500/msi2500.c:135:
+ struct urb *urbs[MAX_ISO_BUFS];
+ int (*convert_stream)(struct msi3101_state *s, u8 *dst, u8 *src,
WARNING: line over 80 characters
#188: FILE: drivers/media/usb/msi2500/msi2500.c:188:
+ sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
#211: FILE: drivers/media/usb/msi2500/msi2500.c:211:
+ if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
This one is a real bug, as jiffies may reset to zero. you should, instead,
use the time macros, like time_is_before_jiffies() and
time_is_after_jiffies().
WARNING: line over 80 characters
#213: FILE: drivers/media/usb/msi2500/msi2500.c:213:
+ unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
This also seems wrong for the same reasons.
WARNING: Missing a blank line after declarations
#215: FILE: drivers/media/usb/msi2500/msi2500.c:215:
+ unsigned int samples = sample_num[i_max - 1] - s->sample;
+ s->jiffies_next = jiffies_now;
WARNING: line over 80 characters
#242: FILE: drivers/media/usb/msi2500/msi2500.c:242:
+ sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
WARNING: Missing a blank line after declarations
#272: FILE: drivers/media/usb/msi2500/msi2500.c:272:
+ unsigned int samples = sample_num[i_max - 1] - s->sample;
+ s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
WARNING: line over 80 characters
#339: FILE: drivers/media/usb/msi2500/msi2500.c:339:
+ sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
#363: FILE: drivers/media/usb/msi2500/msi2500.c:363:
+ if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
Same here.
WARNING: line over 80 characters
#365: FILE: drivers/media/usb/msi2500/msi2500.c:365:
+ unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
Same here.
WARNING: Missing a blank line after declarations
#367: FILE: drivers/media/usb/msi2500/msi2500.c:367:
+ unsigned int samples = sample_num[i_max - 1] - s->sample;
+ s->jiffies_next = jiffies_now;
WARNING: line over 80 characters
#405: FILE: drivers/media/usb/msi2500/msi2500.c:405:
+ sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
#428: FILE: drivers/media/usb/msi2500/msi2500.c:428:
+ if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
Same here.
WARNING: line over 80 characters
#430: FILE: drivers/media/usb/msi2500/msi2500.c:430:
+ unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
WARNING: Missing a blank line after declarations
#432: FILE: drivers/media/usb/msi2500/msi2500.c:432:
+ unsigned int samples = sample_num[i_max - 1] - s->sample;
+ s->jiffies_next = jiffies_now;
WARNING: line over 80 characters
#468: FILE: drivers/media/usb/msi2500/msi2500.c:468:
+ sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
#491: FILE: drivers/media/usb/msi2500/msi2500.c:491:
+ if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
Same here.
WARNING: line over 80 characters
#493: FILE: drivers/media/usb/msi2500/msi2500.c:493:
+ unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
Same here.
WARNING: Missing a blank line after declarations
#495: FILE: drivers/media/usb/msi2500/msi2500.c:495:
+ unsigned int samples = sample_num[i_max - 1] - s->sample;
+ s->jiffies_next = jiffies_now;
ERROR: space required after that ';' (ctx:VxV)
#515: FILE: drivers/media/usb/msi2500/msi2500.c:515:
+ struct {signed int x:14;} se;
^
Better to declare it as:
struct {
signed int x:14;
} se;
That makes easier to read, IMHO, and follows Kernel CodingStyle.
WARNING: line over 80 characters
#521: FILE: drivers/media/usb/msi2500/msi2500.c:521:
+ sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
WARNING: Missing a blank line after declarations
#567: FILE: drivers/media/usb/msi2500/msi2500.c:567:
+ unsigned int samples = sample_num[i_max - 1] - s->sample;
+ s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
WARNING: Missing a blank line after declarations
#661: FILE: drivers/media/usb/msi2500/msi2500.c:661:
+ int i;
+ dev_dbg(&s->udev->dev, "%s:\n", __func__);
WARNING: Missing a blank line after declarations
#676: FILE: drivers/media/usb/msi2500/msi2500.c:676:
+ int i;
+ dev_dbg(&s->udev->dev, "%s:\n", __func__);
WARNING: Missing a blank line after declarations
#709: FILE: drivers/media/usb/msi2500/msi2500.c:709:
+ int i, j, ret;
+ dev_dbg(&s->udev->dev, "%s:\n", __func__);
WARNING: Missing a blank line after declarations
#775: FILE: drivers/media/usb/msi2500/msi2500.c:775:
+ unsigned long flags = 0;
+ dev_dbg(&s->udev->dev, "%s:\n", __func__);
WARNING: Missing a blank line after declarations
#814: FILE: drivers/media/usb/msi2500/msi2500.c:814:
+ struct msi3101_state *s = video_drvdata(file);
+ dev_dbg(&s->udev->dev, "%s:\n", __func__);
WARNING: Missing a blank line after declarations
#831: FILE: drivers/media/usb/msi2500/msi2500.c:831:
+ struct msi3101_state *s = vb2_get_drv_priv(vq);
+ dev_dbg(&s->udev->dev, "%s: *nbuffers=%d\n", __func__, *nbuffers);
WARNING: line over 80 characters
#879: FILE: drivers/media/usb/msi2500/msi2500.c:879:
+ _i & 0xff, _i >> 8, l & 0xff, l >> 8, direction, l, b); \
WARNING: line over 80 characters
#915: FILE: drivers/media/usb/msi2500/msi2500.c:915:
+ bandwidth_auto = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH_AUTO);
WARNING: line over 80 characters
#917: FILE: drivers/media/usb/msi2500/msi2500.c:917:
+ bandwidth = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH);
WARNING: line over 80 characters
#1012: FILE: drivers/media/usb/msi2500/msi2500.c:1012:
+ __func__, f_sr, f_vco, div_n, div_m, div_r_out, reg3, reg4);
WARNING: Missing a blank line after declarations
#1053: FILE: drivers/media/usb/msi2500/msi2500.c:1053:
+ int ret;
+ dev_dbg(&s->udev->dev, "%s:\n", __func__);
WARNING: Missing a blank line after declarations
#1116: FILE: drivers/media/usb/msi2500/msi2500.c:1116:
+ struct msi3101_state *s = video_drvdata(file);
+ dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, f->index);
WARNING: Missing a blank line after declarations
#1131: FILE: drivers/media/usb/msi2500/msi2500.c:1131:
+ struct msi3101_state *s = video_drvdata(file);
+ dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
WARNING: Missing a blank line after declarations
#1146: FILE: drivers/media/usb/msi2500/msi2500.c:1146:
+ int i;
+ dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
WARNING: Missing a blank line after declarations
#1171: FILE: drivers/media/usb/msi2500/msi2500.c:1171:
+ int i;
+ dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
WARNING: Missing a blank line after declarations
#1190: FILE: drivers/media/usb/msi2500/msi2500.c:1190:
+ int ret;
+ dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
WARNING: Missing a blank line after declarations
#1206: FILE: drivers/media/usb/msi2500/msi2500.c:1206:
+ int ret;
+ dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
WARNING: Missing a blank line after declarations
#1229: FILE: drivers/media/usb/msi2500/msi2500.c:1229:
+ int ret = 0;
+ dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d\n",
WARNING: Missing a blank line after declarations
#1250: FILE: drivers/media/usb/msi2500/msi2500.c:1250:
+ int ret;
+ dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d frequency=%u\n",
WARNING: Missing a blank line after declarations
#1274: FILE: drivers/media/usb/msi2500/msi2500.c:1274:
+ int ret;
+ dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d index=%d\n",
total: 1 errors, 45 warnings, 1517 lines checked
drivers/media/usb/msi2500/msi2500.c has style problems, please review.
Regards,
Mauro
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-21 23:50 ` Mauro Carvalho Chehab
@ 2014-07-22 0:05 ` Mauro Carvalho Chehab
2014-07-22 0:16 ` Antti Palosaari
2014-07-22 0:08 ` Antti Palosaari
1 sibling, 1 reply; 11+ messages in thread
From: Mauro Carvalho Chehab @ 2014-07-22 0:05 UTC (permalink / raw)
To: Antti Palosaari; +Cc: LMML, Hans Verkuil
Em Mon, 21 Jul 2014 20:50:05 -0300
Mauro Carvalho Chehab <m.chehab@samsung.com> escreveu:
> Em Fri, 18 Jul 2014 04:14:32 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
> > * AirSpy SDR driver
> > * all SDR drivers moved out of staging
> > * few new SDR stream formats
> >
> > regards
> > Antti
> >
> >
> > The following changes since commit 3445857b22eafb70a6ac258979e955b116bfd2c6:
> >
> > [media] hdpvr: fix two audio bugs (2014-07-04 15:13:02 -0300)
> >
> > are available in the git repository at:
> >
> > git://linuxtv.org/anttip/media_tree.git sdr_pull
> >
> > for you to fetch changes up to 1c3378e1c17d6acd9b6d392ff75addad4c63cc6c:
> >
> > MAINTAINERS: add airspy driver (2014-07-18 04:12:27 +0300)
> >
> > ----------------------------------------------------------------
> > Antti Palosaari (23):
> > v4l: uapi: add SDR format RU12LE
> > DocBook: V4L: add V4L2_SDR_FMT_RU12LE - 'RU12'
> > airspy: AirSpy SDR driver
> > v4l: uapi: add SDR format CS8
> > DocBook: V4L: add V4L2_SDR_FMT_CS8 - 'CS08'
> > v4l: uapi: add SDR format CS14
> > DocBook: V4L: add V4L2_SDR_FMT_CS14LE - 'CS14'
> > msi001: move out of staging
> > MAINTAINERS: update MSI001 driver location
> > Kconfig: add SDR support
> > Kconfig: sub-driver auto-select SPI bus
> > msi2500: move msi3101 out of staging and rename
>
> There are several issues pointed by checkpath on this driver:
>
> WARNING: line over 80 characters
> #55: FILE: drivers/media/usb/msi2500/msi2500.c:55:
> +#define V4L2_PIX_FMT_SDR_S8 v4l2_fourcc('D', 'S', '0', '8') /* signed 8-bit */
>
> WARNING: line over 80 characters
> #56: FILE: drivers/media/usb/msi2500/msi2500.c:56:
> +#define V4L2_PIX_FMT_SDR_S12 v4l2_fourcc('D', 'S', '1', '2') /* signed 12-bit */
>
> WARNING: line over 80 characters
> #57: FILE: drivers/media/usb/msi2500/msi2500.c:57:
> +#define V4L2_PIX_FMT_SDR_S14 v4l2_fourcc('D', 'S', '1', '4') /* signed 14-bit */
>
> WARNING: line over 80 characters
> #58: FILE: drivers/media/usb/msi2500/msi2500.c:58:
> +#define V4L2_PIX_FMT_SDR_MSI2500_384 v4l2_fourcc('M', '3', '8', '4') /* Mirics MSi2500 format 384 */
>
> The above are OK, however those formats should be moved to videodev2.h,
> where those API bits belong.
>
> There are several warnings, not all are mandatory for moving it out of
> staging. I'll point the critical ones below:
>
> WARNING: Missing a blank line after declarations
> #135: FILE: drivers/media/usb/msi2500/msi2500.c:135:
> + struct urb *urbs[MAX_ISO_BUFS];
> + int (*convert_stream)(struct msi3101_state *s, u8 *dst, u8 *src,
>
> WARNING: line over 80 characters
> #188: FILE: drivers/media/usb/msi2500/msi2500.c:188:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #211: FILE: drivers/media/usb/msi2500/msi2500.c:211:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> This one is a real bug, as jiffies may reset to zero. you should, instead,
> use the time macros, like time_is_before_jiffies() and
> time_is_after_jiffies().
>
> WARNING: line over 80 characters
> #213: FILE: drivers/media/usb/msi2500/msi2500.c:213:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> This also seems wrong for the same reasons.
>
> WARNING: Missing a blank line after declarations
> #215: FILE: drivers/media/usb/msi2500/msi2500.c:215:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> WARNING: line over 80 characters
> #242: FILE: drivers/media/usb/msi2500/msi2500.c:242:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Missing a blank line after declarations
> #272: FILE: drivers/media/usb/msi2500/msi2500.c:272:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
>
> WARNING: line over 80 characters
> #339: FILE: drivers/media/usb/msi2500/msi2500.c:339:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #363: FILE: drivers/media/usb/msi2500/msi2500.c:363:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> Same here.
>
> WARNING: line over 80 characters
> #365: FILE: drivers/media/usb/msi2500/msi2500.c:365:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> Same here.
>
> WARNING: Missing a blank line after declarations
> #367: FILE: drivers/media/usb/msi2500/msi2500.c:367:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> WARNING: line over 80 characters
> #405: FILE: drivers/media/usb/msi2500/msi2500.c:405:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #428: FILE: drivers/media/usb/msi2500/msi2500.c:428:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> Same here.
>
> WARNING: line over 80 characters
> #430: FILE: drivers/media/usb/msi2500/msi2500.c:430:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> WARNING: Missing a blank line after declarations
> #432: FILE: drivers/media/usb/msi2500/msi2500.c:432:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> WARNING: line over 80 characters
> #468: FILE: drivers/media/usb/msi2500/msi2500.c:468:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #491: FILE: drivers/media/usb/msi2500/msi2500.c:491:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> Same here.
>
> WARNING: line over 80 characters
> #493: FILE: drivers/media/usb/msi2500/msi2500.c:493:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> Same here.
>
> WARNING: Missing a blank line after declarations
> #495: FILE: drivers/media/usb/msi2500/msi2500.c:495:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> ERROR: space required after that ';' (ctx:VxV)
> #515: FILE: drivers/media/usb/msi2500/msi2500.c:515:
> + struct {signed int x:14;} se;
> ^
>
> Better to declare it as:
> struct {
> signed int x:14;
> } se;
>
> That makes easier to read, IMHO, and follows Kernel CodingStyle.
>
> WARNING: line over 80 characters
> #521: FILE: drivers/media/usb/msi2500/msi2500.c:521:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Missing a blank line after declarations
> #567: FILE: drivers/media/usb/msi2500/msi2500.c:567:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
>
> WARNING: Missing a blank line after declarations
> #661: FILE: drivers/media/usb/msi2500/msi2500.c:661:
> + int i;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #676: FILE: drivers/media/usb/msi2500/msi2500.c:676:
> + int i;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #709: FILE: drivers/media/usb/msi2500/msi2500.c:709:
> + int i, j, ret;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #775: FILE: drivers/media/usb/msi2500/msi2500.c:775:
> + unsigned long flags = 0;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #814: FILE: drivers/media/usb/msi2500/msi2500.c:814:
> + struct msi3101_state *s = video_drvdata(file);
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #831: FILE: drivers/media/usb/msi2500/msi2500.c:831:
> + struct msi3101_state *s = vb2_get_drv_priv(vq);
> + dev_dbg(&s->udev->dev, "%s: *nbuffers=%d\n", __func__, *nbuffers);
>
> WARNING: line over 80 characters
> #879: FILE: drivers/media/usb/msi2500/msi2500.c:879:
> + _i & 0xff, _i >> 8, l & 0xff, l >> 8, direction, l, b); \
>
> WARNING: line over 80 characters
> #915: FILE: drivers/media/usb/msi2500/msi2500.c:915:
> + bandwidth_auto = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH_AUTO);
>
> WARNING: line over 80 characters
> #917: FILE: drivers/media/usb/msi2500/msi2500.c:917:
> + bandwidth = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH);
>
> WARNING: line over 80 characters
> #1012: FILE: drivers/media/usb/msi2500/msi2500.c:1012:
> + __func__, f_sr, f_vco, div_n, div_m, div_r_out, reg3, reg4);
>
> WARNING: Missing a blank line after declarations
> #1053: FILE: drivers/media/usb/msi2500/msi2500.c:1053:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #1116: FILE: drivers/media/usb/msi2500/msi2500.c:1116:
> + struct msi3101_state *s = video_drvdata(file);
> + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, f->index);
>
> WARNING: Missing a blank line after declarations
> #1131: FILE: drivers/media/usb/msi2500/msi2500.c:1131:
> + struct msi3101_state *s = video_drvdata(file);
> + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
>
> WARNING: Missing a blank line after declarations
> #1146: FILE: drivers/media/usb/msi2500/msi2500.c:1146:
> + int i;
> + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
>
> WARNING: Missing a blank line after declarations
> #1171: FILE: drivers/media/usb/msi2500/msi2500.c:1171:
> + int i;
> + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
>
> WARNING: Missing a blank line after declarations
> #1190: FILE: drivers/media/usb/msi2500/msi2500.c:1190:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
>
> WARNING: Missing a blank line after declarations
> #1206: FILE: drivers/media/usb/msi2500/msi2500.c:1206:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
>
> WARNING: Missing a blank line after declarations
> #1229: FILE: drivers/media/usb/msi2500/msi2500.c:1229:
> + int ret = 0;
> + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d\n",
>
> WARNING: Missing a blank line after declarations
> #1250: FILE: drivers/media/usb/msi2500/msi2500.c:1250:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d frequency=%u\n",
>
> WARNING: Missing a blank line after declarations
> #1274: FILE: drivers/media/usb/msi2500/msi2500.c:1274:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d index=%d\n",
>
> total: 1 errors, 45 warnings, 1517 lines checked
>
> drivers/media/usb/msi2500/msi2500.c has style problems, please review.
FYI, I applied the rest of this patch series, except for those patches:
msi2500: move msi3101 out of staging and rename
MAINTAINERS: update MSI3101 / MSI2500 driver location
msi2500: change supported formats
msi2500: print notice to point SDR API is not 100% stable yet
Because the latter ones depend on the first patch.
Regards,
Mauro
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-21 23:50 ` Mauro Carvalho Chehab
2014-07-22 0:05 ` Mauro Carvalho Chehab
@ 2014-07-22 0:08 ` Antti Palosaari
2014-07-22 0:51 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 11+ messages in thread
From: Antti Palosaari @ 2014-07-22 0:08 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: LMML, Hans Verkuil
So what. Those were mostly WARNING only and all but long lines were some
new checks added to checkpatch recently. chekcpatch gets all the time
new and new checks, these were added after I have made that driver. I
will surely clean those later when I do some new changes to driver and
my checkpatch updates.
regards
Antti
On 07/22/2014 02:50 AM, Mauro Carvalho Chehab wrote:
> Em Fri, 18 Jul 2014 04:14:32 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> * AirSpy SDR driver
>> * all SDR drivers moved out of staging
>> * few new SDR stream formats
>>
>> regards
>> Antti
>>
>>
>> The following changes since commit 3445857b22eafb70a6ac258979e955b116bfd2c6:
>>
>> [media] hdpvr: fix two audio bugs (2014-07-04 15:13:02 -0300)
>>
>> are available in the git repository at:
>>
>> git://linuxtv.org/anttip/media_tree.git sdr_pull
>>
>> for you to fetch changes up to 1c3378e1c17d6acd9b6d392ff75addad4c63cc6c:
>>
>> MAINTAINERS: add airspy driver (2014-07-18 04:12:27 +0300)
>>
>> ----------------------------------------------------------------
>> Antti Palosaari (23):
>> v4l: uapi: add SDR format RU12LE
>> DocBook: V4L: add V4L2_SDR_FMT_RU12LE - 'RU12'
>> airspy: AirSpy SDR driver
>> v4l: uapi: add SDR format CS8
>> DocBook: V4L: add V4L2_SDR_FMT_CS8 - 'CS08'
>> v4l: uapi: add SDR format CS14
>> DocBook: V4L: add V4L2_SDR_FMT_CS14LE - 'CS14'
>> msi001: move out of staging
>> MAINTAINERS: update MSI001 driver location
>> Kconfig: add SDR support
>> Kconfig: sub-driver auto-select SPI bus
>> msi2500: move msi3101 out of staging and rename
>
> There are several issues pointed by checkpath on this driver:
>
> WARNING: line over 80 characters
> #55: FILE: drivers/media/usb/msi2500/msi2500.c:55:
> +#define V4L2_PIX_FMT_SDR_S8 v4l2_fourcc('D', 'S', '0', '8') /* signed 8-bit */
>
> WARNING: line over 80 characters
> #56: FILE: drivers/media/usb/msi2500/msi2500.c:56:
> +#define V4L2_PIX_FMT_SDR_S12 v4l2_fourcc('D', 'S', '1', '2') /* signed 12-bit */
>
> WARNING: line over 80 characters
> #57: FILE: drivers/media/usb/msi2500/msi2500.c:57:
> +#define V4L2_PIX_FMT_SDR_S14 v4l2_fourcc('D', 'S', '1', '4') /* signed 14-bit */
>
> WARNING: line over 80 characters
> #58: FILE: drivers/media/usb/msi2500/msi2500.c:58:
> +#define V4L2_PIX_FMT_SDR_MSI2500_384 v4l2_fourcc('M', '3', '8', '4') /* Mirics MSi2500 format 384 */
>
> The above are OK, however those formats should be moved to videodev2.h,
> where those API bits belong.
>
> There are several warnings, not all are mandatory for moving it out of
> staging. I'll point the critical ones below:
>
> WARNING: Missing a blank line after declarations
> #135: FILE: drivers/media/usb/msi2500/msi2500.c:135:
> + struct urb *urbs[MAX_ISO_BUFS];
> + int (*convert_stream)(struct msi3101_state *s, u8 *dst, u8 *src,
>
> WARNING: line over 80 characters
> #188: FILE: drivers/media/usb/msi2500/msi2500.c:188:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #211: FILE: drivers/media/usb/msi2500/msi2500.c:211:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> This one is a real bug, as jiffies may reset to zero. you should, instead,
> use the time macros, like time_is_before_jiffies() and
> time_is_after_jiffies().
>
> WARNING: line over 80 characters
> #213: FILE: drivers/media/usb/msi2500/msi2500.c:213:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> This also seems wrong for the same reasons.
>
> WARNING: Missing a blank line after declarations
> #215: FILE: drivers/media/usb/msi2500/msi2500.c:215:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> WARNING: line over 80 characters
> #242: FILE: drivers/media/usb/msi2500/msi2500.c:242:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Missing a blank line after declarations
> #272: FILE: drivers/media/usb/msi2500/msi2500.c:272:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
>
> WARNING: line over 80 characters
> #339: FILE: drivers/media/usb/msi2500/msi2500.c:339:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #363: FILE: drivers/media/usb/msi2500/msi2500.c:363:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> Same here.
>
> WARNING: line over 80 characters
> #365: FILE: drivers/media/usb/msi2500/msi2500.c:365:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> Same here.
>
> WARNING: Missing a blank line after declarations
> #367: FILE: drivers/media/usb/msi2500/msi2500.c:367:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> WARNING: line over 80 characters
> #405: FILE: drivers/media/usb/msi2500/msi2500.c:405:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #428: FILE: drivers/media/usb/msi2500/msi2500.c:428:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> Same here.
>
> WARNING: line over 80 characters
> #430: FILE: drivers/media/usb/msi2500/msi2500.c:430:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> WARNING: Missing a blank line after declarations
> #432: FILE: drivers/media/usb/msi2500/msi2500.c:432:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> WARNING: line over 80 characters
> #468: FILE: drivers/media/usb/msi2500/msi2500.c:468:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> #491: FILE: drivers/media/usb/msi2500/msi2500.c:491:
> + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
>
> Same here.
>
> WARNING: line over 80 characters
> #493: FILE: drivers/media/usb/msi2500/msi2500.c:493:
> + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
>
> Same here.
>
> WARNING: Missing a blank line after declarations
> #495: FILE: drivers/media/usb/msi2500/msi2500.c:495:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies_now;
>
> ERROR: space required after that ';' (ctx:VxV)
> #515: FILE: drivers/media/usb/msi2500/msi2500.c:515:
> + struct {signed int x:14;} se;
> ^
>
> Better to declare it as:
> struct {
> signed int x:14;
> } se;
>
> That makes easier to read, IMHO, and follows Kernel CodingStyle.
>
> WARNING: line over 80 characters
> #521: FILE: drivers/media/usb/msi2500/msi2500.c:521:
> + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
>
> WARNING: Missing a blank line after declarations
> #567: FILE: drivers/media/usb/msi2500/msi2500.c:567:
> + unsigned int samples = sample_num[i_max - 1] - s->sample;
> + s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
>
> WARNING: Missing a blank line after declarations
> #661: FILE: drivers/media/usb/msi2500/msi2500.c:661:
> + int i;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #676: FILE: drivers/media/usb/msi2500/msi2500.c:676:
> + int i;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #709: FILE: drivers/media/usb/msi2500/msi2500.c:709:
> + int i, j, ret;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #775: FILE: drivers/media/usb/msi2500/msi2500.c:775:
> + unsigned long flags = 0;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #814: FILE: drivers/media/usb/msi2500/msi2500.c:814:
> + struct msi3101_state *s = video_drvdata(file);
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #831: FILE: drivers/media/usb/msi2500/msi2500.c:831:
> + struct msi3101_state *s = vb2_get_drv_priv(vq);
> + dev_dbg(&s->udev->dev, "%s: *nbuffers=%d\n", __func__, *nbuffers);
>
> WARNING: line over 80 characters
> #879: FILE: drivers/media/usb/msi2500/msi2500.c:879:
> + _i & 0xff, _i >> 8, l & 0xff, l >> 8, direction, l, b); \
>
> WARNING: line over 80 characters
> #915: FILE: drivers/media/usb/msi2500/msi2500.c:915:
> + bandwidth_auto = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH_AUTO);
>
> WARNING: line over 80 characters
> #917: FILE: drivers/media/usb/msi2500/msi2500.c:917:
> + bandwidth = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH);
>
> WARNING: line over 80 characters
> #1012: FILE: drivers/media/usb/msi2500/msi2500.c:1012:
> + __func__, f_sr, f_vco, div_n, div_m, div_r_out, reg3, reg4);
>
> WARNING: Missing a blank line after declarations
> #1053: FILE: drivers/media/usb/msi2500/msi2500.c:1053:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s:\n", __func__);
>
> WARNING: Missing a blank line after declarations
> #1116: FILE: drivers/media/usb/msi2500/msi2500.c:1116:
> + struct msi3101_state *s = video_drvdata(file);
> + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, f->index);
>
> WARNING: Missing a blank line after declarations
> #1131: FILE: drivers/media/usb/msi2500/msi2500.c:1131:
> + struct msi3101_state *s = video_drvdata(file);
> + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
>
> WARNING: Missing a blank line after declarations
> #1146: FILE: drivers/media/usb/msi2500/msi2500.c:1146:
> + int i;
> + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
>
> WARNING: Missing a blank line after declarations
> #1171: FILE: drivers/media/usb/msi2500/msi2500.c:1171:
> + int i;
> + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
>
> WARNING: Missing a blank line after declarations
> #1190: FILE: drivers/media/usb/msi2500/msi2500.c:1190:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
>
> WARNING: Missing a blank line after declarations
> #1206: FILE: drivers/media/usb/msi2500/msi2500.c:1206:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
>
> WARNING: Missing a blank line after declarations
> #1229: FILE: drivers/media/usb/msi2500/msi2500.c:1229:
> + int ret = 0;
> + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d\n",
>
> WARNING: Missing a blank line after declarations
> #1250: FILE: drivers/media/usb/msi2500/msi2500.c:1250:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d frequency=%u\n",
>
> WARNING: Missing a blank line after declarations
> #1274: FILE: drivers/media/usb/msi2500/msi2500.c:1274:
> + int ret;
> + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d index=%d\n",
>
> total: 1 errors, 45 warnings, 1517 lines checked
>
> drivers/media/usb/msi2500/msi2500.c has style problems, please review.
>
> Regards,
> Mauro
>
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-22 0:05 ` Mauro Carvalho Chehab
@ 2014-07-22 0:16 ` Antti Palosaari
0 siblings, 0 replies; 11+ messages in thread
From: Antti Palosaari @ 2014-07-22 0:16 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: LMML, Hans Verkuil
On 07/22/2014 03:05 AM, Mauro Carvalho Chehab wrote:
>> total: 1 errors, 45 warnings, 1517 lines checked
>>
>> drivers/media/usb/msi2500/msi2500.c has style problems, please review.
>
> FYI, I applied the rest of this patch series, except for those patches:
> msi2500: move msi3101 out of staging and rename
> MAINTAINERS: update MSI3101 / MSI2500 driver location
> msi2500: change supported formats
> msi2500: print notice to point SDR API is not 100% stable yet
>
> Because the latter ones depend on the first patch.
Hey, just apply these too. As I explained, those are coming from new
checkpatch checks added recently, after I have made that MSi3101/MSi2500
driver. We should not start begin run new checkpatch tests for old
drivers. One reason I really want these out from staging is checkpatch
terrorism newbies are doing in staging. There is all kind of people
doing some eucalyptys challenge, running very latest checkpatch and
sending useless patches for these driver, just wasting only my time.
regards
Antti
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-22 0:08 ` Antti Palosaari
@ 2014-07-22 0:51 ` Mauro Carvalho Chehab
2014-07-22 1:05 ` Antti Palosaari
0 siblings, 1 reply; 11+ messages in thread
From: Mauro Carvalho Chehab @ 2014-07-22 0:51 UTC (permalink / raw)
To: Antti Palosaari; +Cc: LMML, Hans Verkuil
Em Tue, 22 Jul 2014 03:08:19 +0300
Antti Palosaari <crope@iki.fi> escreveu:
> So what. Those were mostly WARNING only and all but long lines were some
> new checks added to checkpatch recently. chekcpatch gets all the time
> new and new checks, these were added after I have made that driver. I
> will surely clean those later when I do some new changes to driver and
> my checkpatch updates.
Antti,
I think you didn't read my comments in the middle of the checkpatch stuff.
Please read my email again. I'm not requiring you to fix the newer checkpatch
warning (Missing a blank line after declarations), and not even about the
80-cols warning. The thing is that there are two issues there:
1) you're adding API bits at msi2500 driver, instead of moving them
to videodev2.h (or reusing the fourcc types you already added there);
2) you're handling jiffies wrong inside the driver.
As you may know, adding a driver at staging is easier than at the main
tree, as we don't care much about checkpatch issues (and not even about
some more serious issues). However, when moving stuff out of staging,
we review the entire driver again, to be sure that it is ok.
Regards,
Mauro
>
>
> regards
> Antti
>
>
>
> On 07/22/2014 02:50 AM, Mauro Carvalho Chehab wrote:
> > Em Fri, 18 Jul 2014 04:14:32 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:
> >
> >> * AirSpy SDR driver
> >> * all SDR drivers moved out of staging
> >> * few new SDR stream formats
> >>
> >> regards
> >> Antti
> >>
> >>
> >> The following changes since commit 3445857b22eafb70a6ac258979e955b116bfd2c6:
> >>
> >> [media] hdpvr: fix two audio bugs (2014-07-04 15:13:02 -0300)
> >>
> >> are available in the git repository at:
> >>
> >> git://linuxtv.org/anttip/media_tree.git sdr_pull
> >>
> >> for you to fetch changes up to 1c3378e1c17d6acd9b6d392ff75addad4c63cc6c:
> >>
> >> MAINTAINERS: add airspy driver (2014-07-18 04:12:27 +0300)
> >>
> >> ----------------------------------------------------------------
> >> Antti Palosaari (23):
> >> v4l: uapi: add SDR format RU12LE
> >> DocBook: V4L: add V4L2_SDR_FMT_RU12LE - 'RU12'
> >> airspy: AirSpy SDR driver
> >> v4l: uapi: add SDR format CS8
> >> DocBook: V4L: add V4L2_SDR_FMT_CS8 - 'CS08'
> >> v4l: uapi: add SDR format CS14
> >> DocBook: V4L: add V4L2_SDR_FMT_CS14LE - 'CS14'
> >> msi001: move out of staging
> >> MAINTAINERS: update MSI001 driver location
> >> Kconfig: add SDR support
> >> Kconfig: sub-driver auto-select SPI bus
> >> msi2500: move msi3101 out of staging and rename
> >
> > There are several issues pointed by checkpath on this driver:
> >
> > WARNING: line over 80 characters
> > #55: FILE: drivers/media/usb/msi2500/msi2500.c:55:
> > +#define V4L2_PIX_FMT_SDR_S8 v4l2_fourcc('D', 'S', '0', '8') /* signed 8-bit */
> >
> > WARNING: line over 80 characters
> > #56: FILE: drivers/media/usb/msi2500/msi2500.c:56:
> > +#define V4L2_PIX_FMT_SDR_S12 v4l2_fourcc('D', 'S', '1', '2') /* signed 12-bit */
> >
> > WARNING: line over 80 characters
> > #57: FILE: drivers/media/usb/msi2500/msi2500.c:57:
> > +#define V4L2_PIX_FMT_SDR_S14 v4l2_fourcc('D', 'S', '1', '4') /* signed 14-bit */
> >
> > WARNING: line over 80 characters
> > #58: FILE: drivers/media/usb/msi2500/msi2500.c:58:
> > +#define V4L2_PIX_FMT_SDR_MSI2500_384 v4l2_fourcc('M', '3', '8', '4') /* Mirics MSi2500 format 384 */
> >
> > The above are OK, however those formats should be moved to videodev2.h,
> > where those API bits belong.
> >
> > There are several warnings, not all are mandatory for moving it out of
> > staging. I'll point the critical ones below:
> >
> > WARNING: Missing a blank line after declarations
> > #135: FILE: drivers/media/usb/msi2500/msi2500.c:135:
> > + struct urb *urbs[MAX_ISO_BUFS];
> > + int (*convert_stream)(struct msi3101_state *s, u8 *dst, u8 *src,
> >
> > WARNING: line over 80 characters
> > #188: FILE: drivers/media/usb/msi2500/msi2500.c:188:
> > + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
> >
> > WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> > #211: FILE: drivers/media/usb/msi2500/msi2500.c:211:
> > + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
> >
> > This one is a real bug, as jiffies may reset to zero. you should, instead,
> > use the time macros, like time_is_before_jiffies() and
> > time_is_after_jiffies().
> >
> > WARNING: line over 80 characters
> > #213: FILE: drivers/media/usb/msi2500/msi2500.c:213:
> > + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
> >
> > This also seems wrong for the same reasons.
> >
> > WARNING: Missing a blank line after declarations
> > #215: FILE: drivers/media/usb/msi2500/msi2500.c:215:
> > + unsigned int samples = sample_num[i_max - 1] - s->sample;
> > + s->jiffies_next = jiffies_now;
> >
> > WARNING: line over 80 characters
> > #242: FILE: drivers/media/usb/msi2500/msi2500.c:242:
> > + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
> >
> > WARNING: Missing a blank line after declarations
> > #272: FILE: drivers/media/usb/msi2500/msi2500.c:272:
> > + unsigned int samples = sample_num[i_max - 1] - s->sample;
> > + s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
> >
> > WARNING: line over 80 characters
> > #339: FILE: drivers/media/usb/msi2500/msi2500.c:339:
> > + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
> >
> > WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> > #363: FILE: drivers/media/usb/msi2500/msi2500.c:363:
> > + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
> >
> > Same here.
> >
> > WARNING: line over 80 characters
> > #365: FILE: drivers/media/usb/msi2500/msi2500.c:365:
> > + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
> >
> > Same here.
> >
> > WARNING: Missing a blank line after declarations
> > #367: FILE: drivers/media/usb/msi2500/msi2500.c:367:
> > + unsigned int samples = sample_num[i_max - 1] - s->sample;
> > + s->jiffies_next = jiffies_now;
> >
> > WARNING: line over 80 characters
> > #405: FILE: drivers/media/usb/msi2500/msi2500.c:405:
> > + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
> >
> > WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> > #428: FILE: drivers/media/usb/msi2500/msi2500.c:428:
> > + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
> >
> > Same here.
> >
> > WARNING: line over 80 characters
> > #430: FILE: drivers/media/usb/msi2500/msi2500.c:430:
> > + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
> >
> > WARNING: Missing a blank line after declarations
> > #432: FILE: drivers/media/usb/msi2500/msi2500.c:432:
> > + unsigned int samples = sample_num[i_max - 1] - s->sample;
> > + s->jiffies_next = jiffies_now;
> >
> > WARNING: line over 80 characters
> > #468: FILE: drivers/media/usb/msi2500/msi2500.c:468:
> > + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
> >
> > WARNING: Comparing jiffies is almost always wrong; prefer time_after, time_before and friends
> > #491: FILE: drivers/media/usb/msi2500/msi2500.c:491:
> > + if ((s->jiffies_next + msecs_to_jiffies(10000)) <= jiffies) {
> >
> > Same here.
> >
> > WARNING: line over 80 characters
> > #493: FILE: drivers/media/usb/msi2500/msi2500.c:493:
> > + unsigned long msecs = jiffies_to_msecs(jiffies_now) - jiffies_to_msecs(s->jiffies_next);
> >
> > Same here.
> >
> > WARNING: Missing a blank line after declarations
> > #495: FILE: drivers/media/usb/msi2500/msi2500.c:495:
> > + unsigned int samples = sample_num[i_max - 1] - s->sample;
> > + s->jiffies_next = jiffies_now;
> >
> > ERROR: space required after that ';' (ctx:VxV)
> > #515: FILE: drivers/media/usb/msi2500/msi2500.c:515:
> > + struct {signed int x:14;} se;
> > ^
> >
> > Better to declare it as:
> > struct {
> > signed int x:14;
> > } se;
> >
> > That makes easier to read, IMHO, and follows Kernel CodingStyle.
> >
> > WARNING: line over 80 characters
> > #521: FILE: drivers/media/usb/msi2500/msi2500.c:521:
> > + sample_num[i] = src[3] << 24 | src[2] << 16 | src[1] << 8 | src[0] << 0;
> >
> > WARNING: Missing a blank line after declarations
> > #567: FILE: drivers/media/usb/msi2500/msi2500.c:567:
> > + unsigned int samples = sample_num[i_max - 1] - s->sample;
> > + s->jiffies_next = jiffies + msecs_to_jiffies(MSECS);
> >
> > WARNING: Missing a blank line after declarations
> > #661: FILE: drivers/media/usb/msi2500/msi2500.c:661:
> > + int i;
> > + dev_dbg(&s->udev->dev, "%s:\n", __func__);
> >
> > WARNING: Missing a blank line after declarations
> > #676: FILE: drivers/media/usb/msi2500/msi2500.c:676:
> > + int i;
> > + dev_dbg(&s->udev->dev, "%s:\n", __func__);
> >
> > WARNING: Missing a blank line after declarations
> > #709: FILE: drivers/media/usb/msi2500/msi2500.c:709:
> > + int i, j, ret;
> > + dev_dbg(&s->udev->dev, "%s:\n", __func__);
> >
> > WARNING: Missing a blank line after declarations
> > #775: FILE: drivers/media/usb/msi2500/msi2500.c:775:
> > + unsigned long flags = 0;
> > + dev_dbg(&s->udev->dev, "%s:\n", __func__);
> >
> > WARNING: Missing a blank line after declarations
> > #814: FILE: drivers/media/usb/msi2500/msi2500.c:814:
> > + struct msi3101_state *s = video_drvdata(file);
> > + dev_dbg(&s->udev->dev, "%s:\n", __func__);
> >
> > WARNING: Missing a blank line after declarations
> > #831: FILE: drivers/media/usb/msi2500/msi2500.c:831:
> > + struct msi3101_state *s = vb2_get_drv_priv(vq);
> > + dev_dbg(&s->udev->dev, "%s: *nbuffers=%d\n", __func__, *nbuffers);
> >
> > WARNING: line over 80 characters
> > #879: FILE: drivers/media/usb/msi2500/msi2500.c:879:
> > + _i & 0xff, _i >> 8, l & 0xff, l >> 8, direction, l, b); \
> >
> > WARNING: line over 80 characters
> > #915: FILE: drivers/media/usb/msi2500/msi2500.c:915:
> > + bandwidth_auto = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH_AUTO);
> >
> > WARNING: line over 80 characters
> > #917: FILE: drivers/media/usb/msi2500/msi2500.c:917:
> > + bandwidth = v4l2_ctrl_find(&s->hdl, V4L2_CID_RF_TUNER_BANDWIDTH);
> >
> > WARNING: line over 80 characters
> > #1012: FILE: drivers/media/usb/msi2500/msi2500.c:1012:
> > + __func__, f_sr, f_vco, div_n, div_m, div_r_out, reg3, reg4);
> >
> > WARNING: Missing a blank line after declarations
> > #1053: FILE: drivers/media/usb/msi2500/msi2500.c:1053:
> > + int ret;
> > + dev_dbg(&s->udev->dev, "%s:\n", __func__);
> >
> > WARNING: Missing a blank line after declarations
> > #1116: FILE: drivers/media/usb/msi2500/msi2500.c:1116:
> > + struct msi3101_state *s = video_drvdata(file);
> > + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, f->index);
> >
> > WARNING: Missing a blank line after declarations
> > #1131: FILE: drivers/media/usb/msi2500/msi2500.c:1131:
> > + struct msi3101_state *s = video_drvdata(file);
> > + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
> >
> > WARNING: Missing a blank line after declarations
> > #1146: FILE: drivers/media/usb/msi2500/msi2500.c:1146:
> > + int i;
> > + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
> >
> > WARNING: Missing a blank line after declarations
> > #1171: FILE: drivers/media/usb/msi2500/msi2500.c:1171:
> > + int i;
> > + dev_dbg(&s->udev->dev, "%s: pixelformat fourcc %4.4s\n", __func__,
> >
> > WARNING: Missing a blank line after declarations
> > #1190: FILE: drivers/media/usb/msi2500/msi2500.c:1190:
> > + int ret;
> > + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
> >
> > WARNING: Missing a blank line after declarations
> > #1206: FILE: drivers/media/usb/msi2500/msi2500.c:1206:
> > + int ret;
> > + dev_dbg(&s->udev->dev, "%s: index=%d\n", __func__, v->index);
> >
> > WARNING: Missing a blank line after declarations
> > #1229: FILE: drivers/media/usb/msi2500/msi2500.c:1229:
> > + int ret = 0;
> > + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d\n",
> >
> > WARNING: Missing a blank line after declarations
> > #1250: FILE: drivers/media/usb/msi2500/msi2500.c:1250:
> > + int ret;
> > + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d frequency=%u\n",
> >
> > WARNING: Missing a blank line after declarations
> > #1274: FILE: drivers/media/usb/msi2500/msi2500.c:1274:
> > + int ret;
> > + dev_dbg(&s->udev->dev, "%s: tuner=%d type=%d index=%d\n",
> >
> > total: 1 errors, 45 warnings, 1517 lines checked
> >
> > drivers/media/usb/msi2500/msi2500.c has style problems, please review.
> >
> > Regards,
> > Mauro
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-22 0:51 ` Mauro Carvalho Chehab
@ 2014-07-22 1:05 ` Antti Palosaari
2014-07-22 2:09 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 11+ messages in thread
From: Antti Palosaari @ 2014-07-22 1:05 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: LMML, Hans Verkuil
On 07/22/2014 03:51 AM, Mauro Carvalho Chehab wrote:
> Em Tue, 22 Jul 2014 03:08:19 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> So what. Those were mostly WARNING only and all but long lines were some
>> new checks added to checkpatch recently. chekcpatch gets all the time
>> new and new checks, these were added after I have made that driver. I
>> will surely clean those later when I do some new changes to driver and
>> my checkpatch updates.
>
> Antti,
>
> I think you didn't read my comments in the middle of the checkpatch stuff.
> Please read my email again. I'm not requiring you to fix the newer checkpatch
> warning (Missing a blank line after declarations), and not even about the
> 80-cols warning. The thing is that there are two issues there:
>
> 1) you're adding API bits at msi2500 driver, instead of moving them
> to videodev2.h (or reusing the fourcc types you already added there);
If you look inside driver code, you will see those defines are not used
- but commented out. It is simply dead definition compiler optimizes
away. It is code I used on my tests, but finally decided to comment out
to leave some time add those later to API. I later moved 2 of those to
API, that is done in same patch serie.
No issue here.
> 2) you're handling jiffies wrong inside the driver.
>
> As you may know, adding a driver at staging is easier than at the main
> tree, as we don't care much about checkpatch issues (and not even about
> some more serious issues). However, when moving stuff out of staging,
> we review the entire driver again, to be sure that it is ok.
That jiffie check is also rather new and didn't exists time drive was
done. Jiffie is used to calculate debug sample rate. There is multiple
times very similar code piece, which could be optimized to one. My plan
merge all those ~5 functions to one and use jiffies using macros as
checkpatch now likes. I don't see meaningful fix it now as you are going
to rewrite that stuff in near future in any case.
Silencing all those checkpatch things is not very hard job though. If
you merge that stuff to media/master I can do it right away (I am
running older kernel and older checkpatch currently).
regards
Antti
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-22 1:05 ` Antti Palosaari
@ 2014-07-22 2:09 ` Mauro Carvalho Chehab
2014-07-22 2:48 ` Antti Palosaari
0 siblings, 1 reply; 11+ messages in thread
From: Mauro Carvalho Chehab @ 2014-07-22 2:09 UTC (permalink / raw)
To: Antti Palosaari; +Cc: LMML, Hans Verkuil
Em Tue, 22 Jul 2014 04:05:05 +0300
Antti Palosaari <crope@iki.fi> escreveu:
> On 07/22/2014 03:51 AM, Mauro Carvalho Chehab wrote:
> > Em Tue, 22 Jul 2014 03:08:19 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:
> >
> >> So what. Those were mostly WARNING only and all but long lines were some
> >> new checks added to checkpatch recently. chekcpatch gets all the time
> >> new and new checks, these were added after I have made that driver. I
> >> will surely clean those later when I do some new changes to driver and
> >> my checkpatch updates.
> >
> > Antti,
> >
> > I think you didn't read my comments in the middle of the checkpatch stuff.
> > Please read my email again. I'm not requiring you to fix the newer checkpatch
> > warning (Missing a blank line after declarations), and not even about the
> > 80-cols warning. The thing is that there are two issues there:
> >
> > 1) you're adding API bits at msi2500 driver, instead of moving them
> > to videodev2.h (or reusing the fourcc types you already added there);
>
> If you look inside driver code, you will see those defines are not used
> - but commented out. It is simply dead definition compiler optimizes
> away. It is code I used on my tests, but finally decided to comment out
> to leave some time add those later to API. I later moved 2 of those to
> API, that is done in same patch serie.
>
> No issue here.
>
> > 2) you're handling jiffies wrong inside the driver.
> >
> > As you may know, adding a driver at staging is easier than at the main
> > tree, as we don't care much about checkpatch issues (and not even about
> > some more serious issues). However, when moving stuff out of staging,
> > we review the entire driver again, to be sure that it is ok.
>
> That jiffie check is also rather new and didn't exists time drive was
> done. Jiffie is used to calculate debug sample rate. There is multiple
> times very similar code piece, which could be optimized to one. My plan
> merge all those ~5 functions to one and use jiffies using macros as
> checkpatch now likes. I don't see meaningful fix it now as you are going
> to rewrite that stuff in near future in any case.
Ok, I'll apply the remaining patches.
> Silencing all those checkpatch things is not very hard job though. If
> you merge that stuff to media/master I can do it right away (I am
> running older kernel and older checkpatch currently).
FYI, I always use the checkpatch available on our tree, no matter what
Kernel I'm running. My scripts just call ./scripts/checkpatch.pl.
Regards,
Mauro
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-22 2:09 ` Mauro Carvalho Chehab
@ 2014-07-22 2:48 ` Antti Palosaari
2014-07-22 4:27 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 11+ messages in thread
From: Antti Palosaari @ 2014-07-22 2:48 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: LMML, Hans Verkuil
On 07/22/2014 05:09 AM, Mauro Carvalho Chehab wrote:
> Em Tue, 22 Jul 2014 04:05:05 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> On 07/22/2014 03:51 AM, Mauro Carvalho Chehab wrote:
>>> Em Tue, 22 Jul 2014 03:08:19 +0300
>>> Antti Palosaari <crope@iki.fi> escreveu:
>>>
>>>> So what. Those were mostly WARNING only and all but long lines were some
>>>> new checks added to checkpatch recently. chekcpatch gets all the time
>>>> new and new checks, these were added after I have made that driver. I
>>>> will surely clean those later when I do some new changes to driver and
>>>> my checkpatch updates.
>>>
>>> Antti,
>>>
>>> I think you didn't read my comments in the middle of the checkpatch stuff.
>>> Please read my email again. I'm not requiring you to fix the newer checkpatch
>>> warning (Missing a blank line after declarations), and not even about the
>>> 80-cols warning. The thing is that there are two issues there:
>>>
>>> 1) you're adding API bits at msi2500 driver, instead of moving them
>>> to videodev2.h (or reusing the fourcc types you already added there);
>>
>> If you look inside driver code, you will see those defines are not used
>> - but commented out. It is simply dead definition compiler optimizes
>> away. It is code I used on my tests, but finally decided to comment out
>> to leave some time add those later to API. I later moved 2 of those to
>> API, that is done in same patch serie.
>>
>> No issue here.
>>
>>> 2) you're handling jiffies wrong inside the driver.
>>>
>>> As you may know, adding a driver at staging is easier than at the main
>>> tree, as we don't care much about checkpatch issues (and not even about
>>> some more serious issues). However, when moving stuff out of staging,
>>> we review the entire driver again, to be sure that it is ok.
>>
>> That jiffie check is also rather new and didn't exists time drive was
>> done. Jiffie is used to calculate debug sample rate. There is multiple
>> times very similar code piece, which could be optimized to one. My plan
>> merge all those ~5 functions to one and use jiffies using macros as
>> checkpatch now likes. I don't see meaningful fix it now as you are going
>> to rewrite that stuff in near future in any case.
>
> Ok, I'll apply the remaining patches.
>
>> Silencing all those checkpatch things is not very hard job though. If
>> you merge that stuff to media/master I can do it right away (I am
>> running older kernel and older checkpatch currently).
>
> FYI, I always use the checkpatch available on our tree, no matter what
> Kernel I'm running. My scripts just call ./scripts/checkpatch.pl.
I would like to also run/use *always* media/master, but it is not
usually possible. Let me list here all the issues which makes it
impossible in practice (issues which I see on current development process):
1) media/master is far behind Linus kernel master tree. It is usually
updated only 2 times per RC cycle, RC1 and around RC5. Even I want test
upstream RC, that makes it impossible.
2) RC1 is very often (more than 50%) unusable. It is simply too buggy.
For example 3.16 there was GPU related problems at least (and again).
3) I send one fix (PULL requested) for brand new Silicon Labs Si2168
driver on 2014-06-15. That was just before RC1 was released. It was
merged to media/fixes, but not media/master. "Unfortunately" that driver
gets very much interest and multiple other developers started hacking
with it, adding support for new hw and so. There was huge headache to
lead that development because code base was divided to 2 upstream git
trees; one in media/fixes and another in media/development. Critical and
problematic patch was still only in media/fixes, leaving media/master
driver broken. So that forces me to use media/fixes as a base and add
new patches top of it. Media/fixes is 3.15 whilst media/master is 3.16.
I mentioned that already you and I have got bug reports too, latest one
6 hours ago reporting his PCTV 292e stopped working...
So could it be possible to address those issues? Like media/master
contains all latest upstream patches and is updated/rebased weekly top
of Linus main tree?
regards
Antti
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] SDR stuff
2014-07-22 2:48 ` Antti Palosaari
@ 2014-07-22 4:27 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 11+ messages in thread
From: Mauro Carvalho Chehab @ 2014-07-22 4:27 UTC (permalink / raw)
To: Antti Palosaari; +Cc: LMML, Hans Verkuil
Em Tue, 22 Jul 2014 05:48:26 +0300
Antti Palosaari <crope@iki.fi> escreveu:
> On 07/22/2014 05:09 AM, Mauro Carvalho Chehab wrote:
> > Em Tue, 22 Jul 2014 04:05:05 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:
> >
> >> On 07/22/2014 03:51 AM, Mauro Carvalho Chehab wrote:
> >>> Em Tue, 22 Jul 2014 03:08:19 +0300
> >>> Antti Palosaari <crope@iki.fi> escreveu:
> >>>
> >>>> So what. Those were mostly WARNING only and all but long lines were some
> >>>> new checks added to checkpatch recently. chekcpatch gets all the time
> >>>> new and new checks, these were added after I have made that driver. I
> >>>> will surely clean those later when I do some new changes to driver and
> >>>> my checkpatch updates.
> >>>
> >>> Antti,
> >>>
> >>> I think you didn't read my comments in the middle of the checkpatch stuff.
> >>> Please read my email again. I'm not requiring you to fix the newer checkpatch
> >>> warning (Missing a blank line after declarations), and not even about the
> >>> 80-cols warning. The thing is that there are two issues there:
> >>>
> >>> 1) you're adding API bits at msi2500 driver, instead of moving them
> >>> to videodev2.h (or reusing the fourcc types you already added there);
> >>
> >> If you look inside driver code, you will see those defines are not used
> >> - but commented out. It is simply dead definition compiler optimizes
> >> away. It is code I used on my tests, but finally decided to comment out
> >> to leave some time add those later to API. I later moved 2 of those to
> >> API, that is done in same patch serie.
> >>
> >> No issue here.
> >>
> >>> 2) you're handling jiffies wrong inside the driver.
> >>>
> >>> As you may know, adding a driver at staging is easier than at the main
> >>> tree, as we don't care much about checkpatch issues (and not even about
> >>> some more serious issues). However, when moving stuff out of staging,
> >>> we review the entire driver again, to be sure that it is ok.
> >>
> >> That jiffie check is also rather new and didn't exists time drive was
> >> done. Jiffie is used to calculate debug sample rate. There is multiple
> >> times very similar code piece, which could be optimized to one. My plan
> >> merge all those ~5 functions to one and use jiffies using macros as
> >> checkpatch now likes. I don't see meaningful fix it now as you are going
> >> to rewrite that stuff in near future in any case.
> >
> > Ok, I'll apply the remaining patches.
> >
> >> Silencing all those checkpatch things is not very hard job though. If
> >> you merge that stuff to media/master I can do it right away (I am
> >> running older kernel and older checkpatch currently).
> >
> > FYI, I always use the checkpatch available on our tree, no matter what
> > Kernel I'm running. My scripts just call ./scripts/checkpatch.pl.
>
> I would like to also run/use *always* media/master, but it is not
> usually possible. Let me list here all the issues which makes it
> impossible in practice (issues which I see on current development process):
>
> 1) media/master is far behind Linus kernel master tree. It is usually
> updated only 2 times per RC cycle, RC1 and around RC5. Even I want test
> upstream RC, that makes it impossible.
Hmm... you complained before that I was using a too new version of
checkpatch... Now you're complaining about just the opposite...
Anyway, nothing prevents you to pull from both Linus and my tree
at least for your testing purposes.
>
> 2) RC1 is very often (more than 50%) unusable. It is simply too buggy.
> For example 3.16 there was GPU related problems at least (and again).
>
> 3) I send one fix (PULL requested) for brand new Silicon Labs Si2168
> driver on 2014-06-15. That was just before RC1 was released. It was
> merged to media/fixes, but not media/master. "Unfortunately" that driver
> gets very much interest and multiple other developers started hacking
> with it, adding support for new hw and so. There was huge headache to
> lead that development because code base was divided to 2 upstream git
> trees; one in media/fixes and another in media/development. Critical and
> problematic patch was still only in media/fixes, leaving media/master
> driver broken. So that forces me to use media/fixes as a base and add
> new patches top of it. Media/fixes is 3.15 whilst media/master is 3.16.
> I mentioned that already you and I have got bug reports too, latest one
> 6 hours ago reporting his PCTV 292e stopped working...
>
>
> So could it be possible to address those issues? Like media/master
> contains all latest upstream patches and is updated/rebased weekly top
> of Linus main tree?
If you take a look at other big subsystems (x86 tip, arm, network, ...)
most of them are even more fragmented: they use topic branches, e. g.
each "conceptual" patchset goes to its own branch.
One big advantage of topic branches is that maintainers can send
pull requests from each topic directly to Linus, avoiding the risk
that one broken topic would harm the entire patch merging.
I use topic branch on some cases, but v4l2 core changes too often
to allow doing it on a broader scale.
My main concern, as maintainer, is to be sure that the patches will
flow fine during the merge window, so my flow is optimized for this
task.
Also, Linus hates to see lots of merging back from his tree at the
Maintainers trees, because it makes the patch history dirty.
So, what I do is that I merge back from his tree after he pulls
a pull request that I send him, and when such pull request is required
(e. g. a latter patch depends on something changed at fixes).
What I do on the drivers I develop is that I don't always base
on the master branch of my tree. Sometimes, I pull from both
fixes and master, and sometimes I pull from upstream. Whatever
works best for the driver I'm working with.
On my development flow, when I have something ready to be sent is that
I send the patches to the ML from whatever tree I use, then I add
them (or I rebase when possible) on a separate branch, based on my
master tree (or fixes when this is the case).
That's said, it makes sense to have one branch that merges both
fixes and master, and use such branch for the media_build tree
to be based. I may try to work on that after the merge window,
in order to help people to test our drivers, but sending pull
requests based on such tree can be complex, as patches from fixes
could end by being merged twice, and rebasing on a merged tree
won't work properly, as it create newer hashes for the already
existing patches.
>
> regards
> Antti
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-07-22 4:27 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-18 1:14 [GIT PULL] SDR stuff Antti Palosaari
2014-07-20 16:41 ` Antti Palosaari
2014-07-21 23:50 ` Mauro Carvalho Chehab
2014-07-22 0:05 ` Mauro Carvalho Chehab
2014-07-22 0:16 ` Antti Palosaari
2014-07-22 0:08 ` Antti Palosaari
2014-07-22 0:51 ` Mauro Carvalho Chehab
2014-07-22 1:05 ` Antti Palosaari
2014-07-22 2:09 ` Mauro Carvalho Chehab
2014-07-22 2:48 ` Antti Palosaari
2014-07-22 4:27 ` Mauro Carvalho Chehab
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.