[PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes

[PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes

[Heikki Krogerus]

Hi guys,

The software nodes support node hierarchy. By using them with fusb302
we can add a separate fwnode also for the USB connector as a child of
fusb302. We can then use the "standard" USB connector device
properties with the connector node, and stop using the deprecated
fusb302 specific properties.

Since the goal is to ultimately move to the software node API from the
old device property API, converting also max17047 in this series.

If you test this now (before v5.1-rc1 is out), then the series depends
Greg's latest usb-next:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-next

and on a patch in Rafael's latest linux-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=344798206f171c5abea7ab1f9762fa526d7f539d

thanks,

Heikki Krogerus (2):
  platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector
  platform/x86: intel_cht_int33fe: Create fwnode for max17047

 drivers/platform/x86/intel_cht_int33fe.c | 122 +++++++++++++++++------
 1 file changed, 91 insertions(+), 31 deletions(-)

-- 
2.20.1

[PATCH 1/2] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Heikki Krogerus]

In ACPI, and now also in DT, the USB connectors usually have
their own device nodes. In case of USB Type-C, those
connector (port) nodes are child nodes of the controller or
PHY device, in our case the fusb302. The software fwnodes
allow us to create a similar child node for fusb302 that
represents the connector also on Intel CHT.

This makes it possible replace the fusb302 specific device
properties which were deprecated with the common USB
connector properties that tcpm.c is able to use directly.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 48 ++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 6fa3cced6f8e..4857a57cfff5 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/usb/pd.h>
 
 #define EXPECTED_PTYPE		4
 
@@ -31,6 +32,8 @@ struct cht_int33fe_data {
 	struct i2c_client *max17047;
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
+	struct fwnode_handle *port_node;
+	struct fwnode_handle *fusb302_node;
 	/* Contain a list-head must be per device */
 	struct device_connection connections[4];
 };
@@ -80,9 +83,29 @@ static const struct property_entry max17047_props[] = {
 
 static const struct property_entry fusb302_props[] = {
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
-	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
-	PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
-	PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),
+	{ }
+};
+
+#define PDO_FIXED_FLAGS \
+	(PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)
+
+static const u32 src_pdo[] = {
+	PDO_FIXED(5000, 1500, PDO_FIXED_FLAGS),
+};
+
+static const u32 snk_pdo[] = {
+	PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
+	PDO_VAR(5000, 12000, 3000),
+};
+
+static const struct property_entry usb_connector_props[] = {
+	PROPERTY_ENTRY_STRING("name", "connector"),
+	PROPERTY_ENTRY_STRING("data-role", "dual"),
+	PROPERTY_ENTRY_STRING("power-role", "dual"),
+	PROPERTY_ENTRY_STRING("try-power-role", "sink"),
+	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
+	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
+	PROPERTY_ENTRY_U32("op-sink-microwatt", 36000000),
 	{ }
 };
 
@@ -151,6 +174,19 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	/* Node for the FUSB302 controller */
+	data->fusb302_node = fwnode_create_software_node(fusb302_props, NULL);
+	if (IS_ERR(data->fusb302_node))
+		return PTR_ERR(data->fusb302_node);
+
+	/* Node for the connector (FUSB302 is the parent) */
+	data->port_node = fwnode_create_software_node(usb_connector_props,
+						      data->fusb302_node);
+	if (IS_ERR(data->port_node)) {
+		fwnode_remove_software_node(data->fusb302_node);
+		return PTR_ERR(data->port_node);
+	}
+
 	/* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */
 	max17047 = cht_int33fe_find_max17047();
 	if (max17047) {
@@ -187,7 +223,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	memset(&board_info, 0, sizeof(board_info));
 	strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
 	board_info.dev_name = "fusb302";
-	board_info.properties = fusb302_props;
+	board_info.fwnode = data->fusb302_node;
 	board_info.irq = fusb302_irq;
 
 	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
@@ -217,6 +253,8 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	i2c_unregister_device(data->max17047);
 
 	device_connections_remove(data->connections);
+	fwnode_remove_software_node(data->port_node);
+	fwnode_remove_software_node(data->fusb302_node);
 
 	return ret;
 }
@@ -230,6 +268,8 @@ static int cht_int33fe_remove(struct platform_device *pdev)
 	i2c_unregister_device(data->max17047);
 
 	device_connections_remove(data->connections);
+	fwnode_remove_software_node(data->port_node);
+	fwnode_remove_software_node(data->fusb302_node);
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH 1/2] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Andy Shevchenko]

