| Message ID | 1466718134-29921-3-git-send-email-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thu, Jun 23, 2016 at 11:42 PM, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > This driver adds support for Dynamic Platform and Thermal Framework > (DPTF) Platform Power Participant device support. > This participant is responsible for exposing platform telemetry such as > platform Power, battery Information such as state of Charge, estimated > maximum sustainable power (PMax), SMART battery spec information. > > This driver is implemented as a platform driver for INT3407 and presented > as power_supply device. Since this has common features with the ACPI > battery, existing interface provide by battery_common driver are reused > to present as a battery power supply device. > > When both CONFIG_ACPI_BATTERY and CONFIG_DPTF_POWER are defined and > platform has support for INT3407, then dptf power registration is > delayed for 100ms. In 100 ms, if there is no ACPI battery is registered > then dptf power will be registered. Since both can be modules and > battery driver loads in async thread, there can be race even if we > specify loading order for initialization. First, does the waiting actually help, though? Second, to my eyes, even if acpi_battery load first, the dptf_power thing will still bind to the same device, but via the platform bus instead of the ACPI bus type. I don't see anything preventing that from happening in the patch, but maybe I missed something? Also please put the part of the changelog below into a comment in dptf_power.c (if you don't want to add a separate doc for that). > There are two types of objects in INT3407: > - Same as ACPI Battery objects: _BST and _BIX. These are exposed as > standard power supply attributes. > - Specific to INT3407, which are related to platform power > There are some objects, for which it doesn't make sense to enhance > power_supply class and add attributes there. They are represented as > sysfs attribute under acpi device. Here the bid for INT3407 is TPWR > in the following example. > /sys/class/power_supply/TPWR > ├── alarm > ├── capacity > ├── capacity_level > ├── cycle_count > ├── device -> ../../../INT3407:00 > │ ├── dptf_power > │ │ ├── adapter_rating > │ │ ├── battery_steady_power > │ │ ├── charger_type > │ │ ├── max_platform_power > │ │ ├── platform_power_source > │ │ ├── power_sampling_period > > For example representing AC/adapter properties as a power_supply > properties will not make sense for a battery device. In some case > when there is an existing property, the meaning is different. > For example charger_type has a equivalent power_supply property, > which has different symantics than INT3407. > > ACPI methods description used in this driver: > PSOC: Platform Battery State Of Charge as a percentage. > PMAX: Maximum platform power that can be supported by the battery in mW. > PSRC: System charge source, > 0x00 = DC > 0x01 = AC > 0x02 = USB > 0x03 = Wireless Charger > ARTG: Adapter rating in mW (Maximum Adapter power) Must be 0 if no AC > Adaptor is plugged in. > CTYP: Charger Type, > Traditional : 0x01 > Hybrid: 0x02 > NVDC: 0x03 > PBSS: Returns max sustained power for battery in milliWatts. > DPSP: Sets the polling interval in 10ths of seconds. A value of 0 tells > the driver to use event notification for PMAX and PBSS > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2016-06-24 at 00:31 +0200, Rafael J. Wysocki wrote: > On Thu, Jun 23, 2016 at 11:42 PM, Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > > > This driver adds support for Dynamic Platform and Thermal Framework > > (DPTF) Platform Power Participant device support. > > This participant is responsible for exposing platform telemetry > > such as > > platform Power, battery Information such as state of Charge, > > estimated > > maximum sustainable power (PMax), SMART battery spec information. > > > > This driver is implemented as a platform driver for INT3407 and > > presented > > as power_supply device. Since this has common features with the > > ACPI > > battery, existing interface provide by battery_common driver are > > reused > > to present as a battery power supply device. > > > > When both CONFIG_ACPI_BATTERY and CONFIG_DPTF_POWER are defined and > > platform has support for INT3407, then dptf power registration is > > delayed for 100ms. In 100 ms, if there is no ACPI battery is > > registered > > then dptf power will be registered. Since both can be modules and > > battery driver loads in async thread, there can be race even if we > > specify loading order for initialization. > First, does the waiting actually help, though? Yes, if the acpi_battery registered then if (!battery_registered) will be false. > > Second, to my eyes, even if acpi_battery load first, the dptf_power > thing will still bind to the same device, but via the platform bus > instead of the ACPI bus type. I don't see anything preventing that > from happening in the patch, but maybe I missed something? > The INT3407 object also has battery _BST and _BIX, so driver will bind to this device not with PNP0C0A battery device. Either of them should be able to register and provide battery information. But the names are different (BAT vs TPWR on my platform) The check if (!battery_registered) will fail, so dptf_power will not register. So we will not see two batteries registered with power supply class (and hence no two batteries in desktop UI). Thanks, Srinivas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 24, 2016 at 1:19 AM, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Fri, 2016-06-24 at 00:31 +0200, Rafael J. Wysocki wrote: >> On Thu, Jun 23, 2016 at 11:42 PM, Srinivas Pandruvada >> <srinivas.pandruvada@linux.intel.com> wrote: >> > >> > This driver adds support for Dynamic Platform and Thermal Framework >> > (DPTF) Platform Power Participant device support. >> > This participant is responsible for exposing platform telemetry >> > such as >> > platform Power, battery Information such as state of Charge, >> > estimated >> > maximum sustainable power (PMax), SMART battery spec information. >> > >> > This driver is implemented as a platform driver for INT3407 and >> > presented >> > as power_supply device. Since this has common features with the >> > ACPI >> > battery, existing interface provide by battery_common driver are >> > reused >> > to present as a battery power supply device. >> > >> > When both CONFIG_ACPI_BATTERY and CONFIG_DPTF_POWER are defined and >> > platform has support for INT3407, then dptf power registration is >> > delayed for 100ms. In 100 ms, if there is no ACPI battery is >> > registered >> > then dptf power will be registered. Since both can be modules and >> > battery driver loads in async thread, there can be race even if we >> > specify loading order for initialization. >> >> First, does the waiting actually help, though? > > Yes, if the acpi_battery registered then > if (!battery_registered) will be false. > Ah, so [1/3] adds battery_registered while moving code around and the changelog in there doesn't mention that. It is added there, but not really used, only set and reset. I wouldn't do it that way. Just add it in a separate patch or in [2/3] itself, because it is the real user of it. Also this is racy to my eyes, because acpi_battery can register after you've checked battery_registered in dptf_power_init_work_fn() and before calling dptf_power_init(). It actually can register just fine at any time after that AFAICS. >> >> Second, to my eyes, even if acpi_battery load first, the dptf_power >> thing will still bind to the same device, but via the platform bus >> instead of the ACPI bus type. I don't see anything preventing that >> from happening in the patch, but maybe I missed something? >> > The INT3407 object also has battery _BST and _BIX, so driver will bind > to this device not with PNP0C0A battery device. Either of them should > be able to register and provide battery information. But the names are > different (BAT vs TPWR on my platform) > The check if (!battery_registered) will fail, so dptf_power will not > register. So we will not see two batteries registered with power supply > class (and hence no two batteries in desktop UI). Ah OK. I thought that the DPTF device had a _CID("PNP0C0A"). If that's not the case, then the battery driver will not bind to it, so I misunderstood the problem you wanted to address. I think what you need is that if acpi_battery is bound to at least one device, you don't want to bind dptf_power to anything. Conversely, if dptf_power has been bound to at least one device, you don't want to bind acpi_battery to anything. That may be achieved with a lock and two counters, one (A) incremented only by acpi_battery and the other (B) incremented only by dptf_power and such that you can't increment A if B is different from 0 and you can't increment B if A is different from 0. Of course, each driver would need to specify which counter it wants to use (A or B), so that would take an additional argument to acpi_battery_common_add() and an additional field in struct acpi_battery (for the remove operation). With that, I think it should only be possible to build both acpi_battery and dptf_power if they are both modules. IOW, DPTF_POWER should depend on (!ACPI_BATTERY || ACPI_BATTERY=m) or similar. And if they are both modules, let user space manage that. And the waiting itself doesn't add any value then IMO. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2016-06-24 at 02:26 +0200, Rafael J. Wysocki wrote: > > > > > On Fri, Jun 24, 2016 at 1:19 AM, Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > On Fri, 2016-06-24 at 00:31 +0200, Rafael J. Wysocki wrote: > > > On Thu, Jun 23, 2016 at 11:42 PM, Srinivas Pandruvada > > > <srinivas.pandruvada@linux.intel.com> wrote: [...] > I think what you need is that if acpi_battery is bound to at least > one > device, you don't want to bind dptf_power to anything. Conversely, > if > dptf_power has been bound to at least one device, you don't want to > bind acpi_battery to anything. > > That may be achieved with a lock and two counters, one (A) > incremented > only by acpi_battery and the other (B) incremented only by dptf_power > and such that you can't increment A if B is different from 0 and you > can't increment B if A is different from 0. Of course, each driver > would need to specify which counter it wants to use (A or B), so that > would take an additional argument to acpi_battery_common_add() and an > additional field in struct acpi_battery (for the remove operation). > > With that, I think it should only be possible to build both > acpi_battery and dptf_power if they are both modules. IOW, > DPTF_POWER > should depend on (!ACPI_BATTERY || ACPI_BATTERY=m) or similar. And > if > they are both modules, let user space manage that. > > And the waiting itself doesn't add any value then IMO. Yes. I think the best solution is not to let define DPTF_POWER when the ACPI_BATTERY is defined same as my first version of the patch or let both add as there is no harm as they will show same levels. The reason is: We have some devices with two ACPI_BATTERIES (primary and secondary/backup) and they must be presented as two power supply devices to user space. In those devices DPTF_POWER may be equivalent to only one of the ACPI_BATTERY (Will point to same battery for Battery levels). So we can't simply refuse to add ACPI_BATTERY device addition because DPTF_POWER device is registered before. Thanks, Srinivas > Thanks, > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 27, 2016 at 3:31 AM, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Fri, 2016-06-24 at 02:26 +0200, Rafael J. Wysocki wrote: >> > > > >> On Fri, Jun 24, 2016 at 1:19 AM, Srinivas Pandruvada >> <srinivas.pandruvada@linux.intel.com> wrote: >> > On Fri, 2016-06-24 at 00:31 +0200, Rafael J. Wysocki wrote: >> > > On Thu, Jun 23, 2016 at 11:42 PM, Srinivas Pandruvada >> > > <srinivas.pandruvada@linux.intel.com> wrote: > [...] > >> I think what you need is that if acpi_battery is bound to at least >> one >> device, you don't want to bind dptf_power to anything. Conversely, >> if >> dptf_power has been bound to at least one device, you don't want to >> bind acpi_battery to anything. >> >> That may be achieved with a lock and two counters, one (A) >> incremented >> only by acpi_battery and the other (B) incremented only by dptf_power >> and such that you can't increment A if B is different from 0 and you >> can't increment B if A is different from 0. Of course, each driver >> would need to specify which counter it wants to use (A or B), so that >> would take an additional argument to acpi_battery_common_add() and an >> additional field in struct acpi_battery (for the remove operation). >> >> With that, I think it should only be possible to build both >> acpi_battery and dptf_power if they are both modules. IOW, >> DPTF_POWER >> should depend on (!ACPI_BATTERY || ACPI_BATTERY=m) or similar. And >> if >> they are both modules, let user space manage that. >> >> And the waiting itself doesn't add any value then IMO. > Yes. I think the best solution is not to let define DPTF_POWER when the > ACPI_BATTERY is defined same as my first version of the patch or let > both add as there is no harm as they will show same levels. The reason > is: > We have some devices with two ACPI_BATTERIES (primary and > secondary/backup) and they must be presented as two power supply > devices to user space. In those devices DPTF_POWER may be equivalent to > only one of the ACPI_BATTERY (Will point to same battery for Battery > levels). So we can't simply refuse to add ACPI_BATTERY device addition > because DPTF_POWER device is registered before. OK Say dptf_power points to the same battery device as acpi_battery. Is there any way to tell when that happens? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGksIFJhZmFlbA0KDQpKdXN0IGFuIGlkZWEgdGhhdC4NCg0KQWNjb3JkaW5nIHRvIHRoZSBBQ1BJ IHNwZWMsIF9XQUsgbXVzdCBiZSBldmFsdWF0ZWQgYmVmb3JlIHJlc3VtaW5nIHRoZSBkZXZpY2Vz Lg0KU28gaXQgaXMgc3VwcG9zZWQgdGhhdCB0aGUgZGV2aWNlIGRyaXZlcnMgc2hvdWxkIGJlIHJl YWR5IHRvIGhhbmRsZSBub3RpZnkoKSBkdXJpbmcgdGhlIHN1c3BlbmQvcmVzdW1lIHBlcmlvZCwg aW4gdGhlIHNhbWUgd2F5cyBhcyBoYW5kbGluZyBhbiBJUlEgZHVyaW5nIHRoaXMgcGVyaW9kLg0K SWYgdGhlIGRyaXZlciBpcyBub3QgYWJsZSB0byBkbyB0aGF0IChoYW5kbGluZyBub3RpZnkgZHVy aW5nIHN1c3BlbmQvcmVzdW1lIGN5Y2xlKSwgc2hvdWxkIGl0IGJlIGRvbmUgYnkgdGhlIGRyaXZl ciBpbiB0aGUgd2F5IG9mIGV4cGxpY2l0bHkgImRpc2FibGluZyBub3RpZnkiLCBsaWtlIHdoYXQg dGhlIGRpc2FibGVfaXJxKCkgZG9lcz8NCk1heWJlIGRvIHRoaXMgdmlhIGEgbmV3IHNldCBvZiBB UHMgZnJvbSB0aGUgQUNQSSBzdWJzeXN0ZW06IGFjcGlfZGlzYWJsZV9ub3RpZnkoKS9hY3BpX2Vu YWJsZV9ub3RpZnkoKS4NCg0KVGhhbmtzIGFuZCBiZXN0IHJlZ2FyZHMNCi1Mdg0KDQo+IEZyb206 IGxpbnV4LWFjcGktb3duZXJAdmdlci5rZXJuZWwub3JnIFttYWlsdG86bGludXgtYWNwaS0NCj4g b3duZXJAdmdlci5rZXJuZWwub3JnXSBPbiBCZWhhbGYgT2YgUmFmYWVsIEouIFd5c29ja2kNCj4g U3ViamVjdDogUmU6IFtQQVRDSCB2MyAyLzNdIGFjcGk6IGRwdGZfcG93ZXI6IEFkZCBEUFRGIHBv d2VyIHBhcnRpY2lwYW50DQo+IA0KPiBPbiBNb24sIEp1biAyNywgMjAxNiBhdCAzOjMxIEFNLCBT cmluaXZhcyBQYW5kcnV2YWRhDQo+IDxzcmluaXZhcy5wYW5kcnV2YWRhQGxpbnV4LmludGVsLmNv bT4gd3JvdGU6DQo+ID4gT24gRnJpLCAyMDE2LTA2LTI0IGF0IDAyOjI2ICswMjAwLCBSYWZhZWwg Si4gV3lzb2NraSB3cm90ZToNCj4gPj4gPiA+ID4NCj4gPj4gT24gRnJpLCBKdW4gMjQsIDIwMTYg YXQgMToxOSBBTSwgU3Jpbml2YXMgUGFuZHJ1dmFkYQ0KPiA+PiA8c3Jpbml2YXMucGFuZHJ1dmFk YUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOg0KPiA+PiA+IE9uIEZyaSwgMjAxNi0wNi0yNCBhdCAw MDozMSArMDIwMCwgUmFmYWVsIEouIFd5c29ja2kgd3JvdGU6DQo+ID4+ID4gPiBPbiBUaHUsIEp1 biAyMywgMjAxNiBhdCAxMTo0MiBQTSwgU3Jpbml2YXMgUGFuZHJ1dmFkYQ0KPiA+PiA+ID4gPHNy aW5pdmFzLnBhbmRydXZhZGFAbGludXguaW50ZWwuY29tPiB3cm90ZToNCj4gPiBbLi4uXQ0KPiA+ DQo+ID4+IEkgdGhpbmsgd2hhdCB5b3UgbmVlZCBpcyB0aGF0IGlmIGFjcGlfYmF0dGVyeSBpcyBi b3VuZCB0byBhdCBsZWFzdA0KPiA+PiBvbmUNCj4gPj4gZGV2aWNlLCB5b3UgZG9uJ3Qgd2FudCB0 byBiaW5kIGRwdGZfcG93ZXIgdG8gYW55dGhpbmcuICBDb252ZXJzZWx5LA0KPiA+PiBpZg0KPiA+ PiBkcHRmX3Bvd2VyIGhhcyBiZWVuIGJvdW5kIHRvIGF0IGxlYXN0IG9uZSBkZXZpY2UsIHlvdSBk b24ndCB3YW50IHRvDQo+ID4+IGJpbmQgYWNwaV9iYXR0ZXJ5IHRvIGFueXRoaW5nLg0KPiA+Pg0K PiA+PiBUaGF0IG1heSBiZSBhY2hpZXZlZCB3aXRoIGEgbG9jayBhbmQgdHdvIGNvdW50ZXJzLCBv bmUgKEEpDQo+ID4+IGluY3JlbWVudGVkDQo+ID4+IG9ubHkgYnkgYWNwaV9iYXR0ZXJ5IGFuZCB0 aGUgb3RoZXIgKEIpIGluY3JlbWVudGVkIG9ubHkgYnkgZHB0Zl9wb3dlcg0KPiA+PiBhbmQgc3Vj aCB0aGF0IHlvdSBjYW4ndCBpbmNyZW1lbnQgQSBpZiBCIGlzIGRpZmZlcmVudCBmcm9tIDAgYW5k IHlvdQ0KPiA+PiBjYW4ndCBpbmNyZW1lbnQgQiBpZiBBIGlzIGRpZmZlcmVudCBmcm9tIDAuICBP ZiBjb3Vyc2UsIGVhY2ggZHJpdmVyDQo+ID4+IHdvdWxkIG5lZWQgdG8gc3BlY2lmeSB3aGljaCBj b3VudGVyIGl0IHdhbnRzIHRvIHVzZSAoQSBvciBCKSwgc28gdGhhdA0KPiA+PiB3b3VsZCB0YWtl IGFuIGFkZGl0aW9uYWwgYXJndW1lbnQgdG8gYWNwaV9iYXR0ZXJ5X2NvbW1vbl9hZGQoKQ0KPiBh bmQgYW4NCj4gPj4gYWRkaXRpb25hbCBmaWVsZCBpbiBzdHJ1Y3QgYWNwaV9iYXR0ZXJ5IChmb3Ig dGhlIHJlbW92ZSBvcGVyYXRpb24pLg0KPiA+Pg0KPiA+PiBXaXRoIHRoYXQsIEkgdGhpbmsgaXQg c2hvdWxkIG9ubHkgYmUgcG9zc2libGUgdG8gYnVpbGQgYm90aA0KPiA+PiBhY3BpX2JhdHRlcnkg YW5kIGRwdGZfcG93ZXIgaWYgdGhleSBhcmUgYm90aCBtb2R1bGVzLiAgSU9XLA0KPiA+PiBEUFRG X1BPV0VSDQo+ID4+IHNob3VsZCBkZXBlbmQgb24gKCFBQ1BJX0JBVFRFUlkgfHwgQUNQSV9CQVRU RVJZPW0pIG9yIHNpbWlsYXIuDQo+IEFuZA0KPiA+PiBpZg0KPiA+PiB0aGV5IGFyZSBib3RoIG1v ZHVsZXMsIGxldCB1c2VyIHNwYWNlIG1hbmFnZSB0aGF0Lg0KPiA+Pg0KPiA+PiBBbmQgdGhlIHdh aXRpbmcgaXRzZWxmIGRvZXNuJ3QgYWRkIGFueSB2YWx1ZSB0aGVuIElNTy4NCj4gPiBZZXMuIEkg dGhpbmsgdGhlIGJlc3Qgc29sdXRpb24gaXMgbm90IHRvIGxldCBkZWZpbmUgRFBURl9QT1dFUiB3 aGVuIHRoZQ0KPiA+IEFDUElfQkFUVEVSWSBpcyBkZWZpbmVkIHNhbWUgYXMgbXkgZmlyc3QgdmVy c2lvbiBvZiB0aGUgcGF0Y2ggb3IgbGV0DQo+ID4gYm90aCBhZGQgYXMgdGhlcmUgaXMgbm8gaGFy bSBhcyB0aGV5IHdpbGwgc2hvdyBzYW1lIGxldmVscy4gVGhlIHJlYXNvbg0KPiA+IGlzOg0KPiA+ IFdlIGhhdmUgc29tZSBkZXZpY2VzIHdpdGggdHdvIEFDUElfQkFUVEVSSUVTIChwcmltYXJ5IGFu ZA0KPiA+IHNlY29uZGFyeS9iYWNrdXApIGFuZCB0aGV5IG11c3QgYmUgcHJlc2VudGVkIGFzIHR3 byBwb3dlciBzdXBwbHkNCj4gPiBkZXZpY2VzIHRvIHVzZXIgc3BhY2UuIEluIHRob3NlIGRldmlj ZXMgRFBURl9QT1dFUiBtYXkgYmUgZXF1aXZhbGVudA0KPiB0bw0KPiA+IG9ubHkgb25lIG9mIHRo ZSBBQ1BJX0JBVFRFUlkgKFdpbGwgcG9pbnQgdG8gc2FtZSBiYXR0ZXJ5IGZvciBCYXR0ZXJ5DQo+ ID4gbGV2ZWxzKS4gU28gd2UgY2FuJ3Qgc2ltcGx5IHJlZnVzZSB0byBhZGQgQUNQSV9CQVRURVJZ IGRldmljZSBhZGRpdGlvbg0KPiA+IGJlY2F1c2UgRFBURl9QT1dFUiBkZXZpY2UgaXMgcmVnaXN0 ZXJlZCBiZWZvcmUuDQo+IA0KPiBPSw0KPiANCj4gU2F5IGRwdGZfcG93ZXIgcG9pbnRzIHRvIHRo ZSBzYW1lIGJhdHRlcnkgZGV2aWNlIGFzIGFjcGlfYmF0dGVyeS4gIElzDQo+IHRoZXJlIGFueSB3 YXkgdG8gdGVsbCB3aGVuIHRoYXQgaGFwcGVucz8NCj4gLS0NCj4gVG8gdW5zdWJzY3JpYmUgZnJv bSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LWFjcGkiIGluDQo+ IHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnDQo+IE1v cmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWlu Zm8uaHRtbA0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2016-06-27 at 03:45 +0200, Rafael J. Wysocki wrote: > On Mon, Jun 27, 2016 at 3:31 AM, Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > > > On Fri, 2016-06-24 at 02:26 +0200, Rafael J. Wysocki wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Jun 24, 2016 at 1:19 AM, Srinivas Pandruvada > > > <srinivas.pandruvada@linux.intel.com> wrote: > > > > > > > > On Fri, 2016-06-24 at 00:31 +0200, Rafael J. Wysocki wrote: > > > > > > > > > > On Thu, Jun 23, 2016 at 11:42 PM, Srinivas Pandruvada > > > > > <srinivas.pandruvada@linux.intel.com> wrote: > > [...] > > > > > > > > I think what you need is that if acpi_battery is bound to at > > > least > > > one > > > device, you don't want to bind dptf_power to > > > anything. Conversely, > > > if > > > dptf_power has been bound to at least one device, you don't want > > > to > > > bind acpi_battery to anything. > > > > > > That may be achieved with a lock and two counters, one (A) > > > incremented > > > only by acpi_battery and the other (B) incremented only by > > > dptf_power > > > and such that you can't increment A if B is different from 0 and > > > you > > > can't increment B if A is different from 0. Of course, each > > > driver > > > would need to specify which counter it wants to use (A or B), so > > > that > > > would take an additional argument to acpi_battery_common_add() > > > and an > > > additional field in struct acpi_battery (for the remove > > > operation). > > > > > > With that, I think it should only be possible to build both > > > acpi_battery and dptf_power if they are both modules. IOW, > > > DPTF_POWER > > > should depend on (!ACPI_BATTERY || ACPI_BATTERY=m) or > > > similar. And > > > if > > > they are both modules, let user space manage that. > > > > > > And the waiting itself doesn't add any value then IMO. > > Yes. I think the best solution is not to let define DPTF_POWER when > > the > > ACPI_BATTERY is defined same as my first version of the patch or > > let > > both add as there is no harm as they will show same levels. The > > reason > > is: > > We have some devices with two ACPI_BATTERIES (primary and > > secondary/backup) and they must be presented as two power supply > > devices to user space. In those devices DPTF_POWER may be > > equivalent to > > only one of the ACPI_BATTERY (Will point to same battery for > > Battery > > levels). So we can't simply refuse to add ACPI_BATTERY device > > addition > > because DPTF_POWER device is registered before. > OK > > Say dptf_power points to the same battery device as acpi_battery. Is > there any way to tell when that happens? May be we can memcmp the output of _BIX (Battery Information Extended). If they are same it belongs to same battery. If there are two ACPI batteries, hope atleast they have difference in one element in the following package. Package { // ASCIIZ is ASCII character string terminated with a 0x00. Revision //Integer Power Unit //Integer (DWORD) Design Capacity //Integer (DWORD) Last Full Charge Capacity //Integer (DWORD) Battery Technology //Integer (DWORD) Design Voltage //Integer (DWORD) Design Capacity of Warning //Integer (DWORD) Design Capacity of Low //Integer (DWORD) Cycle Count //Integer (DWORD) Measurement Accuracy Max Sampling Time Min Sampling Time Max Averaging Interval Min Averaging Interval //Integer //Integer //Integer //Integer //Integer (DWORD) (DWORD) (DWORD) (DWORD) (DWORD) Battery Capacity Granularity 1 Battery Capacity Granularity 2 Model Number Serial Number Battery Type OEM Information } Thanks, Srinivas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 27ae351..da8c575 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -480,6 +480,7 @@ config ACPI_NFIT_DEBUG issue. source "drivers/acpi/apei/Kconfig" +source "drivers/acpi/dptf/Kconfig" config ACPI_EXTLOG tristate "Extended Error Log support" diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 950bf8e..8555c14 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -102,3 +102,4 @@ obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o video-objs += acpi_video.o video_detect.o +obj-y += dptf/ diff --git a/drivers/acpi/dptf/Kconfig b/drivers/acpi/dptf/Kconfig new file mode 100644 index 0000000..bce8edf --- /dev/null +++ b/drivers/acpi/dptf/Kconfig @@ -0,0 +1,17 @@ +config DPTF_POWER + tristate "DPTF Platform Power Participant" + depends on X86 + select ACPI_BATTERY_COMMON + select POWER_SUPPLY + default y + help + This driver adds support for Dynamic Platform and Thermal Framework + (DPTF) Platform Power Participant device support. + This participant is responsible for exposing platform telemetry such + as platform Power, battery Information such as state of Charge, + estimated maximum sustainable power (PMax), SMART battery spec + information. + + To compile this driver as a module, choose M here: + the module will be called dptf_power. + diff --git a/drivers/acpi/dptf/Makefile b/drivers/acpi/dptf/Makefile new file mode 100644 index 0000000..1b6d8bd --- /dev/null +++ b/drivers/acpi/dptf/Makefile @@ -0,0 +1,3 @@ +obj-$(CONFIG_DPTF_POWER) += dptf_power.o + +ccflags-y += -Idrivers/acpi diff --git a/drivers/acpi/dptf/dptf_power.c b/drivers/acpi/dptf/dptf_power.c new file mode 100644 index 0000000..013b7f3 --- /dev/null +++ b/drivers/acpi/dptf/dptf_power.c @@ -0,0 +1,184 @@ +/* + * dptf_power: DPTF platform power driver + * Copyright (c) 2016, Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/acpi.h> +#include <linux/platform_device.h> +#include "battery.h" + +#define DPTF_POWER_SHOW(name, object) \ +static ssize_t name##_show(struct device *dev,\ + struct device_attribute *attr,\ + char *buf)\ +{\ + struct acpi_device *acpi_dev = to_acpi_device(dev);\ + unsigned long long val;\ + acpi_status status;\ +\ + status = acpi_evaluate_integer(acpi_dev->handle, #object,\ + NULL, &val);\ + if (ACPI_SUCCESS(status))\ + return sprintf(buf, "%llu\n", val);\ + else\ + return -EINVAL;\ +} + +DPTF_POWER_SHOW(max_platform_power, PMAX) +DPTF_POWER_SHOW(platform_power_source, PSRC) +DPTF_POWER_SHOW(adapter_rating, ARTG) +DPTF_POWER_SHOW(charger_type, CTYP) +DPTF_POWER_SHOW(battery_steady_power, PBSS) +DPTF_POWER_SHOW(power_sampling_period, DPSP) + + +static DEVICE_ATTR_RO(max_platform_power); +static DEVICE_ATTR_RO(platform_power_source); +static DEVICE_ATTR_RO(adapter_rating); +static DEVICE_ATTR_RO(battery_steady_power); +static DEVICE_ATTR_RO(power_sampling_period); +static DEVICE_ATTR_RO(charger_type); + +static struct attribute *dptf_power_attrs[] = { + &dev_attr_max_platform_power.attr, + &dev_attr_platform_power_source.attr, + &dev_attr_adapter_rating.attr, + &dev_attr_charger_type.attr, + &dev_attr_battery_steady_power.attr, + &dev_attr_power_sampling_period.attr, + NULL +}; + +static struct attribute_group dptf_power_attribute_group = { + .attrs = dptf_power_attrs, + .name = "dptf_power" +}; + +struct platform_device *dptf_power_pdev; + +static void dptf_power_notify(acpi_handle handle, u32 event, void *data) +{ + struct acpi_device *device = data; + + acpi_battery_common_notify(device, event); +} + +static int dptf_power_init(struct platform_device *pdev) +{ + struct acpi_device *acpi_dev; + acpi_status status; + unsigned long long ptype; + int result; + + acpi_dev = ACPI_COMPANION(&(pdev->dev)); + if (!acpi_dev) + return -ENODEV; + + status = acpi_evaluate_integer(acpi_dev->handle, "PTYP", NULL, &ptype); + if (ACPI_FAILURE(status)) + return -ENODEV; + + if (ptype != 0x11) + return -ENODEV; + + + result = acpi_battery_common_add(acpi_dev); + if (result) + return result; + + result = sysfs_create_group(&acpi_dev->dev.kobj, + &dptf_power_attribute_group); + if (result) + goto error_remove_battery; + + result = acpi_install_notify_handler(acpi_dev->handle, + ACPI_DEVICE_NOTIFY, + dptf_power_notify, + (void *)acpi_dev); + if (result) + goto error_remove_sysfs; + + platform_set_drvdata(pdev, acpi_dev); + + return 0; + +error_remove_sysfs: + sysfs_remove_group(&acpi_dev->dev.kobj, &dptf_power_attribute_group); +error_remove_battery: + acpi_battery_common_remove(acpi_dev); + + return result; +} + +static void dptf_power_init_work_fn(struct work_struct *work) +{ + if (!battery_registered) + dptf_power_init(dptf_power_pdev); +} + +static DECLARE_DELAYED_WORK(dptf_power_init_work, dptf_power_init_work_fn); + +static int dptf_power_add(struct platform_device *pdev) +{ + int ret = 0; + +#if IS_ENABLED(CONFIG_ACPI_BATTERY) + /* Wait for battery driver to initialize and register for 100ms */ + dptf_power_pdev = pdev; + schedule_delayed_work(&dptf_power_init_work, msecs_to_jiffies(100)); +#else + ret = dptf_power_init(pdev); +#endif + return ret; +} + +static int dptf_power_remove(struct platform_device *pdev) +{ + struct acpi_device *acpi_dev = platform_get_drvdata(pdev); + +#if defined(CONFIG_ACPI_BATTERY) + cancel_delayed_work_sync(&dptf_power_init_work); +#endif + if (!acpi_dev) + return 0; + + acpi_remove_notify_handler(acpi_dev->handle, ACPI_DEVICE_NOTIFY, + dptf_power_notify); + sysfs_remove_group(&acpi_dev->dev.kobj, &dptf_power_attribute_group); + acpi_battery_common_remove(acpi_dev); + + return 0; +} + +static const struct acpi_device_id int3407_device_ids[] = { + {"INT3407", 0}, + {"", 0}, +}; +MODULE_DEVICE_TABLE(acpi, int3407_device_ids); + +static struct platform_driver dptf_power_driver = { + .probe = dptf_power_add, + .remove = dptf_power_remove, + .driver = { + .name = "DPTF Platform Power", + .acpi_match_table = int3407_device_ids, + }, +}; + +module_platform_driver(dptf_power_driver); + +MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>"); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ACPI DPTF platform power driver");
This driver adds support for Dynamic Platform and Thermal Framework (DPTF) Platform Power Participant device support. This participant is responsible for exposing platform telemetry such as platform Power, battery Information such as state of Charge, estimated maximum sustainable power (PMax), SMART battery spec information. This driver is implemented as a platform driver for INT3407 and presented as power_supply device. Since this has common features with the ACPI battery, existing interface provide by battery_common driver are reused to present as a battery power supply device. When both CONFIG_ACPI_BATTERY and CONFIG_DPTF_POWER are defined and platform has support for INT3407, then dptf power registration is delayed for 100ms. In 100 ms, if there is no ACPI battery is registered then dptf power will be registered. Since both can be modules and battery driver loads in async thread, there can be race even if we specify loading order for initialization. There are two types of objects in INT3407: - Same as ACPI Battery objects: _BST and _BIX. These are exposed as standard power supply attributes. - Specific to INT3407, which are related to platform power There are some objects, for which it doesn't make sense to enhance power_supply class and add attributes there. They are represented as sysfs attribute under acpi device. Here the bid for INT3407 is TPWR in the following example. /sys/class/power_supply/TPWR ├── alarm ├── capacity ├── capacity_level ├── cycle_count ├── device -> ../../../INT3407:00 │ ├── dptf_power │ │ ├── adapter_rating │ │ ├── battery_steady_power │ │ ├── charger_type │ │ ├── max_platform_power │ │ ├── platform_power_source │ │ ├── power_sampling_period For example representing AC/adapter properties as a power_supply properties will not make sense for a battery device. In some case when there is an existing property, the meaning is different. For example charger_type has a equivalent power_supply property, which has different symantics than INT3407. ACPI methods description used in this driver: PSOC: Platform Battery State Of Charge as a percentage. PMAX: Maximum platform power that can be supported by the battery in mW. PSRC: System charge source, 0x00 = DC 0x01 = AC 0x02 = USB 0x03 = Wireless Charger ARTG: Adapter rating in mW (Maximum Adapter power) Must be 0 if no AC Adaptor is plugged in. CTYP: Charger Type, Traditional : 0x01 Hybrid: 0x02 NVDC: 0x03 PBSS: Returns max sustained power for battery in milliWatts. DPSP: Sets the polling interval in 10ths of seconds. A value of 0 tells the driver to use event notification for PMAX and PBSS Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/acpi/Kconfig | 1 + drivers/acpi/Makefile | 1 + drivers/acpi/dptf/Kconfig | 17 ++++ drivers/acpi/dptf/Makefile | 3 + drivers/acpi/dptf/dptf_power.c | 184 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 206 insertions(+) create mode 100644 drivers/acpi/dptf/Kconfig create mode 100644 drivers/acpi/dptf/Makefile create mode 100644 drivers/acpi/dptf/dptf_power.c