Re: [GIT PULL] SDR stuff

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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@xxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux