Re: [PATCH 3/4] power: Add Qualcomm SMBB driver
On Mon 27 Jul 07:06 PDT 2015, Sebastian Reichel wrote: > 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 I spoke with Courtney about this and he pointed out that it really is the limit of the current flowing into the battery, hence his original naming. > > So what's the difference to "fast-charge-safe-current"? > The "safe" values are write-once values that should be set once during boot to protect the hardware. The fast-charge-current-limit can be modified in runtime by e.g. userspace or a thermal mitigation solution - but will never be allowed to go above the safe limit. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] power: Add Qualcomm SMBB driver
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 signature.asc Description: Digital signature
Re: [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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] power: Add Qualcomm SMBB driver
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 signature.asc Description: Digital signature
Re: [PATCH 3/4] power: Add Qualcomm SMBB driver
On Fri, Jun 19, 2015 at 10:01 AM, Paul Bolle wrote:
> On Thu, 2015-06-18 at 14:13 -0700, Bjorn Andersson wrote:
>> --- /dev/null
>> +++ b/drivers/power/qcom_smbb.c
>
>> +MODULE_ALIAS("platform:qcom_smbb");
>
> (The day before yesterday and yesterday I had a, well, lively
> conversation regarding this macro. The interesting bits start at
> https://lkml.org/lkml/2015/6/17/383 .
>
> But in a converstaion today things were rather silent. See
> https://lkml.org/lkml/2015/6/19/68 and the reply, of sorts, in
> https://lkml.org/lkml/2015/6/19/117. Let's see what happens here.)
>
> As I understand it, this alias is only useful if there's a corresponding
> struct platform_device, somewhere. Ie, this alias implies a
> platform_device that will fire of a "MODALIAS=platform:qcom_smbb" uevent
> when it's created. That would be a platform_device using a "qcom_smbb"
> name.
>
As far as I could see the uevents, including MODALIAS, is sent when
the device is added or removed; but the content comes from the device
tree alias, not the MODULE_ALIAS.
The MODULE_ALIAS seems to add an alias to the kernel module
information, which can be used to find this driver during modprobe; in
addition to the actual name of the module.
> If that's correct, then I think this MODULE_ALIAS macro isn't needed
> here, as I couldn't find a platform_device using that name. (But perhaps
> a patch that adds it is pending, somewhere.)
>
I don't see any reason for keeping the MODULE_ALIAS, so I think it
should be removed for v2.
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] power: Add Qualcomm SMBB driver
On Thu, 2015-06-18 at 14:13 -0700, Bjorn Andersson wrote:
> --- /dev/null
> +++ b/drivers/power/qcom_smbb.c
> +MODULE_ALIAS("platform:qcom_smbb");
(The day before yesterday and yesterday I had a, well, lively
conversation regarding this macro. The interesting bits start at
https://lkml.org/lkml/2015/6/17/383 .
But in a converstaion today things were rather silent. See
https://lkml.org/lkml/2015/6/19/68 and the reply, of sorts, in
https://lkml.org/lkml/2015/6/19/117. Let's see what happens here.)
As I understand it, this alias is only useful if there's a corresponding
struct platform_device, somewhere. Ie, this alias implies a
platform_device that will fire of a "MODALIAS=platform:qcom_smbb" uevent
when it's created. That would be a platform_device using a "qcom_smbb"
name.
If that's correct, then I think this MODULE_ALIAS macro isn't needed
here, as I couldn't find a platform_device using that name. (But perhaps
a patch that adds it is pending, somewhere.)
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/
