From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751390AbcACDsD (ORCPT ); Sat, 2 Jan 2016 22:48:03 -0500 Received: from smtprelay0239.hostedemail.com ([216.40.44.239]:35949 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751054AbcACDr7 (ORCPT ); Sat, 2 Jan 2016 22:47:59 -0500 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,joe@perches.com,:::::::::::::::::::::,RULES_HIT:41:355:379:421:541:599:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1535:1544:1593:1594:1605:1711:1730:1747:1777:1792:2393:2559:2562:2828:3138:3139:3140:3141:3142:3622:3743:3866:3867:3872:4321:5007:6119:6120:6261:7875:7903:8603:10004:10848:11026:11232:11473:11657:11658:11783:11914:12043:12048:12438:12517:12519:12555:12740:13894:14659:21080:30029:30054:30080:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:1,LUA_SUMMARY:none X-HE-Tag: jeans66_7f5e25517a25e X-Filterd-Recvd-Size: 5603 Message-ID: <1451792869.4334.33.camel@perches.com> Subject: Re: [RFC PATCH v0] Add tw5864 driver From: Joe Perches To: Andrey Utkin , Linux Media , "linux-kernel@vger.kernel.org" , "kernel-mentors@selenic.com" , devel@driverdev.osuosl.org, kernel-janitors , Hans Verkuil , Mauro Carvalho Chehab , Ezequiel Garcia , Greg Kroah-Hartman Cc: Andrey Utkin Date: Sat, 02 Jan 2016 19:47:49 -0800 In-Reply-To: <1451785302-3173-1-git-send-email-andrey.utkin@corp.bluecherry.net> References: <1451785302-3173-1-git-send-email-andrey.utkin@corp.bluecherry.net> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.18.3-1ubuntu1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 2016-01-03 at 03:41 +0200, Andrey Utkin wrote: > (Disclaimer up to scissors mark) > > Please be so kind to take a look at a new driver. trivial comments only: > diff --git a/drivers/staging/media/tw5864/tw5864-bs.h b/drivers/staging/media/tw5864/tw5864-bs.h [] > +static inline int bs_pos(struct bs *s) > +{ > + return (8 * (s->p - s->p_start) + 8 - s->i_left); > +} several of these have unnecessary parentheses > +static inline int bs_eof(struct bs *s) > +{ > + return (s->p >= s->p_end ? 1 : 0); > +} Maybe use bool a bit more > +/* golomb functions */ > +static inline void bs_write_ue(struct bs *s, u32 val) > +{ > + int i_size = 0; > + static const int i_size0_255[256] = { Maybe use s8 instead of int to reduce object size > + 1, 1, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5, > + 5, 5, > + 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, > + 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, Perhaps it'd be clearer to use gcc's ranged initializer syntax [  0 ...   1] = 1, [  2 ...   3] = 2, [  4 ...   7] = 3, [  8 ...  15] = 4, [ 16 ...  31] = 5, [ 32 ...  63] = 6, etc... or maybe just use fls > +static inline int bs_size_ue(unsigned int val) > +{ > + int i_size = 0; > + static const int i_size0_254[255] = { Same sort of thing > diff --git a/drivers/staging/media/tw5864/tw5864-config.c b/drivers/staging/media/tw5864/tw5864-config.c [] > +u8 tw_indir_readb(struct tw5864_dev *dev, u16 addr) > +{ > + int timeout = 30000; misleading name, retries would be more proper, or maybe use real timed loops. > + u32 data = 0; > + > + addr <<= 2; > + > + while ((tw_readl(TW5864_IND_CTL) >> 31) && (timeout--)) > + ; > + if (!timeout) > + dev_err(&dev->pci->dev, > + "tw_indir_writel() timeout before reading\n"); > + > + tw_writel(TW5864_IND_CTL, addr | TW5864_ENABLE); > + > + timeout = 30000; > + while ((tw_readl(TW5864_IND_CTL) >> 31) && (timeout--)) > + ; > + if (!timeout) > + dev_err(&dev->pci->dev, > + "tw_indir_writel() timeout at reading\n"); > + > + data = tw_readl(TW5864_IND_DATA); > + return data & 0xff; > +} [] > +static size_t regs_dump(struct tw5864_dev *dev, char *buf, size_t size) > +{ > + size_t count = 0; > + > + u32 reg_addr; > + u32 value; > + > + for (reg_addr = 0x0000; (count < size) && (reg_addr <= 0x2FFC); > +      reg_addr += 4) { > + value = tw_readl(reg_addr); > + count += scnprintf(buf + count, size - count, > +    "[0x%05x] = 0x%08x\n", reg_addr, value); > + } > + > + for (reg_addr = 0x4000; (count < size) && (reg_addr <= 0x4FFC); > +      reg_addr += 4) { > + value = tw_readl(reg_addr); > + count += scnprintf(buf + count, size - count, > +    "[0x%05x] = 0x%08x\n", reg_addr, value); > + } > + > + for (reg_addr = 0x8000; (count < size) && (reg_addr <= 0x180DC); > +      reg_addr += 4) { > + value = tw_readl(reg_addr); > + count += scnprintf(buf + count, size - count, > +    "[0x%05x] = 0x%08x\n", reg_addr, value); > + } > + > + for (reg_addr = 0x18100; (count < size) && (reg_addr <= 0x1817C); > +      reg_addr += 4) { > + value = tw_readl(reg_addr); > + count += scnprintf(buf + count, size - count, > +    "[0x%05x] = 0x%08x\n", reg_addr, value); > + } > + > + for (reg_addr = 0x80000; (count < size) && (reg_addr <= 0x87FFF); > +      reg_addr += 4) { > + value = tw_readl(reg_addr); > + count += scnprintf(buf + count, size - count, > +    "[0x%05x] = 0x%08x\n", reg_addr, value); > + } This seems a little repetitive. > diff --git a/drivers/staging/media/tw5864/tw5864-tables.h b/drivers/staging/media/tw5864/tw5864-tables.h [] > +static const u32 forward_quantization_table[QUANTIZATION_TABLE_LEN] = { u16? > + static const struct v4l2_ctrl_config tw5864_md_thresholds = { > + .ops = &tw5864_ctrl_ops, > + .id = V4L2_CID_DETECT_MD_THRESHOLD_GRID, > + .dims = {MD_CELLS_HOR, MD_CELLS_VERT}, > + .def = 14, > + /* See tw5864_md_metric_from_mvd() */ > + .max = 2 * 0x0f, > + .step = 1, > + }; odd indentation > +#ifdef DEBUG > + dev_dbg(&input->root->pci->dev, > + "input %d, frame md stats: min %u, max %u, avg %u, cells above threshold: %u\n", > + input->input_number, min, max, sum / md_cells, > + cnt_above_thresh); > +#endif unnecessary #ifdef