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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
