[Meego-kernel] [PATCH 1/10] usb: penwell_otg: add USB charger detection

[Wu, Hao]

>From: Alan Cox [mailto:alan at linux.intel.com]

Alan, Thanks for your comments.

>> otg_transceiver *otg, static int penwell_otg_set_power(struct
>> otg_transceiver *otg, unsigned mA)
>>  {
>> +	struct penwell_otg	*pnw = the_transceiver;
>> +
>> +	dev_dbg(pnw->dev, "set_power = %d\n", mA);
>> +
>
>This doesn't actually seem to do anything except warn that pnw is
>unused ?

I agree, will fix it.

>
>> +	addr = MSIC_ULPIACCESSMODE;
>> +	data = enabled ? SPIMODE : 0;
>> +	mask = SPIMODE;
>> +
>> +	dev_dbg(pnw->dev, "%s --->\n", __func__);
>> +
>> +	if (intel_scu_ipc_iowrite8(addr, data)) {
>
>Why keep using variables "addr" and "data" when you could just stick
>the values in the function call ?
>
>> +	dev_dbg(pnw->dev, "%s <---\n", __func__);
>
>Much too much debug, at least for once you think it works

Will remove unnecessary ones.

>> +}
>> +
>> +/* USB Battery Charger detection related functions */
>> +/* Data contact detection is the first step for charger detection */
>> +static int penwell_otg_data_contact_detect(void)
>> +{
>> +	struct penwell_otg	*pnw = the_transceiver;
>> +	u16			addr;
>> +	u8			data;
>> +	int			count = 10;
>> +
>> +	dev_dbg(pnw->dev, "%s --->\n", __func__);
>> +
>> +	/* Enable SPI access */
>> +	penwell_otg_msic_spi_access(true);
>
>Doesn't seem to be in the tree - where does this come from ?

You mean the function penwell_otg_msic_spi_access ?

It is from the same patch.
+static void penwell_otg_msic_spi_access(bool enabled)
+{
+	struct penwell_otg	*pnw = the_transceiver;
+	u16			addr;
+	u8			data, mask;
+
+	/* Set ULPI ACCESS MODE */
+	addr = MSIC_ULPIACCESSMODE;
+	data = enabled ? SPIMODE : 0;
+	mask = SPIMODE;
+
+	dev_dbg(pnw->dev, "%s --->\n", __func__);
+
+	if (intel_scu_ipc_iowrite8(addr, data)) {
+		dev_warn(pnw->dev, "Failed to update MSIC register %x\n", addr);
+		return ;
+	}
+
+	dev_dbg(pnw->dev, "%s <---\n", __func__);
+}

>
>> +	/* Set OTG_CTRL_CLR */
>> +	addr = MSIC_OTGCTRLCLR;
>> +	data = DMPULLDOWN | DPPULLDOWN;
>> +
>> +	if (intel_scu_ipc_iowrite8(addr, data)) {
>
>I'd make this a function which does the write/warning, that way you'll
>only have to put the dev_warn code in once and it'll also help make it
>easier and shorter to read

Thanks for the comment, I will fix it.

>> +static enum usb_charger_type penwell_otg_charger_type_detect(void)
>> +{
>> +	struct penwell_otg		*pnw = the_transceiver;
>> +	u16				addr;
>> +	u8				data;
>> +	enum usb_charger_type		charger;
>> +
>> +	dev_dbg(pnw->dev, "%s --->\n", __func__);
>> +
>> +	addr = MSIC_VS3CLR;
>> +	data = DATACONEN;
>> +
>> +	if (intel_scu_ipc_iowrite8(addr, data)) {
>> +		dev_warn(pnw->dev, "Failed to update MSIC register
>> %x\n", addr);
>> +		return CHRG_UNKNOWN;
>
>In general it would probably be better to return +ve values of charger
>types, and negative error codes. That fits the Linux way of doing
>things better and means you can pass up the error from the scu write

Agree, will fix it.

Hao