From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [04/15] power: supply: bq24190_charger: Add no_register_reset pdata flag Date: Mon, 20 Mar 2017 14:54:57 +0100 Message-ID: <6de63319-f30f-cae7-6d7a-0df2edd12859@redhat.com> References: <20170318071019.4561-1-liam@networkimprov.net> <6ad6ecd2-ea0f-b613-8519-66ad424c623a@redhat.com> <6a0e54e3-581d-6162-b521-82b7567b6e74@redhat.com> <12321f48-def6-33cb-1df5-853ecc8f1061@redhat.com> <20170320052715.hrgaxsxwdrv7ynbu@earth> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43922 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753116AbdCTNzl (ORCPT ); Mon, 20 Mar 2017 09:55:41 -0400 In-Reply-To: <20170320052715.hrgaxsxwdrv7ynbu@earth> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Sebastian Reichel Cc: Liam Breck , Andy Shevchenko , Tony Lindgren , linux-pm@vger.kernel.org, Liam Breck Hi, On 20-03-17 06:27, Sebastian Reichel wrote: > Hi, > > On Sun, Mar 19, 2017 at 10:42:00AM +0100, Hans de Goede wrote: >> We want one driver which is solidly in control of the charger >> since getting that wrong is quite bad and we want to extend >> the information it is exporting to userspace in the form of >> power_supply properties with some extra info. >> >> So taking that as a starting point and generalizing that, >> I think that if we want we can make something more generic then >> my original patch for this. My idea is to introduce something >> called a power_supply_properties_provider, with an API >> like this: > > Thanks for thinking about this. From what I can see it should > be enough to just avoid exposing a battery device at all in > the charger driver, though. It does not really provide useful > information anyways. Interesting suggestion, 2 remarks / questions: 1) Given that we do not want to break existing setups which may depend on the existing battery interface on the bq24190 driver and add a flag to not register it, or are you suggesting to simply remove it all together? Removing it all together would be better from a maintenance pov. Liam, you indicated that you are the main user of the bq24190 driver currently, do you need the battery power_supply interface for your use case ? 2) Not using the battery interface of the bq24190 driver at all means that the fuelgauge driver needs to grow some extra properties, specifically it will need to start reporting status, something which the bq24190 driver really has a better idea of the the fuel gauge. Note that I do not have documentation on the fuel-gauge (not even an out of tree driver from android-x86). It is all based on dumping registers every 5 minutes during a charge / discharge cycle and then puzzling together which register is what. I'm quite confident I've this all right, but this does make adding a status property harder (might involve some heuristics instead of being able to get the absolute truth from the charge ic itself). As such I think the power_supply_properties_provider proposal I'm doing might still be a good idea. Anyways I will dig through the dumps I've some more to see if I can find a good way to get charging status from the fuel gauge and get back to you when I'm done with looking at the dumps. Regards, Hans > >> typedef int (*power_supply_properties_provider_get_property_t)( >> enum power_supply_property prop, union power_supply_propval *val, >> void *driver_data); >> >> struct power_supply_properties_provider; >> >> struct power_supply_properties_provider * >> power_supply_properties_provider_register( >> const char *name, >> power_supply_properties_provider_get_property_t get_property, >> const enum power_supply_property *properties, >> size_t num_properties, >> void *driver_data); >> >> void power_supply_properties_provider_unregister( >> struct power_supply_properties_provider *pspp); >> >> struct power_supply_properties_provider * >> power_supply_properties_provider_get(const char *name); >> >> void power_supply_properties_provider_put( >> struct power_supply_properties_provider *pspp); >> >> int power_supply_properties_provider_merge_properties( >> struct power_supply_properties_provider *pspp, >> const enum power_supply_property *properties, >> size_t num_properties, >> enum power_supply_property **merged_properties_ret, >> size_t *merged_num_properties_ret); >> >> int power_supply_properties_provider_get_property( >> struct power_supply_properties_provider *pspp, >> enum power_supply_property prop, >> union power_supply_propval *val); >> >> So in this case the faul-gauge driver would call >> power_supply_properties_provider_register() from probe and >> power_supply_properties_provider_unregister() from remove. >> >> The struct power_supply_properties_provider will be ref-counted >> so that it will stick around after unregister in case any >> consumers who have gotten a ref through >> power_supply_properties_provider_get can still call >> power_supply_properties_provider_get_property, which after >> unregister will simply always return -ENXIO. >> >> A driver wanting to use extra properties like bq24190_charger >> will call power_supply_properties_provider_get (with a name >> provided through platform_data), use a dynamically allocated >> struct power_supply_desc and fill the properties of that >> using power_supply_properties_provider_merge_properties >> to merge its own properties with the >> power_supply_properties_provider's properties and in its >> get_property method have a default label which calls >> power_supply_properties_provider_get_property. >> >> This would still add quite a bit more code then my original >> patch for what may very well end up being a one-of solution, >> but I guess we may encounter this problem more often and >> this will offer a nice generic and clean way for dealing >> with adding extra info from other sources to a power_supply >> driver, so I'm happy to turn this idea / design into working >> code if people like this better then my original patch. >> >> Liam, Sebastian what do you think of the above proposal? >> >> Regards, >> >> Hans >>