On Tue, Feb 19, 2019 at 2:00 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> In ACPI, and now also in DT, the USB connectors usually have
> their own device nodes. In case of USB Type-C, those
> connector (port) nodes are child nodes of the controller or
> PHY device, in our case the fusb302. The software fwnodes
> allow us to create a similar child node for fusb302 that
> represents the connector also on Intel CHT.
>
> This makes it possible replace the fusb302 specific device
> properties which were deprecated with the common USB
> connector properties that tcpm.c is able to use directly.



> +       /* Node for the FUSB302 controller */
> +       data->fusb302_node = fwnode_create_software_node(fusb302_props, NULL);
> +       if (IS_ERR(data->fusb302_node))
> +               return PTR_ERR(data->fusb302_node);
> +
> +       /* Node for the connector (FUSB302 is the parent) */
> +       data->port_node = fwnode_create_software_node(usb_connector_props,
> +                                                     data->fusb302_node);
> +       if (IS_ERR(data->port_node)) {

> +               fwnode_remove_software_node(data->fusb302_node);

Sounds like a candidate for
devm_fwnode_create_software_node()

> +               return PTR_ERR(data->port_node);
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Heikki Krogerus]

On Wed, Feb 20, 2019 at 05:55:21PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 19, 2019 at 2:00 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > In ACPI, and now also in DT, the USB connectors usually have
> > their own device nodes. In case of USB Type-C, those
> > connector (port) nodes are child nodes of the controller or
> > PHY device, in our case the fusb302. The software fwnodes
> > allow us to create a similar child node for fusb302 that
> > represents the connector also on Intel CHT.
> >
> > This makes it possible replace the fusb302 specific device
> > properties which were deprecated with the common USB
> > connector properties that tcpm.c is able to use directly.
> 
> > +       /* Node for the FUSB302 controller */
> > +       data->fusb302_node = fwnode_create_software_node(fusb302_props, NULL);
> > +       if (IS_ERR(data->fusb302_node))
> > +               return PTR_ERR(data->fusb302_node);
> > +
> > +       /* Node for the connector (FUSB302 is the parent) */
> > +       data->port_node = fwnode_create_software_node(usb_connector_props,
> > +                                                     data->fusb302_node);
> > +       if (IS_ERR(data->port_node)) {
> 
> > +               fwnode_remove_software_node(data->fusb302_node);
> 
> Sounds like a candidate for
> devm_fwnode_create_software_node()

If you don't mind, let's think about that later.

I'm not comfortable at all with the idea of introducing
devm_fwnode_create_software_node(). I would like to talk about that
separately. It's not going to be a problem to change this driver later
even if we decide to add the function.


thanks,

-- 
heikki

Re: [PATCH 1/2] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Andy Shevchenko]

