[PATCH 00/18] Hookup typec power-negotation to the PMIC and charger

[PATCH 00/18] Hookup typec power-negotation to the PMIC and charger

[Hans de Goede]

Hi All,

This series implements a number of typec changes discussed a while back:

- It exports the negotiated voltage and max-current in the form of a
  power-supply class device which represents the USB Type-C power-brick
  (adapter/charger)
- It adds a power_supply_set_input_current_limit_from_supplier helper
  function which charger drivers can use to get the max-current from
  their supplier
- It adds regulator support to the charger IC on the device I've. The
  exported regulator controls the 5v boost convertor which generates the
  5V USB vbus which gets output when the Type-C port is in host / power-src
  mode
- It adds a bunch of misc. related fixes and glue code to tie everything
  together

One thing which was undecided in the previous discussion was how to make
port-controller drivers hookup to external ICs (e.g. a non Type-C aware PMIC)
to decect the input-current-limit for USB2 power-sources (through e.g. BC1.2
detection). Since a number of existing drivers, including the one for the
PMIC used on the 2 mini laptops I'm working on, already use the extcon
framework to communicate the detected USB2 charger-type, I've decided to
simply hook into this existing code. As this patch set shows this can be
done with zero changes to the existing PMIC/extcon drivers.

With this series the GPD win and GPD pocket mini laptops both fully
support any type of Type-C charging. When hooked up with:
-A -> C cable and plugged into a regular port they charge at 5V 0.5A
-A -> C cable and plugged into a dedictaed charger they charge at 5V 2A
-C -> C cable and plugged into a fixed 5V 3A charger, at 5V 3A
-C -> C cable and plugged into a PD capable charger, which delivers max 12V, 2A
 they charge at 12V, 2A

And when a Type-C to USB-A receptacle (so host mode) cable gets plugged in
the port correctly supplies 5V to any plugged in USB-A peripherals.

Assuming this series gets a favorable review then the question becomes how
to merge this. This series has staging/typec, drivers/power/supply,
drivers/platform/x86 and drivers/i2c patches.

All these patches can be merged indepently, but the drivers/platform/x86
and drivers/i2c glue patches really should not be applied until the other
patches are in place.

Assuming no one nacks the concept of using a power-supply class device which
represents the USB Type-C power-brick, then all the typec and power patches
can be merged indepdently and as soon as they are deemed ready.

The only exception is the "power: supply: bq24190_charger: Remove extcon
handling" patch, which should not be merged until the drivers/i2c patch
removing the extcon handling from the bq24190 i2c-client instantiated there
has been merged.

Please let me know what you think and feel free to merge any patches you
like as is, then I can do a v2 addressing comments in the remaining patches.

Regards,

Hans

[PATCH 01/18] staging: typec: tcpm: Add get_usb2_current_limit tcpc_dev callback

[Hans de Goede]

A Rp signalling the default current limit indicates that we're possibly
connected to an USB2 power-source. In some cases the type-c
port-controller may provide the capability to detect the current-limit
for USB2 power-sources (through e.g. BC1.2 detection).

This commit adds an optional get_usb2_current_limit tcpc_dev callback
which allows the port-controller to return the detected current-limit
if the CC pin is pulled up with Rp.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/tcpm.c | 5 ++++-
 drivers/staging/typec/tcpm.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
index 20eb4ebcf8c3..9f5adace4309 100644
--- a/drivers/staging/typec/tcpm.c
+++ b/drivers/staging/typec/tcpm.c
@@ -660,7 +660,10 @@ static u32 tcpm_get_current_limit(struct tcpm_port *port)
 		break;
 	case TYPEC_CC_RP_DEF:
 	default:
-		limit = 0;
+		if (port->tcpc->get_usb2_current_limit)
+			limit = port->tcpc->get_usb2_current_limit(port->tcpc);
+		else
+			limit = 0;
 		break;
 	}
 
diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
index 19c307d31a5a..01b7d89379a3 100644
--- a/drivers/staging/typec/tcpm.h
+++ b/drivers/staging/typec/tcpm.h
@@ -108,6 +108,7 @@ struct tcpc_dev {
 
 	int (*init)(struct tcpc_dev *dev);
 	int (*get_vbus)(struct tcpc_dev *dev);
+	int (*get_usb2_current_limit)(struct tcpc_dev *dev); /* Optional */
 	int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc);
 	int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1,
 		      enum typec_cc_status *cc2);
-- 
2.13.3

Re: [PATCH 01/18] staging: typec: tcpm: Add get_usb2_current_limit tcpc_dev callback

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> A Rp signalling the default current limit indicates that we're possibly
> connected to an USB2 power-source. In some cases the type-c
> port-controller may provide the capability to detect the current-limit
> for USB2 power-sources (through e.g. BC1.2 detection).
> 
> This commit adds an optional get_usb2_current_limit tcpc_dev callback
> which allows the port-controller to return the detected current-limit
> if the CC pin is pulled up with Rp.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/staging/typec/tcpm.c | 5 ++++-
>   drivers/staging/typec/tcpm.h | 1 +
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
> index 20eb4ebcf8c3..9f5adace4309 100644
> --- a/drivers/staging/typec/tcpm.c
> +++ b/drivers/staging/typec/tcpm.c
> @@ -660,7 +660,10 @@ static u32 tcpm_get_current_limit(struct tcpm_port *port)
>   		break;
>   	case TYPEC_CC_RP_DEF:
>   	default:
> -		limit = 0;
> +		if (port->tcpc->get_usb2_current_limit)

I think this callback should just be get_current_limit.

> +			limit = port->tcpc->get_usb2_current_limit(port->tcpc);
> +		else
> +			limit = 0;
>   		break;
>   	}
>   
> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
> index 19c307d31a5a..01b7d89379a3 100644
> --- a/drivers/staging/typec/tcpm.h
> +++ b/drivers/staging/typec/tcpm.h
> @@ -108,6 +108,7 @@ struct tcpc_dev {
>   
>   	int (*init)(struct tcpc_dev *dev);
>   	int (*get_vbus)(struct tcpc_dev *dev);
> +	int (*get_usb2_current_limit)(struct tcpc_dev *dev); /* Optional */
>   	int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc);
>   	int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1,
>   		      enum typec_cc_status *cc2);
> 

Re: [PATCH 01/18] staging: typec: tcpm: Add get_usb2_current_limit tcpc_dev callback

[Hans de Goede]

Hi,

On 06-08-17 16:18, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> A Rp signalling the default current limit indicates that we're possibly
>> connected to an USB2 power-source. In some cases the type-c
>> port-controller may provide the capability to detect the current-limit
>> for USB2 power-sources (through e.g. BC1.2 detection).
>>
>> This commit adds an optional get_usb2_current_limit tcpc_dev callback
>> which allows the port-controller to return the detected current-limit
>> if the CC pin is pulled up with Rp.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/staging/typec/tcpm.c | 5 ++++-
>>   drivers/staging/typec/tcpm.h | 1 +
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
>> index 20eb4ebcf8c3..9f5adace4309 100644
>> --- a/drivers/staging/typec/tcpm.c
>> +++ b/drivers/staging/typec/tcpm.c
>> @@ -660,7 +660,10 @@ static u32 tcpm_get_current_limit(struct tcpm_port *port)
>>           break;
>>       case TYPEC_CC_RP_DEF:
>>       default:
>> -        limit = 0;
>> +        if (port->tcpc->get_usb2_current_limit)
> 
> I think this callback should just be get_current_limit.

This is only used in the Rp-def case, which usually indicates being connected
with an A->C cable to a USB-2 device and the intend is for the callback to
implement some USB-2 specific method to detect the supported current-limit,
hence the name. If you prefer to name it just get_current_limit I can
change it for v2, but IMHO the usb2 part of the name is important as this
will not get called when USB-PD negotiation is used, nor when Rp has the Rp15
or Rp30 values.

Regards,

Hans



> 
>> +            limit = port->tcpc->get_usb2_current_limit(port->tcpc);
>> +        else
>> +            limit = 0;
>>           break;
>>       }
>> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
>> index 19c307d31a5a..01b7d89379a3 100644
>> --- a/drivers/staging/typec/tcpm.h
>> +++ b/drivers/staging/typec/tcpm.h
>> @@ -108,6 +108,7 @@ struct tcpc_dev {
>>       int (*init)(struct tcpc_dev *dev);
>>       int (*get_vbus)(struct tcpc_dev *dev);
>> +    int (*get_usb2_current_limit)(struct tcpc_dev *dev); /* Optional */
>>       int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc);
>>       int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1,
>>                 enum typec_cc_status *cc2);
>>
> 

Re: [PATCH 01/18] staging: typec: tcpm: Add get_usb2_current_limit tcpc_dev callback

[Guenter Roeck]

On 08/06/2017 07:29 AM, Hans de Goede wrote:
> Hi,
> 
> On 06-08-17 16:18, Guenter Roeck wrote:
>> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>>> A Rp signalling the default current limit indicates that we're possibly
>>> connected to an USB2 power-source. In some cases the type-c
>>> port-controller may provide the capability to detect the current-limit
>>> for USB2 power-sources (through e.g. BC1.2 detection).
>>>
>>> This commit adds an optional get_usb2_current_limit tcpc_dev callback
>>> which allows the port-controller to return the detected current-limit
>>> if the CC pin is pulled up with Rp.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/staging/typec/tcpm.c | 5 ++++-
>>>   drivers/staging/typec/tcpm.h | 1 +
>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
>>> index 20eb4ebcf8c3..9f5adace4309 100644
>>> --- a/drivers/staging/typec/tcpm.c
>>> +++ b/drivers/staging/typec/tcpm.c
>>> @@ -660,7 +660,10 @@ static u32 tcpm_get_current_limit(struct tcpm_port *port)
>>>           break;
>>>       case TYPEC_CC_RP_DEF:
>>>       default:
>>> -        limit = 0;
>>> +        if (port->tcpc->get_usb2_current_limit)
>>
>> I think this callback should just be get_current_limit.
> 
> This is only used in the Rp-def case, which usually indicates being connected
> with an A->C cable to a USB-2 device and the intend is for the callback to
> implement some USB-2 specific method to detect the supported current-limit,
> hence the name. If you prefer to name it just get_current_limit I can
> change it for v2, but IMHO the usb2 part of the name is important as this
> will not get called when USB-PD negotiation is used, nor when Rp has the Rp15
> or Rp30 values.
> 

As you say, it gets called in the Rp-def case, which can be set anytime.
I'd rather have a generic name and explain in tcpm.h as part of the API
description when it is used/called.

Note that it is also called in SNK_DISCOVERY if cc=Rp-def to calculate
and set the initial current limit, even with pd.

Thanks,
Guenter

> Regards,
> 
> Hans
> 
> 
> 
>>
>>> +            limit = port->tcpc->get_usb2_current_limit(port->tcpc);
>>> +        else
>>> +            limit = 0;
>>>           break;
>>>       }
>>> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
>>> index 19c307d31a5a..01b7d89379a3 100644
>>> --- a/drivers/staging/typec/tcpm.h
>>> +++ b/drivers/staging/typec/tcpm.h
>>> @@ -108,6 +108,7 @@ struct tcpc_dev {
>>>       int (*init)(struct tcpc_dev *dev);
>>>       int (*get_vbus)(struct tcpc_dev *dev);
>>> +    int (*get_usb2_current_limit)(struct tcpc_dev *dev); /* Optional */
>>>       int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc);
>>>       int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1,
>>>                 enum typec_cc_status *cc2);
>>>
>>
> 

[PATCH 02/18] staging: typec: tcpm: Add extcon helper functions for USB2 current limit detect

[Hans de Goede]

Some type-c port-controllers, such as the fusb302 port-controller, rely
on an external device doing USB2 charger-type detection.

Existing PMIC (and charger) drivers already use extcon to communicate the
detected charger-type from the PMIC (extcon) driver to the charger driver.

Rather then inventing a new API for USB2 charger-type detection
specifically for use with the tcpm code, lets simply re-use the existing
support. This will also allow re-using existing PMIC extcon drivers such
as the axp288 and Intel Whiskey Cove drivers as is on devices where these
are combined with a fusb302 (or in the future another port-controller
which relies on external USB2 charger-type detection).

This commit adds a helper function which tcpc drivers can use to easily
hook into existing PMIC extcon drivers for USB2 charger-type detection:

int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/tcpm.c | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/staging/typec/tcpm.h |  6 ++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
index 9f5adace4309..06bb0e640bcf 100644
--- a/drivers/staging/typec/tcpm.c
+++ b/drivers/staging/typec/tcpm.c
@@ -16,6 +16,7 @@
 
 #include <linux/completion.h>
 #include <linux/debugfs.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -3532,6 +3533,45 @@ void tcpm_unregister_port(struct tcpm_port *port)
 }
 EXPORT_SYMBOL_GPL(tcpm_unregister_port);
 
+/* Generic (helper) implementations for some tcpc_dev callbacks */
+int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
+{
+	struct extcon_dev *extcon = tcpc->usb2_extcon;
+	int current_limit = 0;
+	unsigned long timeout;
+
+	if (!extcon)
+		return 0;
+
+	/*
+	 * USB2 Charger detection may still be in progress when we get here,
+	 * this can take upto 600ms, wait 800ms max.
+	 */
+	timeout = jiffies + msecs_to_jiffies(800);
+	do {
+		if (extcon_get_state(extcon, EXTCON_CHG_USB_SDP) == 1) {
+			current_limit = 500;
+			break;
+		}
+
+		if (extcon_get_state(extcon, EXTCON_CHG_USB_CDP) == 1 ||
+		    extcon_get_state(extcon, EXTCON_CHG_USB_ACA) == 1) {
+			current_limit = 1500;
+			break;
+		}
+
+		if (extcon_get_state(extcon, EXTCON_CHG_USB_DCP) == 1) {
+			current_limit = 2000;
+			break;
+		}
+
+		msleep(50);
+	} while (time_before(jiffies, timeout));
+
+	return current_limit;
+}
+EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
+
 MODULE_AUTHOR("Guenter Roeck <groeck@chromium.org>");
 MODULE_DESCRIPTION("USB Type-C Port Manager");
 MODULE_LICENSE("GPL");
diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
index 01b7d89379a3..35e8c1e7dba0 100644
--- a/drivers/staging/typec/tcpm.h
+++ b/drivers/staging/typec/tcpm.h
@@ -16,6 +16,7 @@
 #define __LINUX_USB_TCPM_H
 
 #include <linux/bitops.h>
+#include <linux/extcon.h>
 #include <linux/usb/typec.h>
 #include "pd.h"
 
@@ -126,6 +127,8 @@ struct tcpc_dev {
 	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
 			   const struct pd_message *msg);
 	struct tcpc_mux_dev *mux;
+	/* Used by tcpm_get_usb2_current_limit_extcon helpers */
+	struct extcon_dev *usb2_extcon;
 };
 
 struct tcpm_port;
@@ -151,4 +154,7 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
 void tcpm_pd_hard_reset(struct tcpm_port *port);
 void tcpm_tcpc_reset(struct tcpm_port *port);
 
+/* Generic (helper) implementations for some tcpc_dev callbacks */
+int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);
+
 #endif /* __LINUX_USB_TCPM_H */
-- 
2.13.3

Re: [PATCH 02/18] staging: typec: tcpm: Add extcon helper functions for USB2 current limit detect

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> Some type-c port-controllers, such as the fusb302 port-controller, rely
> on an external device doing USB2 charger-type detection.
> 
> Existing PMIC (and charger) drivers already use extcon to communicate the
> detected charger-type from the PMIC (extcon) driver to the charger driver.
> 
> Rather then inventing a new API for USB2 charger-type detection
> specifically for use with the tcpm code, lets simply re-use the existing
> support. This will also allow re-using existing PMIC extcon drivers such
> as the axp288 and Intel Whiskey Cove drivers as is on devices where these
> are combined with a fusb302 (or in the future another port-controller
> which relies on external USB2 charger-type detection).
> 
> This commit adds a helper function which tcpc drivers can use to easily
> hook into existing PMIC extcon drivers for USB2 charger-type detection:
> 
> int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/staging/typec/tcpm.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   drivers/staging/typec/tcpm.h |  6 ++++++
>   2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
> index 9f5adace4309..06bb0e640bcf 100644
> --- a/drivers/staging/typec/tcpm.c
> +++ b/drivers/staging/typec/tcpm.c
> @@ -16,6 +16,7 @@
>   
>   #include <linux/completion.h>
>   #include <linux/debugfs.h>
> +#include <linux/delay.h>
>   #include <linux/device.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> @@ -3532,6 +3533,45 @@ void tcpm_unregister_port(struct tcpm_port *port)
>   }
>   EXPORT_SYMBOL_GPL(tcpm_unregister_port);
>   
> +/* Generic (helper) implementations for some tcpc_dev callbacks */
> +int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
> +{
> +	struct extcon_dev *extcon = tcpc->usb2_extcon;
> +	int current_limit = 0;
> +	unsigned long timeout;
> +
> +	if (!extcon)
> +		return 0;
> +
> +	/*
> +	 * USB2 Charger detection may still be in progress when we get here,
> +	 * this can take upto 600ms, wait 800ms max.
> +	 */
> +	timeout = jiffies + msecs_to_jiffies(800);
> +	do {
> +		if (extcon_get_state(extcon, EXTCON_CHG_USB_SDP) == 1) {
> +			current_limit = 500;
> +			break;
> +		}
> +
> +		if (extcon_get_state(extcon, EXTCON_CHG_USB_CDP) == 1 ||
> +		    extcon_get_state(extcon, EXTCON_CHG_USB_ACA) == 1) {
> +			current_limit = 1500;
> +			break;
> +		}
> +
> +		if (extcon_get_state(extcon, EXTCON_CHG_USB_DCP) == 1) {
> +			current_limit = 2000;
> +			break;
> +		}
> +
> +		msleep(50);
> +	} while (time_before(jiffies, timeout));
> +
> +	return current_limit;
> +}
> +EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
> +

Not really sure about this one. Should it be part of low level drivers ?

Guenter

>   MODULE_AUTHOR("Guenter Roeck <groeck@chromium.org>");
>   MODULE_DESCRIPTION("USB Type-C Port Manager");
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
> index 01b7d89379a3..35e8c1e7dba0 100644
> --- a/drivers/staging/typec/tcpm.h
> +++ b/drivers/staging/typec/tcpm.h
> @@ -16,6 +16,7 @@
>   #define __LINUX_USB_TCPM_H
>   
>   #include <linux/bitops.h>
> +#include <linux/extcon.h>
>   #include <linux/usb/typec.h>
>   #include "pd.h"
>   
> @@ -126,6 +127,8 @@ struct tcpc_dev {
>   	int (*pd_transmit)(struct tcpc_dev *dev, enum tcpm_transmit_type type,
>   			   const struct pd_message *msg);
>   	struct tcpc_mux_dev *mux;
> +	/* Used by tcpm_get_usb2_current_limit_extcon helpers */
> +	struct extcon_dev *usb2_extcon;
>   };
>   
>   struct tcpm_port;
> @@ -151,4 +154,7 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
>   void tcpm_pd_hard_reset(struct tcpm_port *port);
>   void tcpm_tcpc_reset(struct tcpm_port *port);
>   
> +/* Generic (helper) implementations for some tcpc_dev callbacks */
> +int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);
> +
>   #endif /* __LINUX_USB_TCPM_H */
> 

[PATCH 03/18] staging: typec: tcpm: Split tcpm code into tcpm-core.c and tcpm-helpers.c

[Hans de Goede]

This is a preparation patch for adding more helpers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/Makefile                |  2 +
 drivers/staging/typec/{tcpm.c => tcpm-core.c} | 40 ------------------
 drivers/staging/typec/tcpm-helpers.c          | 60 +++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 40 deletions(-)
 rename drivers/staging/typec/{tcpm.c => tcpm-core.c} (98%)
 create mode 100644 drivers/staging/typec/tcpm-helpers.c

