The struct 'cros_typec_port' incorporates three type-C parameters,
'ori_sw', 'retimer', and 'mux'.
These parameters in the struct 'typec_port' are being already
configured when 'typec_register_port' is being called.
The bigger picture - 'typec_register_port' can return an error and
the type-C parameters could go unconfigured. However, the callback
will return EPROBE_DEFER and the driver will again be getting probed
trying to configure them again.
However, currently, they are again tried to get fetched for the
'cros_typec_port' struct, which sometimes could result in an error
and these parameters could go unconfigured (no provision of deferring
the probe in this case, so we are left with unconfigured parameters).
Hence, assign the parameters to the corresponding ones in the struct
'typec_port' after they are fetched in 'typec_register_port'.
Signed-off-by: Rajat Khandelwal <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++---------------
1 file changed, 22 insertions(+), 36 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 001b0de95a46..0265c0d38bd6 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -24,6 +24,8 @@
#include <linux/usb/typec_tbt.h>
#include <linux/usb/role.h>
+#include "../../usb/typec/class.h"
+
#define DRV_NAME "cros-ec-typec"
#define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \
@@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
return 0;
}
-static int cros_typec_get_switch_handles(struct cros_typec_port *port,
- struct fwnode_handle *fwnode,
- struct device *dev)
+static int cros_typec_get_role_switch_handle(struct cros_typec_port *port,
+ struct fwnode_handle *fwnode,
+ struct device *dev)
{
- port->mux = fwnode_typec_mux_get(fwnode, NULL);
- if (IS_ERR(port->mux)) {
- dev_dbg(dev, "Mux handle not found.\n");
- goto mux_err;
- }
-
- port->retimer = fwnode_typec_retimer_get(fwnode);
- if (IS_ERR(port->retimer)) {
- dev_dbg(dev, "Retimer handle not found.\n");
- goto retimer_sw_err;
- }
-
- port->ori_sw = fwnode_typec_switch_get(fwnode);
- if (IS_ERR(port->ori_sw)) {
- dev_dbg(dev, "Orientation switch handle not found.\n");
- goto ori_sw_err;
- }
-
port->role_sw = fwnode_usb_role_switch_get(fwnode);
if (IS_ERR(port->role_sw)) {
dev_dbg(dev, "USB role switch handle not found.\n");
- goto role_sw_err;
+ return -ENODEV;
}
return 0;
-
-role_sw_err:
- typec_switch_put(port->ori_sw);
- port->ori_sw = NULL;
-ori_sw_err:
- typec_retimer_put(port->retimer);
- port->retimer = NULL;
-retimer_sw_err:
- typec_mux_put(port->mux);
- port->mux = NULL;
-mux_err:
- return -ENODEV;
}
static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
@@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
return 0;
}
+static void cros_typec_copy_port_params(struct cros_typec_port *cros_port)
+{
+ struct typec_port *port = cros_port->port;
+
+ if (IS_ERR_OR_NULL(port))
+ return;
+
+ cros_port->mux = port->mux;
+ cros_port->retimer = port->retimer;
+ cros_port->ori_sw = port->sw;
+}
+
static int cros_typec_init_ports(struct cros_typec_data *typec)
{
struct device *dev = typec->dev;
@@ -422,9 +406,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
goto unregister_ports;
}
- ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
+ cros_typec_copy_port_params(cros_port);
+
+ ret = cros_typec_get_role_switch_handle(cros_port, fwnode, dev);
if (ret)
- dev_dbg(dev, "No switch control for port %d\n",
+ dev_dbg(dev, "No role-switch control for port %d\n",
port_num);
ret = cros_typec_register_port_altmodes(typec, port_num);
--
2.34.1
Hi Rajat,
Please use the right header in the commit message. There are plenty of
examples in the git log.
On Sun, Feb 5, 2023 at 11:34 PM Rajat Khandelwal
<[email protected]> wrote:
>
> The struct 'cros_typec_port' incorporates three type-C parameters,
> 'ori_sw', 'retimer', and 'mux'.
> These parameters in the struct 'typec_port' are being already
> configured when 'typec_register_port' is being called.
>
> The bigger picture - 'typec_register_port' can return an error and
> the type-C parameters could go unconfigured. However, the callback
> will return EPROBE_DEFER and the driver will again be getting probed
> trying to configure them again.
> However, currently, they are again tried to get fetched for the
> 'cros_typec_port' struct, which sometimes could result in an error
> and these parameters could go unconfigured (no provision of deferring
> the probe in this case, so we are left with unconfigured parameters).
This is by design. If the switch handles cannot be obtained for any reason
other that -EPROBE_DEFER, we will not re-attempt to acquire them, and
should continue driver probe and carry on with limited functionality.
If there is a sporadic error other than -EPROBE_DEFER, that points to a
deeper issue in firmware which should be investigated.
>
> Hence, assign the parameters to the corresponding ones in the struct
> 'typec_port' after they are fetched in 'typec_register_port'.
>
> Signed-off-by: Rajat Khandelwal <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++---------------
> 1 file changed, 22 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 001b0de95a46..0265c0d38bd6 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -24,6 +24,8 @@
> #include <linux/usb/typec_tbt.h>
> #include <linux/usb/role.h>
>
> +#include "../../usb/typec/class.h"
That is a local header. We cannot use it outside of drivers/usb/typec/
> +
> #define DRV_NAME "cros-ec-typec"
>
> #define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \
> @@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
> return 0;
> }
>
> -static int cros_typec_get_switch_handles(struct cros_typec_port *port,
> - struct fwnode_handle *fwnode,
> - struct device *dev)
> +static int cros_typec_get_role_switch_handle(struct cros_typec_port *port,
> + struct fwnode_handle *fwnode,
> + struct device *dev)
> {
> - port->mux = fwnode_typec_mux_get(fwnode, NULL);
> - if (IS_ERR(port->mux)) {
> - dev_dbg(dev, "Mux handle not found.\n");
> - goto mux_err;
> - }
> -
> - port->retimer = fwnode_typec_retimer_get(fwnode);
> - if (IS_ERR(port->retimer)) {
> - dev_dbg(dev, "Retimer handle not found.\n");
> - goto retimer_sw_err;
> - }
> -
> - port->ori_sw = fwnode_typec_switch_get(fwnode);
> - if (IS_ERR(port->ori_sw)) {
> - dev_dbg(dev, "Orientation switch handle not found.\n");
> - goto ori_sw_err;
> - }
> -
> port->role_sw = fwnode_usb_role_switch_get(fwnode);
> if (IS_ERR(port->role_sw)) {
> dev_dbg(dev, "USB role switch handle not found.\n");
> - goto role_sw_err;
> + return -ENODEV;
> }
>
> return 0;
> -
> -role_sw_err:
> - typec_switch_put(port->ori_sw);
> - port->ori_sw = NULL;
> -ori_sw_err:
> - typec_retimer_put(port->retimer);
> - port->retimer = NULL;
> -retimer_sw_err:
> - typec_mux_put(port->mux);
> - port->mux = NULL;
> -mux_err:
> - return -ENODEV;
> }
>
> static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
> @@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> return 0;
> }
>
> +static void cros_typec_copy_port_params(struct cros_typec_port *cros_port)
> +{
> + struct typec_port *port = cros_port->port;
> +
> + if (IS_ERR_OR_NULL(port))
> + return;
> +
> + cros_port->mux = port->mux;
> + cros_port->retimer = port->retimer;
> + cros_port->ori_sw = port->sw;
> +}
Sorry, but this is not the right approach. These handles are
ref-counted. We intentionally
take extra references to these using the provided API. Please don't
use raw references like this.
Also, if the fwnode_*_get() functions are failing, why are we to
assume that they worked for the
Type-C class code (that code uses the same functions) [1] ?
This approach is not right.
BR,
-Prashant
[1] https://elixir.bootlin.com/linux/v6.2-rc7/source/drivers/usb/typec/class.c#L2317
Hey Rajat,
On Tue, Feb 7, 2023 at 12:53 AM Rajat Khandelwal
<[email protected]> wrote:
>
> Hi Prashant,
> Please find the comments inline.
>
> On 2/7/2023 1:25 AM, Prashant Malani wrote:
>
> Hi Rajat,
>
> Please use the right header in the commit message. There are plenty of
> examples in the git log.
>
> Got it Prashant.
>
> On Sun, Feb 5, 2023 at 11:34 PM Rajat Khandelwal
> <[email protected]> wrote:
>
> The struct 'cros_typec_port' incorporates three type-C parameters,
> 'ori_sw', 'retimer', and 'mux'.
> These parameters in the struct 'typec_port' are being already
> configured when 'typec_register_port' is being called.
>
> The bigger picture - 'typec_register_port' can return an error and
> the type-C parameters could go unconfigured. However, the callback
> will return EPROBE_DEFER and the driver will again be getting probed
> trying to configure them again.
> However, currently, they are again tried to get fetched for the
> 'cros_typec_port' struct, which sometimes could result in an error
> and these parameters could go unconfigured (no provision of deferring
> the probe in this case, so we are left with unconfigured parameters).
>
> This is by design. If the switch handles cannot be obtained for any reason
> other that -EPROBE_DEFER, we will not re-attempt to acquire them, and
> should continue driver probe and carry on with limited functionality.
>
> Why should we even try to acquire them in cros-ec-typec, when we can use them
> natively populated?
Those handles are internal to the Type-C class code. Anything outside shouldn't
be accessing them (that applies to anything within struct typec_port); they are
intentionally kept opaque to protect implementation internals.
>
> If there is a sporadic error other than -EPROBE_DEFER, that points to a
> deeper issue in firmware which should be investigated.
>
> Hence, assign the parameters to the corresponding ones in the struct
> 'typec_port' after they are fetched in 'typec_register_port'.
>
> Signed-off-by: Rajat Khandelwal <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++---------------
> 1 file changed, 22 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 001b0de95a46..0265c0d38bd6 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -24,6 +24,8 @@
> #include <linux/usb/typec_tbt.h>
> #include <linux/usb/role.h>
>
> +#include "../../usb/typec/class.h"
>
> That is a local header. We cannot use it outside of drivers/usb/typec/
>
> Ok, sure. To avoid restructuring the struct definitions, I simply put it for now.
> This can be rectified after we arrive at a culmination.
>
> +
> #define DRV_NAME "cros-ec-typec"
>
> #define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \
> @@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
> return 0;
> }
>
> -static int cros_typec_get_switch_handles(struct cros_typec_port *port,
> - struct fwnode_handle *fwnode,
> - struct device *dev)
> +static int cros_typec_get_role_switch_handle(struct cros_typec_port *port,
> + struct fwnode_handle *fwnode,
> + struct device *dev)
> {
> - port->mux = fwnode_typec_mux_get(fwnode, NULL);
> - if (IS_ERR(port->mux)) {
> - dev_dbg(dev, "Mux handle not found.\n");
> - goto mux_err;
> - }
> -
> - port->retimer = fwnode_typec_retimer_get(fwnode);
> - if (IS_ERR(port->retimer)) {
> - dev_dbg(dev, "Retimer handle not found.\n");
> - goto retimer_sw_err;
> - }
> -
> - port->ori_sw = fwnode_typec_switch_get(fwnode);
> - if (IS_ERR(port->ori_sw)) {
> - dev_dbg(dev, "Orientation switch handle not found.\n");
> - goto ori_sw_err;
> - }
> -
> port->role_sw = fwnode_usb_role_switch_get(fwnode);
> if (IS_ERR(port->role_sw)) {
> dev_dbg(dev, "USB role switch handle not found.\n");
> - goto role_sw_err;
> + return -ENODEV;
> }
>
> return 0;
> -
> -role_sw_err:
> - typec_switch_put(port->ori_sw);
> - port->ori_sw = NULL;
> -ori_sw_err:
> - typec_retimer_put(port->retimer);
> - port->retimer = NULL;
> -retimer_sw_err:
> - typec_mux_put(port->mux);
> - port->mux = NULL;
> -mux_err:
> - return -ENODEV;
> }
>
> static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
> @@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> return 0;
> }
>
> +static void cros_typec_copy_port_params(struct cros_typec_port *cros_port)
> +{
> + struct typec_port *port = cros_port->port;
> +
> + if (IS_ERR_OR_NULL(port))
> + return;
> +
> + cros_port->mux = port->mux;
> + cros_port->retimer = port->retimer;
> + cros_port->ori_sw = port->sw;
> +}
>
> Sorry, but this is not the right approach. These handles are
> ref-counted. We intentionally
> take extra references to these using the provided API. Please don't
> use raw references like this.
>
> Aah, didn't see they were getting ref-counted. Even so, I don't understand
> the part that why do we need to take extra references? Can't we not use the
> same reference to the nodes which were populated in the type-C driver?
No; they have to be refcounted (that's the way all firmware node handles
are used). Any "client" or "caller" that wants to use the fwnode handles
has to use the correct APIs. Refcounting is necessary for the fwnode/property
framework to ensure that there are no stale references for firmware device
nodes (needed to prevent leaks during device creation/removal). If the fwnode
framework doesn't know that you (driver) have a reference, it is free
to invalidate
that reference (and remove the underlying device) without you(driver)
knowing anything about it (I'm sure I'm oversimplifying a lot over
here, but that
is the gist AFAIK).
>
> On another note, maybe remove these 3 handles from 'cros_typec_port' altogether,
> and use the handles from 'typec_port' inside the 'cros_typec_struct'?
>Other drivers
> in the type-C domain use the same concept AFAIK. They just have the 'typec_port' from
> which they use all the handles natively populated.
What drivers are these? No port driver should be using these handles without
refcounting them with the right API calls.
>
> Further, the same 'fwnode' will be matched in 'typec_mux_match'/'typec_switch_match',
> irrespective of it being called from 'typec_register_port' or 'cros_typec_get_switch_handles'.
> Thus, mux->mux_dev->dev and sw->sw_dev->dev will represent the same 'dev' struct.
> Is there actually a point of referencing them again in cros-ec-typec? Even when we decrement the
> ref-counter, it will be decremented of the same 'dev'!
> Please correct me if I'm wrong. :)
>
> Also, if the fwnode_*_get() functions are failing, why are we to
> assume that they worked for the
> Type-C class code (that code uses the same functions) [1] ?
>
> They surely worked for the type-C class code, since if either of the 'mux' and 'switch'
> handles are ERR, it will return an error. cros-ec-typec will then try to probe again.
> Since the handling reaches after the 'typec_register_port', I think we can be sure that they
> had to be succeeded for the type-C class driver.
If those calls are succeeding when they are called within typec_register_port(),
there is no reason why they should fail when called from cros-ec-typec (we
are just taking references to existing handles after all).
I would suggest looking into why the calls to get the firmware handles
are failing
from the port driver.
On a unrelated note: Are you responding in plain text mode? I suspect
you may not be,
since the indentation of responses is getting messed up.
BR,
-Prashant
Hi Prashant,
I think I got it now. Ultimately, the 'device' mux_dev has to be ref-incremented
for another driver (in our case - cros-ec-typec).
Was in confusion if that'd be necessary since one of the type-C drivers - ANX7411
registers the 'typec_port' inside its struct by 'typec_register_port', and uses the
'typec_mux' from within the struct rather than ref-incrementing it again.
To be more specific,
return typec_set_mode(ctx->typec.port, mode);
//Here the port corresponds to 'typec_port' and ultimately it will use 'typec_mux' from
//it and call 'typec_mux_set'.
Maybe something else could be going on here, or I missed something? Not sure.
Ok, raw assigning the pointers is not the solution, I agree. How about something like this?
static void cros_typec_params_get(struct cros_typec_port *cros_port)
{
struct typec_port *port = cros_port->port;
struct typec_switch_dev *sw_dev;
struct typec_mux_dev *mux_dev;
if (IS_ERR_OR_NULL(port))
return;
cros_port->mux = port->mux;
mux_dev = cros_port->mux->mux_dev;
get_device(&mux_dev->dev);
cros_port->ori_sw = port->sw;
sw_dev = cros_port->ori_sw->sw_dev;
get_device(&sw_dev->dev);
}
This will increment the ref-count of the device nodes (pardon me, if any compilation error :)).
All I am pushing for is to not go through fwnode_match again and find the same mux_dev/sw_dev.
If ref-count has to be incremented, we can do it like above.
Should this not be a more robust way we assign the device nodes to our cros-ec-typec port node,
which we have already populated?
Thanks
Rajat
On 2/8/2023 12:06 AM, Prashant Malani wrote:
> Hey Rajat,
>
> On Tue, Feb 7, 2023 at 12:53 AM Rajat Khandelwal
> <[email protected]> wrote:
>> Hi Prashant,
>> Please find the comments inline.
>>
>> On 2/7/2023 1:25 AM, Prashant Malani wrote:
>>
>> Hi Rajat,
>>
>> Please use the right header in the commit message. There are plenty of
>> examples in the git log.
>>
>> Got it Prashant.
>>
>> On Sun, Feb 5, 2023 at 11:34 PM Rajat Khandelwal
>> <[email protected]> wrote:
>>
>> The struct 'cros_typec_port' incorporates three type-C parameters,
>> 'ori_sw', 'retimer', and 'mux'.
>> These parameters in the struct 'typec_port' are being already
>> configured when 'typec_register_port' is being called.
>>
>> The bigger picture - 'typec_register_port' can return an error and
>> the type-C parameters could go unconfigured. However, the callback
>> will return EPROBE_DEFER and the driver will again be getting probed
>> trying to configure them again.
>> However, currently, they are again tried to get fetched for the
>> 'cros_typec_port' struct, which sometimes could result in an error
>> and these parameters could go unconfigured (no provision of deferring
>> the probe in this case, so we are left with unconfigured parameters).
>>
>> This is by design. If the switch handles cannot be obtained for any reason
>> other that -EPROBE_DEFER, we will not re-attempt to acquire them, and
>> should continue driver probe and carry on with limited functionality.
>>
>> Why should we even try to acquire them in cros-ec-typec, when we can use them
>> natively populated?
> Those handles are internal to the Type-C class code. Anything outside shouldn't
> be accessing them (that applies to anything within struct typec_port); they are
> intentionally kept opaque to protect implementation internals.
>
>> If there is a sporadic error other than -EPROBE_DEFER, that points to a
>> deeper issue in firmware which should be investigated.
>>
>> Hence, assign the parameters to the corresponding ones in the struct
>> 'typec_port' after they are fetched in 'typec_register_port'.
>>
>> Signed-off-by: Rajat Khandelwal <[email protected]>
>> ---
>> drivers/platform/chrome/cros_ec_typec.c | 58 ++++++++++---------------
>> 1 file changed, 22 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>> index 001b0de95a46..0265c0d38bd6 100644
>> --- a/drivers/platform/chrome/cros_ec_typec.c
>> +++ b/drivers/platform/chrome/cros_ec_typec.c
>> @@ -24,6 +24,8 @@
>> #include <linux/usb/typec_tbt.h>
>> #include <linux/usb/role.h>
>>
>> +#include "../../usb/typec/class.h"
>>
>> That is a local header. We cannot use it outside of drivers/usb/typec/
>>
>> Ok, sure. To avoid restructuring the struct definitions, I simply put it for now.
>> This can be rectified after we arrive at a culmination.
>>
>> +
>> #define DRV_NAME "cros-ec-typec"
>>
>> #define DP_PORT_VDO (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \
>> @@ -141,47 +143,17 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
>> return 0;
>> }
>>
>> -static int cros_typec_get_switch_handles(struct cros_typec_port *port,
>> - struct fwnode_handle *fwnode,
>> - struct device *dev)
>> +static int cros_typec_get_role_switch_handle(struct cros_typec_port *port,
>> + struct fwnode_handle *fwnode,
>> + struct device *dev)
>> {
>> - port->mux = fwnode_typec_mux_get(fwnode, NULL);
>> - if (IS_ERR(port->mux)) {
>> - dev_dbg(dev, "Mux handle not found.\n");
>> - goto mux_err;
>> - }
>> -
>> - port->retimer = fwnode_typec_retimer_get(fwnode);
>> - if (IS_ERR(port->retimer)) {
>> - dev_dbg(dev, "Retimer handle not found.\n");
>> - goto retimer_sw_err;
>> - }
>> -
>> - port->ori_sw = fwnode_typec_switch_get(fwnode);
>> - if (IS_ERR(port->ori_sw)) {
>> - dev_dbg(dev, "Orientation switch handle not found.\n");
>> - goto ori_sw_err;
>> - }
>> -
>> port->role_sw = fwnode_usb_role_switch_get(fwnode);
>> if (IS_ERR(port->role_sw)) {
>> dev_dbg(dev, "USB role switch handle not found.\n");
>> - goto role_sw_err;
>> + return -ENODEV;
>> }
>>
>> return 0;
>> -
>> -role_sw_err:
>> - typec_switch_put(port->ori_sw);
>> - port->ori_sw = NULL;
>> -ori_sw_err:
>> - typec_retimer_put(port->retimer);
>> - port->retimer = NULL;
>> -retimer_sw_err:
>> - typec_mux_put(port->mux);
>> - port->mux = NULL;
>> -mux_err:
>> - return -ENODEV;
>> }
>>
>> static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
>> @@ -363,6 +335,18 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
>> return 0;
>> }
>>
>> +static void cros_typec_copy_port_params(struct cros_typec_port *cros_port)
>> +{
>> + struct typec_port *port = cros_port->port;
>> +
>> + if (IS_ERR_OR_NULL(port))
>> + return;
>> +
>> + cros_port->mux = port->mux;
>> + cros_port->retimer = port->retimer;
>> + cros_port->ori_sw = port->sw;
>> +}
>>
>> Sorry, but this is not the right approach. These handles are
>> ref-counted. We intentionally
>> take extra references to these using the provided API. Please don't
>> use raw references like this.
>>
>> Aah, didn't see they were getting ref-counted. Even so, I don't understand
>> the part that why do we need to take extra references? Can't we not use the
>> same reference to the nodes which were populated in the type-C driver?
> No; they have to be refcounted (that's the way all firmware node handles
> are used). Any "client" or "caller" that wants to use the fwnode handles
> has to use the correct APIs. Refcounting is necessary for the fwnode/property
> framework to ensure that there are no stale references for firmware device
> nodes (needed to prevent leaks during device creation/removal). If the fwnode
> framework doesn't know that you (driver) have a reference, it is free
> to invalidate
> that reference (and remove the underlying device) without you(driver)
> knowing anything about it (I'm sure I'm oversimplifying a lot over
> here, but that
> is the gist AFAIK).
>
>> On another note, maybe remove these 3 handles from 'cros_typec_port' altogether,
>> and use the handles from 'typec_port' inside the 'cros_typec_struct'?
>> Other drivers
>> in the type-C domain use the same concept AFAIK. They just have the 'typec_port' from
>> which they use all the handles natively populated.
> What drivers are these? No port driver should be using these handles without
> refcounting them with the right API calls.
>
>> Further, the same 'fwnode' will be matched in 'typec_mux_match'/'typec_switch_match',
>> irrespective of it being called from 'typec_register_port' or 'cros_typec_get_switch_handles'.
>> Thus, mux->mux_dev->dev and sw->sw_dev->dev will represent the same 'dev' struct.
>> Is there actually a point of referencing them again in cros-ec-typec? Even when we decrement the
>> ref-counter, it will be decremented of the same 'dev'!
>> Please correct me if I'm wrong. :)
>>
>> Also, if the fwnode_*_get() functions are failing, why are we to
>> assume that they worked for the
>> Type-C class code (that code uses the same functions) [1] ?
>>
>> They surely worked for the type-C class code, since if either of the 'mux' and 'switch'
>> handles are ERR, it will return an error. cros-ec-typec will then try to probe again.
>> Since the handling reaches after the 'typec_register_port', I think we can be sure that they
>> had to be succeeded for the type-C class driver.
> If those calls are succeeding when they are called within typec_register_port(),
> there is no reason why they should fail when called from cros-ec-typec (we
> are just taking references to existing handles after all).
> I would suggest looking into why the calls to get the firmware handles
> are failing
> from the port driver.
>
>
> On a unrelated note: Are you responding in plain text mode? I suspect
> you may not be,
> since the indentation of responses is getting messed up.
>
> BR,
>
> -Prashant