| Message ID | 20170806123555.5124-9-hdegoede@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
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; >
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
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
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;
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(+)