Re: [PATCH] [media]: Driver for Toshiba et8ek8 5MP sensor

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 05/24/2016 01:19 PM, Pavel Machek wrote:
>> +/*
>> > + * Write a list of registers to i2c device.
>> > + *
>> > + * The list of registers is terminated by ET8EK8_REG_TERM.
>> > + * Returns zero if successful, or non-zero otherwise.
>> > + */
>> > +static int et8ek8_i2c_write_regs(struct i2c_client *client,
>> > +				 const struct et8ek8_reg reglist[])
>> > +{
>> > +	int r, cnt = 0;
>> > +	const struct et8ek8_reg *next, *wnext;
>> > +
>> > +	if (!client->adapter)
>> > +		return -ENODEV;
>> > +
>> > +	if (reglist == NULL)
>
> (!reglist) ? :-). Actually, you can keep your preffered style there,
> but maybe ammount of if (something that can not happen) return
> ... should be reduced. Noone should ever call this without valid
> reglist or client->adapter, right?
> 
>> > +		return -EINVAL;
>> > +
>> > +	/* Initialize list pointers to the start of the list */
>> > +	next = wnext = reglist;
>> > +
>> > +	do {
>> > +		/*
>> > +		 * We have to go through the list to figure out how
>> > +		 * many regular writes we have in a row
>> > +		 */
>> > +		while (next->type != ET8EK8_REG_TERM
>> > +		       && next->type != ET8EK8_REG_DELAY) {
>> > +			/*
>> > +			 * Here we check that the actual length fields
>> > +			 * are valid
>> > +			 */
>> > +			if (next->type != ET8EK8_REG_8BIT
>> > +			    &&  next->type != ET8EK8_REG_16BIT) {
> Extra space after &&
> 
>> > +				dev_err(&client->dev,
>> > +					"Invalid value on entry %d 0x%x\n",
>> > +					cnt, next->type);
>> > +				return -EINVAL;
>> > +			}
>
> And maybe this could be just BUG_ON(). 

It definitively doesn't look like a BUG_ON() would be appropriate here,
it's just an unexpected condition in some I2C write function of a rather
not critical device from the whole system operation stability point
of view. Perhaps you just meant WARN_ON()?

BUG_ON() should be used with care, when the condition is not recoverable,
otherwise we are just making debugging unnecessarily harder.

http://lkml.iu.edu/hypermail/linux/kernel/1506.1/00062.html
http://yarchive.net/comp/linux/BUG.html

-- 
Thanks,
Sylwester