linux-kernel - Re: [RFC PATCH v2 6/7] typec: tcpm: Represent source supply through power

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171124121902.GA2403@kuha.fi.intel.com>
Date:   Fri, 24 Nov 2017 14:19:02 +0200
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
        Hans de Goede <hdegoede@...hat.com>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sebastian Reichel <sre@...nel.org>,
        Yueyao Zhu <yueyao.zhu@...il.com>,
        Rui Miguel Silva <rmfrfs@...il.com>,
        linux-usb@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, support.opensource@...semi.com
Subject: Re: [RFC PATCH v2 6/7] typec: tcpm: Represent source supply through
 power_supply class

Hi,

On Tue, Nov 14, 2017 at 11:44:47AM +0000, Adam Thomson wrote:
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 78983e1..7c26c3d 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -12,6 +12,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/power_supply.h>
>  #include <linux/proc_fs.h>
>  #include <linux/sched/clock.h>
>  #include <linux/seq_file.h>
> @@ -277,6 +278,10 @@ struct tcpm_port {
>  	u32 current_limit;
>  	u32 supply_voltage;
>  
> +	/* Used to export TA voltage and current */
> +	struct power_supply *psy;
> +	struct power_supply_desc psy_desc;
> +
>  	u32 bist_request;
>  
>  	/* PD state for Vendor Defined Messages */
> @@ -1756,6 +1761,7 @@ static int tcpm_pd_select_pdo(struct tcpm_port *port)
>  	int ret = -EINVAL;
>  
>  	port->pps_data.supported = false;
> +	port->psy_desc.type = POWER_SUPPLY_TYPE_USB_PD;
>  
>  	/*
>  	 * Select the source PDO providing the most power while staying within
> @@ -1775,8 +1781,10 @@ static int tcpm_pd_select_pdo(struct tcpm_port *port)
>  			mv = pdo_min_voltage(pdo);
>  			break;
>  		case PDO_TYPE_APDO:
> -			if (pdo_apdo_type(pdo) == APDO_TYPE_PPS)
> +			if (pdo_apdo_type(pdo) == APDO_TYPE_PPS) {
>  				port->pps_data.supported = true;
> +				port->psy_desc.type = POWER_SUPPLY_TYPE_USB_PD_PPS;
> +			}
>  			continue;
>  		default:
>  			tcpm_log(port, "Invalid PDO type, ignoring");
> @@ -2248,6 +2256,9 @@ static void tcpm_reset_port(struct tcpm_port *port)
>  	port->try_snk_count = 0;
>  	port->supply_voltage = 0;
>  	port->current_limit = 0;
> +	port->psy_desc.type = POWER_SUPPLY_TYPE_USB_TYPE_C;

Is it OK to everybody that the type of the psy is changed like that?
Hans?!

We do have drivers that already change the type, for example
drivers/power/supply/isp1704_charger.c, but what does the user space
expect? The ABI for the power supply class was never documented I
guess.

I'm not against changing the type, but I think that we should have an
attribute file listing all supported types a psy can have if we go
forward with this. Ideally the type file would just list them as space
separated values, and show the current one with asterisk in front of
it. The output would be similar we have with some of the other files
under /sys/power, at least /sys/power/state, but that would break the
ABI.


Thanks,

-- 
heikki