RE: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Adam Thomson]

On 09 February 2018 15:02, Sebastian Reichel wrote:

> Hi,
> 
> On Fri, Feb 09, 2018 at 11:28:42AM +, Adam Thomson wrote:
> > On 08 February 2018 21:31, Sebastian Reichel wrote:
> > > On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> > > > This commit adds the 'connected_type' property to represent supplies
> > > > which can report a number of different types of supply based on a
> > > > connection event.
> > > >
> > > > Examples of this already exist in drivers whereby the existing 'type'
> > > > property is updated, based on an event, to represent what was
> > > > connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> > > > however don't show all supported connectable types, so this knowledge
> > > > has to be exlicitly known for each driver that supports this.
> > > >
> > > > The 'connected_type' property is intended to fill this void and show
> > > > users all possible types supported by a driver. The property, when
> > > > read, shows all available types for the driver, and the one currently
> > > > chosen is highlighted/bracketed. It is expected that the 'type'
> > > > property would then just show the top-level type, such as 'USB', and
> > > > this would be static.
> > > >
> > > > Currently the 'conn_type' enum contains all of the USB variant types
> > > > that exist for the 'type' enum at this time, and in addition has
> > > > the PPS type. In the future this can be extended further for other
> > > > types which have multiple connected types supported. The mirroring
> > > > is intentional so as to not impact existing usage of the 'type'
> > > > property.
> > > >
> > > > Signed-off-by: Adam Thomson 
> > >
> > > Thanks for the patch. I think, that it's a good idea to provide the
> > > subtype in its own property and just set the type property to "USB".
> > > I would prefer to name this "usb_type". Otherwise
> > >
> > > Reviewed-by: Sebastian Reichel 
> > >
> > > -- Sebastian
> >
> > Thanks for your review. I chose 'connected_type' as I thought this would be 
> > more
> > generic in case other main types would have some dynamic connection element 
> > to
> > them and could also benefit from this. If we don't see that as being 
> > something
> > we want though then I can make this just for USB.
> 
> If such a thing appears at some point we can add another property.
> I think usb_type is easier to understand from a user point of view
> and it helps to keep our enums clean.

Ok, fair enough. Will update accordingly.

RE: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Adam Thomson]

On 09 February 2018 15:02, Sebastian Reichel wrote:

> Hi,
> 
> On Fri, Feb 09, 2018 at 11:28:42AM +, Adam Thomson wrote:
> > On 08 February 2018 21:31, Sebastian Reichel wrote:
> > > On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> > > > This commit adds the 'connected_type' property to represent supplies
> > > > which can report a number of different types of supply based on a
> > > > connection event.
> > > >
> > > > Examples of this already exist in drivers whereby the existing 'type'
> > > > property is updated, based on an event, to represent what was
> > > > connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> > > > however don't show all supported connectable types, so this knowledge
> > > > has to be exlicitly known for each driver that supports this.
> > > >
> > > > The 'connected_type' property is intended to fill this void and show
> > > > users all possible types supported by a driver. The property, when
> > > > read, shows all available types for the driver, and the one currently
> > > > chosen is highlighted/bracketed. It is expected that the 'type'
> > > > property would then just show the top-level type, such as 'USB', and
> > > > this would be static.
> > > >
> > > > Currently the 'conn_type' enum contains all of the USB variant types
> > > > that exist for the 'type' enum at this time, and in addition has
> > > > the PPS type. In the future this can be extended further for other
> > > > types which have multiple connected types supported. The mirroring
> > > > is intentional so as to not impact existing usage of the 'type'
> > > > property.
> > > >
> > > > Signed-off-by: Adam Thomson 
> > >
> > > Thanks for the patch. I think, that it's a good idea to provide the
> > > subtype in its own property and just set the type property to "USB".
> > > I would prefer to name this "usb_type". Otherwise
> > >
> > > Reviewed-by: Sebastian Reichel 
> > >
> > > -- Sebastian
> >
> > Thanks for your review. I chose 'connected_type' as I thought this would be 
> > more
> > generic in case other main types would have some dynamic connection element 
> > to
> > them and could also benefit from this. If we don't see that as being 
> > something
> > we want though then I can make this just for USB.
> 
> If such a thing appears at some point we can add another property.
> I think usb_type is easier to understand from a user point of view
> and it helps to keep our enums clean.

