[lkml]   [2015]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 3/4] power: Add Qualcomm SMBB driver
On Sat 25 Jul 08:42 PDT 2015, Sebastian Reichel wrote:

> Hi,
> On Thu, Jun 18, 2015 at 02:13:44PM -0700, Bjorn Andersson wrote:
> > Add the Qualcomm Switch-Mode Battery Charger and Boost driver, found in
> > pm8941.
> The driver's sourcecode looks fine to me.


> I'm not convinced by all those new DT properties, though. I think
> "watermark" should be replaced with "threshold" and "control" with
> "current" for all properties. Additionally some comments.

I think both of these comes from the documentation, but I agree with
your suggestion.

> Note, that I only used the driver's sourcecode as reference, since the
> DT binding document was neither send to me, nor to linux-pm
> mailinglist.

Sorry about that, I will make sure to double check my recipients in the

> * battery-charge-control-limit
> It's unclear, what this property is used for. Is the limit only
> for "normal" charging or also for fast charging?

This is described as the current limit during fast charging. However,
"fast charging" is the normal state.

I think the most consistent (regards documentation and other properties)
would be:


> * fast-charge-low-watermark
> * fast-charge-high-watermark
> Add a unit to this property. Maybe "fast-charge-start-voltage"
> and "fast-charge-stop-voltage"?

Will update to:


> * fast-charge-safe-voltage
> * fast-charge-safe-current
> These properties are fine to me. I wonder if they should be named
> fast-charge-max-*, though.

The safe naming is in accordance with the hw documentation, so I think
we should keep those.

> * auto-recharge-low-watermark
> I think the "low" can be dropped. Instead a -voltage
> should be appended, since it could also be a percentage.


> * minimum-input-voltage
> Add a vendor prefix to this property.

Shouldn't they all have a vendor prefix?

> * usb-charge-control-limit
> I suggest to remove this from DT. If no USB detection is
> implemented, the default should be 100mA according to USB
> standard.

Right, this have been convenient during testing as no-one actually does
implement the USB current limit propagation. But that should be
corrected and then you're right that this should only default to 100mA.

I'll drop it.

> * dc-charge-control-limit
> Please add a vendor prefix and I think "dc-current-limit"
> is a more fitting name.

Sounds good.

> * disable-dc
> Please add a vendor prefix.


> * jeita-extended-temp-range
> Looks ok to me.

Thanks for the review, I'll update the patches accordingly and will send
out v2 (and make sure you get the dt binding document as well).


 \ /
  Last update: 2015-07-26 03:21    [W:0.134 / U:2.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site