On Thu, Feb 21, 2019 at 11:35 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Feb 20, 2019 at 05:55:21PM +0200, Andy Shevchenko wrote:
> > On Tue, Feb 19, 2019 at 2:00 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > In ACPI, and now also in DT, the USB connectors usually have
> > > their own device nodes. In case of USB Type-C, those
> > > connector (port) nodes are child nodes of the controller or
> > > PHY device, in our case the fusb302. The software fwnodes
> > > allow us to create a similar child node for fusb302 that
> > > represents the connector also on Intel CHT.
> > >
> > > This makes it possible replace the fusb302 specific device
> > > properties which were deprecated with the common USB
> > > connector properties that tcpm.c is able to use directly.
> >
> > > +       /* Node for the FUSB302 controller */
> > > +       data->fusb302_node = fwnode_create_software_node(fusb302_props, NULL);
> > > +       if (IS_ERR(data->fusb302_node))
> > > +               return PTR_ERR(data->fusb302_node);
> > > +
> > > +       /* Node for the connector (FUSB302 is the parent) */
> > > +       data->port_node = fwnode_create_software_node(usb_connector_props,
> > > +                                                     data->fusb302_node);
> > > +       if (IS_ERR(data->port_node)) {
> >
> > > +               fwnode_remove_software_node(data->fusb302_node);
> >
> > Sounds like a candidate for
> > devm_fwnode_create_software_node()
>
> If you don't mind, let's think about that later.

Sure.

>
> I'm not comfortable at all with the idea of introducing
> devm_fwnode_create_software_node(). I would like to talk about that
> separately. It's not going to be a problem to change this driver later
> even if we decide to add the function.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Andy Shevchenko]

On Tue, Feb 19, 2019 at 2:00 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> In ACPI, and now also in DT, the USB connectors usually have
> their own device nodes. In case of USB Type-C, those
> connector (port) nodes are child nodes of the controller or
> PHY device, in our case the fusb302. The software fwnodes
> allow us to create a similar child node for fusb302 that
> represents the connector also on Intel CHT.
>
> This makes it possible replace the fusb302 specific device
> properties which were deprecated with the common USB
> connector properties that tcpm.c is able to use directly.
>

This doesn't apply to our for-next branch.

> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/platform/x86/intel_cht_int33fe.c | 48 ++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 6fa3cced6f8e..4857a57cfff5 100644
> --- a/drivers/platform/x86/intel_cht_int33fe.c
> +++ b/drivers/platform/x86/intel_cht_int33fe.c
> @@ -24,6 +24,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> +#include <linux/usb/pd.h>
>
>  #define EXPECTED_PTYPE         4
>
> @@ -31,6 +32,8 @@ struct cht_int33fe_data {
>         struct i2c_client *max17047;
>         struct i2c_client *fusb302;
>         struct i2c_client *pi3usb30532;
> +       struct fwnode_handle *port_node;
> +       struct fwnode_handle *fusb302_node;
>         /* Contain a list-head must be per device */
>         struct device_connection connections[4];
>  };
> @@ -80,9 +83,29 @@ static const struct property_entry max17047_props[] = {
>
>  static const struct property_entry fusb302_props[] = {
>         PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
> -       PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
> -       PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
> -       PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),
> +       { }
> +};
> +
> +#define PDO_FIXED_FLAGS \
> +       (PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)
> +
> +static const u32 src_pdo[] = {
> +       PDO_FIXED(5000, 1500, PDO_FIXED_FLAGS),
> +};
> +
> +static const u32 snk_pdo[] = {
> +       PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
> +       PDO_VAR(5000, 12000, 3000),
> +};
> +
> +static const struct property_entry usb_connector_props[] = {
> +       PROPERTY_ENTRY_STRING("name", "connector"),
> +       PROPERTY_ENTRY_STRING("data-role", "dual"),
> +       PROPERTY_ENTRY_STRING("power-role", "dual"),
> +       PROPERTY_ENTRY_STRING("try-power-role", "sink"),
> +       PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
> +       PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
> +       PROPERTY_ENTRY_U32("op-sink-microwatt", 36000000),
>         { }
>  };
>
> @@ -151,6 +174,19 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>         if (!data)
>                 return -ENOMEM;
>
> +       /* Node for the FUSB302 controller */
> +       data->fusb302_node = fwnode_create_software_node(fusb302_props, NULL);
> +       if (IS_ERR(data->fusb302_node))
> +               return PTR_ERR(data->fusb302_node);
> +
> +       /* Node for the connector (FUSB302 is the parent) */
> +       data->port_node = fwnode_create_software_node(usb_connector_props,
> +                                                     data->fusb302_node);
> +       if (IS_ERR(data->port_node)) {
> +               fwnode_remove_software_node(data->fusb302_node);
> +               return PTR_ERR(data->port_node);
> +       }
> +
>         /* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */
>         max17047 = cht_int33fe_find_max17047();
>         if (max17047) {
> @@ -187,7 +223,7 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>         memset(&board_info, 0, sizeof(board_info));
>         strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
>         board_info.dev_name = "fusb302";
> -       board_info.properties = fusb302_props;
> +       board_info.fwnode = data->fusb302_node;
>         board_info.irq = fusb302_irq;
>
>         data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
> @@ -217,6 +253,8 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>         i2c_unregister_device(data->max17047);
>
>         device_connections_remove(data->connections);
> +       fwnode_remove_software_node(data->port_node);
> +       fwnode_remove_software_node(data->fusb302_node);
>
>         return ret;
>  }
> @@ -230,6 +268,8 @@ static int cht_int33fe_remove(struct platform_device *pdev)
>         i2c_unregister_device(data->max17047);
>
>         device_connections_remove(data->connections);
> +       fwnode_remove_software_node(data->port_node);
> +       fwnode_remove_software_node(data->fusb302_node);
>
>         return 0;
>  }
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Heikki Krogerus]