Ok, fair enough. Will update accordingly.

Re: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Sebastian Reichel]

Hi,

On Fri, Feb 09, 2018 at 11:28:42AM +, Adam Thomson wrote:
> On 08 February 2018 21:31, Sebastian Reichel wrote:
> > On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> > > This commit adds the 'connected_type' property to represent supplies
> > > which can report a number of different types of supply based on a
> > > connection event.
> > >
> > > Examples of this already exist in drivers whereby the existing 'type'
> > > property is updated, based on an event, to represent what was
> > > connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> > > however don't show all supported connectable types, so this knowledge
> > > has to be exlicitly known for each driver that supports this.
> > >
> > > The 'connected_type' property is intended to fill this void and show
> > > users all possible types supported by a driver. The property, when
> > > read, shows all available types for the driver, and the one currently
> > > chosen is highlighted/bracketed. It is expected that the 'type'
> > > property would then just show the top-level type, such as 'USB', and
> > > this would be static.
> > >
> > > Currently the 'conn_type' enum contains all of the USB variant types
> > > that exist for the 'type' enum at this time, and in addition has
> > > the PPS type. In the future this can be extended further for other
> > > types which have multiple connected types supported. The mirroring
> > > is intentional so as to not impact existing usage of the 'type'
> > > property.
> > >
> > > Signed-off-by: Adam Thomson 
> >
> > Thanks for the patch. I think, that it's a good idea to provide the
> > subtype in its own property and just set the type property to "USB".
> > I would prefer to name this "usb_type". Otherwise
> >
> > Reviewed-by: Sebastian Reichel 
> >
> > -- Sebastian
> 
> Thanks for your review. I chose 'connected_type' as I thought this would be 
> more
> generic in case other main types would have some dynamic connection element to
> them and could also benefit from this. If we don't see that as being something
> we want though then I can make this just for USB.

If such a thing appears at some point we can add another property.
I think usb_type is easier to understand from a user point of view
and it helps to keep our enums clean.

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Sebastian Reichel]

Hi,

On Fri, Feb 09, 2018 at 11:28:42AM +, Adam Thomson wrote:
> On 08 February 2018 21:31, Sebastian Reichel wrote:
> > On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> > > This commit adds the 'connected_type' property to represent supplies
> > > which can report a number of different types of supply based on a
> > > connection event.
> > >
> > > Examples of this already exist in drivers whereby the existing 'type'
> > > property is updated, based on an event, to represent what was
> > > connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> > > however don't show all supported connectable types, so this knowledge
> > > has to be exlicitly known for each driver that supports this.
> > >
> > > The 'connected_type' property is intended to fill this void and show
> > > users all possible types supported by a driver. The property, when
> > > read, shows all available types for the driver, and the one currently
> > > chosen is highlighted/bracketed. It is expected that the 'type'
> > > property would then just show the top-level type, such as 'USB', and
> > > this would be static.
> > >
> > > Currently the 'conn_type' enum contains all of the USB variant types
> > > that exist for the 'type' enum at this time, and in addition has
> > > the PPS type. In the future this can be extended further for other
> > > types which have multiple connected types supported. The mirroring
> > > is intentional so as to not impact existing usage of the 'type'
> > > property.
> > >
> > > Signed-off-by: Adam Thomson 
> >
> > Thanks for the patch. I think, that it's a good idea to provide the
> > subtype in its own property and just set the type property to "USB".
> > I would prefer to name this "usb_type". Otherwise
> >
> > Reviewed-by: Sebastian Reichel 
> >
> > -- Sebastian
> 
> Thanks for your review. I chose 'connected_type' as I thought this would be 
> more
> generic in case other main types would have some dynamic connection element to
> them and could also benefit from this. If we don't see that as being something
> we want though then I can make this just for USB.

If such a thing appears at some point we can add another property.
I think usb_type is easier to understand from a user point of view
and it helps to keep our enums clean.

-- Sebastian


signature.asc
Description: PGP signature

