| Message ID | 1425638007-9411-4-git-send-email-jenny.tc@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Hi, On Fri, Mar 06, 2015 at 04:03:26PM +0530, Jenny TC wrote: > Introduce power_supply charger control interfaces to control > charging from charging framework like charger-manager. The interfaces > are similar to the existing power supply get/set interfaces, but > introduce a different set of properties in order to differentiate > itself from other power supply properties which exposed in sysfs > > Signed-off-by: Jenny TC <jenny.tc@intel.com> > --- > include/linux/power_supply.h | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index 30145d8e..a80a3ef 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -176,6 +176,32 @@ union power_supply_propval { > struct device; > struct device_node; > > +enum psy_charger_control_property { > + PSY_CHARGER_PROP_ENABLE_CHARGING = 0, > + PSY_CHARGER_PROP_ENABLE_CHARGER, > + PSY_CHARGER_PROP_RESET_WDT, > +}; > + > +/** > + * struct power_supply_charger - power supply charger driver > + * @get_property: get property function to retrieve charger properties defined > + * in enum power_supply_charger_property > + * @set_property: get property function to retrieve charger properties defined > + * in enum power_supply_charger_property > + * > + * This structure is used by charger drivers to register with power supply > + * charging driver > + */ > + > +struct power_supply_charger { > + int (*get_property)(struct power_supply_charger *psyc, > + enum psy_charger_control_property pspc, > + union power_supply_propval *val); The charging framework can simply call the same get_property as used by sysfs. This is already done by all kind of drivers. > + int (*set_property)(struct power_supply_charger *psyc, > + enum psy_charger_control_property pspc, > + const union power_supply_propval *val); I guess this is needed for values, which are supposed to be writable by the kernel / charging framework, but non-writable by the sysfs. I suggest to add set_property_kernel() instead (and make the above properties part of enum power_supply_property) > +}; > + > struct power_supply { > const char *name; > enum power_supply_type type; > @@ -200,6 +226,8 @@ struct power_supply { > void (*external_power_changed)(struct power_supply *psy); > void (*set_charged)(struct power_supply *psy); > > + struct power_supply_charger *psy_charger; Why is this a pointer? > + > /* > * Set if thermal zone should not be created for this power supply. > * For example for virtual supplies forwarding calls to actual -- Sebastian
Hi, > > +struct power_supply_charger { > > + int (*get_property)(struct power_supply_charger *psyc, > > + enum psy_charger_control_property pspc, > > + union power_supply_propval *val); > > The charging framework can simply call the same get_property > as used by sysfs. This is already done by all kind of drivers. The idea is to separate power supply properties from power supply charger properties. Existing power supply properties exposes a generic property of a power supply. But the properties introduced above, is used to control charging. But I agree, if the charger properties are moved to enum power_supply_property{ }, the existing set_property()/get_property() calls can be used > > > + int (*set_property)(struct power_supply_charger *psyc, > > + enum psy_charger_control_property pspc, > > + const union power_supply_propval *val); > > I guess this is needed for values, which are supposed to be > writable by the kernel / charging framework, but non-writable > by the sysfs. I suggest to add set_property_kernel() instead > (and make the above properties part of enum power_supply_property) > If properties are moved to enum power_supply_property {}, then it's possible to reuse the set_property() call. property_is_writeable() can be used to block user space write access. > > +}; > > + > > struct power_supply { > > const char *name; > > enum power_supply_type type; > > @@ -200,6 +226,8 @@ struct power_supply { > > void (*external_power_changed)(struct power_supply *psy); > > void (*set_charged)(struct power_supply *psy); > > > > + struct power_supply_charger *psy_charger; > > Why is this a pointer? This is introduced to access charger properties using power supply object. If the properties can be accessed using existing set_property/get_property(), then this is not really needed -Jenny -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Mar 09, 2015 at 12:47:18PM +0000, Tc, Jenny wrote: > > > +struct power_supply_charger { > > > + int (*get_property)(struct power_supply_charger *psyc, > > > + enum psy_charger_control_property pspc, > > > + union power_supply_propval *val); > > > > The charging framework can simply call the same get_property > > as used by sysfs. This is already done by all kind of drivers. > > The idea is to separate power supply properties from power supply > charger properties. Existing power supply properties exposes a generic > property of a power supply. But the properties introduced above, is used > to control charging. But I agree, if the charger properties are moved to > enum power_supply_property{ }, the existing set_property()/get_property() > calls can be used I think making them part of power_supply_property and re-using existing functions is sensible. > > > + int (*set_property)(struct power_supply_charger *psyc, > > > + enum psy_charger_control_property pspc, > > > + const union power_supply_propval *val); > > > > I guess this is needed for values, which are supposed to be > > writable by the kernel / charging framework, but non-writable > > by the sysfs. I suggest to add set_property_kernel() instead > > (and make the above properties part of enum power_supply_property) > > If properties are moved to enum power_supply_property {}, then it's possible > to reuse the set_property() call. property_is_writeable() can be used to block > user space write access. Right. > > > +}; > > > + > > > struct power_supply { > > > const char *name; > > > enum power_supply_type type; > > > @@ -200,6 +226,8 @@ struct power_supply { > > > void (*external_power_changed)(struct power_supply *psy); > > > void (*set_charged)(struct power_supply *psy); > > > > > > + struct power_supply_charger *psy_charger; > > > > Why is this a pointer? > > This is introduced to access charger properties using power supply > object. The question was why you choose (1) over (2), considering that the struct contains only two pointers. (1) struct power_supply_charger *psy_charger; (2) struct power_supply_charger psy_charger; > If the properties can be accessed using existing > set_property/get_property(), then this is not really needed Right, so don't worry about my comment :) -- Sebastian
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h index 30145d8e..a80a3ef 100644 --- a/include/linux/power_supply.h +++ b/include/linux/power_supply.h @@ -176,6 +176,32 @@ union power_supply_propval { struct device; struct device_node; +enum psy_charger_control_property { + PSY_CHARGER_PROP_ENABLE_CHARGING = 0, + PSY_CHARGER_PROP_ENABLE_CHARGER, + PSY_CHARGER_PROP_RESET_WDT, +}; + +/** + * struct power_supply_charger - power supply charger driver + * @get_property: get property function to retrieve charger properties defined + * in enum power_supply_charger_property + * @set_property: get property function to retrieve charger properties defined + * in enum power_supply_charger_property + * + * This structure is used by charger drivers to register with power supply + * charging driver + */ + +struct power_supply_charger { + int (*get_property)(struct power_supply_charger *psyc, + enum psy_charger_control_property pspc, + union power_supply_propval *val); + int (*set_property)(struct power_supply_charger *psyc, + enum psy_charger_control_property pspc, + const union power_supply_propval *val); +}; + struct power_supply { const char *name; enum power_supply_type type; @@ -200,6 +226,8 @@ struct power_supply { void (*external_power_changed)(struct power_supply *psy); void (*set_charged)(struct power_supply *psy); + struct power_supply_charger *psy_charger; + /* * Set if thermal zone should not be created for this power supply. * For example for virtual supplies forwarding calls to actual
Introduce power_supply charger control interfaces to control charging from charging framework like charger-manager. The interfaces are similar to the existing power supply get/set interfaces, but introduce a different set of properties in order to differentiate itself from other power supply properties which exposed in sysfs Signed-off-by: Jenny TC <jenny.tc@intel.com> --- include/linux/power_supply.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)