RE: [PATCHv2] power_supply: Add support for Richtek rt9455 battery charger

[Nicolae, Anda-maria]

Hi Krzysztof,

Inline are the answers to your questions.

Thanks,
Anda

________________________________________
From: k.kozlowski.k@xxxxxxxxx [k.kozlowski.k@xxxxxxxxx] on behalf of Krzysztof Kozlowski [k.kozlowski@xxxxxxxxxxx]
Sent: Thursday, April 30, 2015 10:53 AM
To: Nicolae, Anda-maria
Cc: sre@xxxxxxxxxx; dbaryshkov@xxxxxxxxx; robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx; mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx; dwmw2@xxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
Subject: Re: [PATCHv2] power_supply: Add support for Richtek rt9455 battery charger

2015-04-30 5:13 GMT+09:00 Anda-Maria Nicolae <anda-maria.nicolae@xxxxxxxxx>:
> Based on the datasheet found here:
> http://www.richtek.com/download_ds.jsp?p=RT9455
>
> Updates from the previous version:
> - replaced license GPLv2 with GPL
> - replaced vendor prefix rt with richtek
> - replaced uppercase properties names from devicetree with lowercase separated by dash properties names
> - replaced I/O read/write API with regmap_read/write and regmap_field_read/write
> - used kernel multiline comment
> - used DELAYED_WORK and scheduled the works in system_power_efficient_wq. It is high probability that the device is in suspend state while charging (i.e. the user leaves the device to charge during night) and it is needed that the scheduled works to be executed as planned.

I think when device is suspended (e.g. to RAM) no work will ever be
executed. The timers (for delayed work) will not wake up the device...
RTC could wake but this is different story. My proposal of deferred
timers is affected by idle CPU time. Are you sure that you want to
wake up from the system suspend?

Oh, now I better understand it. I don't think I need this feature. Also, the expire times that I use in the driver are not defined in the datasheet, so it should not be a problem if they are fired later.
So, I will use INIT_DEFERRABLE_WORK instead of INIT_DELAYED_WORK.

> - replaced struct power_supply_desc rt9455_charger_desc with static const struct power_supply_desc rt9455_charger_desc
> - replaced if (IS_ERR_OR_NULL(info->charger)) with if (IS_ERR(info->charger))
> - replaced {"RTK9455", 0} with { "RTK9455", 0 }
> - removed .owner = THIS_MODULE
>
> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@xxxxxxxxx>

You missed some of my comments. I mentioned wrong error paths around
put_usb_phy in probe. They do not seem to be fixed...

Now I see what you meant. I looked at usb_get_phy() implementation and I saw that I cannot return NULL. So I replaced IS_ERR_OR_NULL() with IS_ERR(). I did this in rt9455_remove() and in rt9455_irq_handler_thread() functions, too.
Also, this replacement (IS_ERR() instead of IS_ERR_OR_NULL()) should also be done in other power charger drivers: 
drivers/power/pda_power.c lines 319, 375, 410, 445; 
drivers/power/ab8500_charger.c line 3651;
drivers/power/twl4030_charger.c lines 642, 674 and 706.
I will send the patch for RT9455 charger soon. I will also send a patch to replace IS_ERR_OR_NULL() with IS_ERR() in the drivers listed above.



Just one hint - mostly new bindings are posted in separate patches at
the beginning of the patchset. I actually don't mind. but separating
them will probably give you higher chance of review from DT people.
This is also mentioned in:
Documentation/devicetree/bindings/submitting-patches.txt

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/