On Thu, Feb 21, 2019 at 03:46:46PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 19, 2019 at 2:00 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > In ACPI, and now also in DT, the USB connectors usually have
> > their own device nodes. In case of USB Type-C, those
> > connector (port) nodes are child nodes of the controller or
> > PHY device, in our case the fusb302. The software fwnodes
> > allow us to create a similar child node for fusb302 that
> > represents the connector also on Intel CHT.
> >
> > This makes it possible replace the fusb302 specific device
> > properties which were deprecated with the common USB
> > connector properties that tcpm.c is able to use directly.
> >
> 
> This doesn't apply to our for-next branch.

Please read the cover-letter ;-)


thanks,

-- 
heikki

Re: [PATCH 1/2] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

[Andy Shevchenko]

On Thu, Feb 21, 2019 at 3:56 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Thu, Feb 21, 2019 at 03:46:46PM +0200, Andy Shevchenko wrote:
> > On Tue, Feb 19, 2019 at 2:00 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > In ACPI, and now also in DT, the USB connectors usually have
> > > their own device nodes. In case of USB Type-C, those
> > > connector (port) nodes are child nodes of the controller or
> > > PHY device, in our case the fusb302. The software fwnodes
> > > allow us to create a similar child node for fusb302 that
> > > represents the connector also on Intel CHT.
> > >
> > > This makes it possible replace the fusb302 specific device
> > > properties which were deprecated with the common USB
> > > connector properties that tcpm.c is able to use directly.
> > >
> >
> > This doesn't apply to our for-next branch.
>
> Please read the cover-letter ;-)

Ah, sure.
I did it once and even remove from our patchwork queue, but then
forgot details. Now it's all clear.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/2] platform/x86: intel_cht_int33fe: Create fwnode for max17047

[Heikki Krogerus]

Creating a software node for max17047 when ACPI tables don't
have a device node for it.

While here, moving max17047 handling to its own function.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 86 +++++++++++++++---------
 1 file changed, 53 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 4857a57cfff5..81721976b00c 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -34,6 +34,7 @@ struct cht_int33fe_data {
 	struct i2c_client *pi3usb30532;
 	struct fwnode_handle *port_node;
 	struct fwnode_handle *fusb302_node;
+	struct fwnode_handle *max17047_node;
 	/* Contain a list-head must be per device */
 	struct device_connection connections[4];
 };
@@ -66,14 +67,6 @@ static int cht_int33fe_check_for_max17047(struct device *dev, void *data)
 	return 1;
 }
 