diff --git a/drivers/staging/typec/Makefile b/drivers/staging/typec/Makefile
index 30a7e29cbc9e..bb7472ec04ab 100644
--- a/drivers/staging/typec/Makefile
+++ b/drivers/staging/typec/Makefile
@@ -1,3 +1,5 @@
 obj-$(CONFIG_TYPEC_TCPM)	+= tcpm.o
 obj-$(CONFIG_TYPEC_TCPCI)	+= tcpci.o
 obj-y				+= fusb302/
+
+tcpm-objs			= tcpm-core.o tcpm-helpers.o
\ No newline at end of file
diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm-core.c
similarity index 98%
rename from drivers/staging/typec/tcpm.c
rename to drivers/staging/typec/tcpm-core.c
index 06bb0e640bcf..9f5adace4309 100644
--- a/drivers/staging/typec/tcpm.c
+++ b/drivers/staging/typec/tcpm-core.c
@@ -16,7 +16,6 @@
 
 #include <linux/completion.h>
 #include <linux/debugfs.h>
-#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -3533,45 +3532,6 @@ void tcpm_unregister_port(struct tcpm_port *port)
 }
 EXPORT_SYMBOL_GPL(tcpm_unregister_port);
 
-/* Generic (helper) implementations for some tcpc_dev callbacks */
-int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
-{
-	struct extcon_dev *extcon = tcpc->usb2_extcon;
-	int current_limit = 0;
-	unsigned long timeout;
-
-	if (!extcon)
-		return 0;
-
-	/*
-	 * USB2 Charger detection may still be in progress when we get here,
-	 * this can take upto 600ms, wait 800ms max.
-	 */
-	timeout = jiffies + msecs_to_jiffies(800);
-	do {
-		if (extcon_get_state(extcon, EXTCON_CHG_USB_SDP) == 1) {
-			current_limit = 500;
-			break;
-		}
-
-		if (extcon_get_state(extcon, EXTCON_CHG_USB_CDP) == 1 ||
-		    extcon_get_state(extcon, EXTCON_CHG_USB_ACA) == 1) {
-			current_limit = 1500;
-			break;
-		}
-
-		if (extcon_get_state(extcon, EXTCON_CHG_USB_DCP) == 1) {
-			current_limit = 2000;
-			break;
-		}
-
-		msleep(50);
-	} while (time_before(jiffies, timeout));
-
-	return current_limit;
-}
-EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
-
 MODULE_AUTHOR("Guenter Roeck <groeck@chromium.org>");
 MODULE_DESCRIPTION("USB Type-C Port Manager");
 MODULE_LICENSE("GPL");
diff --git a/drivers/staging/typec/tcpm-helpers.c b/drivers/staging/typec/tcpm-helpers.c
new file mode 100644
index 000000000000..0c87ec9936e1
--- /dev/null
+++ b/drivers/staging/typec/tcpm-helpers.c
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/usb/typec.h>
+
+#include "tcpm.h"
+
+/* Generic (helper) implementations for some tcpc_dev callbacks */
+int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
+{
+	struct extcon_dev *extcon = tcpc->usb2_extcon;
+	int current_limit = 0;
+	unsigned long timeout;
+
+	if (!extcon)
+		return 0;
+
+	/*
+	 * USB2 Charger detection may still be in progress when we get here,
+	 * this can take upto 600ms, wait 800ms max.
+	 */
+	timeout = jiffies + msecs_to_jiffies(800);
+	do {
+		if (extcon_get_state(extcon, EXTCON_CHG_USB_SDP) == 1) {
+			current_limit = 500;
+			break;
+		}
+
+		if (extcon_get_state(extcon, EXTCON_CHG_USB_CDP) == 1 ||
+		    extcon_get_state(extcon, EXTCON_CHG_USB_ACA) == 1) {
+			current_limit = 1500;
+			break;
+		}
+
+		if (extcon_get_state(extcon, EXTCON_CHG_USB_DCP) == 1) {
+			current_limit = 2000;
+			break;
+		}
+
+		msleep(50);
+	} while (time_before(jiffies, timeout));
+
+	return current_limit;
+}
+EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
-- 
2.13.3

[PATCH 04/18] staging: typec: tcpm: Add helpers for exporting current-limit through a psy

[Hans de Goede]

Not all type-c port-controller can control the current-limit directly,
in cases where the current limit can not be controlled directly it needs
to be exported so that another driver (e.g. the charger driver) can pick
the limit up and configure the system accordingly.

The power-supply subsys already provides infrastructure for this,
power-supply devices have the notion of being supplied by another
power-supply and have properties through which we can export the
current-limit.

This commits adds 2 helper functions for use by port-controller drivers
which want to export the current-limit info in this way:

int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
		      const char *name);
int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/tcpm-helpers.c | 66 ++++++++++++++++++++++++++++++++++++
 drivers/staging/typec/tcpm.h         |  9 +++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/staging/typec/tcpm-helpers.c b/drivers/staging/typec/tcpm-helpers.c
index 0c87ec9936e1..8f7699576878 100644
--- a/drivers/staging/typec/tcpm-helpers.c
+++ b/drivers/staging/typec/tcpm-helpers.c
@@ -20,6 +20,60 @@
 
 #include "tcpm.h"
 
+static int tcpm_psy_get_property(struct power_supply *psy,
+				 enum power_supply_property psp,
+				 union power_supply_propval *val)
+{
+	struct tcpc_dev *tcpc = power_supply_get_drvdata(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = tcpc->supply_voltage * 1000; /* mV -> µV */
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+		val->intval = tcpc->current_limit * 1000; /* mA -> µA */
+		break;
+	default:
+		return -ENODATA;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property tcpm_psy_properties[] = {
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+};
+
+int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
+		      const char *name)
+{
+	struct power_supply_config psy_cfg = {};
+	struct power_supply_desc *desc;
+	int ret = 0;
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	desc->name		= name;
+	desc->type		= POWER_SUPPLY_TYPE_USB;
+	desc->properties	= tcpm_psy_properties;
+	desc->num_properties	= ARRAY_SIZE(tcpm_psy_properties);
+	desc->get_property	= tcpm_psy_get_property;
+	psy_cfg.drv_data	= tcpc;
+
+	tcpc->psy = devm_power_supply_register(dev, desc, &psy_cfg);
+	if (IS_ERR(tcpc->psy)) {
+		ret = PTR_ERR(tcpc->psy);
+		tcpc->psy = NULL;
+		dev_err(dev, "Error registering power-supply: %d\n", ret);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tcpm_register_psy);
+
 /* Generic (helper) implementations for some tcpc_dev callbacks */
 int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
 {
@@ -58,3 +112,15 @@ int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
 	return current_limit;
 }
 EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
+
+int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv)
+{
+	tcpc->supply_voltage = mv;
+	tcpc->current_limit = max_ma;
+
+	if (tcpc->psy)
+		power_supply_changed(tcpc->psy);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tcpm_set_current_limit_psy);
diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
index 35e8c1e7dba0..1b1475b487b5 100644
--- a/drivers/staging/typec/tcpm.h
+++ b/drivers/staging/typec/tcpm.h
@@ -17,6 +17,7 @@
 
 #include <linux/bitops.h>
 #include <linux/extcon.h>
+#include <linux/power_supply.h>
 #include <linux/usb/typec.h>
 #include "pd.h"
 
@@ -129,6 +130,10 @@ struct tcpc_dev {
 	struct tcpc_mux_dev *mux;
 	/* Used by tcpm_get_usb2_current_limit_extcon helpers */
 	struct extcon_dev *usb2_extcon;
+	/* Used by tcpm_set_current_limit_psy helpers */
+	struct power_supply *psy;
+	u32 current_limit;
+	u32 supply_voltage;
 };
 
 struct tcpm_port;
@@ -154,7 +159,11 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
 void tcpm_pd_hard_reset(struct tcpm_port *port);
 void tcpm_tcpc_reset(struct tcpm_port *port);
 
+int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
+		      const char *name);
+
 /* Generic (helper) implementations for some tcpc_dev callbacks */
 int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);
+int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);
 
 #endif /* __LINUX_USB_TCPM_H */
-- 
2.13.3

Re: [PATCH 04/18] staging: typec: tcpm: Add helpers for exporting current-limit through a psy

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> Not all type-c port-controller can control the current-limit directly,
> in cases where the current limit can not be controlled directly it needs
> to be exported so that another driver (e.g. the charger driver) can pick
> the limit up and configure the system accordingly.
> 
> The power-supply subsys already provides infrastructure for this,
> power-supply devices have the notion of being supplied by another
> power-supply and have properties through which we can export the
> current-limit.
> 
> This commits adds 2 helper functions for use by port-controller drivers
> which want to export the current-limit info in this way:
> 
> int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
> 		      const char *name);
> int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/staging/typec/tcpm-helpers.c | 66 ++++++++++++++++++++++++++++++++++++
>   drivers/staging/typec/tcpm.h         |  9 +++++
>   2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/staging/typec/tcpm-helpers.c b/drivers/staging/typec/tcpm-helpers.c
> index 0c87ec9936e1..8f7699576878 100644
> --- a/drivers/staging/typec/tcpm-helpers.c
> +++ b/drivers/staging/typec/tcpm-helpers.c
> @@ -20,6 +20,60 @@
>   
>   #include "tcpm.h"
>   
> +static int tcpm_psy_get_property(struct power_supply *psy,
> +				 enum power_supply_property psp,
> +				 union power_supply_propval *val)
> +{
> +	struct tcpc_dev *tcpc = power_supply_get_drvdata(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = tcpc->supply_voltage * 1000; /* mV -> µV */
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		val->intval = tcpc->current_limit * 1000; /* mA -> µA */
> +		break;
> +	default:
> +		return -ENODATA;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property tcpm_psy_properties[] = {
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +};
> +
> +int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
> +		      const char *name)

This is misleading since it relies on devm functions, and there is no unregister function.
Overall I am not too happy with tying this all into tcpm. The functions are not really tcpm
related. Not that I have a better idea how to handle this right now, but conceptually it
doesn't seem correct.

> +{
> +	struct power_supply_config psy_cfg = {};
> +	struct power_supply_desc *desc;
> +	int ret = 0;
> +
> +	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		return -ENOMEM;
> +
> +	desc->name		= name;
> +	desc->type		= POWER_SUPPLY_TYPE_USB;
> +	desc->properties	= tcpm_psy_properties;
> +	desc->num_properties	= ARRAY_SIZE(tcpm_psy_properties);
> +	desc->get_property	= tcpm_psy_get_property;
> +	psy_cfg.drv_data	= tcpc;
> +
> +	tcpc->psy = devm_power_supply_register(dev, desc, &psy_cfg);
> +	if (IS_ERR(tcpc->psy)) {
> +		ret = PTR_ERR(tcpc->psy);
> +		tcpc->psy = NULL;
> +		dev_err(dev, "Error registering power-supply: %d\n", ret);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(tcpm_register_psy);
> +
>   /* Generic (helper) implementations for some tcpc_dev callbacks */
>   int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
>   {
> @@ -58,3 +112,15 @@ int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
>   	return current_limit;
>   }
>   EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
> +
> +int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv)
> +{
> +	tcpc->supply_voltage = mv;
> +	tcpc->current_limit = max_ma;
> +
> +	if (tcpc->psy)
> +		power_supply_changed(tcpc->psy);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tcpm_set_current_limit_psy);
> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
> index 35e8c1e7dba0..1b1475b487b5 100644
> --- a/drivers/staging/typec/tcpm.h
> +++ b/drivers/staging/typec/tcpm.h
> @@ -17,6 +17,7 @@
>   
>   #include <linux/bitops.h>
>   #include <linux/extcon.h>
> +#include <linux/power_supply.h>
>   #include <linux/usb/typec.h>
>   #include "pd.h"
>   
> @@ -129,6 +130,10 @@ struct tcpc_dev {
>   	struct tcpc_mux_dev *mux;
>   	/* Used by tcpm_get_usb2_current_limit_extcon helpers */
>   	struct extcon_dev *usb2_extcon;
> +	/* Used by tcpm_set_current_limit_psy helpers */
> +	struct power_supply *psy;
> +	u32 current_limit;
> +	u32 supply_voltage;
>   };
>   
>   struct tcpm_port;
> @@ -154,7 +159,11 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
>   void tcpm_pd_hard_reset(struct tcpm_port *port);
>   void tcpm_tcpc_reset(struct tcpm_port *port);
>   
> +int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
> +		      const char *name);
> +
>   /* Generic (helper) implementations for some tcpc_dev callbacks */
>   int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);
> +int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);
>   
>   #endif /* __LINUX_USB_TCPM_H */
> 

Re: [PATCH 04/18] staging: typec: tcpm: Add helpers for exporting current-limit through a psy

[Hans de Goede]

Hi,

On 06-08-17 16:13, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> Not all type-c port-controller can control the current-limit directly,
>> in cases where the current limit can not be controlled directly it needs
>> to be exported so that another driver (e.g. the charger driver) can pick
>> the limit up and configure the system accordingly.
>>
>> The power-supply subsys already provides infrastructure for this,
>> power-supply devices have the notion of being supplied by another
>> power-supply and have properties through which we can export the
>> current-limit.
>>
>> This commits adds 2 helper functions for use by port-controller drivers
>> which want to export the current-limit info in this way:
>>
>> int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
>>               const char *name);
>> int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/staging/typec/tcpm-helpers.c | 66 ++++++++++++++++++++++++++++++++++++
>>   drivers/staging/typec/tcpm.h         |  9 +++++
>>   2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/staging/typec/tcpm-helpers.c b/drivers/staging/typec/tcpm-helpers.c
>> index 0c87ec9936e1..8f7699576878 100644
>> --- a/drivers/staging/typec/tcpm-helpers.c
>> +++ b/drivers/staging/typec/tcpm-helpers.c
>> @@ -20,6 +20,60 @@
>>   #include "tcpm.h"
>> +static int tcpm_psy_get_property(struct power_supply *psy,
>> +                 enum power_supply_property psp,
>> +                 union power_supply_propval *val)
>> +{
>> +    struct tcpc_dev *tcpc = power_supply_get_drvdata(psy);
>> +
>> +    switch (psp) {
>> +    case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +        val->intval = tcpc->supply_voltage * 1000; /* mV -> µV */
>> +        break;
>> +    case POWER_SUPPLY_PROP_CURRENT_MAX:
>> +        val->intval = tcpc->current_limit * 1000; /* mA -> µA */
>> +        break;
>> +    default:
>> +        return -ENODATA;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static enum power_supply_property tcpm_psy_properties[] = {
>> +    POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> +    POWER_SUPPLY_PROP_CURRENT_MAX,
>> +};
>> +
>> +int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
>> +              const char *name)
> 
> This is misleading since it relies on devm functions, and there is no unregister function.

The reliance on devm functions is intentional, that makes cleanup on probe() failure for users
of this a lot easier. I guess we could rename this tcpm_initialize_psy(), although it does
actually register a psy, so maybe: devm_tcpm_register_psy() to explain why there is no
unregister counter-part ?

> Overall I am not too happy with tying this all into tcpm. The functions are not really tcpm
> related. Not that I have a better idea how to handle this right now, but conceptually it
> doesn't seem correct.

Note that no changes are made to the tcpm core in this entire patch-set, with the exception
of the first patch which adds the get_usb2_current_limit callback.

This really only ties into the port-controller driver, and as such uses tcpc_dev to store some
stuff. I could make this more clear by prefixing the helper function names with tcpc instead of
tcpm if you think that is better ?

Regards,

Hans


> 
>> +{
>> +    struct power_supply_config psy_cfg = {};
>> +    struct power_supply_desc *desc;
>> +    int ret = 0;
>> +
>> +    desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
>> +    if (!desc)
>> +        return -ENOMEM;
>> +
>> +    desc->name        = name;
>> +    desc->type        = POWER_SUPPLY_TYPE_USB;
>> +    desc->properties    = tcpm_psy_properties;
>> +    desc->num_properties    = ARRAY_SIZE(tcpm_psy_properties);
>> +    desc->get_property    = tcpm_psy_get_property;
>> +    psy_cfg.drv_data    = tcpc;
>> +
>> +    tcpc->psy = devm_power_supply_register(dev, desc, &psy_cfg);
>> +    if (IS_ERR(tcpc->psy)) {
>> +        ret = PTR_ERR(tcpc->psy);
>> +        tcpc->psy = NULL;
>> +        dev_err(dev, "Error registering power-supply: %d\n", ret);
>> +    }
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(tcpm_register_psy);
>> +
>>   /* Generic (helper) implementations for some tcpc_dev callbacks */
>>   int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
>>   {
>> @@ -58,3 +112,15 @@ int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
>>       return current_limit;
>>   }
>>   EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
>> +
>> +int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv)
>> +{
>> +    tcpc->supply_voltage = mv;
>> +    tcpc->current_limit = max_ma;
>> +
>> +    if (tcpc->psy)
>> +        power_supply_changed(tcpc->psy);
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(tcpm_set_current_limit_psy);
>> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
>> index 35e8c1e7dba0..1b1475b487b5 100644
>> --- a/drivers/staging/typec/tcpm.h
>> +++ b/drivers/staging/typec/tcpm.h
>> @@ -17,6 +17,7 @@
>>   #include <linux/bitops.h>
>>   #include <linux/extcon.h>
>> +#include <linux/power_supply.h>
>>   #include <linux/usb/typec.h>
>>   #include "pd.h"
>> @@ -129,6 +130,10 @@ struct tcpc_dev {
>>       struct tcpc_mux_dev *mux;
>>       /* Used by tcpm_get_usb2_current_limit_extcon helpers */
>>       struct extcon_dev *usb2_extcon;
>> +    /* Used by tcpm_set_current_limit_psy helpers */
>> +    struct power_supply *psy;
>> +    u32 current_limit;
>> +    u32 supply_voltage;
>>   };
>>   struct tcpm_port;
>> @@ -154,7 +159,11 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
>>   void tcpm_pd_hard_reset(struct tcpm_port *port);
>>   void tcpm_tcpc_reset(struct tcpm_port *port);
>> +int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
>> +              const char *name);
>> +
>>   /* Generic (helper) implementations for some tcpc_dev callbacks */
>>   int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);
>> +int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);
>>   #endif /* __LINUX_USB_TCPM_H */
>>
> 

Re: [PATCH 04/18] staging: typec: tcpm: Add helpers for exporting current-limit through a psy

[Guenter Roeck]