RE: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Adam Thomson]

On 08 February 2018 21:31, Sebastian Reichel wrote:

> Hi,
>
> On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> > This commit adds the 'connected_type' property to represent supplies
> > which can report a number of different types of supply based on a
> > connection event.
> >
> > Examples of this already exist in drivers whereby the existing 'type'
> > property is updated, based on an event, to represent what was
> > connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> > however don't show all supported connectable types, so this knowledge
> > has to be exlicitly known for each driver that supports this.
> >
> > The 'connected_type' property is intended to fill this void and show
> > users all possible types supported by a driver. The property, when
> > read, shows all available types for the driver, and the one currently
> > chosen is highlighted/bracketed. It is expected that the 'type'
> > property would then just show the top-level type, such as 'USB', and
> > this would be static.
> >
> > Currently the 'conn_type' enum contains all of the USB variant types
> > that exist for the 'type' enum at this time, and in addition has
> > the PPS type. In the future this can be extended further for other
> > types which have multiple connected types supported. The mirroring
> > is intentional so as to not impact existing usage of the 'type'
> > property.
> >
> > Signed-off-by: Adam Thomson 
>
> Thanks for the patch. I think, that it's a good idea to provide the
> subtype in its own property and just set the type property to "USB".
> I would prefer to name this "usb_type". Otherwise
>
> Reviewed-by: Sebastian Reichel 
>
> -- Sebastian

Thanks for your review. I chose 'connected_type' as I thought this would be more
generic in case other main types would have some dynamic connection element to
them and could also benefit from this. If we don't see that as being something
we want though then I can make this just for USB.

RE: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Adam Thomson]

On 08 February 2018 21:31, Sebastian Reichel wrote:

> Hi,
>
> On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> > This commit adds the 'connected_type' property to represent supplies
> > which can report a number of different types of supply based on a
> > connection event.
> >
> > Examples of this already exist in drivers whereby the existing 'type'
> > property is updated, based on an event, to represent what was
> > connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> > however don't show all supported connectable types, so this knowledge
> > has to be exlicitly known for each driver that supports this.
> >
> > The 'connected_type' property is intended to fill this void and show
> > users all possible types supported by a driver. The property, when
> > read, shows all available types for the driver, and the one currently
> > chosen is highlighted/bracketed. It is expected that the 'type'
> > property would then just show the top-level type, such as 'USB', and
> > this would be static.
> >
> > Currently the 'conn_type' enum contains all of the USB variant types
> > that exist for the 'type' enum at this time, and in addition has
> > the PPS type. In the future this can be extended further for other
> > types which have multiple connected types supported. The mirroring
> > is intentional so as to not impact existing usage of the 'type'
> > property.
> >
> > Signed-off-by: Adam Thomson 
>
> Thanks for the patch. I think, that it's a good idea to provide the
> subtype in its own property and just set the type property to "USB".
> I would prefer to name this "usb_type". Otherwise
>
> Reviewed-by: Sebastian Reichel 
>
> -- Sebastian

Thanks for your review. I chose 'connected_type' as I thought this would be more
generic in case other main types would have some dynamic connection element to
them and could also benefit from this. If we don't see that as being something
we want though then I can make this just for USB.

Re: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Sebastian Reichel]

Hi,

On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> This commit adds the 'connected_type' property to represent supplies
> which can report a number of different types of supply based on a
> connection event.
> 
> Examples of this already exist in drivers whereby the existing 'type'
> property is updated, based on an event, to represent what was
> connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> however don't show all supported connectable types, so this knowledge
> has to be exlicitly known for each driver that supports this.
> 
> The 'connected_type' property is intended to fill this void and show
> users all possible types supported by a driver. The property, when
> read, shows all available types for the driver, and the one currently
> chosen is highlighted/bracketed. It is expected that the 'type'
> property would then just show the top-level type, such as 'USB', and
> this would be static.
> 
> Currently the 'conn_type' enum contains all of the USB variant types
> that exist for the 'type' enum at this time, and in addition has
> the PPS type. In the future this can be extended further for other
> types which have multiple connected types supported. The mirroring
> is intentional so as to not impact existing usage of the 'type'
> property.
> 
> Signed-off-by: Adam Thomson 