-static struct i2c_client *cht_int33fe_find_max17047(void)
-{
-	struct i2c_client *max17047 = NULL;
-
-	i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047);
-	return max17047;
-}
-
 static const char * const max17047_suppliers[] = { "bq24190-charger" };
 
 static const struct property_entry max17047_props[] = {
@@ -81,6 +74,43 @@ static const struct property_entry max17047_props[] = {
 	{ }
 };
 
+static int
+cht_int33fe_max17047(struct device *dev, struct cht_int33fe_data *data)
+{
+	struct i2c_client *max17047 = NULL;
+	struct i2c_board_info board_info;
+	int ret;
+
+	i2c_for_each_dev(&max17047, cht_int33fe_check_for_max17047);
+	if (max17047) {
+		/* Pre-existing i2c-client for the max17047, add device-props */
+		ret = device_add_properties(&max17047->dev, max17047_props);
+		if (ret)
+			return ret;
+		/* And re-probe to get the new device-props applied. */
+		ret = device_reprobe(&max17047->dev);
+		if (ret)
+			dev_warn(dev, "Reprobing max17047 error: %d\n", ret);
+		return 0;
+	}
+
+	data->max17047_node = fwnode_create_software_node(max17047_props, NULL);
+	if (IS_ERR(data->max17047_node))
+		return PTR_ERR(data->max17047_node);
+
+	memset(&board_info, 0, sizeof(board_info));
+	strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
+	board_info.dev_name = "max17047";
+	board_info.fwnode = data->max17047_node;
+	data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
+	if (IS_ERR(data->max17047)) {
+		fwnode_remove_software_node(data->max17047_node);
+		return PTR_ERR(data->max17047);
+	}
+
+	return 0;
+}
+
 static const struct property_entry fusb302_props[] = {
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
 	{ }
@@ -114,7 +144,6 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct i2c_board_info board_info;
 	struct cht_int33fe_data *data;
-	struct i2c_client *max17047;
 	struct regulator *regulator;
 	unsigned long long ptyp;
 	acpi_status status;
@@ -174,38 +203,25 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	/* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */
+	ret = cht_int33fe_max17047(dev, data);
+	if (ret)
+		return ret;
+
 	/* Node for the FUSB302 controller */
 	data->fusb302_node = fwnode_create_software_node(fusb302_props, NULL);
-	if (IS_ERR(data->fusb302_node))
-		return PTR_ERR(data->fusb302_node);
+	if (IS_ERR(data->fusb302_node)) {
+		ret = PTR_ERR(data->fusb302_node);
+		goto out_remove_max17047_node;
+	}
 
 	/* Node for the connector (FUSB302 is the parent) */
 	data->port_node = fwnode_create_software_node(usb_connector_props,
 						      data->fusb302_node);
 	if (IS_ERR(data->port_node)) {
 		fwnode_remove_software_node(data->fusb302_node);
-		return PTR_ERR(data->port_node);
-	}
-
-	/* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */
-	max17047 = cht_int33fe_find_max17047();
-	if (max17047) {
-		/* Pre-existing i2c-client for the max17047, add device-props */
-		ret = device_add_properties(&max17047->dev, max17047_props);
-		if (ret)
-			return ret;
-		/* And re-probe to get the new device-props applied. */
-		ret = device_reprobe(&max17047->dev);
-		if (ret)
-			dev_warn(dev, "Reprobing max17047 error: %d\n", ret);
-	} else {
-		memset(&board_info, 0, sizeof(board_info));
-		strlcpy(board_info.type, "max17047", I2C_NAME_SIZE);
-		board_info.dev_name = "max17047";
-		board_info.properties = max17047_props;
-		data->max17047 = i2c_acpi_new_device(dev, 1, &board_info);
-		if (IS_ERR(data->max17047))
-			return PTR_ERR(data->max17047);
+		ret = PTR_ERR(data->port_node);
+		goto out_remove_max17047_node;
 	}
 
 	data->connections[0].endpoint[0] = "port0";
@@ -256,6 +272,9 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	fwnode_remove_software_node(data->port_node);
 	fwnode_remove_software_node(data->fusb302_node);
 
+out_remove_max17047_node:
+	fwnode_remove_software_node(data->max17047_node);
+
 	return ret;
 }
 
@@ -270,6 +289,7 @@ static int cht_int33fe_remove(struct platform_device *pdev)
 	device_connections_remove(data->connections);
 	fwnode_remove_software_node(data->port_node);
 	fwnode_remove_software_node(data->fusb302_node);
+	fwnode_remove_software_node(data->max17047_node);
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes

[Hans de Goede]

Hi,

On 2/19/19 12:59 PM, Heikki Krogerus wrote:
> Hi guys,
> 
> The software nodes support node hierarchy. By using them with fusb302
> we can add a separate fwnode also for the USB connector as a child of
> fusb302. We can then use the "standard" USB connector device
> properties with the connector node, and stop using the deprecated
> fusb302 specific properties.
> 
> Since the goal is to ultimately move to the software node API from the
> old device property API, converting also max17047 in this series.
> 
> If you test this now (before v5.1-rc1 is out), then the series depends
> Greg's latest usb-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-next
> 
> and on a patch in Rafael's latest linux-next branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=344798206f171c5abea7ab1f9762fa526d7f539d

Interesting series, I like the direction this is heading in.

Question, I currently have this hack to test DP over Type-C on the
GPD-pocket / GPD-win:

https://github.com/jwrdegoede/linux-sunxi/commit/f481b7032030dcdda1ccc39875eb59f996d3e775

Do this properly we need to add alt-modes support for usb-c-connector nodes to:
Documentation/devicetree/bindings/connector/usb-connector.txt

Do you have any ideas for what the binding this should look like, we need to
specify a svid, mode and vdo tripple in this case. Maybe use an u32 array
with 3, 6, 9, ... entries depending on how much alt-modes the fwnode needs to
specify ?

Regards,

Hans

Re: [PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes

[Heikki Krogerus]

On Fri, Feb 22, 2019 at 05:31:32PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/19/19 12:59 PM, Heikki Krogerus wrote:
> > Hi guys,
> > 
> > The software nodes support node hierarchy. By using them with fusb302
> > we can add a separate fwnode also for the USB connector as a child of
> > fusb302. We can then use the "standard" USB connector device
> > properties with the connector node, and stop using the deprecated
> > fusb302 specific properties.
> > 
> > Since the goal is to ultimately move to the software node API from the
> > old device property API, converting also max17047 in this series.
> > 
> > If you test this now (before v5.1-rc1 is out), then the series depends
> > Greg's latest usb-next:
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-next
> > 
> > and on a patch in Rafael's latest linux-next branch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=344798206f171c5abea7ab1f9762fa526d7f539d
> 
> Interesting series, I like the direction this is heading in.
> 
> Question, I currently have this hack to test DP over Type-C on the
> GPD-pocket / GPD-win:
> 
> https://github.com/jwrdegoede/linux-sunxi/commit/f481b7032030dcdda1ccc39875eb59f996d3e775
> 
> Do this properly we need to add alt-modes support for usb-c-connector nodes to:
> Documentation/devicetree/bindings/connector/usb-connector.txt

Yes, this is a topic I wanted to talk about.

> Do you have any ideas for what the binding this should look like, we need to
> specify a svid, mode and vdo tripple in this case. Maybe use an u32 array
> with 3, 6, 9, ... entries depending on how much alt-modes the fwnode needs to
> specify ?

My idea was to use sub-nodes, i.e. every alt mode a connector supports
would need to have its own child node under the connector node. Those
sub-nodes could then have a device property "svid" and another device
property "vdo", etc.

I think that approach would be OK in DT, and we can now support it also
with the software nodes, and even in ACPI there are now something
called "data nodes" which can be used for this purpose.

There are some questions though. That "USB connector" node description
relies on OF graph, so should we just extend those "endpoint" nodes
(which are sub-nodes), or should be still have dedicated sub-nodes for
the alt modes? I think we may need dedicated nodes for the alt modes
in any case if we choose to use this approach.


thanks,

-- 
heikki

Re: [PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes

[Hans de Goede]

Hi,

On 25-02-19 16:48, Heikki Krogerus wrote:
> On Fri, Feb 22, 2019 at 05:31:32PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/19/19 12:59 PM, Heikki Krogerus wrote:
>>> Hi guys,
>>>
>>> The software nodes support node hierarchy. By using them with fusb302
>>> we can add a separate fwnode also for the USB connector as a child of
>>> fusb302. We can then use the "standard" USB connector device
>>> properties with the connector node, and stop using the deprecated
>>> fusb302 specific properties.
>>>
>>> Since the goal is to ultimately move to the software node API from the
>>> old device property API, converting also max17047 in this series.
>>>
>>> If you test this now (before v5.1-rc1 is out), then the series depends
>>> Greg's latest usb-next:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-next
>>>
>>> and on a patch in Rafael's latest linux-next branch:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=344798206f171c5abea7ab1f9762fa526d7f539d
>>
>> Interesting series, I like the direction this is heading in.
>>
>> Question, I currently have this hack to test DP over Type-C on the
>> GPD-pocket / GPD-win:
>>
>> https://github.com/jwrdegoede/linux-sunxi/commit/f481b7032030dcdda1ccc39875eb59f996d3e775
>>
>> Do this properly we need to add alt-modes support for usb-c-connector nodes to:
>> Documentation/devicetree/bindings/connector/usb-connector.txt
> 
> Yes, this is a topic I wanted to talk about.
> 
>> Do you have any ideas for what the binding this should look like, we need to
>> specify a svid, mode and vdo tripple in this case. Maybe use an u32 array
>> with 3, 6, 9, ... entries depending on how much alt-modes the fwnode needs to
>> specify ?
> 
> My idea was to use sub-nodes, i.e. every alt mode a connector supports
> would need to have its own child node under the connector node. Those
> sub-nodes could then have a device property "svid" and another device
> property "vdo", etc.

Right, after sending this mail I realized myself that using child-nodes
to group the svid and vdo together was the right answer. So we
could add child-nodes with a binding like this:

Optionally an "usb-c-connector" can have child nodes, describing
supported alt-modes.

Required properties for usb-c-connector altmode child-nodes:
compatible:  "usb-type-c-altmode"
svid:        integer, Standard or Vendor ID for the altmode (u16 stored in an u32) property and an u32
vdo:         integer, Vendor Data Object, VDO describing the altmode capabilies, SVID specific

I'm not sure if we also need to specify the altmode index here, or
we simple assign each alt-mode an index while enumerating?

> I think that approach would be OK in DT, and we can now support it also
> with the software nodes, and even in ACPI there are now something
> called "data nodes" which can be used for this purpose.

Ack.

> There are some questions though. That "USB connector" node description
> relies on OF graph, so should we just extend those "endpoint" nodes
> (which are sub-nodes), or should be still have dedicated sub-nodes for
> the alt modes? I think we may need dedicated nodes for the alt modes
> in any case if we choose to use this approach.

Right I'm thinking dedicated nodes for the alt modes.

I guess an alt-mode could have a port child-node to point to say the
DisplayPort pins / mux connection ?  I'm not entirely sure how the graph
stuff will work here, but I guess that from a DT pov it will be desirable
to be able to describe where the datalines for the alt-mode come from,
maybe in combination with what the "mux/select" value is for the mux ???

Regards,

Hans

Re: [PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes

[Heikki Krogerus]

On Mon, Feb 25, 2019 at 08:14:55PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 25-02-19 16:48, Heikki Krogerus wrote:
> > On Fri, Feb 22, 2019 at 05:31:32PM +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 2/19/19 12:59 PM, Heikki Krogerus wrote:
> > > > Hi guys,
> > > > 
> > > > The software nodes support node hierarchy. By using them with fusb302
> > > > we can add a separate fwnode also for the USB connector as a child of
> > > > fusb302. We can then use the "standard" USB connector device
> > > > properties with the connector node, and stop using the deprecated
> > > > fusb302 specific properties.
> > > > 
> > > > Since the goal is to ultimately move to the software node API from the
> > > > old device property API, converting also max17047 in this series.
> > > > 
> > > > If you test this now (before v5.1-rc1 is out), then the series depends
> > > > Greg's latest usb-next:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-next
> > > > 
> > > > and on a patch in Rafael's latest linux-next branch:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=344798206f171c5abea7ab1f9762fa526d7f539d
> > > 
> > > Interesting series, I like the direction this is heading in.
> > > 
> > > Question, I currently have this hack to test DP over Type-C on the
> > > GPD-pocket / GPD-win:
> > > 
> > > https://github.com/jwrdegoede/linux-sunxi/commit/f481b7032030dcdda1ccc39875eb59f996d3e775
> > > 
> > > Do this properly we need to add alt-modes support for usb-c-connector nodes to:
> > > Documentation/devicetree/bindings/connector/usb-connector.txt
> > 
> > Yes, this is a topic I wanted to talk about.
> > 
> > > Do you have any ideas for what the binding this should look like, we need to
> > > specify a svid, mode and vdo tripple in this case. Maybe use an u32 array
> > > with 3, 6, 9, ... entries depending on how much alt-modes the fwnode needs to
> > > specify ?
> > 
> > My idea was to use sub-nodes, i.e. every alt mode a connector supports
> > would need to have its own child node under the connector node. Those
> > sub-nodes could then have a device property "svid" and another device
> > property "vdo", etc.
> 
> Right, after sending this mail I realized myself that using child-nodes
> to group the svid and vdo together was the right answer. So we
> could add child-nodes with a binding like this:
> 
> Optionally an "usb-c-connector" can have child nodes, describing
> supported alt-modes.
> 
> Required properties for usb-c-connector altmode child-nodes:
> compatible:  "usb-type-c-altmode"
> svid:        integer, Standard or Vendor ID for the altmode (u16 stored in an u32) property and an u32
> vdo:         integer, Vendor Data Object, VDO describing the altmode capabilies, SVID specific
> 
> I'm not sure if we also need to specify the altmode index here, or
> we simple assign each alt-mode an index while enumerating?

I don't think we need the index. SVID and VDO are enough.

> > I think that approach would be OK in DT, and we can now support it also
> > with the software nodes, and even in ACPI there are now something
> > called "data nodes" which can be used for this purpose.
> 
> Ack.
> 
> > There are some questions though. That "USB connector" node description
> > relies on OF graph, so should we just extend those "endpoint" nodes
> > (which are sub-nodes), or should be still have dedicated sub-nodes for
> > the alt modes? I think we may need dedicated nodes for the alt modes
> > in any case if we choose to use this approach.
> 
> Right I'm thinking dedicated nodes for the alt modes.
> 
> I guess an alt-mode could have a port child-node to point to say the
> DisplayPort pins / mux connection ?  I'm not entirely sure how the graph
> stuff will work here, but I guess that from a DT pov it will be desirable
> to be able to describe where the datalines for the alt-mode come from,
> maybe in combination with what the "mux/select" value is for the mux ???

I don't know what is the correct approach here. In a way it would feel
logical to assign the mux connection to the alt mode child-node, but
on the other hand the alt mode child-nodes are not real "device nodes"
that represent some physical device like the connector node does. The
alt mode child-node is more like a representation of a device
function. It is unlikely that a connector has multiple muxes to deal
with even when it supports multiple alt modes.

I think if I had to vote on this, I would prefer to link the muxing
with the connector node, and not the alt mode child-node.


thanks,

-- 
heikki