On Fri, 14 Dec 2007 22:24:02 +0530, "Gadiyar, Anand" <gadiyar@xxxxxx>
wrote:
>> > Index: linux-omap-dec10/drivers/i2c/chips/Makefile
>> > ===================================================================
>> > --- linux-omap-dec10.orig/drivers/i2c/chips/Makefile
>> 2007-12-14
>> > 19:06:41.112941439 +0530
>> > +++ linux-omap-dec10/drivers/i2c/chips/Makefile 2007-12-14
>> > 19:12:45.826336129 +0530
>> > @@ -17,8 +17,9 @@
>> > obj-$(CONFIG_GPIOEXPANDER_OMAP) += gpio_expander_omap.o
>> > obj-$(CONFIG_MENELAUS) += menelaus.o
>> > obj-$(CONFIG_SENSORS_TSL2550) += tsl2550.o
>> > -obj-$(CONFIG_TWL4030_CORE) += twl4030_core.o
>> > +obj-$(CONFIG_TWL4030_CORE) += twl4030_core.o
>>
>> Changing spaces into tab?!? It should come as an extra-patch before or
>> after usb series.
>
> Can do. But is it really worth making it a separate patch, given that
this
> is
> not a functional change. If it were, I would gladly make it a separate
> patch.
>
> The idea behind separate patches is to keep functionally separate units
> physically separate. In this case, I don't think that extra patch is
going
> to
> add any extra value and the cost is the additional overhead of pushing
> it.
>
> In my opinion, the costs exceed the benefits and it is not worth the
> time.
> If you really insist, I'll make it a separate patch. (And this is not
> lazy developer syndrome. :-) )
Well there isn't additional overhead at all as Tony will probably run
"git-am mbox" and git will apply each patch by itself.
The idea on providing a separate patch is that change has nothing to do
with usb support on omap34xx. That's my opinion, let's wait someone else
comment on this.
I'd rather see a separate patch but if Tony or any other guy here is ok
with applying it together with your patch, I can live with it. ;-)
>
>
> <snip>
>
>> > +#define VBUS_DEBOUNCE 0xC0
>> > +#define ID_DEBOUNCE 0xC1
>> > +#define VBAT_TIMER 0xD3
>> > +#define PHY_PWR_CTRL 0xFD
>> > +#define PHY_PWR_PHYPWD (1 << 0)
>> > +#define PHY_CLK_CTRL 0xFE
>> > +#define PHY_CLK_CTRL_CLOCKGATING_EN (1 << 2)
>> > +#define PHY_CLK_CTRL_CLK32K_EN (1 << 1)
>>
>> there is extra white space here after PHY_CLK_CTRL_CLK32K_EN,
>> could you remove ?
>
> Will do. Thanks.
>
>
>
>> > +static int twl4030_i2c_write_u8_verify(u8 module, u8 data,
>> u8 address)
>> > +{
>> > + u8 check;
>>
>> Adding an extra blank line after variable definition increase code
>> readability.
>
> Agreed. Will fix it.
>
>
>> > +
>> > +struct twl4030_usb {
>> > + int irq;
>> > + u8 usb_mode; /* pin configuration */
>>
>> extra whitespace after u8.
>
> Will fix it.
>
>
>> > + if (twl4030_usb_write_verify(PHY_CLK_CTRL,
>> > + (u8)val) < 0) {
>> > + printk(KERN_ERR "twl4030_usb: i2c write failed,"
>> > + "line %d\n", __LINE__);
>>
>> The second line has no log level defined. Change it to:
>> printk(KERN_ERR "twl4030_usb: i2c write failed,"
>> KERN_ERR "line %d\n", __LINE__);
>
> This is wrong. I broke the string there and took advantage of string
> literal
> concatenation which is automatically done. The second KERN_ERR __should
> not__
> be used there. Ditto for all further comments on this.
hmm... yeah... that's right. My bad here. :-p
>
>
>
>> > +#if defined(CONFIG_TWL4030_USB_HS_ULPI)
>>
>> #ifdef CONFIG_TWL4030_USB_HS_ULPI
>
> Correct. Will fix.
>
>
>> > +static void __exit twl4030_usb_exit(void)
>> > +{
>> > + struct twl4030_usb *twl = the_transceiver;
>> > + int val;
>> > +
>> > + usb_irq_disable();
>> > + free_irq(twl->irq, twl);
>> > +
>> > + /* set transceiver mode to power on defaults */
>> > + twl4030_usb_set_mode(twl, -1);
>> > +
>> > + /* autogate 60MHz ULPI clock,
>> > + * clear dpll clock request for i2c access,
>> > + * disable 32KHz
>> > + */
>> > + val = twl4030_usb_read(PHY_CLK_CTRL);
>> > + if (val >= 0) {
>> > + val |= PHY_CLK_CTRL_CLOCKGATING_EN;
>> > + val &= ~(PHY_CLK_CTRL_CLK32K_EN | REQ_PHY_DPLL_CLK);
>> > + twl4030_usb_write(PHY_CLK_CTRL, (u8)val);
>> > + }
>> > +
>
>> I'm only looking at cosmetic here as I already tested the previous
> patch
>> and it was working fine. Another cosmetic I'd like to see (although I
> can
>> live without it) is a better variable name convention. "val" has no
> meaning
>> at all, better names would even help us searching data sheets, TRMs and
>> other hw documents.
>
> Point taken overall. A better naming convention is a good thing to have.
>
> However, in this case, "val" is just a temporary variable that really
> doesn't
> need to be called anything else. When you assign PHY_CLK_CTRL to val,
that
> ought
> to give one enough of a clue as to what is happening.
>
> Thanks for taking the time to review this. Appreciate it. :)
>
> Best Regards,
> Anand
>
--
Best Regards,
Felipe Balbi
http://felipebalbi.com
me@xxxxxxxxxxxxxxx
-
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