Thanks for the patch. I think, that it's a good idea to provide the
subtype in its own property and just set the type property to "USB".
I would prefer to name this "usb_type". Otherwise

Reviewed-by: Sebastian Reichel 

-- Sebastian

> ---
>  drivers/power/supply/power_supply_sysfs.c | 50 
> +++
>  include/linux/power_supply.h  | 15 ++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c 
> b/drivers/power/supply/power_supply_sysfs.c
> index 5204f11..1b3b202 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -46,6 +46,11 @@
>   "USB_PD", "USB_PD_DRP", "BrickID"
>  };
>  
> +static const char * const power_supply_conn_type_text[] = {
> + "Unknown", "USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
> + "USB_PD", "USB_PD_DRP", "USB_PD_PPS", "BrickID"
> +};
> +
>  static const char * const power_supply_status_text[] = {
>   "Unknown", "Charging", "Discharging", "Not charging", "Full"
>  };
> @@ -73,6 +78,46 @@
>   "Unknown", "System", "Device"
>  };
>  
> +static ssize_t power_supply_show_conn_type(struct device *dev,
> +enum power_supply_conn_type 
> *conn_types,
> +ssize_t num_conn_types,
> +union power_supply_propval *value,
> +char *buf)
> +{
> + enum power_supply_conn_type conn_type;
> + ssize_t count = 0;
> + bool match = false;
> + int i;
> +
> + if ((!conn_types) || (num_conn_types <= 0)) {
> + dev_warn(dev, "driver has no valid connected types\n");
> + return -ENODATA;
> + }
> +
> + for (i = 0; i < num_conn_types; ++i) {
> + conn_type = conn_types[i];
> +
> + if (value->intval == conn_type) {
> + count += sprintf(buf + count, "[%s] ",
> +  
> power_supply_conn_type_text[conn_type]);
> + match = true;
> + } else {
> + count += sprintf(buf + count, "%s ",
> +  
> power_supply_conn_type_text[conn_type]);
> + }
> + }
> +
> + if (!match) {
> + dev_warn(dev, "driver reporting unsupported connected type\n");
> + return -EINVAL;
> + }
> +
> + if (count)
> + buf[count - 1] = '\n';
> +
> + return count;
> +}
> +
>  static ssize_t power_supply_show_property(struct device *dev,
> struct device_attribute *attr,
> char *buf) {
> @@ -115,6 +160,10 @@ static ssize_t power_supply_show_property(struct device 
> *dev,
>   else if (off == POWER_SUPPLY_PROP_TYPE)
>   return sprintf(buf, "%s\n",
>  power_supply_type_text[value.intval]);
> + else if (off == POWER_SUPPLY_PROP_CONNECTED_TYPE)
> + return power_supply_show_conn_type(dev, psy->desc->conn_types,
> +psy->desc->num_conn_types,
> +, buf);
>   else if (off == POWER_SUPPLY_PROP_SCOPE)
>   return sprintf(buf, "%s\n",
>  power_supply_scope_text[value.intval]);
> @@ -241,6 +290,7 @@ static ssize_t power_supply_store_property(struct device 
> *dev,
>   POWER_SUPPLY_ATTR(time_to_full_now),
>   

Re: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Sebastian Reichel]

Hi,

On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> This commit adds the 'connected_type' property to represent supplies
> which can report a number of different types of supply based on a
> connection event.
> 
> Examples of this already exist in drivers whereby the existing 'type'
> property is updated, based on an event, to represent what was
> connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> however don't show all supported connectable types, so this knowledge
> has to be exlicitly known for each driver that supports this.
> 
> The 'connected_type' property is intended to fill this void and show
> users all possible types supported by a driver. The property, when
> read, shows all available types for the driver, and the one currently
> chosen is highlighted/bracketed. It is expected that the 'type'
> property would then just show the top-level type, such as 'USB', and
> this would be static.
> 
> Currently the 'conn_type' enum contains all of the USB variant types
> that exist for the 'type' enum at this time, and in addition has
> the PPS type. In the future this can be extended further for other
> types which have multiple connected types supported. The mirroring
> is intentional so as to not impact existing usage of the 'type'
> property.
> 
> Signed-off-by: Adam Thomson 

Thanks for the patch. I think, that it's a good idea to provide the
subtype in its own property and just set the type property to "USB".
I would prefer to name this "usb_type". Otherwise

Reviewed-by: Sebastian Reichel 

-- Sebastian

> ---
>  drivers/power/supply/power_supply_sysfs.c | 50 
> +++
>  include/linux/power_supply.h  | 15 ++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c 
> b/drivers/power/supply/power_supply_sysfs.c
> index 5204f11..1b3b202 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -46,6 +46,11 @@
>   "USB_PD", "USB_PD_DRP", "BrickID"
>  };
>  
> +static const char * const power_supply_conn_type_text[] = {
> + "Unknown", "USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
> + "USB_PD", "USB_PD_DRP", "USB_PD_PPS", "BrickID"
> +};
> +
>  static const char * const power_supply_status_text[] = {
>   "Unknown", "Charging", "Discharging", "Not charging", "Full"
>  };
> @@ -73,6 +78,46 @@
>   "Unknown", "System", "Device"
>  };
>  
> +static ssize_t power_supply_show_conn_type(struct device *dev,
> +enum power_supply_conn_type 
> *conn_types,
> +ssize_t num_conn_types,
> +union power_supply_propval *value,
> +char *buf)
> +{
> + enum power_supply_conn_type conn_type;
> + ssize_t count = 0;
> + bool match = false;
> + int i;
> +
> + if ((!conn_types) || (num_conn_types <= 0)) {
> + dev_warn(dev, "driver has no valid connected types\n");
> + return -ENODATA;
> + }
> +
> + for (i = 0; i < num_conn_types; ++i) {
> + conn_type = conn_types[i];
> +
> + if (value->intval == conn_type) {
> + count += sprintf(buf + count, "[%s] ",
> +  
> power_supply_conn_type_text[conn_type]);
> + match = true;
> + } else {
> + count += sprintf(buf + count, "%s ",
> +  
> power_supply_conn_type_text[conn_type]);
> + }
> + }
> +
> + if (!match) {
> + dev_warn(dev, "driver reporting unsupported connected type\n");
> + return -EINVAL;
> + }
> +
> + if (count)
> + buf[count - 1] = '\n';
> +
> + return count;
> +}
> +
>  static ssize_t power_supply_show_property(struct device *dev,
> struct device_attribute *attr,
> char *buf) {
> @@ -115,6 +160,10 @@ static ssize_t power_supply_show_property(struct device 
> *dev,
>   else if (off == POWER_SUPPLY_PROP_TYPE)
>   return sprintf(buf, "%s\n",
>  power_supply_type_text[value.intval]);
> + else if (off == POWER_SUPPLY_PROP_CONNECTED_TYPE)
> + return power_supply_show_conn_type(dev, psy->desc->conn_types,
> +psy->desc->num_conn_types,
> +, buf);
>   else if (off == POWER_SUPPLY_PROP_SCOPE)
>   return sprintf(buf, "%s\n",
>  power_supply_scope_text[value.intval]);
> @@ -241,6 +290,7 @@ static ssize_t power_supply_store_property(struct device 
> *dev,
>   POWER_SUPPLY_ATTR(time_to_full_now),
>   POWER_SUPPLY_ATTR(time_to_full_avg),
>   POWER_SUPPLY_ATTR(type),
> + 

Re: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Heikki Krogerus]

On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> This commit adds the 'connected_type' property to represent supplies
> which can report a number of different types of supply based on a
> connection event.
> 
> Examples of this already exist in drivers whereby the existing 'type'
> property is updated, based on an event, to represent what was
> connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> however don't show all supported connectable types, so this knowledge
> has to be exlicitly known for each driver that supports this.
> 
> The 'connected_type' property is intended to fill this void and show
> users all possible types supported by a driver. The property, when
> read, shows all available types for the driver, and the one currently
> chosen is highlighted/bracketed. It is expected that the 'type'
> property would then just show the top-level type, such as 'USB', and
> this would be static.
> 
> Currently the 'conn_type' enum contains all of the USB variant types
> that exist for the 'type' enum at this time, and in addition has
> the PPS type. In the future this can be extended further for other
> types which have multiple connected types supported. The mirroring
> is intentional so as to not impact existing usage of the 'type'
> property.
> 
> Signed-off-by: Adam Thomson 

Looks good to me:

Reviewed-by: Heikki Krogerus 


> ---
>  drivers/power/supply/power_supply_sysfs.c | 50 
> +++
>  include/linux/power_supply.h  | 15 ++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c 
> b/drivers/power/supply/power_supply_sysfs.c
> index 5204f11..1b3b202 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -46,6 +46,11 @@
>   "USB_PD", "USB_PD_DRP", "BrickID"
>  };
>  
> +static const char * const power_supply_conn_type_text[] = {
> + "Unknown", "USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
> + "USB_PD", "USB_PD_DRP", "USB_PD_PPS", "BrickID"
> +};
> +
>  static const char * const power_supply_status_text[] = {
>   "Unknown", "Charging", "Discharging", "Not charging", "Full"
>  };
> @@ -73,6 +78,46 @@
>   "Unknown", "System", "Device"
>  };
>  
> +static ssize_t power_supply_show_conn_type(struct device *dev,
> +enum power_supply_conn_type 
> *conn_types,
> +ssize_t num_conn_types,
> +union power_supply_propval *value,
> +char *buf)
> +{
> + enum power_supply_conn_type conn_type;
> + ssize_t count = 0;
> + bool match = false;
> + int i;
> +
> + if ((!conn_types) || (num_conn_types <= 0)) {
> + dev_warn(dev, "driver has no valid connected types\n");
> + return -ENODATA;
> + }
> +
> + for (i = 0; i < num_conn_types; ++i) {
> + conn_type = conn_types[i];
> +
> + if (value->intval == conn_type) {
> + count += sprintf(buf + count, "[%s] ",
> +  
> power_supply_conn_type_text[conn_type]);
> + match = true;
> + } else {
> + count += sprintf(buf + count, "%s ",
> +  
> power_supply_conn_type_text[conn_type]);
> + }
> + }
> +
> + if (!match) {
> + dev_warn(dev, "driver reporting unsupported connected type\n");
> + return -EINVAL;
> + }
> +
> + if (count)
> + buf[count - 1] = '\n';
> +
> + return count;
> +}
> +
>  static ssize_t power_supply_show_property(struct device *dev,
> struct device_attribute *attr,
> char *buf) {
> @@ -115,6 +160,10 @@ static ssize_t power_supply_show_property(struct device 
> *dev,
>   else if (off == POWER_SUPPLY_PROP_TYPE)
>   return sprintf(buf, "%s\n",
>  power_supply_type_text[value.intval]);
> + else if (off == POWER_SUPPLY_PROP_CONNECTED_TYPE)
> + return power_supply_show_conn_type(dev, psy->desc->conn_types,
> +psy->desc->num_conn_types,
> +, buf);
>   else if (off == POWER_SUPPLY_PROP_SCOPE)
>   return sprintf(buf, "%s\n",
>  power_supply_scope_text[value.intval]);
> @@ -241,6 +290,7 @@ static ssize_t power_supply_store_property(struct device 
> *dev,
>   POWER_SUPPLY_ATTR(time_to_full_now),
>   POWER_SUPPLY_ATTR(time_to_full_avg),
>   POWER_SUPPLY_ATTR(type),
> + POWER_SUPPLY_ATTR(connected_type),
>   POWER_SUPPLY_ATTR(scope),
>   POWER_SUPPLY_ATTR(precharge_current),
>   

Re: [PATCH v4 5/7] power: supply: Add 'connected_type' property and supporting code

[Heikki Krogerus]

On Tue, Jan 02, 2018 at 03:50:53PM +, Adam Thomson wrote:
> This commit adds the 'connected_type' property to represent supplies
> which can report a number of different types of supply based on a
> connection event.
> 
> Examples of this already exist in drivers whereby the existing 'type'
> property is updated, based on an event, to represent what was
> connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
> however don't show all supported connectable types, so this knowledge
> has to be exlicitly known for each driver that supports this.
> 
> The 'connected_type' property is intended to fill this void and show
> users all possible types supported by a driver. The property, when
> read, shows all available types for the driver, and the one currently
> chosen is highlighted/bracketed. It is expected that the 'type'
> property would then just show the top-level type, such as 'USB', and
> this would be static.
> 
> Currently the 'conn_type' enum contains all of the USB variant types
> that exist for the 'type' enum at this time, and in addition has
> the PPS type. In the future this can be extended further for other
> types which have multiple connected types supported. The mirroring
> is intentional so as to not impact existing usage of the 'type'
> property.
> 
> Signed-off-by: Adam Thomson 

Looks good to me:

Reviewed-by: Heikki Krogerus 


> ---
>  drivers/power/supply/power_supply_sysfs.c | 50 
> +++
>  include/linux/power_supply.h  | 15 ++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c 
> b/drivers/power/supply/power_supply_sysfs.c
> index 5204f11..1b3b202 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -46,6 +46,11 @@
>   "USB_PD", "USB_PD_DRP", "BrickID"
>  };
>  
> +static const char * const power_supply_conn_type_text[] = {
> + "Unknown", "USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
> + "USB_PD", "USB_PD_DRP", "USB_PD_PPS", "BrickID"
> +};
> +
>  static const char * const power_supply_status_text[] = {
>   "Unknown", "Charging", "Discharging", "Not charging", "Full"
>  };
> @@ -73,6 +78,46 @@
>   "Unknown", "System", "Device"
>  };
>  
> +static ssize_t power_supply_show_conn_type(struct device *dev,
> +enum power_supply_conn_type 
> *conn_types,
> +ssize_t num_conn_types,
> +union power_supply_propval *value,
> +char *buf)
> +{
> + enum power_supply_conn_type conn_type;
> + ssize_t count = 0;
> + bool match = false;
> + int i;
> +
> + if ((!conn_types) || (num_conn_types <= 0)) {
> + dev_warn(dev, "driver has no valid connected types\n");
> + return -ENODATA;
> + }
> +
> + for (i = 0; i < num_conn_types; ++i) {
> + conn_type = conn_types[i];
> +
> + if (value->intval == conn_type) {
> + count += sprintf(buf + count, "[%s] ",
> +  
> power_supply_conn_type_text[conn_type]);
> + match = true;
> + } else {
> + count += sprintf(buf + count, "%s ",
> +  
> power_supply_conn_type_text[conn_type]);
> + }
> + }
> +
> + if (!match) {
> + dev_warn(dev, "driver reporting unsupported connected type\n");
> + return -EINVAL;
> + }
> +
> + if (count)
> + buf[count - 1] = '\n';
> +
> + return count;
> +}
> +
>  static ssize_t power_supply_show_property(struct device *dev,
> struct device_attribute *attr,
> char *buf) {
> @@ -115,6 +160,10 @@ static ssize_t power_supply_show_property(struct device 
> *dev,
>   else if (off == POWER_SUPPLY_PROP_TYPE)
>   return sprintf(buf, "%s\n",
>  power_supply_type_text[value.intval]);
> + else if (off == POWER_SUPPLY_PROP_CONNECTED_TYPE)
> + return power_supply_show_conn_type(dev, psy->desc->conn_types,
> +psy->desc->num_conn_types,
> +, buf);
>   else if (off == POWER_SUPPLY_PROP_SCOPE)
>   return sprintf(buf, "%s\n",
>  power_supply_scope_text[value.intval]);
> @@ -241,6 +290,7 @@ static ssize_t power_supply_store_property(struct device 
> *dev,
>   POWER_SUPPLY_ATTR(time_to_full_now),
>   POWER_SUPPLY_ATTR(time_to_full_avg),
>   POWER_SUPPLY_ATTR(type),
> + POWER_SUPPLY_ATTR(connected_type),
>   POWER_SUPPLY_ATTR(scope),
>   POWER_SUPPLY_ATTR(precharge_current),
>   POWER_SUPPLY_ATTR(charge_term_current),
> diff --git