On 08/06/2017 07:21 AM, Hans de Goede wrote:
> Hi,
> 
> On 06-08-17 16:13, Guenter Roeck wrote:
>> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>>> Not all type-c port-controller can control the current-limit directly,
>>> in cases where the current limit can not be controlled directly it needs
>>> to be exported so that another driver (e.g. the charger driver) can pick
>>> the limit up and configure the system accordingly.
>>>
>>> The power-supply subsys already provides infrastructure for this,
>>> power-supply devices have the notion of being supplied by another
>>> power-supply and have properties through which we can export the
>>> current-limit.
>>>
>>> This commits adds 2 helper functions for use by port-controller drivers
>>> which want to export the current-limit info in this way:
>>>
>>> int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
>>>               const char *name);
>>> int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/staging/typec/tcpm-helpers.c | 66 ++++++++++++++++++++++++++++++++++++
>>>   drivers/staging/typec/tcpm.h         |  9 +++++
>>>   2 files changed, 75 insertions(+)
>>>
>>> diff --git a/drivers/staging/typec/tcpm-helpers.c b/drivers/staging/typec/tcpm-helpers.c
>>> index 0c87ec9936e1..8f7699576878 100644
>>> --- a/drivers/staging/typec/tcpm-helpers.c
>>> +++ b/drivers/staging/typec/tcpm-helpers.c
>>> @@ -20,6 +20,60 @@
>>>   #include "tcpm.h"
>>> +static int tcpm_psy_get_property(struct power_supply *psy,
>>> +                 enum power_supply_property psp,
>>> +                 union power_supply_propval *val)
>>> +{
>>> +    struct tcpc_dev *tcpc = power_supply_get_drvdata(psy);
>>> +
>>> +    switch (psp) {
>>> +    case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>>> +        val->intval = tcpc->supply_voltage * 1000; /* mV -> µV */
>>> +        break;
>>> +    case POWER_SUPPLY_PROP_CURRENT_MAX:
>>> +        val->intval = tcpc->current_limit * 1000; /* mA -> µA */
>>> +        break;
>>> +    default:
>>> +        return -ENODATA;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static enum power_supply_property tcpm_psy_properties[] = {
>>> +    POWER_SUPPLY_PROP_VOLTAGE_NOW,
>>> +    POWER_SUPPLY_PROP_CURRENT_MAX,
>>> +};
>>> +
>>> +int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
>>> +              const char *name)
>>
>> This is misleading since it relies on devm functions, and there is no unregister function.
> 
> The reliance on devm functions is intentional, that makes cleanup on probe() failure for users
> of this a lot easier. I guess we could rename this tcpm_initialize_psy(), although it does
> actually register a psy, so maybe: devm_tcpm_register_psy() to explain why there is no
> unregister counter-part ?
> 

I don't question the use of devm functions. Yes, I think devm_tcpm_register_psy() would be better.

>> Overall I am not too happy with tying this all into tcpm. The functions are not really tcpm
>> related. Not that I have a better idea how to handle this right now, but conceptually it
>> doesn't seem correct.
> 
> Note that no changes are made to the tcpm core in this entire patch-set, with the exception
> of the first patch which adds the get_usb2_current_limit callback.
> 
Yes, but you are using tcpm data structures. Of this entire series, I think only the callback
(renamed to get_usb_current_limit) should really be in tcpm code. Maybe it makes sense to find
a way to provide helpers in a generic way, if it turns out that the same code is needed by multiple
drivers, but right now they are only used by fusb302 and might as well stay there.

Until other drivers need them, we don't really know if the helpers are useful for multiple drivers.
I would prefer to add such helpers if and when we have more than one driver using them.

Thanks,
Guenter

> This really only ties into the port-controller driver, and as such uses tcpc_dev to store some
> stuff. I could make this more clear by prefixing the helper function names with tcpc instead of
> tcpm if you think that is better ?
> 
> Regards,
> 
> Hans
> 
> 
>>
>>> +{
>>> +    struct power_supply_config psy_cfg = {};
>>> +    struct power_supply_desc *desc;
>>> +    int ret = 0;
>>> +
>>> +    desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
>>> +    if (!desc)
>>> +        return -ENOMEM;
>>> +
>>> +    desc->name        = name;
>>> +    desc->type        = POWER_SUPPLY_TYPE_USB;
>>> +    desc->properties    = tcpm_psy_properties;
>>> +    desc->num_properties    = ARRAY_SIZE(tcpm_psy_properties);
>>> +    desc->get_property    = tcpm_psy_get_property;
>>> +    psy_cfg.drv_data    = tcpc;
>>> +
>>> +    tcpc->psy = devm_power_supply_register(dev, desc, &psy_cfg);
>>> +    if (IS_ERR(tcpc->psy)) {
>>> +        ret = PTR_ERR(tcpc->psy);
>>> +        tcpc->psy = NULL;
>>> +        dev_err(dev, "Error registering power-supply: %d\n", ret);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(tcpm_register_psy);
>>> +
>>>   /* Generic (helper) implementations for some tcpc_dev callbacks */
>>>   int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
>>>   {
>>> @@ -58,3 +112,15 @@ int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc)
>>>       return current_limit;
>>>   }
>>>   EXPORT_SYMBOL_GPL(tcpm_get_usb2_current_limit_extcon);
>>> +
>>> +int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv)
>>> +{
>>> +    tcpc->supply_voltage = mv;
>>> +    tcpc->current_limit = max_ma;
>>> +
>>> +    if (tcpc->psy)
>>> +        power_supply_changed(tcpc->psy);
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(tcpm_set_current_limit_psy);
>>> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
>>> index 35e8c1e7dba0..1b1475b487b5 100644
>>> --- a/drivers/staging/typec/tcpm.h
>>> +++ b/drivers/staging/typec/tcpm.h
>>> @@ -17,6 +17,7 @@
>>>   #include <linux/bitops.h>
>>>   #include <linux/extcon.h>
>>> +#include <linux/power_supply.h>
>>>   #include <linux/usb/typec.h>
>>>   #include "pd.h"
>>> @@ -129,6 +130,10 @@ struct tcpc_dev {
>>>       struct tcpc_mux_dev *mux;
>>>       /* Used by tcpm_get_usb2_current_limit_extcon helpers */
>>>       struct extcon_dev *usb2_extcon;
>>> +    /* Used by tcpm_set_current_limit_psy helpers */
>>> +    struct power_supply *psy;
>>> +    u32 current_limit;
>>> +    u32 supply_voltage;
>>>   };
>>>   struct tcpm_port;
>>> @@ -154,7 +159,11 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
>>>   void tcpm_pd_hard_reset(struct tcpm_port *port);
>>>   void tcpm_tcpc_reset(struct tcpm_port *port);
>>> +int tcpm_register_psy(struct device *dev, struct tcpc_dev *tcpc,
>>> +              const char *name);
>>> +
>>>   /* Generic (helper) implementations for some tcpc_dev callbacks */
>>>   int tcpm_get_usb2_current_limit_extcon(struct tcpc_dev *tcpc);
>>> +int tcpm_set_current_limit_psy(struct tcpc_dev *tcpc, u32 max_ma, u32 mv);
>>>   #endif /* __LINUX_USB_TCPM_H */
>>>
>>
> 

[PATCH 05/18] staging: typec: fusb302: Set max supply voltage to 5V

[Hans de Goede]

Anything higher then 5V may damage hardware not capable of it, so
the only sane default here is 5V. If a board is able to handle a
higher voltage that should come from board specific data such as
device-tree and not be hard coded into the fusb302 code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/fusb302/fusb302.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
index 03a3809d18f0..6baed06a3c0d 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -1187,9 +1187,9 @@ static const struct tcpc_config fusb302_tcpc_config = {
 	.nr_src_pdo = ARRAY_SIZE(src_pdo),
 	.snk_pdo = snk_pdo,
 	.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
-	.max_snk_mv = 9000,
+	.max_snk_mv = 5000,
 	.max_snk_ma = 3000,
-	.max_snk_mw = 27000,
+	.max_snk_mw = 15000,
 	.operating_snk_mw = 2500,
 	.type = TYPEC_PORT_DRP,
 	.default_role = TYPEC_SINK,
-- 
2.13.3

[PATCH 06/18] staging: typec: fusb302: Get max snk mv/ma/mw from device-properties

[Hans de Goede]

This is board specific info so it should come from board config, such
as devicetree.

I've chosen to prefix these with "fcs," treating them as fusb302 driver
specific. We may want to revisit to replace these with properties which
are part of a (to be written) generic type-c controller devicetree
binding.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/TODO              |  5 +++++
 drivers/staging/typec/fusb302/fusb302.c | 18 +++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/typec/TODO b/drivers/staging/typec/TODO
index bc1f97a2d1bf..98b0dbd5752b 100644
--- a/drivers/staging/typec/TODO
+++ b/drivers/staging/typec/TODO
@@ -11,5 +11,10 @@ tcpm:
 tcpci:
 - Test with real hardware
 
+fusb302:
+- We may want to replace the  "fcs,max-snk-mv", "fcs,max-snk-ma",
+  "fcs,max-snk-mw" and "fcs,operating-snk-mw" device(tree) properties with
+  properties which are part of a generic type-c controller devicetree binding.
+
 Please send patches to Guenter Roeck <linux@roeck-us.net> and copy
 Heikki Krogerus <heikki.krogerus@linux.intel.com>.
diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
index 6baed06a3c0d..8ceaef8ecf17 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -90,6 +90,7 @@ struct fusb302_chip {
 	struct i2c_client *i2c_client;
 	struct tcpm_port *tcpm_port;
 	struct tcpc_dev tcpc_dev;
+	struct tcpc_config tcpc_config;
 
 	struct regulator *vbus;
 
@@ -1198,7 +1199,6 @@ static const struct tcpc_config fusb302_tcpc_config = {
 
 static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
 {
-	fusb302_tcpc_dev->config = &fusb302_tcpc_config;
 	fusb302_tcpc_dev->init = tcpm_init;
 	fusb302_tcpc_dev->get_vbus = tcpm_get_vbus;
 	fusb302_tcpc_dev->set_cc = tcpm_set_cc;
@@ -1684,7 +1684,9 @@ static int fusb302_probe(struct i2c_client *client,
 {
 	struct fusb302_chip *chip;
 	struct i2c_adapter *adapter;
+	struct device *dev = &client->dev;
 	int ret = 0;
+	u32 val;
 
 	adapter = to_i2c_adapter(client->dev.parent);
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
@@ -1699,8 +1701,22 @@ static int fusb302_probe(struct i2c_client *client,
 	chip->i2c_client = client;
 	i2c_set_clientdata(client, chip);
 	chip->dev = &client->dev;
+	chip->tcpc_config = fusb302_tcpc_config;
+	chip->tcpc_dev.config = &chip->tcpc_config;
 	mutex_init(&chip->lock);
 
+	if (device_property_read_u32(dev, "fcs,max-snk-mv", &val) == 0)
+		chip->tcpc_config.max_snk_mv = val;
+
+	if (device_property_read_u32(dev, "fcs,max-snk-ma", &val) == 0)
+		chip->tcpc_config.max_snk_ma = val;
+
+	if (device_property_read_u32(dev, "fcs,max-snk-mw", &val) == 0)
+		chip->tcpc_config.max_snk_mw = val;
+
+	if (device_property_read_u32(dev, "fcs,operating-snk-mw", &val) == 0)
+		chip->tcpc_config.operating_snk_mw = val;
+
 	ret = fusb302_debugfs_init(chip);
 	if (ret < 0)
 		return ret;
-- 
2.13.3

Re: [PATCH 06/18] staging: typec: fusb302: Get max snk mv/ma/mw from device-properties

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> This is board specific info so it should come from board config, such
> as devicetree.
> 
> I've chosen to prefix these with "fcs," treating them as fusb302 driver
> specific. We may want to revisit to replace these with properties which
> are part of a (to be written) generic type-c controller devicetree
> binding.
> 
You might want to add dt maintainers here.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > ---
>   drivers/staging/typec/TODO              |  5 +++++
>   drivers/staging/typec/fusb302/fusb302.c | 18 +++++++++++++++++-
>   2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/typec/TODO b/drivers/staging/typec/TODO
> index bc1f97a2d1bf..98b0dbd5752b 100644
> --- a/drivers/staging/typec/TODO
> +++ b/drivers/staging/typec/TODO
> @@ -11,5 +11,10 @@ tcpm:
>   tcpci:
>   - Test with real hardware
>   
> +fusb302:
> +- We may want to replace the  "fcs,max-snk-mv", "fcs,max-snk-ma",
> +  "fcs,max-snk-mw" and "fcs,operating-snk-mw" device(tree) properties with
> +  properties which are part of a generic type-c controller devicetree binding.
> +
I think preferred units would be -microvolt, -,icroamp, and -microwatt.

>   Please send patches to Guenter Roeck <linux@roeck-us.net> and copy
>   Heikki Krogerus <heikki.krogerus@linux.intel.com>.
> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
> index 6baed06a3c0d..8ceaef8ecf17 100644
> --- a/drivers/staging/typec/fusb302/fusb302.c
> +++ b/drivers/staging/typec/fusb302/fusb302.c
> @@ -90,6 +90,7 @@ struct fusb302_chip {
>   	struct i2c_client *i2c_client;
>   	struct tcpm_port *tcpm_port;
>   	struct tcpc_dev tcpc_dev;
> +	struct tcpc_config tcpc_config;
>   
>   	struct regulator *vbus;
>   
> @@ -1198,7 +1199,6 @@ static const struct tcpc_config fusb302_tcpc_config = {
>   
>   static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
>   {
> -	fusb302_tcpc_dev->config = &fusb302_tcpc_config;
>   	fusb302_tcpc_dev->init = tcpm_init;
>   	fusb302_tcpc_dev->get_vbus = tcpm_get_vbus;
>   	fusb302_tcpc_dev->set_cc = tcpm_set_cc;
> @@ -1684,7 +1684,9 @@ static int fusb302_probe(struct i2c_client *client,
>   {
>   	struct fusb302_chip *chip;
>   	struct i2c_adapter *adapter;
> +	struct device *dev = &client->dev;
>   	int ret = 0;
> +	u32 val;
>   
>   	adapter = to_i2c_adapter(client->dev.parent);
>   	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
> @@ -1699,8 +1701,22 @@ static int fusb302_probe(struct i2c_client *client,
>   	chip->i2c_client = client;
>   	i2c_set_clientdata(client, chip);
>   	chip->dev = &client->dev;
> +	chip->tcpc_config = fusb302_tcpc_config;
> +	chip->tcpc_dev.config = &chip->tcpc_config;
>   	mutex_init(&chip->lock);
>   
> +	if (device_property_read_u32(dev, "fcs,max-snk-mv", &val) == 0)
> +		chip->tcpc_config.max_snk_mv = val;
> +
> +	if (device_property_read_u32(dev, "fcs,max-snk-ma", &val) == 0)
> +		chip->tcpc_config.max_snk_ma = val;
> +
> +	if (device_property_read_u32(dev, "fcs,max-snk-mw", &val) == 0)
> +		chip->tcpc_config.max_snk_mw = val;
> +
> +	if (device_property_read_u32(dev, "fcs,operating-snk-mw", &val) == 0)
> +		chip->tcpc_config.operating_snk_mw = val;
> +
>   	ret = fusb302_debugfs_init(chip);
>   	if (ret < 0)
>   		return ret;
> 

[PATCH 07/18] staging: typec: fusb302: Use client->irq as irq if set

[Hans de Goede]

The fusb302 is also used on x86 systems where the platform code sets
the irq in client->irq and there is no gpio named fcs,int_n.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/fusb302/fusb302.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
index 8ceaef8ecf17..be454b5ff6c7 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -1735,9 +1735,13 @@ static int fusb302_probe(struct i2c_client *client,
 		goto destroy_workqueue;
 	}
 
-	ret = init_gpio(chip);
-	if (ret < 0)
-		goto destroy_workqueue;
+	if (client->irq) {
+		chip->gpio_int_n_irq = client->irq;
+	} else {
+		ret = init_gpio(chip);
+		if (ret < 0)
+			goto destroy_workqueue;
+	}
 
 	chip->tcpm_port = tcpm_register_port(&client->dev, &chip->tcpc_dev);
 	if (IS_ERR(chip->tcpm_port)) {
-- 
2.13.3

[PATCH 08/18] staging: typec: fusb302: Add support for USB2 charger detection through extcon

[Hans de Goede]

The fusb302 port-controller relies on an external device doing USB2
charger-type detection.

The Intel Whiskey Cove PMIC with which the fusb302 is combined on some
X86/ACPI platforms already has a charger-type detection driver which
uses extcon to communicate the detected charger-type.

This commit uses the tcpm_get_usb2_current_limit_extcon helper to enable
USB2 charger detection on these systems. Note that the "fcs,extcon-name"
property name is only for kernel internal use by X86/ACPI platform code
and as such is NOT documented in the devicetree bindings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/fusb302/fusb302.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
index be454b5ff6c7..1d8c9b66df2f 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -1201,6 +1201,8 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
 {
 	fusb302_tcpc_dev->init = tcpm_init;
 	fusb302_tcpc_dev->get_vbus = tcpm_get_vbus;
+	fusb302_tcpc_dev->get_usb2_current_limit =
+		tcpm_get_usb2_current_limit_extcon;
 	fusb302_tcpc_dev->set_cc = tcpm_set_cc;
 	fusb302_tcpc_dev->get_cc = tcpm_get_cc;
 	fusb302_tcpc_dev->set_polarity = tcpm_set_polarity;
@@ -1685,6 +1687,7 @@ static int fusb302_probe(struct i2c_client *client,
 	struct fusb302_chip *chip;
 	struct i2c_adapter *adapter;
 	struct device *dev = &client->dev;
+	const char *name;
 	int ret = 0;
 	u32 val;
 
@@ -1717,6 +1720,19 @@ static int fusb302_probe(struct i2c_client *client,
 	if (device_property_read_u32(dev, "fcs,operating-snk-mw", &val) == 0)
 		chip->tcpc_config.operating_snk_mw = val;
 
+	/*
+	 * Devicetree platforms should get extcon via phandle (not yet
+	 * supported). On ACPI platforms, we get the name from a device prop.
+	 * This device prop is for kernel internal use only and is expected
+	 * to be set by the platform code which also registers the i2c client
+	 * for the fusb302.
+	 */
+	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
+		chip->tcpc_dev.usb2_extcon = extcon_get_extcon_dev(name);
+		if (!chip->tcpc_dev.usb2_extcon)
+			return -EPROBE_DEFER;
+	}
+
 	ret = fusb302_debugfs_init(chip);
 	if (ret < 0)
 		return ret;
-- 
2.13.3

Re: [PATCH 08/18] staging: typec: fusb302: Add support for USB2 charger detection through extcon

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> The fusb302 port-controller relies on an external device doing USB2
> charger-type detection.
> 
> The Intel Whiskey Cove PMIC with which the fusb302 is combined on some
> X86/ACPI platforms already has a charger-type detection driver which
> uses extcon to communicate the detected charger-type.
> 
> This commit uses the tcpm_get_usb2_current_limit_extcon helper to enable
> USB2 charger detection on these systems. Note that the "fcs,extcon-name"
> property name is only for kernel internal use by X86/ACPI platform code
> and as such is NOT documented in the devicetree bindings.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/staging/typec/fusb302/fusb302.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
> index be454b5ff6c7..1d8c9b66df2f 100644
> --- a/drivers/staging/typec/fusb302/fusb302.c
> +++ b/drivers/staging/typec/fusb302/fusb302.c
> @@ -1201,6 +1201,8 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
>   {
>   	fusb302_tcpc_dev->init = tcpm_init;
>   	fusb302_tcpc_dev->get_vbus = tcpm_get_vbus;
> +	fusb302_tcpc_dev->get_usb2_current_limit =
> +		tcpm_get_usb2_current_limit_extcon;
>   	fusb302_tcpc_dev->set_cc = tcpm_set_cc;
>   	fusb302_tcpc_dev->get_cc = tcpm_get_cc;
>   	fusb302_tcpc_dev->set_polarity = tcpm_set_polarity;
> @@ -1685,6 +1687,7 @@ static int fusb302_probe(struct i2c_client *client,
>   	struct fusb302_chip *chip;
>   	struct i2c_adapter *adapter;
>   	struct device *dev = &client->dev;
> +	const char *name;
>   	int ret = 0;
>   	u32 val;
>   
> @@ -1717,6 +1720,19 @@ static int fusb302_probe(struct i2c_client *client,
>   	if (device_property_read_u32(dev, "fcs,operating-snk-mw", &val) == 0)
>   		chip->tcpc_config.operating_snk_mw = val;
>   
> +	/*
> +	 * Devicetree platforms should get extcon via phandle (not yet
> +	 * supported). On ACPI platforms, we get the name from a device prop.
> +	 * This device prop is for kernel internal use only and is expected
> +	 * to be set by the platform code which also registers the i2c client
> +	 * for the fusb302.
> +	 */
> +	if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {

Those new devicetree properties need to be documented and approved by dt maintainers.

> +		chip->tcpc_dev.usb2_extcon = extcon_get_extcon_dev(name);
> +		if (!chip->tcpc_dev.usb2_extcon)

extcon_get_extcon_dev() can also return an ERR_PTR.

As before, I am not really happy typing this into tcpm via tcpc_dev.
Until we have a better solution, I would prefer for that code to stay with the fusb302
code.

> +			return -EPROBE_DEFER;
> +	}
> +
>   	ret = fusb302_debugfs_init(chip);
>   	if (ret < 0)
>   		return ret;
> 

Re: [PATCH 08/18] staging: typec: fusb302: Add support for USB2 charger detection through extcon

[Hans de Goede]

Hi,

On 06-08-17 16:22, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> The fusb302 port-controller relies on an external device doing USB2
>> charger-type detection.
>>
>> The Intel Whiskey Cove PMIC with which the fusb302 is combined on some
>> X86/ACPI platforms already has a charger-type detection driver which
>> uses extcon to communicate the detected charger-type.
>>
>> This commit uses the tcpm_get_usb2_current_limit_extcon helper to enable
>> USB2 charger detection on these systems. Note that the "fcs,extcon-name"
>> property name is only for kernel internal use by X86/ACPI platform code
>> and as such is NOT documented in the devicetree bindings.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/staging/typec/fusb302/fusb302.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
>> index be454b5ff6c7..1d8c9b66df2f 100644
>> --- a/drivers/staging/typec/fusb302/fusb302.c
>> +++ b/drivers/staging/typec/fusb302/fusb302.c
>> @@ -1201,6 +1201,8 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
>>   {
>>       fusb302_tcpc_dev->init = tcpm_init;
>>       fusb302_tcpc_dev->get_vbus = tcpm_get_vbus;
>> +    fusb302_tcpc_dev->get_usb2_current_limit =
>> +        tcpm_get_usb2_current_limit_extcon;
>>       fusb302_tcpc_dev->set_cc = tcpm_set_cc;
>>       fusb302_tcpc_dev->get_cc = tcpm_get_cc;
>>       fusb302_tcpc_dev->set_polarity = tcpm_set_polarity;
>> @@ -1685,6 +1687,7 @@ static int fusb302_probe(struct i2c_client *client,
>>       struct fusb302_chip *chip;
>>       struct i2c_adapter *adapter;
>>       struct device *dev = &client->dev;
>> +    const char *name;
>>       int ret = 0;
>>       u32 val;
>> @@ -1717,6 +1720,19 @@ static int fusb302_probe(struct i2c_client *client,
>>       if (device_property_read_u32(dev, "fcs,operating-snk-mw", &val) == 0)
>>           chip->tcpc_config.operating_snk_mw = val;
>> +    /*
>> +     * Devicetree platforms should get extcon via phandle (not yet
>> +     * supported). On ACPI platforms, we get the name from a device prop.
>> +     * This device prop is for kernel internal use only and is expected
>> +     * to be set by the platform code which also registers the i2c client
>> +     * for the fusb302.
>> +     */
>> +    if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
> 
> Those new devicetree properties need to be documented and approved by dt maintainers.

As indicated by the comment, this property should not be used in the devicetree
case, notice this is a device-property and not an of property and since it is
not intended to be used with devicetree at all (in devicetree a phandle rather
then a name would be used), it has no place under Documentation/devicetree at all.

Also there is no current binding documentation for the "fcs,fusb302" compatible
and the weird "fcs,int_n" gpio which really is an irq which it already uses.


> 
>> +        chip->tcpc_dev.usb2_extcon = extcon_get_extcon_dev(name);
>> +        if (!chip->tcpc_dev.usb2_extcon)
> 
> extcon_get_extcon_dev() can also return an ERR_PTR.
> 
> As before, I am not really happy typing this into tcpm via tcpc_dev.
> Until we have a better solution, I would prefer for that code to stay with the fusb302
> code.

Ok, I tried to make this all re-usable since I think other port-controller drivers
will need something similar, but I can kill the entire tcpm-helpers.c file in v2
and then put everything in fusb302.c. I take it that that is what you want ?

Regards,

Hans

Re: [PATCH 08/18] staging: typec: fusb302: Add support for USB2 charger detection through extcon

[Guenter Roeck]

On 08/06/2017 07:36 AM, Hans de Goede wrote:
> Hi,
> 
> On 06-08-17 16:22, Guenter Roeck wrote:
>> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>>> The fusb302 port-controller relies on an external device doing USB2
>>> charger-type detection.
>>>
>>> The Intel Whiskey Cove PMIC with which the fusb302 is combined on some
>>> X86/ACPI platforms already has a charger-type detection driver which
>>> uses extcon to communicate the detected charger-type.
>>>
>>> This commit uses the tcpm_get_usb2_current_limit_extcon helper to enable
>>> USB2 charger detection on these systems. Note that the "fcs,extcon-name"
>>> property name is only for kernel internal use by X86/ACPI platform code
>>> and as such is NOT documented in the devicetree bindings.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/staging/typec/fusb302/fusb302.c | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
>>> index be454b5ff6c7..1d8c9b66df2f 100644
>>> --- a/drivers/staging/typec/fusb302/fusb302.c
>>> +++ b/drivers/staging/typec/fusb302/fusb302.c
>>> @@ -1201,6 +1201,8 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
>>>   {
>>>       fusb302_tcpc_dev->init = tcpm_init;
>>>       fusb302_tcpc_dev->get_vbus = tcpm_get_vbus;
>>> +    fusb302_tcpc_dev->get_usb2_current_limit =
>>> +        tcpm_get_usb2_current_limit_extcon;
>>>       fusb302_tcpc_dev->set_cc = tcpm_set_cc;
>>>       fusb302_tcpc_dev->get_cc = tcpm_get_cc;
>>>       fusb302_tcpc_dev->set_polarity = tcpm_set_polarity;
>>> @@ -1685,6 +1687,7 @@ static int fusb302_probe(struct i2c_client *client,
>>>       struct fusb302_chip *chip;
>>>       struct i2c_adapter *adapter;
>>>       struct device *dev = &client->dev;
>>> +    const char *name;
>>>       int ret = 0;
>>>       u32 val;
>>> @@ -1717,6 +1720,19 @@ static int fusb302_probe(struct i2c_client *client,
>>>       if (device_property_read_u32(dev, "fcs,operating-snk-mw", &val) == 0)
>>>           chip->tcpc_config.operating_snk_mw = val;
>>> +    /*
>>> +     * Devicetree platforms should get extcon via phandle (not yet
>>> +     * supported). On ACPI platforms, we get the name from a device prop.
>>> +     * This device prop is for kernel internal use only and is expected
>>> +     * to be set by the platform code which also registers the i2c client
>>> +     * for the fusb302.
>>> +     */
>>> +    if (device_property_read_string(dev, "fcs,extcon-name", &name) == 0) {
>>
>> Those new devicetree properties need to be documented and approved by dt maintainers.
> 
> As indicated by the comment, this property should not be used in the devicetree
> case, notice this is a device-property and not an of property and since it is
> not intended to be used with devicetree at all (in devicetree a phandle rather
> then a name would be used), it has no place under Documentation/devicetree at all.
> 

Ok. I thought device properties were supposed to unify dt and non-dt situations,
but apparently not. Oh well :-(.

> Also there is no current binding documentation for the "fcs,fusb302" compatible
> and the weird "fcs,int_n" gpio which really is an irq which it already uses.
> 

Yes, one of those TODO items for moving the code out of staging. "fcs," should
possibly be "fusb302,", and int_n _is_ weird.

> 
>>
>>> +        chip->tcpc_dev.usb2_extcon = extcon_get_extcon_dev(name);
>>> +        if (!chip->tcpc_dev.usb2_extcon)
>>
>> extcon_get_extcon_dev() can also return an ERR_PTR.
>>
>> As before, I am not really happy typing this into tcpm via tcpc_dev.
>> Until we have a better solution, I would prefer for that code to stay with the fusb302
>> code.
> 
> Ok, I tried to make this all re-usable since I think other port-controller drivers
> will need something similar, but I can kill the entire tcpm-helpers.c file in v2
> and then put everything in fusb302.c. I take it that that is what you want ?
> 
Yes, please.

Also, please copy Yueyao Zhu for the fusb302 changes in v2.

Thanks,
Guenter

[PATCH 09/18] staging: typec: fusb302: Use tcpm_set_current_limit_psy

[Hans de Goede]

Register a power_supply and use tcpm_set_current_limit_psy as
set_current_limit so that another driver (e.g. the charger driver) can
pick the limit up and configure the system accordingly.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/fusb302/fusb302.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
index 1d8c9b66df2f..e1e08f57af99 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -854,17 +854,6 @@ static int tcpm_set_vbus(struct tcpc_dev *dev, bool on, bool charge)
 	return ret;
 }
 
-static int tcpm_set_current_limit(struct tcpc_dev *dev, u32 max_ma, u32 mv)
-{
-	struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
-						 tcpc_dev);
-
-	fusb302_log(chip, "current limit: %d ma, %d mv (not implemented)",
-		    max_ma, mv);
-
-	return 0;
-}
-
 static int fusb302_pd_tx_flush(struct fusb302_chip *chip)
 {
 	return fusb302_i2c_set_bits(chip, FUSB_REG_CONTROL0,
@@ -1208,7 +1197,7 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
 	fusb302_tcpc_dev->set_polarity = tcpm_set_polarity;
 	fusb302_tcpc_dev->set_vconn = tcpm_set_vconn;
 	fusb302_tcpc_dev->set_vbus = tcpm_set_vbus;
-	fusb302_tcpc_dev->set_current_limit = tcpm_set_current_limit;
+	fusb302_tcpc_dev->set_current_limit = tcpm_set_current_limit_psy;
 	fusb302_tcpc_dev->set_pd_rx = tcpm_set_pd_rx;
 	fusb302_tcpc_dev->set_roles = tcpm_set_roles;
 	fusb302_tcpc_dev->start_drp_toggling = tcpm_start_drp_toggling;
@@ -1733,6 +1722,11 @@ static int fusb302_probe(struct i2c_client *client,
 			return -EPROBE_DEFER;
 	}
 
+	ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
+				"fusb302-typec-source");
+	if (ret < 0)
+		return ret;
+
 	ret = fusb302_debugfs_init(chip);
 	if (ret < 0)
 		return ret;
-- 
2.13.3

Re: [PATCH 09/18] staging: typec: fusb302: Use tcpm_set_current_limit_psy

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> Register a power_supply and use tcpm_set_current_limit_psy as
> set_current_limit so that another driver (e.g. the charger driver) can
> pick the limit up and configure the system accordingly.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/staging/typec/fusb302/fusb302.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
> index 1d8c9b66df2f..e1e08f57af99 100644
> --- a/drivers/staging/typec/fusb302/fusb302.c
> +++ b/drivers/staging/typec/fusb302/fusb302.c
> @@ -854,17 +854,6 @@ static int tcpm_set_vbus(struct tcpc_dev *dev, bool on, bool charge)
>   	return ret;
>   }
>   
> -static int tcpm_set_current_limit(struct tcpc_dev *dev, u32 max_ma, u32 mv)
> -{
> -	struct fusb302_chip *chip = container_of(dev, struct fusb302_chip,
> -						 tcpc_dev);
> -
> -	fusb302_log(chip, "current limit: %d ma, %d mv (not implemented)",
> -		    max_ma, mv);
> -
> -	return 0;
> -}
> -

Same comment as before. I think the fusb code should do what tcpm_set_current_limit_psy
is doing, ie register with a power supply.

>   static int fusb302_pd_tx_flush(struct fusb302_chip *chip)
>   {
>   	return fusb302_i2c_set_bits(chip, FUSB_REG_CONTROL0,
> @@ -1208,7 +1197,7 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
>   	fusb302_tcpc_dev->set_polarity = tcpm_set_polarity;
>   	fusb302_tcpc_dev->set_vconn = tcpm_set_vconn;
>   	fusb302_tcpc_dev->set_vbus = tcpm_set_vbus;
> -	fusb302_tcpc_dev->set_current_limit = tcpm_set_current_limit;
> +	fusb302_tcpc_dev->set_current_limit = tcpm_set_current_limit_psy;
>   	fusb302_tcpc_dev->set_pd_rx = tcpm_set_pd_rx;
>   	fusb302_tcpc_dev->set_roles = tcpm_set_roles;
>   	fusb302_tcpc_dev->start_drp_toggling = tcpm_start_drp_toggling;
> @@ -1733,6 +1722,11 @@ static int fusb302_probe(struct i2c_client *client,
>   			return -EPROBE_DEFER;
>   	}
>   
> +	ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
> +				"fusb302-typec-source");
> +	if (ret < 0)
> +		return ret;
> +
>   	ret = fusb302_debugfs_init(chip);
>   	if (ret < 0)
>   		return ret;
> 

[PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Hans de Goede]

On devicetree platforms the fusb302 dt-node will have a vbus regulator
property with a phandle to the regulator.

On ACPI platforms, there are no phandles and we need to get the vbus by a
system wide unique name. Add support for a new "fcs,vbus-regulator-name"
device-property which ACPI platform code can set to pass the name.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
index e1e08f57af99..c3bcc5484ade 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client,
 			return -EPROBE_DEFER;
 	}
 
+	/*
+	 * Devicetree platforms should get vbus from their dt-node.
+	 * On ACPI platforms, we need to get the vbus by a system wide unique
+	 * name, which is set in a device prop by the platform code.
+	 */
+	if (device_property_read_string(dev, "fcs,vbus-regulator-name",
+					&name) == 0) {
+		/*
+		 * Use regulator_get_optional so that we can detect if we need
+		 * to defer the probe rather then getting the dummy-regulator.
+		 */
+		chip->vbus = devm_regulator_get_optional(dev, name);
+		if (IS_ERR(chip->vbus)) {
+			ret = PTR_ERR(chip->vbus);
+			return (ret == -ENODEV) ? -EPROBE_DEFER : ret;
+		}
+	} else {
+		chip->vbus = devm_regulator_get(dev, "vbus");
+		if (IS_ERR(chip->vbus))
+			return PTR_ERR(chip->vbus);
+	}
+
 	ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
 				"fusb302-typec-source");
 	if (ret < 0)
@@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client,
 	INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
 	init_tcpc_dev(&chip->tcpc_dev);
 
-	chip->vbus = devm_regulator_get(chip->dev, "vbus");
-	if (IS_ERR(chip->vbus)) {
-		ret = PTR_ERR(chip->vbus);
-		goto destroy_workqueue;
-	}
-
 	if (client->irq) {
 		chip->gpio_int_n_irq = client->irq;
 	} else {
-- 
2.13.3

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> On devicetree platforms the fusb302 dt-node will have a vbus regulator
> property with a phandle to the regulator.
> 
> On ACPI platforms, there are no phandles and we need to get the vbus by a
> system wide unique name. Add support for a new "fcs,vbus-regulator-name"
> device-property which ACPI platform code can set to pass the name.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
> index e1e08f57af99..c3bcc5484ade 100644
> --- a/drivers/staging/typec/fusb302/fusb302.c
> +++ b/drivers/staging/typec/fusb302/fusb302.c
> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client,
>   			return -EPROBE_DEFER;
>   	}
>   
> +	/*
> +	 * Devicetree platforms should get vbus from their dt-node.
> +	 * On ACPI platforms, we need to get the vbus by a system wide unique
> +	 * name, which is set in a device prop by the platform code.
> +	 */
> +	if (device_property_read_string(dev, "fcs,vbus-regulator-name",
> +					&name) == 0) {

Another property to be documented and approved.

Also, isn't there a better way to get regulator names for dt- and non-dt systems ?
This would apply to every driver supporting both and using regulators, which seems
awkward.

> +		/*
> +		 * Use regulator_get_optional so that we can detect if we need
> +		 * to defer the probe rather then getting the dummy-regulator.
> +		 */

Wouldn't this apply to dt systems as well ?

> +		chip->vbus = devm_regulator_get_optional(dev, name);
> +		if (IS_ERR(chip->vbus)) {
> +			ret = PTR_ERR(chip->vbus);
> +			return (ret == -ENODEV) ? -EPROBE_DEFER : ret;
> +		}
> +	} else {
> +		chip->vbus = devm_regulator_get(dev, "vbus");
> +		if (IS_ERR(chip->vbus))
> +			return PTR_ERR(chip->vbus);
> +	}
> +

You might also want to explain why you moved this code.

>   	ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
>   				"fusb302-typec-source");
>   	if (ret < 0)
> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client,
>   	INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
>   	init_tcpc_dev(&chip->tcpc_dev);
>   
> -	chip->vbus = devm_regulator_get(chip->dev, "vbus");
> -	if (IS_ERR(chip->vbus)) {
> -		ret = PTR_ERR(chip->vbus);
> -		goto destroy_workqueue;
> -	}
> -
>   	if (client->irq) {
>   		chip->gpio_int_n_irq = client->irq;
>   	} else {
> 

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Hans de Goede]

Hi,

On 06-08-17 16:30, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> On devicetree platforms the fusb302 dt-node will have a vbus regulator
>> property with a phandle to the regulator.
>>
>> On ACPI platforms, there are no phandles and we need to get the vbus by a
>> system wide unique name. Add support for a new "fcs,vbus-regulator-name"
>> device-property which ACPI platform code can set to pass the name.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
>> index e1e08f57af99..c3bcc5484ade 100644
>> --- a/drivers/staging/typec/fusb302/fusb302.c
>> +++ b/drivers/staging/typec/fusb302/fusb302.c
>> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client,
>>               return -EPROBE_DEFER;
>>       }
>> +    /*
>> +     * Devicetree platforms should get vbus from their dt-node.
>> +     * On ACPI platforms, we need to get the vbus by a system wide unique
>> +     * name, which is set in a device prop by the platform code.
>> +     */
>> +    if (device_property_read_string(dev, "fcs,vbus-regulator-name",
>> +                    &name) == 0) {
> 
> Another property to be documented and approved.

Again this is for kernel internal use on non-dt platforms only, so documenting
it in the devicetree bindings is not necessary.
> Also, isn't there a better way to get regulator names for dt- and non-dt systems ?
> This would apply to every driver supporting both and using regulators, which seems
> awkward.

While working on this I noticed that it is possible to add a regulator_match
table entry when registering a regulator, but that requires describing this
in regulator_init_data. Which would mean passing regulator_init_data from the
place where it is instantiated to where it gets registered, which would
mean passing a pointer through a device-property, given that this is purely kernel
internal that is possible, but not really how device-props are supposed to be used.

Also since the regulator-core only adds the mapping when registering the
regulator, this means that if we try to get the regulator before it has been
registered; and there is another regulator with the rather generic "vbus"
name then that will be returned instead.

Basically regulators are practically almost unused on x86 systems. I had to
add CONFIG_REGULATOR=y to my .config which is based on the Fedora 26 kernel
.config, so it has pretty much everything under the sun enabled. So it seems
that we are covering new ground here.

An alternative would be to not use the regulator subsys for this at all, but
it does seem the logical thing to use and using get-by-name is no different
then what we've doing for setting the the "fusb302-typec-source" psy as supplier
for the charger psy class device registered by the bq24190_charger driver.

TL;DR: It seems that on x86, at least for existing devices where we cannot
control the ACPI tables that getting things by name is the thing to do.

>> +        /*
>> +         * Use regulator_get_optional so that we can detect if we need
>> +         * to defer the probe rather then getting the dummy-regulator.
>> +         */
> 
> Wouldn't this apply to dt systems as well ?

No because there will be a property named "vbus-supply" in the fusb302
node containing a phandle to the regulator, if the regulator to which the phandle
points has not been registered yet regulator_get will automatically return
-EPROBE_DEFER because there is a "vbus-supply" property, only if there is
no such property at all will it return a dummy regulator.

>> +        chip->vbus = devm_regulator_get_optional(dev, name);
>> +        if (IS_ERR(chip->vbus)) {
>> +            ret = PTR_ERR(chip->vbus);
>> +            return (ret == -ENODEV) ? -EPROBE_DEFER : ret;
>> +        }
>> +    } else {
>> +        chip->vbus = devm_regulator_get(dev, "vbus");
>> +        if (IS_ERR(chip->vbus))
>> +            return PTR_ERR(chip->vbus);
>> +    }
>> +
> 
> You might also want to explain why you moved this code.

Right, I did that because it may fail with -EPROBE_DEFER and
I wanted to do that before the register_psy. But as I just
explained the old code could do that too, so I properly should
just put the register_psy later.

Regards,

Hans



>>       ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
>>                   "fusb302-typec-source");
>>       if (ret < 0)
>> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client,
>>       INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
>>       init_tcpc_dev(&chip->tcpc_dev);
>> -    chip->vbus = devm_regulator_get(chip->dev, "vbus");
>> -    if (IS_ERR(chip->vbus)) {
>> -        ret = PTR_ERR(chip->vbus);
>> -        goto destroy_workqueue;
>> -    }
>> -
>>       if (client->irq) {
>>           chip->gpio_int_n_irq = client->irq;
>>       } else {
>>
> 

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Guenter Roeck]

On 08/06/2017 07:52 AM, Hans de Goede wrote:
> Hi,
> 
> On 06-08-17 16:30, Guenter Roeck wrote:
>> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>>> On devicetree platforms the fusb302 dt-node will have a vbus regulator
>>> property with a phandle to the regulator.
>>>
>>> On ACPI platforms, there are no phandles and we need to get the vbus by a
>>> system wide unique name. Add support for a new "fcs,vbus-regulator-name"
>>> device-property which ACPI platform code can set to pass the name.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------
>>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
>>> index e1e08f57af99..c3bcc5484ade 100644
>>> --- a/drivers/staging/typec/fusb302/fusb302.c
>>> +++ b/drivers/staging/typec/fusb302/fusb302.c
>>> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client,
>>>               return -EPROBE_DEFER;
>>>       }
>>> +    /*
>>> +     * Devicetree platforms should get vbus from their dt-node.
>>> +     * On ACPI platforms, we need to get the vbus by a system wide unique
>>> +     * name, which is set in a device prop by the platform code.
>>> +     */
>>> +    if (device_property_read_string(dev, "fcs,vbus-regulator-name",
>>> +                    &name) == 0) {
>>
>> Another property to be documented and approved.
> 
> Again this is for kernel internal use on non-dt platforms only, so documenting
> it in the devicetree bindings is not necessary.

Ok.

>> Also, isn't there a better way to get regulator names for dt- and non-dt systems ?
>> This would apply to every driver supporting both and using regulators, which seems
>> awkward.
> 
> While working on this I noticed that it is possible to add a regulator_match
> table entry when registering a regulator, but that requires describing this
> in regulator_init_data. Which would mean passing regulator_init_data from the
> place where it is instantiated to where it gets registered, which would
> mean passing a pointer through a device-property, given that this is purely kernel
> internal that is possible, but not really how device-props are supposed to be used.
> 
> Also since the regulator-core only adds the mapping when registering the
> regulator, this means that if we try to get the regulator before it has been
> registered; and there is another regulator with the rather generic "vbus"
> name then that will be returned instead.
> 
> Basically regulators are practically almost unused on x86 systems. I had to
> add CONFIG_REGULATOR=y to my .config which is based on the Fedora 26 kernel
> .config, so it has pretty much everything under the sun enabled. So it seems
> that we are covering new ground here.
> 

We have some in hwmon, but they get by with using devm_regulator_get_optional()
for both dt and non-dt systems. Only problem with that is that it returns
-ENODEV if regulators are not configured, which by itself is weird/odd
(and there have been endless discussions about it).

> An alternative would be to not use the regulator subsys for this at all, but
> it does seem the logical thing to use and using get-by-name is no different
> then what we've doing for setting the the "fusb302-typec-source" psy as supplier
> for the charger psy class device registered by the bq24190_charger driver.
> 
> TL;DR: It seems that on x86, at least for existing devices where we cannot
> control the ACPI tables that getting things by name is the thing to do.
> 

Messy :-(. I don't have a better idea, unfortunately.

>>> +        /*
>>> +         * Use regulator_get_optional so that we can detect if we need
>>> +         * to defer the probe rather then getting the dummy-regulator.
>>> +         */
>>
>> Wouldn't this apply to dt systems as well ?
> 
> No because there will be a property named "vbus-supply" in the fusb302
> node containing a phandle to the regulator, if the regulator to which the phandle
> points has not been registered yet regulator_get will automatically return
> -EPROBE_DEFER because there is a "vbus-supply" property, only if there is
> no such property at all will it return a dummy regulator.
> 

More messy. Again, I don't have a better idea, but it is really weird that we
need all this code. There should really be some generic code handling all those
differences.

>>> +        chip->vbus = devm_regulator_get_optional(dev, name);
>>> +        if (IS_ERR(chip->vbus)) {
>>> +            ret = PTR_ERR(chip->vbus);
>>> +            return (ret == -ENODEV) ? -EPROBE_DEFER : ret;

This will be stuck in returning -EPROBE_DEFER if the regulator subsystem
is disabled. Is this acceptable ?

>>> +        }
>>> +    } else {
>>> +        chip->vbus = devm_regulator_get(dev, "vbus");
>>> +        if (IS_ERR(chip->vbus))
>>> +            return PTR_ERR(chip->vbus);
>>> +    }
>>> +
>>
>> You might also want to explain why you moved this code.
> 
> Right, I did that because it may fail with -EPROBE_DEFER and
> I wanted to do that before the register_psy. But as I just
> explained the old code could do that too, so I properly should
> just put the register_psy later.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>>       ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
>>>                   "fusb302-typec-source");
>>>       if (ret < 0)
>>> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client,
>>>       INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
>>>       init_tcpc_dev(&chip->tcpc_dev);
>>> -    chip->vbus = devm_regulator_get(chip->dev, "vbus");
>>> -    if (IS_ERR(chip->vbus)) {
>>> -        ret = PTR_ERR(chip->vbus);
>>> -        goto destroy_workqueue;
>>> -    }
>>> -
>>>       if (client->irq) {
>>>           chip->gpio_int_n_irq = client->irq;
>>>       } else {
>>>
>>
> 

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Hans de Goede]

<resend with Liam and Mark added to the Cc as they may want to weigh in on this too>


Hi,

On 06-08-17 16:30, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> On devicetree platforms the fusb302 dt-node will have a vbus regulator
>> property with a phandle to the regulator.
>>
>> On ACPI platforms, there are no phandles and we need to get the vbus by a
>> system wide unique name. Add support for a new "fcs,vbus-regulator-name"
>> device-property which ACPI platform code can set to pass the name.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
>> index e1e08f57af99..c3bcc5484ade 100644
>> --- a/drivers/staging/typec/fusb302/fusb302.c
>> +++ b/drivers/staging/typec/fusb302/fusb302.c
>> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client,
>>               return -EPROBE_DEFER;
>>       }
>> +    /*
>> +     * Devicetree platforms should get vbus from their dt-node.
>> +     * On ACPI platforms, we need to get the vbus by a system wide unique
>> +     * name, which is set in a device prop by the platform code.
>> +     */
>> +    if (device_property_read_string(dev, "fcs,vbus-regulator-name",
>> +                    &name) == 0) {
> 
> Another property to be documented and approved.

Again this is for kernel internal use on non-dt platforms only, so documenting
it in the devicetree bindings is not necessary.
> Also, isn't there a better way to get regulator names for dt- and non-dt systems ?
> This would apply to every driver supporting both and using regulators, which seems
> awkward.

While working on this I noticed that it is possible to add a regulator_match
table entry when registering a regulator, but that requires describing this
in regulator_init_data. Which would mean passing regulator_init_data from the
place where it is instantiated to where it gets registered, which would
mean passing a pointer through a device-property, given that this is purely kernel
internal that is possible, but not really how device-props are supposed to be used.

Also since the regulator-core only adds the mapping when registering the
regulator, this means that if we try to get the regulator before it has been
registered; and there is another regulator with the rather generic "vbus"
name then that will be returned instead.

Basically regulators are practically almost unused on x86 systems. I had to
add CONFIG_REGULATOR=y to my .config which is based on the Fedora 26 kernel
.config, so it has pretty much everything under the sun enabled. So it seems
that we are covering new ground here.

An alternative would be to not use the regulator subsys for this at all, but
it does seem the logical thing to use and using get-by-name is no different
then what we've doing for setting the the "fusb302-typec-source" psy as supplier
for the charger psy class device registered by the bq24190_charger driver.

TL;DR: It seems that on x86, at least for existing devices where we cannot
control the ACPI tables that getting things by name is the thing to do.

>> +        /*
>> +         * Use regulator_get_optional so that we can detect if we need
>> +         * to defer the probe rather then getting the dummy-regulator.
>> +         */
> 
> Wouldn't this apply to dt systems as well ?

No because there will be a property named "vbus-supply" in the fusb302
node containing a phandle to the regulator, if the regulator to which the phandle
points has not been registered yet regulator_get will automatically return
-EPROBE_DEFER because there is a "vbus-supply" property, only if there is
no such property at all will it return a dummy regulator.

>> +        chip->vbus = devm_regulator_get_optional(dev, name);
>> +        if (IS_ERR(chip->vbus)) {
>> +            ret = PTR_ERR(chip->vbus);
>> +            return (ret == -ENODEV) ? -EPROBE_DEFER : ret;
>> +        }
>> +    } else {
>> +        chip->vbus = devm_regulator_get(dev, "vbus");
>> +        if (IS_ERR(chip->vbus))
>> +            return PTR_ERR(chip->vbus);
>> +    }
>> +
> 
> You might also want to explain why you moved this code.

Right, I did that because it may fail with -EPROBE_DEFER and
I wanted to do that before the register_psy. But as I just
explained the old code could do that too, so I properly should
just put the register_psy later.

Regards,

Hans



>>       ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
>>                   "fusb302-typec-source");
>>       if (ret < 0)
>> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client,
>>       INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
>>       init_tcpc_dev(&chip->tcpc_dev);
>> -    chip->vbus = devm_regulator_get(chip->dev, "vbus");
>> -    if (IS_ERR(chip->vbus)) {
>> -        ret = PTR_ERR(chip->vbus);
>> -        goto destroy_workqueue;
>> -    }
>> -
>>       if (client->irq) {
>>           chip->gpio_int_n_irq = client->irq;
>>       } else {
>>
> 

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

On Sun, Aug 06, 2017 at 05:44:36PM +0200, Hans de Goede wrote:
> On 06-08-17 16:30, Guenter Roeck wrote:
> > On 08/06/2017 05:35 AM, Hans de Goede wrote:

> > > On ACPI platforms, there are no phandles and we need to get the vbus by a
> > > system wide unique name. Add support for a new "fcs,vbus-regulator-name"
> > > device-property which ACPI platform code can set to pass the name.

> > Another property to be documented and approved.

> Again this is for kernel internal use on non-dt platforms only, so documenting
> it in the devicetree bindings is not necessary.

However it *is* for use on ACPI platforms and is impacting power
management (which is something ACPI definitely models) so should be
being documented in an ASWG spec.  We don't want Linux systems to start
breaking the ACPI power management model with uncontrolled extensions,
it's fine to add new bindings for things where there's just no ACPI
specification at all but power management isn't one of those areas.

> TL;DR: It seems that on x86, at least for existing devices where we cannot
> control the ACPI tables that getting things by name is the thing to do.

The idiomatic thing to do on an ACPI system at present appears to be to
have a big DMI quirk table somewhere that instantiates the regulators
and mappings required for them based on the machine's DMI data.  Or if
it's a self contained PCI device or something with both regulator and
consumer do it as part of the subfunction instantiation there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Hans de Goede]

Hi Mark,

On 07-08-17 13:10, Mark Brown wrote:
> On Sun, Aug 06, 2017 at 05:44:36PM +0200, Hans de Goede wrote:
>> On 06-08-17 16:30, Guenter Roeck wrote:
>>> On 08/06/2017 05:35 AM, Hans de Goede wrote:
> 
>>>> On ACPI platforms, there are no phandles and we need to get the vbus by a
>>>> system wide unique name. Add support for a new "fcs,vbus-regulator-name"
>>>> device-property which ACPI platform code can set to pass the name.
> 
>>> Another property to be documented and approved.
> 
>> Again this is for kernel internal use on non-dt platforms only, so documenting
>> it in the devicetree bindings is not necessary.
> 
> However it *is* for use on ACPI platforms and is impacting power
> management (which is something ACPI definitely models) so should be
> being documented in an ASWG spec.  We don't want Linux systems to start
> breaking the ACPI power management model with uncontrolled extensions,
> it's fine to add new bindings for things where there's just no ACPI
> specification at all but power management isn't one of those areas.

This regulator is used to enable/disable driving vbus on the Type-C connector
from a 5V boost converter or not depending on the power direction (sink
or source) negotiated by the Type-C port-controller. As such this is never
under firmware/ACPI control it always gets controlled by the Type-C
port-manager, so there is no need for ACPI to control it. The problem is
that the Type-C setup on these boards consist of a bunch of ICs chained
together / driving different pins of the Type-C connector. So we need to
somehow tell the bq24292i charger-IC to turn on/off its 5V boost converter
from the Type-C port-controller driver. This discussion (and this patch)
is about getting a handle to the regulator-device for the 5V boost converter
from the Type-C port-controller driver.

For added fun the bq24292i charger-IC is not described in ACPI at all,
but we know that the Whiskey Cove PMIC used is always paired with it.

The fusb302 Type-c port-controller itself is enumerated to the weird
INT33FE ACPI device node (which describes 3 different i2c ICs, including
the fusb302)

>> TL;DR: It seems that on x86, at least for existing devices where we cannot
>> control the ACPI tables that getting things by name is the thing to do.
> 
> The idiomatic thing to do on an ACPI system at present appears to be to
> have a big DMI quirk table somewhere that instantiates the regulators
> and mappings required for them based on the machine's DMI data.  Or if
> it's a self contained PCI device or something with both regulator and
> consumer do it as part of the subfunction instantiation there.

Thanks for your input. I've taken a look at the possibility to specify
a mapping via regualtor_init_data, rather then falling back to finding the
regulator by name. I've found 2 problems with this:

Problem 1)

The regulator in question is part of the bq24292i charger-IC attached to
a private i2c bus between the PMIC and the charger. The driver for the i2c
controller inside the PMIC which drivers this bus currently also instantiates
the i2c-client for the charger:

drivers/i2c/busses/i2c-cht-wc.c:

static const char * const bq24190_suppliers[] = { "fusb302-typec-source" };

static const struct property_entry bq24190_props[] = {
         PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq24190_suppliers),
         PROPERTY_ENTRY_BOOL("input-current-limit-from-supplier"),
         PROPERTY_ENTRY_BOOL("omit-battery-class"),
         PROPERTY_ENTRY_BOOL("disable-reset"),
         { }
};

static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
{
         struct i2c_board_info board_info = {
                 .type = "bq24190",
                 .addr = 0x6b,
                 .properties = bq24190_props,
         };
	...
	adap->client_irq = irq_create_mapping(adap->irq_domain, 0);
	ret = i2c_add_adapter(&adap->adapter);

	board_info.irq = adap->client_irq;
	adap->client = i2c_new_device(&adap->adapter, &board_info);
	...
}

Note that the bq24190 driver is a generic driver, so to pass the
board specific regulator_init_data to it I would need to somehow
pass it here, but I don't see how, except by storing a pointer to
it in an u64 device-property which seems like a bad idea


Problem 2)

Even if I could add the mapping through regulator_init_data
then it may well be too late, if the regulator_get happens
before the bq24190 driver registers its regulator (and thus
the mapping) the regulator_get for it may have already
happened and returned a dummy-regulator, or another regulator
with the rather generic vbus name.


TL;DR: It is a mess and I cannot come up with anything better then
just using a globally-unique name, suggestions for a better
solution are welcome.

Regards,

Hans

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Mon, Aug 07, 2017 at 04:41:18PM +0200, Hans de Goede wrote:
> On 07-08-17 13:10, Mark Brown wrote:

> Problem 1)

> The regulator in question is part of the bq24292i charger-IC attached to
> a private i2c bus between the PMIC and the charger. The driver for the i2c
> controller inside the PMIC which drivers this bus currently also instantiates
> the i2c-client for the charger:

...

> Note that the bq24190 driver is a generic driver, so to pass the
> board specific regulator_init_data to it I would need to somehow
> pass it here, but I don't see how, except by storing a pointer to
> it in an u64 device-property which seems like a bad idea

I2C has a perfectly good platform_data pointer in the board info for
this stuff.

> Problem 2)

> Even if I could add the mapping through regulator_init_data
> then it may well be too late, if the regulator_get happens
> before the bq24190 driver registers its regulator (and thus
> the mapping) the regulator_get for it may have already
> happened and returned a dummy-regulator, or another regulator
> with the rather generic vbus name.

If you don't have control over the instantiation ordering but you have a
firmware which claims to provide a complete description of regulators
then you'd need to add an interface that allows mappings to be
registered separately to regulator registration.

Whatever you're doing the answer isn't to try to specify the name of the
supply through some firmware binding, that's just obviously not sensible
both in terms of a firmware abstraction and in terms of how the
abstractions in Linux work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Hans de Goede]

Hi,

On 07-08-17 17:41, Mark Brown wrote:
> On Mon, Aug 07, 2017 at 04:41:18PM +0200, Hans de Goede wrote:
>> On 07-08-17 13:10, Mark Brown wrote:
> 
>> Problem 1)
> 
>> The regulator in question is part of the bq24292i charger-IC attached to
>> a private i2c bus between the PMIC and the charger. The driver for the i2c
>> controller inside the PMIC which drivers this bus currently also instantiates
>> the i2c-client for the charger:
> 
> ...
> 
>> Note that the bq24190 driver is a generic driver, so to pass the
>> board specific regulator_init_data to it I would need to somehow
>> pass it here, but I don't see how, except by storing a pointer to
>> it in an u64 device-property which seems like a bad idea
> 
> I2C has a perfectly good platform_data pointer in the board info for
> this stuff.

True, so you are suggesting that I define a bq24190_platform_data
struct with a regulator_init_data pointer in there I guess?

At least I would not want to just claim that pointer for
just regulator_init_data and more-over assuming that what
is in there will be regulator_init_data feels wrong.

I don't think the power-supply maintainers will be enthusiastic
about this (hi Sebastian). But that does make sense and is
actually a good idea for tackling the problem of regulator_init_data.

>> Problem 2)
> 
>> Even if I could add the mapping through regulator_init_data
>> then it may well be too late, if the regulator_get happens
>> before the bq24190 driver registers its regulator (and thus
>> the mapping) the regulator_get for it may have already
>> happened and returned a dummy-regulator, or another regulator
>> with the rather generic vbus name.
> 
> If you don't have control over the instantiation ordering

It is not just device-instantiation ordering, it is also driver
loading order, the event around which ordering needs to happen is
the registration of the regulator (as things are now).

> but you have a firmware which claims to provide a complete description of regulators
> then you'd need to add an interface that allows mappings to be
> registered separately to regulator registration.

So the pwm subsys has this pwm_add_table thing which can add lookup
entries indepdentent of pwm_registration and which uses supply/device_name
matching to find the entry for the caller of pwm_get which is the same as
the current lookup code in the regulator-core, but since it is
independent of the registration the lookup-table does not contain
direct pointers to pwmchip-s instead it uses a string which gets
matches against the pwm (parent) dev's dev_name().

Would extending the struct regulator_map with a const char *provider_name:

struct regulator_map {
         struct list_head list;
         const char *dev_name;   /* The dev_name() for the consumer */
         const char *supply;
         struct regulator_dev *regulator;
	const char *provider;	/* The dev_name() for the regulator parent-dev */
};

And having a regulator_add_lookup function which adds an entry to the
regulator_map_list which sets provider_name instead of regulator
be acceptable ?

lookup of such entries would look for regulators where supply
matches the regulator-name and provider matches the
regulators parent-dev-name.

Alternatively the entry could additionally contain a provider_supply_name
so that we can make arbitrary consumer-dev-name + consumer-supply-name
provider-dev-name + provider-supply-name matches. That would probably
be more flexible then requiring the supply name to match.

So would something like this (including returning -EPROBE_DEFER if there
is a pwm_map_list entry and no matching regulator can be found) acceptable ?

> Whatever you're doing the answer isn't to try to specify the name of the
> supply through some firmware binding, that's just obviously not sensible
> both in terms of a firmware abstraction and in terms of how the
> abstractions in Linux work.

Ok.

Regards,

Hans

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

On Mon, Aug 07, 2017 at 09:20:05PM +0200, Hans de Goede wrote:
> On 07-08-17 17:41, Mark Brown wrote:

> > I2C has a perfectly good platform_data pointer in the board info for
> > this stuff.

> True, so you are suggesting that I define a bq24190_platform_data
> struct with a regulator_init_data pointer in there I guess?

Yes.  

> I don't think the power-supply maintainers will be enthusiastic
> about this (hi Sebastian). But that does make sense and is
> actually a good idea for tackling the problem of regulator_init_data.

Why not?  This is just really standard usage of platform data.

> Would extending the struct regulator_map with a const char *provider_name:

> struct regulator_map {
>         struct list_head list;
>         const char *dev_name;   /* The dev_name() for the consumer */
>         const char *supply;
>         struct regulator_dev *regulator;
> 	const char *provider;	/* The dev_name() for the regulator parent-dev */
> };

Please don't invent new terminology like this.  Just call it a regulator
name.

> Alternatively the entry could additionally contain a provider_supply_name
> so that we can make arbitrary consumer-dev-name + consumer-supply-name
> provider-dev-name + provider-supply-name matches. That would probably
> be more flexible then requiring the supply name to match.

I'm sorry but I can't follow what you mean here.  What do you mean by
"provider_supply_name"?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

[Hans de Goede]

<resend with the CC really added back>

Hi,

On 08/08/2017 04:42 PM, Mark Brown wrote:
> On Tue, Aug 08, 2017 at 02:56:46PM +0100, Hans de Goede wrote:
>> Hi,
> 
> Please don't take things off-list unless there is a really strong reason
> to do so.  Sending things to the list ensures that everyone gets a
> chance to read and comment on things.

Sorry, that was unintentional I probably accidentally hit reply instead
of reply-to-all.

I've re-added the lists to the Cc.

>> On 08/08/2017 10:39 AM, Mark Brown wrote:
>>> On Mon, Aug 07, 2017 at 09:20:05PM +0200, Hans de Goede wrote:
> 
>>> Why not?  This is just really standard usage of platform data.
> 
>> Right, but in general in most cases we are trying to get rid of
>> platform data (where possible). So introducing new platform_data
>> is not going to be popular, but I agree that it likely is the
>> best solution here.
> 
> No, we aren't.  The majority of architectures are still platform data
> only and x86 as you're finding uses it extensively along with ACPI.

Ok.

>>>> Alternatively the entry could additionally contain a provider_supply_name
>>>> so that we can make arbitrary consumer-dev-name + consumer-supply-name
>>>> provider-dev-name + provider-supply-name matches. That would probably
>>>> be more flexible then requiring the supply name to match.
> 
>>> I'm sorry but I can't follow what you mean here.  What do you mean by
>>> "provider_supply_name"?
> 
>> The current "const char *supply" in regulator_map is the supply name
>> passed to regulator_get, so the rdev_get_name requested by the consumer
>> (assuming no mapping is in place)
> 
> The name on the parent is *NOT* something anything else should
> reference, it's just some internal documentation intended exclusively
> for human consmption and can be overridden by the platforms.  It should
> never be referenced by anything outside the device.
> 
>> One regulator parent-device can register multiple regulator names, iow
>> multiple supplies, basically what I want to do is have the map
>> (when not using the regulator pointer) match the following 2 pairs:
> 
>> dev_name + supply
> 
>> regulator_parent_dev_name + rdev_get_name
> 
> Have your platform register identifiers that are useful within your
> platform, don't rely on the drivers.

Ok, I need to think a bit about this. I think I've enough info to
come up with a new patch-set not introducing the fcs,vbus-regulator-name
device-property ugliness. But this is a side project and I'm rather busy with $dayjob
atm, so it may take a while for me to come up with a new patch.

Regards,

Hans

[PATCH 11/18] power: supply: Fix power_supply_am_i_supplied to return -ENODEV when apropriate

[Hans de Goede]

Commit 2848e039c562 ("power: supply: Make power_supply_am_i_supplied return
-ENODEV if there are no suppliers") was supposed to make
power_supply_am_i_supplied() return -ENODEV when there are no supplies
which supply the supply passed to it.

But instead it will only return -ENODEV when there are no supplies at
all as data->count++; is incremented on every call of the iterator, rather
then only when __power_supply_is_supplied_by returns true. This commit
fixes this.

Fixes: 2848e039c562 ("power: supply: Make power_supply_am_i_supplied ...")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/power_supply_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 540d3e0aa011..0741fcef3b44 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -314,11 +314,12 @@ static int __power_supply_am_i_supplied(struct device *dev, void *_data)
 	struct power_supply *epsy = dev_get_drvdata(dev);
 	struct psy_am_i_supplied_data *data = _data;
 
-	data->count++;
-	if (__power_supply_is_supplied_by(epsy, data->psy))
+	if (__power_supply_is_supplied_by(epsy, data->psy)) {
+		data->count++;
 		if (!epsy->desc->get_property(epsy, POWER_SUPPLY_PROP_ONLINE,
 					&ret))
 			return ret.intval;
+	}
 
 	return 0;
 }
-- 
2.13.3

Re: [PATCH 11/18] power: supply: Fix power_supply_am_i_supplied to return -ENODEV when apropriate

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> Commit 2848e039c562 ("power: supply: Make power_supply_am_i_supplied return
> -ENODEV if there are no suppliers") was supposed to make
> power_supply_am_i_supplied() return -ENODEV when there are no supplies
> which supply the supply passed to it.
> 
> But instead it will only return -ENODEV when there are no supplies at
> all as data->count++; is incremented on every call of the iterator, rather
> then only when __power_supply_is_supplied_by returns true. This commit
> fixes this.
> 
> Fixes: 2848e039c562 ("power: supply: Make power_supply_am_i_supplied ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Independent of this series ?

> ---
>   drivers/power/supply/power_supply_core.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 540d3e0aa011..0741fcef3b44 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -314,11 +314,12 @@ static int __power_supply_am_i_supplied(struct device *dev, void *_data)
>   	struct power_supply *epsy = dev_get_drvdata(dev);
>   	struct psy_am_i_supplied_data *data = _data;
>   
> -	data->count++;
> -	if (__power_supply_is_supplied_by(epsy, data->psy))
> +	if (__power_supply_is_supplied_by(epsy, data->psy)) {
> +		data->count++;
>   		if (!epsy->desc->get_property(epsy, POWER_SUPPLY_PROP_ONLINE,
>   					&ret))
>   			return ret.intval;
> +	}
>   
>   	return 0;
>   }
> 

Re: [PATCH 11/18] power: supply: Fix power_supply_am_i_supplied to return -ENODEV when apropriate

[Hans de Goede]

Hi,

On 06-08-17 16:31, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> Commit 2848e039c562 ("power: supply: Make power_supply_am_i_supplied return
>> -ENODEV if there are no suppliers") was supposed to make
>> power_supply_am_i_supplied() return -ENODEV when there are no supplies
>> which supply the supply passed to it.
>>
>> But instead it will only return -ENODEV when there are no supplies at
>> all as data->count++; is incremented on every call of the iterator, rather
>> then only when __power_supply_is_supplied_by returns true. This commit
>> fixes this.
>>
>> Fixes: 2848e039c562 ("power: supply: Make power_supply_am_i_supplied ...")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Independent of this series ?

Correct, in hindsight I should have send it out as a standalone patch, I will
do so for v2.

Regards,

Hans





> 
>> ---
>>   drivers/power/supply/power_supply_core.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index 540d3e0aa011..0741fcef3b44 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -314,11 +314,12 @@ static int __power_supply_am_i_supplied(struct device *dev, void *_data)
>>       struct power_supply *epsy = dev_get_drvdata(dev);
>>       struct psy_am_i_supplied_data *data = _data;
>> -    data->count++;
>> -    if (__power_supply_is_supplied_by(epsy, data->psy))
>> +    if (__power_supply_is_supplied_by(epsy, data->psy)) {
>> +        data->count++;
>>           if (!epsy->desc->get_property(epsy, POWER_SUPPLY_PROP_ONLINE,
>>                       &ret))
>>               return ret.intval;
>> +    }
>>       return 0;
>>   }
>>
> 

[PATCH 12/18] power: supply: Add power_supply_set_input_current_limit_from_supplier helper

[Hans de Goede]

On some devices the USB Type-C port power (USB PD 2.0) negotiation is
done by a separate port-controller IC, while the current limit is
controlled through another (charger) IC.

It has been decided to model this by modelling the external Type-C
power brick (adapter/charger) as a power-supply class device which
supplies the charger-IC, with its voltage-now and current-max representing
the negotiated voltage and max current draw.

This commit adds a power_supply_set_input_current_limit_from_supplier
helper function which charger power-supply drivers can call to get
the max-current from their supplier and have this applied
through their set_property call-back to their input-current-limit.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/power_supply_core.c | 41 ++++++++++++++++++++++++++++++++
 include/linux/power_supply.h             |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 0741fcef3b44..3f92574222de 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -375,6 +375,47 @@ int power_supply_is_system_supplied(void)
 }
 EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
 
+static int __power_supply_get_supplier_max_current(struct device *dev,
+						   void *data)
+{
+	union power_supply_propval ret = {0,};
+	struct power_supply *epsy = dev_get_drvdata(dev);
+	struct power_supply *psy = data;
+
+	if (__power_supply_is_supplied_by(epsy, psy))
+		if (!epsy->desc->get_property(epsy,
+					      POWER_SUPPLY_PROP_CURRENT_MAX,
+					      &ret))
+			return ret.intval;
+
+	return 0;
+}
+
+int power_supply_set_input_current_limit_from_supplier(struct power_supply *psy)
+{
+	union power_supply_propval val = {0,};
+	int curr;
+
+	if (!psy->desc->set_property)
+		return -EINVAL;
+
+	/*
+	 * This function is not intended for use with a supply with multiple
+	 * suppliers, we simply pick the first supply to report a non 0
+	 * max-current.
+	 */
+	curr = class_for_each_device(power_supply_class, NULL, psy,
+				      __power_supply_get_supplier_max_current);
+	if (curr <= 0)
+		return (curr == 0) ? -ENODEV : curr;
+
+	val.intval = curr;
+
+	return psy->desc->set_property(psy,
+				POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val);
+}
+EXPORT_SYMBOL_GPL(power_supply_set_input_current_limit_from_supplier);
+
 int power_supply_set_battery_charged(struct power_supply *psy)
 {
 	if (atomic_read(&psy->use_cnt) >= 0 &&
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index de89066b72b1..79e90b3d3288 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -332,6 +332,8 @@ extern int power_supply_get_battery_info(struct power_supply *psy,
 					 struct power_supply_battery_info *info);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
+extern int power_supply_set_input_current_limit_from_supplier(
+					 struct power_supply *psy);
 extern int power_supply_set_battery_charged(struct power_supply *psy);
 
 #ifdef CONFIG_POWER_SUPPLY
-- 
2.13.3

[PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Hans de Goede]

Register the 5V boost converter as a regulator named
"regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
the bq24190 family is also used on ACPI devices where there are no
device-tree phandles, so regulator_get will fallback to the name and thus
it must be unique on the system.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 121 +++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index d5a707e14526..f25ea9c4acca 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -16,6 +16,8 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/power_supply.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include <linux/workqueue.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
@@ -504,6 +506,121 @@ static int bq24190_sysfs_create_group(struct bq24190_dev_info *bdi)
 static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info *bdi) {}
 #endif
 
+#ifdef CONFIG_REGULATOR
+static int bq24190_vbus_enable(struct regulator_dev *dev)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		pm_runtime_put_noidle(bdi->dev);
+		return ret;
+	}
+
+	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
+				 BQ24190_REG_POC_CHG_CONFIG_MASK,
+				 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
+				 BQ24190_REG_POC_CHG_CONFIG_OTG);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return ret;
+}
+
+static int bq24190_vbus_disable(struct regulator_dev *dev)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		pm_runtime_put_noidle(bdi->dev);
+		return ret;
+	}
+
+	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
+				 BQ24190_REG_POC_CHG_CONFIG_MASK,
+				 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
+				 BQ24190_REG_POC_CHG_CONFIG_CHARGE);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return ret;
+}
+
+static int bq24190_vbus_is_enabled(struct regulator_dev *dev)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	int ret;
+	u8 val;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		pm_runtime_put_noidle(bdi->dev);
+		return ret;
+	}
+
+	ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
+				BQ24190_REG_POC_CHG_CONFIG_MASK,
+				BQ24190_REG_POC_CHG_CONFIG_SHIFT, &val);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return ret ? ret : val == BQ24190_REG_POC_CHG_CONFIG_OTG;
+}
+
+static const struct regulator_ops bq24190_vbus_ops = {
+	.enable = bq24190_vbus_enable,
+	.disable = bq24190_vbus_disable,
+	.is_enabled = bq24190_vbus_is_enabled,
+};
+
+static const struct regulator_desc bq24190_vbus_desc = {
+	.name = "regulator-bq24190-usb-vbus",
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+	.ops = &bq24190_vbus_ops,
+	.fixed_uV = 5000000,
+	.n_voltages = 1,
+};
+
+static const struct regulator_init_data bq24190_vbus_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
+
+static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
+{
+	struct regulator_config cfg = { };
+	struct regulator_dev *reg;
+	int ret = 0;
+
+	cfg.dev = bdi->dev;
+	cfg.init_data = &bq24190_vbus_init_data;
+	cfg.driver_data = bdi;
+	reg = devm_regulator_register(bdi->dev, &bq24190_vbus_desc, &cfg);
+	if (IS_ERR(reg)) {
+		ret = PTR_ERR(reg);
+		dev_err(bdi->dev, "Can't register regulator: %d\n", ret);
+	}
+
+	return ret;
+}
+#else
+static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
+{
+	return 0;
+}
+#endif
+
 /*
  * According to the "Host Mode and default Mode" section of the
  * manual, a write to any register causes the bq24190 to switch
@@ -1530,6 +1647,10 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out_pmrt;
 	}
 
+	ret = bq24190_register_vbus_regulator(bdi);
+	if (ret < 0)
+		goto out_pmrt;
+
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-- 
2.13.3

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170806 05:37]:
> Register the 5V boost converter as a regulator named
> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
> the bq24190 family is also used on ACPI devices where there are no
> device-tree phandles, so regulator_get will fallback to the name and thus
> it must be unique on the system.

Nice, this makes VBUS easy to use for USB PHY drivers :)

Tony

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Liam Breck]

Hi Hans,

On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Register the 5V boost converter as a regulator named
> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
> the bq24190 family is also used on ACPI devices where there are no
> device-tree phandles, so regulator_get will fallback to the name and thus
> it must be unique on the system.

What we're enabling here is 5V boost for otg host mode, not vbus
generally, so maybe the name should indicate that...

regulator-bq24190-usb-5volt
regulator-bq24190-usb-host
regulator-bq24190-usb-otg-5v

Tony, thoughts?

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Hans de Goede]

Hi,

On 08-08-17 10:39, Liam Breck wrote:
> Hi Hans,
> 
> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Register the 5V boost converter as a regulator named
>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>> the bq24190 family is also used on ACPI devices where there are no
>> device-tree phandles, so regulator_get will fallback to the name and thus
>> it must be unique on the system.
> 
> What we're enabling here is 5V boost for otg host mode, not vbus
> generally, so maybe the name should indicate that...
> 
> regulator-bq24190-usb-5volt
> regulator-bq24190-usb-host
> regulator-bq24190-usb-otg-5v

I picked vbus because that gets used a lot already in similar cases,
but I agree that we should probably come up with a better name.

I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?

Regards,

Hans

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Liam Breck]

On Tue, Aug 8, 2017 at 2:00 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 08-08-17 10:39, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Register the 5V boost converter as a regulator named
>>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>>> the bq24190 family is also used on ACPI devices where there are no
>>> device-tree phandles, so regulator_get will fallback to the name and thus
>>> it must be unique on the system.
>>
>>
>> What we're enabling here is 5V boost for otg host mode, not vbus
>> generally, so maybe the name should indicate that...
>>
>> regulator-bq24190-usb-5volt
>> regulator-bq24190-usb-host
>> regulator-bq24190-usb-otg-5v
>
>
> I picked vbus because that gets used a lot already in similar cases,
> but I agree that we should probably come up with a better name.
>
> I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?

There is this upstream, with "otg-vbus":
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=61274eff0ddee8f10deaa5f79085e981db52930a

Related search:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?qt=grep&q=otg+regulator

I don't think it needs a "regulator-" prefix, and maybe the driver
name is a suffix. We could also recommend the "regulator-name" value
for OTG here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/regulator/regulator.txt

a. Make the above patch wrong :-)
usb-otg-5v (generic)
usb-otg-5v-bq2419x (specific)

b. Follow a weak precedent
otg-vbus (generic)
otg-vbus-bq2419x (specific)

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Hans de Goede]

Hi,

On 08/08/2017 08:57 PM, Liam Breck wrote:
> On Tue, Aug 8, 2017 at 2:00 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 08-08-17 10:39, Liam Breck wrote:
>>>
>>> Hi Hans,
>>>
>>> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Register the 5V boost converter as a regulator named
>>>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>>>> the bq24190 family is also used on ACPI devices where there are no
>>>> device-tree phandles, so regulator_get will fallback to the name and thus
>>>> it must be unique on the system.
>>>
>>>
>>> What we're enabling here is 5V boost for otg host mode, not vbus
>>> generally, so maybe the name should indicate that...
>>>
>>> regulator-bq24190-usb-5volt
>>> regulator-bq24190-usb-host
>>> regulator-bq24190-usb-otg-5v
>>
>>
>> I picked vbus because that gets used a lot already in similar cases,
>> but I agree that we should probably come up with a better name.
>>
>> I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?
> 
> There is this upstream, with "otg-vbus":
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=61274eff0ddee8f10deaa5f79085e981db52930a
> 
> Related search:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?qt=grep&q=otg+regulator
> 
> I don't think it needs a "regulator-" prefix, and maybe the driver
> name is a suffix. We could also recommend the "regulator-name" value
> for OTG here:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/regulator/regulator.txt
> 
> a. Make the above patch wrong :-)
> usb-otg-5v (generic)
> usb-otg-5v-bq2419x (specific)
> 
> b. Follow a weak precedent
> otg-vbus (generic)
> otg-vbus-bq2419x (specific)

Looking at dts files under arch/arm/boot/dts the most used name seems to be
usb_otg_vbus (with underscores) so I will use that for v2 of this patch.

As for making the name more specific, as Mark Brown has correctly pointed
out the right thing to do on x86 is to add a mapping for the consumer of the
regulator using regulator_init_data, which can be passed through platform_data
(and I do control the i2c_client instantiation on x86, so adding that is easy),
which means that we can keep the name generic, which is a much better solution
then relying on the name. This means that we can keep the name generic as
one normally does for a regulator-name.

Regards,

Hans

[PATCH 14/18] power: supply: bq24190_charger: Add input_current_limit property

[Hans de Goede]

Export the input current limit of the charger as a
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property on the charger
power_supply class device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 35 ++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index f25ea9c4acca..d78e2c6dc127 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -982,6 +982,33 @@ static int bq24190_charger_set_voltage(struct bq24190_dev_info *bdi,
 			ARRAY_SIZE(bq24190_cvc_vreg_values), val->intval);
 }
 
+static int bq24190_charger_get_iinlimit(struct bq24190_dev_info *bdi,
+		union power_supply_propval *val)
+{
+	int iinlimit, ret;
+
+	ret = bq24190_get_field_val(bdi, BQ24190_REG_ISC,
+			BQ24190_REG_ISC_IINLIM_MASK,
+			BQ24190_REG_ISC_IINLIM_SHIFT,
+			bq24190_isc_iinlim_values,
+			ARRAY_SIZE(bq24190_isc_iinlim_values), &iinlimit);
+	if (ret < 0)
+		return ret;
+
+	val->intval = iinlimit;
+	return 0;
+}
+
+static int bq24190_charger_set_iinlimit(struct bq24190_dev_info *bdi,
+		const union power_supply_propval *val)
+{
+	return bq24190_set_field_val(bdi, BQ24190_REG_ISC,
+			BQ24190_REG_ISC_IINLIM_MASK,
+			BQ24190_REG_ISC_IINLIM_SHIFT,
+			bq24190_isc_iinlim_values,
+			ARRAY_SIZE(bq24190_isc_iinlim_values), val->intval);
+}
+
 static int bq24190_charger_get_property(struct power_supply *psy,
 		enum power_supply_property psp, union power_supply_propval *val)
 {
@@ -1022,6 +1049,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
 		ret = bq24190_charger_get_voltage_max(bdi, val);
 		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = bq24190_charger_get_iinlimit(bdi, val);
+		break;
 	case POWER_SUPPLY_PROP_SCOPE:
 		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
 		ret = 0;
@@ -1073,6 +1103,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 		ret = bq24190_charger_set_voltage(bdi, val);
 		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = bq24190_charger_set_iinlimit(bdi, val);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -1094,6 +1127,7 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		ret = 1;
 		break;
 	default:
@@ -1113,6 +1147,7 @@ static enum power_supply_property bq24190_charger_properties[] = {
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
-- 
2.13.3

[PATCH 15/18] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Hans de Goede]

On some devices the USB Type-C port power (USB PD 2.0) negotiation is
done by a separate port-controller IC, while the current limit is
controlled through another (charger) IC.

It has been decided to model this by modelling the external Type-C
power brick (adapter/charger) as a power-supply class device which
supplies the charger-IC, with its voltage-now and current-max representing
the negotiated voltage and max current draw.

This commit adds support for this to the bq24190_charger driver by calling
power_supply_set_input_current_limit_from_supplier helper if the
"input-current-limit-from-supplier" device-property is set.

Note this replaces the functionality to get the current-limit from an
extcon device, which will be removed in a follow-up commit.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index d78e2c6dc127..1f6424f0772f 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -161,6 +161,7 @@ struct bq24190_dev_info {
 	char				model_name[I2C_NAME_SIZE];
 	bool				initialized;
 	bool				irq_event;
+	bool				input_current_limit_from_supplier;
 	struct mutex			f_reg_lock;
 	u8				f_reg;
 	u8				ss_reg;
@@ -1137,6 +1138,14 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
 	return ret;
 }
 
+static void bq24190_charger_external_power_changed(struct power_supply *psy)
+{
+	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
+
+	if (bdi->input_current_limit_from_supplier)
+		power_supply_set_input_current_limit_from_supplier(psy);
+}
+
 static enum power_supply_property bq24190_charger_properties[] = {
 	POWER_SUPPLY_PROP_CHARGE_TYPE,
 	POWER_SUPPLY_PROP_HEALTH,
@@ -1165,6 +1174,7 @@ static const struct power_supply_desc bq24190_charger_desc = {
 	.get_property		= bq24190_charger_get_property,
 	.set_property		= bq24190_charger_set_property,
 	.property_is_writeable	= bq24190_charger_property_is_writeable,
+	.external_power_changed	= bq24190_charger_external_power_changed,
 };
 
 /* Battery power supply property routines */
@@ -1654,6 +1664,10 @@ static int bq24190_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
+	bdi->input_current_limit_from_supplier =
+		device_property_read_bool(dev,
+					  "input-current-limit-from-supplier");
+
 	/*
 	 * Devicetree platforms should get extcon via phandle (not yet supported).
 	 * On ACPI platforms, extcon clients may invoke us with:
-- 
2.13.3

Re: [PATCH 15/18] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hallo Hans :-)


On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> done by a separate port-controller IC, while the current limit is
> controlled through another (charger) IC.
>
> It has been decided to model this by modelling the external Type-C
> power brick (adapter/charger) as a power-supply class device which
> supplies the charger-IC, with its voltage-now and current-max representing
> the negotiated voltage and max current draw.
>
> This commit adds support for this to the bq24190_charger driver by calling
> power_supply_set_input_current_limit_from_supplier helper if the
> "input-current-limit-from-supplier" device-property is set.
>
> Note this replaces the functionality to get the current-limit from an
> extcon device, which will be removed in a follow-up commit.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index d78e2c6dc127..1f6424f0772f 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -161,6 +161,7 @@ struct bq24190_dev_info {
>         char                            model_name[I2C_NAME_SIZE];
>         bool                            initialized;
>         bool                            irq_event;
> +       bool                            input_current_limit_from_supplier;
>         struct mutex                    f_reg_lock;
>         u8                              f_reg;
>         u8                              ss_reg;
> @@ -1137,6 +1138,14 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
>         return ret;
>  }
>
> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
> +{
> +       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> +
> +       if (bdi->input_current_limit_from_supplier)
> +               power_supply_set_input_current_limit_from_supplier(psy);
> +}
> +
>  static enum power_supply_property bq24190_charger_properties[] = {
>         POWER_SUPPLY_PROP_CHARGE_TYPE,
>         POWER_SUPPLY_PROP_HEALTH,
> @@ -1165,6 +1174,7 @@ static const struct power_supply_desc bq24190_charger_desc = {
>         .get_property           = bq24190_charger_get_property,
>         .set_property           = bq24190_charger_set_property,
>         .property_is_writeable  = bq24190_charger_property_is_writeable,
> +       .external_power_changed = bq24190_charger_external_power_changed,
>  };
>
>  /* Battery power supply property routines */
> @@ -1654,6 +1664,10 @@ static int bq24190_probe(struct i2c_client *client,
>                 return -EINVAL;
>         }
>
> +       bdi->input_current_limit_from_supplier =
> +               device_property_read_bool(dev,
> +                                         "input-current-limit-from-supplier");

Since this invokes the new power_supply_class function, should this be
a devicetree property, not just a driver-to-driver switch? If so, the
property name should probably be defined in
Doc...bindings/power/supply/power_supply.txt.

My latest bq24190 series has a new DT doc, which should ref a new DT property.

Re: [PATCH 15/18] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Hans de Goede]

Hi,

On 08-08-17 10:24, Liam Breck wrote:
> Hallo Hans :-)
> 
> 
> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>> done by a separate port-controller IC, while the current limit is
>> controlled through another (charger) IC.
>>
>> It has been decided to model this by modelling the external Type-C
>> power brick (adapter/charger) as a power-supply class device which
>> supplies the charger-IC, with its voltage-now and current-max representing
>> the negotiated voltage and max current draw.
>>
>> This commit adds support for this to the bq24190_charger driver by calling
>> power_supply_set_input_current_limit_from_supplier helper if the
>> "input-current-limit-from-supplier" device-property is set.
>>
>> Note this replaces the functionality to get the current-limit from an
>> extcon device, which will be removed in a follow-up commit.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/power/supply/bq24190_charger.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index d78e2c6dc127..1f6424f0772f 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -161,6 +161,7 @@ struct bq24190_dev_info {
>>          char                            model_name[I2C_NAME_SIZE];
>>          bool                            initialized;
>>          bool                            irq_event;
>> +       bool                            input_current_limit_from_supplier;
>>          struct mutex                    f_reg_lock;
>>          u8                              f_reg;
>>          u8                              ss_reg;
>> @@ -1137,6 +1138,14 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
>>          return ret;
>>   }
>>
>> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
>> +{
>> +       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>> +
>> +       if (bdi->input_current_limit_from_supplier)
>> +               power_supply_set_input_current_limit_from_supplier(psy);
>> +}
>> +
>>   static enum power_supply_property bq24190_charger_properties[] = {
>>          POWER_SUPPLY_PROP_CHARGE_TYPE,
>>          POWER_SUPPLY_PROP_HEALTH,
>> @@ -1165,6 +1174,7 @@ static const struct power_supply_desc bq24190_charger_desc = {
>>          .get_property           = bq24190_charger_get_property,
>>          .set_property           = bq24190_charger_set_property,
>>          .property_is_writeable  = bq24190_charger_property_is_writeable,
>> +       .external_power_changed = bq24190_charger_external_power_changed,
>>   };
>>
>>   /* Battery power supply property routines */
>> @@ -1654,6 +1664,10 @@ static int bq24190_probe(struct i2c_client *client,
>>                  return -EINVAL;
>>          }
>>
>> +       bdi->input_current_limit_from_supplier =
>> +               device_property_read_bool(dev,
>> +                                         "input-current-limit-from-supplier");
> 
> Since this invokes the new power_supply_class function, should this be
> a devicetree property, not just a driver-to-driver switch? If so, the
> property name should probably be defined in
> Doc...bindings/power/supply/power_supply.txt.

Well this is a kernel internal thing, specifying a supplier through devicetree
should already be documented, this tells a driver to set its input-current-limit
based on the max-current of its supplier.

So we could documented it, but I think it should probably be prefixed with
"linux," then.

Anyways first lets see what Sebastian thinks of this approach if he nacks
it we don't have to worry about documenting it either :)

Regards,

Hans

[PATCH 16/18] power: supply: bq24190_charger: Remove extcon handling

[Hans de Goede]

Now that drivers/i2c/busses/i2c-cht-wc.c uses
"input-current-limit-from-supplier" instead of "extcon-name" the last
user of the bq24190 extcon code is gone, remove it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 107 ---------------------------------
 1 file changed, 107 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 1f6424f0772f..0376de6d8e70 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -11,7 +11,6 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
-#include <linux/extcon.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
@@ -155,9 +154,6 @@ struct bq24190_dev_info {
 	struct device			*dev;
 	struct power_supply		*charger;
 	struct power_supply		*battery;
-	struct extcon_dev		*extcon;
-	struct notifier_block		extcon_nb;
-	struct delayed_work		extcon_work;
 	char				model_name[I2C_NAME_SIZE];
 	bool				initialized;
 	bool				irq_event;
@@ -1530,75 +1526,6 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void bq24190_extcon_work(struct work_struct *work)
-{
-	struct bq24190_dev_info *bdi =
-		container_of(work, struct bq24190_dev_info, extcon_work.work);
-	int error, iinlim = 0;
-	u8 v;
-
-	error = pm_runtime_get_sync(bdi->dev);
-	if (error < 0) {
-		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
-		pm_runtime_put_noidle(bdi->dev);
-		return;
-	}
-
-	if      (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
-		iinlim =  500000;
-	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
-		 extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
-		iinlim = 1500000;
-	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
-		iinlim = 2000000;
-
-	if (iinlim) {
-		error = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
-					      BQ24190_REG_ISC_IINLIM_MASK,
-					      BQ24190_REG_ISC_IINLIM_SHIFT,
-					      bq24190_isc_iinlim_values,
-					      ARRAY_SIZE(bq24190_isc_iinlim_values),
-					      iinlim);
-		if (error < 0)
-			dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
-	}
-
-	/* if no charger found and in USB host mode, set OTG 5V boost, else normal */
-	if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
-		v = BQ24190_REG_POC_CHG_CONFIG_OTG;
-	else
-		v = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
-
-	error = bq24190_write_mask(bdi, BQ24190_REG_POC,
-				   BQ24190_REG_POC_CHG_CONFIG_MASK,
-				   BQ24190_REG_POC_CHG_CONFIG_SHIFT,
-				   v);
-	if (error < 0)
-		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
-
-	pm_runtime_mark_last_busy(bdi->dev);
-	pm_runtime_put_autosuspend(bdi->dev);
-}
-
-static int bq24190_extcon_event(struct notifier_block *nb, unsigned long event,
-				void *param)
-{
-	struct bq24190_dev_info *bdi =
-		container_of(nb, struct bq24190_dev_info, extcon_nb);
-
-	/*
-	 * The Power-Good detection may take up to 220ms, sometimes
-	 * the external charger detection is quicker, and the bq24190 will
-	 * reset to iinlim based on its own charger detection (which is not
-	 * hooked up when using external charger detection) resulting in
-	 * a too low default 500mA iinlim. Delay applying the extcon value
-	 * for 300ms to avoid this.
-	 */
-	queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300));
-
-	return NOTIFY_OK;
-}
-
 static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 {
 	u8 v;
@@ -1636,7 +1563,6 @@ static int bq24190_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct power_supply_config charger_cfg = {}, battery_cfg = {};
 	struct bq24190_dev_info *bdi;
-	const char *name;
 	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
@@ -1668,25 +1594,6 @@ static int bq24190_probe(struct i2c_client *client,
 		device_property_read_bool(dev,
 					  "input-current-limit-from-supplier");
 
-	/*
-	 * Devicetree platforms should get extcon via phandle (not yet supported).
-	 * On ACPI platforms, extcon clients may invoke us with:
-	 * struct property_entry pe[] =
-	 *   { PROPERTY_ENTRY_STRING("extcon-name", client_name), ... };
-	 * struct i2c_board_info bi =
-	 *   { .type = "bq24190", .addr = 0x6b, .properties = pe, .irq = irq };
-	 * struct i2c_adapter ad = { ... };
-	 * i2c_add_adapter(&ad);
-	 * i2c_new_device(&ad, &bi);
-	 */
-	if (device_property_read_string(dev, "extcon-name", &name) == 0) {
-		bdi->extcon = extcon_get_extcon_dev(name);
-		if (!bdi->extcon)
-			return -EPROBE_DEFER;
-
-		dev_info(bdi->dev, "using extcon device %s\n", name);
-	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 600);
@@ -1747,20 +1654,6 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out_sysfs;
 	}
 
-	if (bdi->extcon) {
-		INIT_DELAYED_WORK(&bdi->extcon_work, bq24190_extcon_work);
-		bdi->extcon_nb.notifier_call = bq24190_extcon_event;
-		ret = devm_extcon_register_notifier_all(dev, bdi->extcon,
-							&bdi->extcon_nb);
-		if (ret) {
-			dev_err(dev, "Can't register extcon\n");
-			goto out_sysfs;
-		}
-
-		/* Sync initial cable state */
-		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
-	}
-
 	enable_irq_wake(client->irq);
 
 	pm_runtime_mark_last_busy(dev);
-- 
2.13.3

Re: [PATCH 16/18] power: supply: bq24190_charger: Remove extcon handling

[Liam Breck]

Hi Hans,

On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Now that drivers/i2c/busses/i2c-cht-wc.c uses
> "input-current-limit-from-supplier" instead of "extcon-name" the last
> user of the bq24190 extcon code is gone, remove it.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 107 ---------------------------------
>  1 file changed, 107 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 1f6424f0772f..0376de6d8e70 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -11,7 +11,6 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> -#include <linux/extcon.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
> @@ -155,9 +154,6 @@ struct bq24190_dev_info {
>         struct device                   *dev;
>         struct power_supply             *charger;
>         struct power_supply             *battery;
> -       struct extcon_dev               *extcon;
> -       struct notifier_block           extcon_nb;
> -       struct delayed_work             extcon_work;
>         char                            model_name[I2C_NAME_SIZE];
>         bool                            initialized;
>         bool                            irq_event;
> @@ -1530,75 +1526,6 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>         return IRQ_HANDLED;
>  }
>
> -static void bq24190_extcon_work(struct work_struct *work)
> -{
> -       struct bq24190_dev_info *bdi =
> -               container_of(work, struct bq24190_dev_info, extcon_work.work);
> -       int error, iinlim = 0;
> -       u8 v;
> -
> -       error = pm_runtime_get_sync(bdi->dev);
> -       if (error < 0) {
> -               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> -               pm_runtime_put_noidle(bdi->dev);
> -               return;
> -       }
> -
> -       if      (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
> -               iinlim =  500000;
> -       else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
> -                extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
> -               iinlim = 1500000;
> -       else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
> -               iinlim = 2000000;
> -
> -       if (iinlim) {
> -               error = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
> -                                             BQ24190_REG_ISC_IINLIM_MASK,
> -                                             BQ24190_REG_ISC_IINLIM_SHIFT,
> -                                             bq24190_isc_iinlim_values,
> -                                             ARRAY_SIZE(bq24190_isc_iinlim_values),
> -                                             iinlim);
> -               if (error < 0)
> -                       dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
> -       }
> -
> -       /* if no charger found and in USB host mode, set OTG 5V boost, else normal */
> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
> -               v = BQ24190_REG_POC_CHG_CONFIG_OTG;
> -       else
> -               v = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
> -
> -       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
> -                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
> -                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
> -                                  v);
> -       if (error < 0)
> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
> -
> -       pm_runtime_mark_last_busy(bdi->dev);
> -       pm_runtime_put_autosuspend(bdi->dev);
> -}
> -
> -static int bq24190_extcon_event(struct notifier_block *nb, unsigned long event,
> -                               void *param)
> -{
> -       struct bq24190_dev_info *bdi =
> -               container_of(nb, struct bq24190_dev_info, extcon_nb);
> -
> -       /*
> -        * The Power-Good detection may take up to 220ms, sometimes
> -        * the external charger detection is quicker, and the bq24190 will
> -        * reset to iinlim based on its own charger detection (which is not
> -        * hooked up when using external charger detection) resulting in
> -        * a too low default 500mA iinlim. Delay applying the extcon value
> -        * for 300ms to avoid this.
> -        */
> -       queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300));
> -
> -       return NOTIFY_OK;
> -}
> -
>  static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  {
>         u8 v;
> @@ -1636,7 +1563,6 @@ static int bq24190_probe(struct i2c_client *client,
>         struct device *dev = &client->dev;
>         struct power_supply_config charger_cfg = {}, battery_cfg = {};
>         struct bq24190_dev_info *bdi;
> -       const char *name;
>         int ret;
>
>         if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> @@ -1668,25 +1594,6 @@ static int bq24190_probe(struct i2c_client *client,
>                 device_property_read_bool(dev,
>                                           "input-current-limit-from-supplier");
>
> -       /*
> -        * Devicetree platforms should get extcon via phandle (not yet supported).
> -        * On ACPI platforms, extcon clients may invoke us with:
> -        * struct property_entry pe[] =
> -        *   { PROPERTY_ENTRY_STRING("extcon-name", client_name), ... };
> -        * struct i2c_board_info bi =
> -        *   { .type = "bq24190", .addr = 0x6b, .properties = pe, .irq = irq };
> -        * struct i2c_adapter ad = { ... };
> -        * i2c_add_adapter(&ad);
> -        * i2c_new_device(&ad, &bi);
> -        */

Maybe we should keep a variation of the above comment, documenting how
we're invoked by another driver, and the dev props it sets for us.

> -       if (device_property_read_string(dev, "extcon-name", &name) == 0) {
> -               bdi->extcon = extcon_get_extcon_dev(name);
> -               if (!bdi->extcon)
> -                       return -EPROBE_DEFER;
> -
> -               dev_info(bdi->dev, "using extcon device %s\n", name);
> -       }
> -
>         pm_runtime_enable(dev);
>         pm_runtime_use_autosuspend(dev);
>         pm_runtime_set_autosuspend_delay(dev, 600);
> @@ -1747,20 +1654,6 @@ static int bq24190_probe(struct i2c_client *client,
>                 goto out_sysfs;
>         }
>
> -       if (bdi->extcon) {
> -               INIT_DELAYED_WORK(&bdi->extcon_work, bq24190_extcon_work);
> -               bdi->extcon_nb.notifier_call = bq24190_extcon_event;
> -               ret = devm_extcon_register_notifier_all(dev, bdi->extcon,
> -                                                       &bdi->extcon_nb);
> -               if (ret) {
> -                       dev_err(dev, "Can't register extcon\n");
> -                       goto out_sysfs;
> -               }
> -
> -               /* Sync initial cable state */
> -               queue_delayed_work(system_wq, &bdi->extcon_work, 0);
> -       }
> -
>         enable_irq_wake(client->irq);
>
>         pm_runtime_mark_last_busy(dev);
> --
> 2.13.3
>

Re: [PATCH 16/18] power: supply: bq24190_charger: Remove extcon handling

[Hans de Goede]

Hi,

On 08-08-17 10:27, Liam Breck wrote:
> Hi Hans,
> 
> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Now that drivers/i2c/busses/i2c-cht-wc.c uses
>> "input-current-limit-from-supplier" instead of "extcon-name" the last
>> user of the bq24190 extcon code is gone, remove it.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/power/supply/bq24190_charger.c | 107 ---------------------------------
>>   1 file changed, 107 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index 1f6424f0772f..0376de6d8e70 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -11,7 +11,6 @@
>>   #include <linux/module.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/delay.h>
>> -#include <linux/extcon.h>
>>   #include <linux/of_irq.h>
>>   #include <linux/of_device.h>
>>   #include <linux/pm_runtime.h>
>> @@ -155,9 +154,6 @@ struct bq24190_dev_info {
>>          struct device                   *dev;
>>          struct power_supply             *charger;
>>          struct power_supply             *battery;
>> -       struct extcon_dev               *extcon;
>> -       struct notifier_block           extcon_nb;
>> -       struct delayed_work             extcon_work;
>>          char                            model_name[I2C_NAME_SIZE];
>>          bool                            initialized;
>>          bool                            irq_event;
>> @@ -1530,75 +1526,6 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>>          return IRQ_HANDLED;
>>   }
>>
>> -static void bq24190_extcon_work(struct work_struct *work)
>> -{
>> -       struct bq24190_dev_info *bdi =
>> -               container_of(work, struct bq24190_dev_info, extcon_work.work);
>> -       int error, iinlim = 0;
>> -       u8 v;
>> -
>> -       error = pm_runtime_get_sync(bdi->dev);
>> -       if (error < 0) {
>> -               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>> -               pm_runtime_put_noidle(bdi->dev);
>> -               return;
>> -       }
>> -
>> -       if      (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
>> -               iinlim =  500000;
>> -       else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
>> -                extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
>> -               iinlim = 1500000;
>> -       else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
>> -               iinlim = 2000000;
>> -
>> -       if (iinlim) {
>> -               error = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
>> -                                             BQ24190_REG_ISC_IINLIM_MASK,
>> -                                             BQ24190_REG_ISC_IINLIM_SHIFT,
>> -                                             bq24190_isc_iinlim_values,
>> -                                             ARRAY_SIZE(bq24190_isc_iinlim_values),
>> -                                             iinlim);
>> -               if (error < 0)
>> -                       dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
>> -       }
>> -
>> -       /* if no charger found and in USB host mode, set OTG 5V boost, else normal */
>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
>> -               v = BQ24190_REG_POC_CHG_CONFIG_OTG;
>> -       else
>> -               v = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>> -
>> -       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>> -                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>> -                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>> -                                  v);
>> -       if (error < 0)
>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>> -
>> -       pm_runtime_mark_last_busy(bdi->dev);
>> -       pm_runtime_put_autosuspend(bdi->dev);
>> -}
>> -
>> -static int bq24190_extcon_event(struct notifier_block *nb, unsigned long event,
>> -                               void *param)
>> -{
>> -       struct bq24190_dev_info *bdi =
>> -               container_of(nb, struct bq24190_dev_info, extcon_nb);
>> -
>> -       /*
>> -        * The Power-Good detection may take up to 220ms, sometimes
>> -        * the external charger detection is quicker, and the bq24190 will
>> -        * reset to iinlim based on its own charger detection (which is not
>> -        * hooked up when using external charger detection) resulting in
>> -        * a too low default 500mA iinlim. Delay applying the extcon value
>> -        * for 300ms to avoid this.
>> -        */
>> -       queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300));
>> -
>> -       return NOTIFY_OK;
>> -}
>> -
>>   static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>>   {
>>          u8 v;
>> @@ -1636,7 +1563,6 @@ static int bq24190_probe(struct i2c_client *client,
>>          struct device *dev = &client->dev;
>>          struct power_supply_config charger_cfg = {}, battery_cfg = {};
>>          struct bq24190_dev_info *bdi;
>> -       const char *name;
>>          int ret;
>>
>>          if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
>> @@ -1668,25 +1594,6 @@ static int bq24190_probe(struct i2c_client *client,
>>                  device_property_read_bool(dev,
>>                                            "input-current-limit-from-supplier");
>>
>> -       /*
>> -        * Devicetree platforms should get extcon via phandle (not yet supported).
>> -        * On ACPI platforms, extcon clients may invoke us with:
>> -        * struct property_entry pe[] =
>> -        *   { PROPERTY_ENTRY_STRING("extcon-name", client_name), ... };
>> -        * struct i2c_board_info bi =
>> -        *   { .type = "bq24190", .addr = 0x6b, .properties = pe, .irq = irq };
>> -        * struct i2c_adapter ad = { ... };
>> -        * i2c_add_adapter(&ad);
>> -        * i2c_new_device(&ad, &bi);
>> -        */
> 
> Maybe we should keep a variation of the above comment, documenting how
> we're invoked by another driver, and the dev props it sets for us.

Ok, I will move this comment to one of the other props for v2.

Regards,

Hans

[PATCH 17/18] platform/x86: intel_cht_int33fe Update fusb302 type string, add properties

[Hans de Goede]

The fusb302 driver as merged in staging uses "typec_fusb302" as i2c-id
rather then just "fusb302" and needs us to set a number of device-
properties, adjust the intel_cht_int33fe driver accordingly.

One of the properties set is max-snk-mv which makes the fusb302 driver
negotiate up to 12V charging voltage, which is a bad idea on boards
which are not setup to handle this, so this commit also adds 2 extra
sanity checks to make sure that the expected Whiskey Cove PMIC +
TI bq24292i charger combo, which can handle 12V, is present.

Unfortunately the entire INT33FE device is undocumented, so there is no
other way to ensure doing 12V charging is safe.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/Kconfig             |  6 ++++-
 drivers/platform/x86/intel_cht_int33fe.c | 39 ++++++++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ef738602f897..2e412a3dacb1 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -793,7 +793,7 @@ config ACPI_CMPC
 
 config INTEL_CHT_INT33FE
 	tristate "Intel Cherry Trail ACPI INT33FE Driver"
-	depends on X86 && ACPI && I2C
+	depends on X86 && ACPI && I2C && REGULATOR
 	---help---
 	  This driver add support for the INT33FE ACPI device found on
 	  some Intel Cherry Trail devices.
@@ -804,6 +804,10 @@ config INTEL_CHT_INT33FE
 	  This driver instantiates i2c-clients for these, so that standard
 	  i2c drivers for these chips can bind to the them.
 
+	  If you enable this driver it is advised to also select
+	  CONFIG_CHARGER_BQ24190=m, CONFIG_BATTERY_MAX17042=m and
+	  CONFIG_TYPEC_FUSB302=m (currently in drivers/staging).
+
 config INTEL_INT0002_VGPIO
 	tristate "Intel ACPI INT0002 Virtual GPIO driver"
 	depends on GPIOLIB && ACPI
diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index da706e2c4232..69f8ab0d8d55 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -24,9 +24,11 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 #define EXPECTED_PTYPE		4
+#define BQ24292I_REGULATOR	"regulator-bq24190-usb-vbus"
 
 struct cht_int33fe_data {
 	struct i2c_client *max17047;
@@ -41,14 +43,24 @@ static const struct property_entry max17047_props[] = {
 	{ }
 };
 
+static const struct property_entry fusb302_props[] = {
+	PROPERTY_ENTRY_STRING("fcs,extcon-name", "cht_wcove_pwrsrc"),
+	PROPERTY_ENTRY_STRING("fcs,vbus-regulator-name", BQ24292I_REGULATOR),
+	PROPERTY_ENTRY_U32("fcs,max-snk-mv", 12000),
+	PROPERTY_ENTRY_U32("fcs,max-snk-ma", 3000),
+	PROPERTY_ENTRY_U32("fcs,max-snk-mw", 36000),
+	{ }
+};
+
 static int cht_int33fe_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct i2c_board_info board_info;
 	struct cht_int33fe_data *data;
+	struct regulator *regulator;
 	unsigned long long ptyp;
 	acpi_status status;
-	int fusb302_irq;
+	int ret, fusb302_irq;
 
 	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp);
 	if (ACPI_FAILURE(status)) {
@@ -63,6 +75,28 @@ static int cht_int33fe_probe(struct i2c_client *client)
 	if (ptyp != EXPECTED_PTYPE)
 		return -ENODEV;
 
+	/* Check presence of INT34D3 (hardware-rev 3) expected for ptype == 4 */
+	if (!acpi_dev_present("INT34D3", "1", 3)) {
+		dev_err(dev, "Error PTYPE == %d, but no INT34D3 device\n",
+			EXPECTED_PTYPE);
+		return -ENODEV;
+	}
+
+	/*
+	 * We expect the Whiskey Cove PMIC to be paired with a TI bq24292i
+	 * charger-IC, allowing charging with up to 12V, so we set the fusb302
+	 * "fcs,max-snk-mv" device property to 12000 mV. Allowing 12V with
+	 * another charger-IC is not a good idea, so we get the bq24292i vbus
+	 * regulator here, to ensure that things are as expected.
+	 * Use regulator_get_optional so that we don't get a dummy-regulator.
+	 */
+	regulator = regulator_get_optional(dev, BQ24292I_REGULATOR);
+	if (IS_ERR(regulator)) {
+		ret = PTR_ERR(regulator);
+		return (ret == -ENODEV) ? -EPROBE_DEFER : ret;
+	}
+	regulator_put(regulator);
+
 	/* The FUSB302 uses the irq at index 1 and is the only irq user */
 	fusb302_irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 1);
 	if (fusb302_irq < 0) {
@@ -84,7 +118,8 @@ static int cht_int33fe_probe(struct i2c_client *client)
 		return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
 
 	memset(&board_info, 0, sizeof(board_info));
-	strlcpy(board_info.type, "fusb302", I2C_NAME_SIZE);
+	strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
+	board_info.properties = fusb302_props;
 	board_info.irq = fusb302_irq;
 
 	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
-- 
2.13.3

[PATCH 18/18] i2c-cht-wc: Add device-properties for fusb302 integration

[Hans de Goede]

Add device-properties to make the bq24292i controller connected to
the bus get its input-current-limit from the fusb302 Type-C port
controller which is used on boards with the cht-wc PMIC.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/Kconfig      | 5 +++++
 drivers/i2c/busses/i2c-cht-wc.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f20b1f84013a..6de21a81b00b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -197,6 +197,11 @@ config I2C_CHT_WC
 	  SMBus controller found in the Intel Cherry Trail Whiskey Cove PMIC
 	  found on some Intel Cherry Trail systems.
 
+	  Note this controller is hooked up to a TI bq24292i charger-IC,
+	  combined with a FUSB302 Type-C port-controller as such it is advised
+	  to also select CONFIG_CHARGER_BQ24190=m and CONFIG_TYPEC_FUSB302=m
+	  (the fusb302 driver currently is in drivers/staging).
+
 config I2C_NFORCE2
 	tristate "Nvidia nForce2, nForce3 and nForce4"
 	depends on PCI
diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index ccf0785bcb75..08229fb12615 100644
--- a/drivers/i2c/busses/i2c-cht-wc.c
+++ b/drivers/i2c/busses/i2c-cht-wc.c
@@ -211,8 +211,11 @@ static const struct irq_chip cht_wc_i2c_irq_chip = {
 	.name			= "cht_wc_ext_chrg_irq_chip",
 };
 
+static const char * const bq24190_suppliers[] = { "fusb302-typec-source" };
+
 static const struct property_entry bq24190_props[] = {
-	PROPERTY_ENTRY_STRING("extcon-name", "cht_wcove_pwrsrc"),
+	PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq24190_suppliers),
+	PROPERTY_ENTRY_BOOL("input-current-limit-from-supplier"),
 	PROPERTY_ENTRY_BOOL("omit-battery-class"),
 	PROPERTY_ENTRY_BOOL("disable-reset"),
 	{ }
-- 
2.13.3

Re: [PATCH 18/18] i2c-cht-wc: Add device-properties for fusb302 integration

[Guenter Roeck]

On 08/06/2017 05:35 AM, Hans de Goede wrote:
> Add device-properties to make the bq24292i controller connected to
> the bus get its input-current-limit from the fusb302 Type-C port
> controller which is used on boards with the cht-wc PMIC.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/i2c/busses/Kconfig      | 5 +++++
>   drivers/i2c/busses/i2c-cht-wc.c | 5 ++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index f20b1f84013a..6de21a81b00b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -197,6 +197,11 @@ config I2C_CHT_WC
>   	  SMBus controller found in the Intel Cherry Trail Whiskey Cove PMIC
>   	  found on some Intel Cherry Trail systems.
>   
> +	  Note this controller is hooked up to a TI bq24292i charger-IC,
> +	  combined with a FUSB302 Type-C port-controller as such it is advised
> +	  to also select CONFIG_CHARGER_BQ24190=m and CONFIG_TYPEC_FUSB302=m
> +	  (the fusb302 driver currently is in drivers/staging).
> +

Just wondering - is this always the case ? What if someone builds a system with
different charger and port controller ICs ?

>   config I2C_NFORCE2
>   	tristate "Nvidia nForce2, nForce3 and nForce4"
>   	depends on PCI
> diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
> index ccf0785bcb75..08229fb12615 100644
> --- a/drivers/i2c/busses/i2c-cht-wc.c
> +++ b/drivers/i2c/busses/i2c-cht-wc.c
> @@ -211,8 +211,11 @@ static const struct irq_chip cht_wc_i2c_irq_chip = {
>   	.name			= "cht_wc_ext_chrg_irq_chip",
>   };
>   
> +static const char * const bq24190_suppliers[] = { "fusb302-typec-source" };
> +
>   static const struct property_entry bq24190_props[] = {
> -	PROPERTY_ENTRY_STRING("extcon-name", "cht_wcove_pwrsrc"),
> +	PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq24190_suppliers),
> +	PROPERTY_ENTRY_BOOL("input-current-limit-from-supplier"),
>   	PROPERTY_ENTRY_BOOL("omit-battery-class"),
>   	PROPERTY_ENTRY_BOOL("disable-reset"),
>   	{ }
> 

Re: [PATCH 18/18] i2c-cht-wc: Add device-properties for fusb302 integration

[Hans de Goede]

Hi,

On 06-08-17 16:35, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> Add device-properties to make the bq24292i controller connected to
>> the bus get its input-current-limit from the fusb302 Type-C port
>> controller which is used on boards with the cht-wc PMIC.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/i2c/busses/Kconfig      | 5 +++++
>>   drivers/i2c/busses/i2c-cht-wc.c | 5 ++++-
>>   2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index f20b1f84013a..6de21a81b00b 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -197,6 +197,11 @@ config I2C_CHT_WC
>>         SMBus controller found in the Intel Cherry Trail Whiskey Cove PMIC
>>         found on some Intel Cherry Trail systems.
>> +      Note this controller is hooked up to a TI bq24292i charger-IC,
>> +      combined with a FUSB302 Type-C port-controller as such it is advised
>> +      to also select CONFIG_CHARGER_BQ24190=m and CONFIG_TYPEC_FUSB302=m
>> +      (the fusb302 driver currently is in drivers/staging).
>> +
> 
> Just wondering - is this always the case ? What if someone builds a system with
> different charger and port controller ICs ?

A very valid question, if another charger is used then hopefully it will
have a different i2c address and if it doesn't it should fail the id-register
check in the bq24190 driver unless we get really unlucky. So if this happens
the bq24190 driver should fail to probe.

The code handling the INT33FE ACPI device, which contains the i2c bus
and address info for the FUSB302 has this check:

         /*
          * We expect the Whiskey Cove PMIC to be paired with a TI bq24292i
          * charger-IC, allowing charging with up to 12V, so we set the fusb302
          * "fcs,max-snk-mv" device property to 12000 mV. Allowing 12V with
          * another charger-IC is not a good idea, so we get the bq24292i vbus
          * regulator here, to ensure that things are as expected.
          * Use regulator_get_optional so that we don't get a dummy-regulator.
          */
         regulator = regulator_get_optional(dev, BQ24292I_REGULATOR);
         if (IS_ERR(regulator)) {
                 ret = PTR_ERR(regulator);
                 return (ret == -ENODEV) ? -EPROBE_DEFER : ret;
         }
         regulator_put(regulator);

So if the bq24190 probe fails and it does not register its regulator, the
i2c-client for the fusb302 will never gets instantiated. Which means that
if a different charger-IC is used the user will get a bunch of errors and
nothing will happen.

If a different port-controller is used, then I would expect there to no
be a INT33FE ACPI device, with PTYP==4 as PTYP==4 seems to be used
to describe the Whiskey Cove PMIC + bq24190 charger + fusb302 combo,
but this is all undocumented, so no guarantees. I've added the above
code because of this, because I really don't want to negotiate a voltage
higher then 5V with an unknown charger-IC.

FWIW all DSTDs I've seen are all copy and paste and all declare a INT33FE ACPI
device with identical i2c client addresses which strongly suggests the
use of the same combo. Note that on most devices this part of the DSTD is
not active (_STA returns 0) because they actually use a different config.

The only extra safe-guard I can come up with is a DMI string check, but that
is sub-optimal since the DMI strings on these devices contain useful values
as "Default String" still we could add it as an extra check.

Since I had the same concern I've done a web search and I've been unable
to find any other devices which seem to use a Whiskey Cove PMIC, but that
does not mean that there aren't any.

Regards,

Hans






> 
>>   config I2C_NFORCE2
>>       tristate "Nvidia nForce2, nForce3 and nForce4"
>>       depends on PCI
>> diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
>> index ccf0785bcb75..08229fb12615 100644
>> --- a/drivers/i2c/busses/i2c-cht-wc.c
>> +++ b/drivers/i2c/busses/i2c-cht-wc.c
>> @@ -211,8 +211,11 @@ static const struct irq_chip cht_wc_i2c_irq_chip = {
>>       .name            = "cht_wc_ext_chrg_irq_chip",
>>   };
>> +static const char * const bq24190_suppliers[] = { "fusb302-typec-source" };
>> +
>>   static const struct property_entry bq24190_props[] = {
>> -    PROPERTY_ENTRY_STRING("extcon-name", "cht_wcove_pwrsrc"),
>> +    PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq24190_suppliers),
>> +    PROPERTY_ENTRY_BOOL("input-current-limit-from-supplier"),
>>       PROPERTY_ENTRY_BOOL("omit-battery-class"),
>>       PROPERTY_ENTRY_BOOL("disable-reset"),
>>       { }
>>
> 

Re: [PATCH 18/18] i2c-cht-wc: Add device-properties for fusb302 integration

[Andy Shevchenko]

On Sun, Aug 6, 2017 at 6:05 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 06-08-17 16:35, Guenter Roeck wrote:
>> On 08/06/2017 05:35 AM, Hans de Goede wrote:

> FWIW all DSTDs I've seen are all copy and paste and all declare a INT33FE
> ACPI
> device with identical i2c client addresses which strongly suggests the
> use of the same combo. Note that on most devices this part of the DSTD is
> not active (_STA returns 0) because they actually use a different config.

Which is quite likely just blindly copied from a reference BIOS code.
(Reminds me that bug with GPIO pin setting for interrupt as output only)

> The only extra safe-guard I can come up with is a DMI string check, but that
> is sub-optimal since the DMI strings on these devices contain useful values
> as "Default String" still we could add it as an extra check.

...and in worst cases CPU model ID.

> Since I had the same concern I've done a web search and I've been unable
> to find any other devices which seem to use a Whiskey Cove PMIC, but that
> does not mean that there aren't any.


-- 
With Best Regards,
Andy Shevchenko