-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/13/2014 02:38 PM, Wolfram Sang wrote:
> Hi,
>
> Thank you for this submission! Wow, this is a lot of code and I
> have only very minor comments. Good work and thanks to all
> previous reviewers!
>
>> @@ -0,0 +1,1415 @@ +/* + * I2C adapter for the IMG Serial Control
>> Bus (SCB) IP block. + * + * Copyright (C) 2009, 2010, 2012, 2014
>> Imagination Technologies Ltd. + * + * This program is free
>> software; you can redistribute it and/or modify + * it under the
>> terms of the GNU General Public License version 2 as + *
>> published by the Free Software Foundation. + * + * There are
>> three ways that this I2C controller can be driven:
>
> Interesting beast. Offers a lot of possibilities and complexity :)
>
>> + * Notice that the driver implements a timer-based timeout
>> mechanism. + * This is done to avoid slave events interrupts in
>> automatic mode, + * given the driver gets a slave event and
>> transaction done interrupts + * for each atomic mode command
>> (start, data, ack, stop, etc) that gets + * completed, none of
>> which are of interest when using automatic mode + * since those
>> atomic mode commands are managed automatically by hardware + *
>> rather than by the I2C state machine in the interrupt handler.
>
> I read this three times and still have no clear picture. Please
> rephrase. Smaller sentences. Simple ones.
>
Ugh.. that sentence is definitely too long! I'll rephrase.
>
> ... all down to probe()
>
>> + i2c_set_adapdata(&i2c->adap, i2c); + i2c->adap.dev.parent =
>> &pdev->dev; + i2c->adap.dev.of_node = node; + i2c->adap.owner =
>> THIS_MODULE; + i2c->adap.class = I2C_CLASS_HWMON |
>> I2C_CLASS_SPD;
>
> You probably won't need this one. You have DT probing.
>
OK.
>> + i2c->adap.algo = &img_i2c_algo; + i2c->adap.retries = 5; +
>> i2c->adap.nr = pdev->id; + snprintf(i2c->adap.name,
>> sizeof(i2c->adap.name), + "IMG i2c%d", i2c->adap.nr);
>
> Please use a static name, like "IMG <some type> I2C". This
> describes the IP block, not the bus in use.
>
OK.
> Very close to go...
>
> checkpatch --strict says:
>
> CHECK: Please don't use multiple blank lines #234: FILE:
> drivers/i2c/busses/i2c-img-scb.c:181: + +
>
> CHECK: Blank lines aren't necessary before a close brace '}' #621:
> FILE: drivers/i2c/busses/i2c-img-scb.c:568: + + }
>
> CHECK: braces {} should be used on all arms of this statement #810:
> FILE: drivers/i2c/busses/i2c-img-scb.c:757: + if (i2c->msg.len ==
> 0) [...] + else if (i2c->msg.flags & I2C_M_RD) [...] + else {
> [...]
>
> And coccicheck:
>
> drivers/i2c/busses/i2c-img-scb.c:510:1-12: WARNING: Assignment of
bool to 0/1
>
OK, fixed all of these now.
Thanks,
- --
Ezequiel
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBAgAGBQJUZPADAAoJEIOKbhOEIHKiPzQQAIjtDqsjpWZ9HxZzL26Jy2/j
DYct0t2wlo27Q2Y1GHjd34Or3ifTzPApAiOeD8cruY21JgEFv7nlw7isZM8yvd5M
NUNTxfgaAyiJ4ikEnfRioUm+aRabBv1WLWSBmW1PQ6dnpq+8Qe+PIoiQo4MY/sWZ
xBoxLuLpx9lSjhF+GedhI8q732hZrtAhcPVR3Lt44G36ArixLqGRcEaIwmrdtSNC
bUYJIkv7uKIAyyezMtxzUATAY6ZbaXNYwAGfSPoBvgff2gCuONHdpmGJl+5SCI+Y
CqIFw4TCgK5Rx+VNvthIIlFj4TQkKQd1vZBuF5IX4gEnNkyyUTYb4kiF/8KAe5rm
5fV4lp4aQmXjsQpos2A5qsO9LH058+9HdamRgbv68NLKopOrGRdvGEXVRC9quba3
y8EJ6gHphamMcYNd9VAaA+L1FVCsACPhUuJddUNmukmKSODeIAGAzfnXPmeoYcZo
6JNUKXyL/ouwhWA1jPe/FlBEmD77vqu1DBmO/5DlhNmYT6qiIA8Y32tJG5EsdCjb
uaq5inMSgMshFP+P3mbMWp/V7puogLxaaqSoZNIfi3sIfR5Fsad86eu+rlYV8VXR
byz05zurRZZyX0Eo9azvLC5vkCnJL0oXXULETmRwTXbHj6rRajsao5cjuU27c6Pm
uTsvJHndhCaC7S2Z1rLi
=YwwK
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
