Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300170 > unrolled thread

Re: [RFC PATCH v0] Add tw5864 driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v0] Add tw5864 driver Joe Perches <joe@perches.com> - 2016-01-03 04:50 +0100
    Re: [RFC PATCH v0] Add tw5864 driver Andrey Utkin <andrey.utkin@corp.bluecherry.net> - 2016-01-03 14:40 +0100

#1300170 — Re: [RFC PATCH v0] Add tw5864 driver

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300301

On Sun, Jan 3, 2016 at 5:47 AM, Joe Perches <joe@perches.com> wrote:
> several of these have unnecessary parentheses

Thanks, fixed.

> Maybe use bool a bit more

Thanks, fixed.

> or maybe just use fls

Thanks, fls() fit greatly, rewritten the function with compatibility testing.

>> +static inline int bs_size_ue(unsigned int val)
>> +{
>> +     int i_size = 0;
>> +     static const int i_size0_254[255] = {
>
> Same sort of thing

Dropped this procedure because it is not used.
Thanks.

>> 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.

Thanks, renamed to "retries".

> This seems a little repetitive.

Thanks, reworked.

> u16?

Thanks, fixed.

> odd indentation

Indeed. For some mysterious reason, vim + syntastic insists on this way. Fixed.

>> +#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

Not quite. This debug printout works with variables which are declared
in another "#ifdef DEBUG" clause. And it turns out that dev_dbg is
compiled not only if DEBUG is declared, so when I remove this ifdef, I
get "undefined variable" errors. It seems it is compiled if this
condition is met:

#if (defined DEBUG) || (defined CONFIG_DYNAMIC_DEBUG)

so I can wrap my stats variables into this statement instead. But such
change is not equivalent - I guess CONFIG_DYNAMIC_DEBUG is common to
be enabled, so debug stats will be always calculated, even when module
is not under debug. Except if I use DEFINE_DYNAMIC_DEBUG_METADATA etc.
in my code. Please let me know if this can be sorted out in cleaner
way.



On Sun, Jan 3, 2016 at 7:38 AM, Leon Romanovsky <leon@leon.nu> wrote:
> On Sun, Jan 03, 2016 at 03:41:42AM +0200, Andrey Utkin wrote:
> ....
>> +/*
>> + *  TW5864 driver - Exp-Golomb code functions
>> + *
>> + *  Copyright (C) 2015 Bluecherry, LLC <maintainers@bluecherrydvr.com>
>> + *  Copyright (C) 2015 Andrey Utkin <andrey.utkin@corp.bluecherry.net>
>
> I doubt that you have contract with your employer which permits you to
> claim copyright on the work/product.

Thank you for commenting.
I have previously asked my employer to review copyright statment, and
he told this is fine.
Now I have requrested him again with reference to your comment.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web