lkml.org 
[lkml]   [2016]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH v0] Add tw5864 driver
From
Date
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



\
 
 \ /
  Last update: 2016-01-03 05:21    [W:0.381 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site