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