Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1192394 > unrolled thread

Re: [PATCH 3/4] power: Add Qualcomm SMBB driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/4] power: Add Qualcomm SMBB driver Sebastian Reichel <sre@kernel.org> - 2015-07-25 17:50 +0200
    Re: [PATCH 3/4] power: Add Qualcomm SMBB driver Bjorn Andersson <bjorn.andersson@sonymobile.com> - 2015-07-26 03:10 +0200
      Re: [PATCH 3/4] power: Add Qualcomm SMBB driver Sebastian Reichel <sre@kernel.org> - 2015-07-27 16:10 +0200

#1192394 — Re: [PATCH 3/4] power: Add Qualcomm SMBB driver

[Multipart message — attachments visible in raw view] — view raw

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. 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.

 * 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?

 * fast-charge-low-watermark
 * fast-charge-high-watermark

Add a unit to this property. Maybe "fast-charge-start-voltage"
and "fast-charge-stop-voltage"?

 * 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.

 * 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.

 * 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.

 * dc-charge-control-limit

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

 * disable-dc

Please add a vendor prefix.

 * jeita-extended-temp-range

Looks ok to me.

-- Sebastian

smbb_charger_attr_parse

[toc] | [next] | [standalone]

#1192450

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.

Thanks.

> 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
future.

>  * 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:

 qcom,fast-charge-current-limit

>  * 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:

 qcom,fast-charge-{low,high}-threshold-voltage

>  * 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.
> 

qcom,auto-charge-threshold-voltage

>  * 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.
> 

Ok

>  * 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).

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

[toc] | [prev] | [next] | [standalone]

#1193139

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Sat, Jul 25, 2015 at 06:04:14PM -0700, Bjorn Andersson wrote:
> On Sat 25 Jul 08:42 PDT 2015, Sebastian Reichel wrote:
> >  * 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:
> 
>  qcom,fast-charge-current-limit

So what's the difference to "fast-charge-safe-current"?

> >  * minimum-input-voltage
> > 
> > Add a vendor prefix to this property.
> > 
> 
> Shouldn't they all have a vendor prefix?

Some of the properties are quite generic and used on multiple
chips, so we may create a power_supply/battery-fuel-gauge.txt
and power_supply/battery-charger.txt with generic bindings.

I'm fine with just adding vendor prefixed properties for all
instances, though.

> 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).

OK, thanks.

-- Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web