[PATCH] typec: tcpm: Provide of_node pointer as part of psy_cfg

[PATCH] typec: tcpm: Provide of_node pointer as part of psy_cfg

[Adam Thomson]

For supply registration, provide of_node pointer of the port device,
via the power_supply_config structure, to allow other psy drivers
to add us as a supplier using the 'power-supplies' DT property.

Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
 drivers/usb/typec/tcpm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 72996cc..e7c0b95 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -4500,6 +4500,7 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
 	char *psy_name;
 
 	psy_cfg.drv_data = port;
+	psy_cfg.of_node = port->dev->of_node;
 	psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
 	if (!psy_name)
 		return -ENOMEM;
-- 
1.9.1

Re: [PATCH] typec: tcpm: Provide of_node pointer as part of psy_cfg

[Heikki Krogerus]

Hi Adam,

On Wed, May 16, 2018 at 05:00:46PM +0100, Adam Thomson wrote:
> For supply registration, provide of_node pointer of the port device,
> via the power_supply_config structure, to allow other psy drivers
> to add us as a supplier using the 'power-supplies' DT property.
> 
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
>  drivers/usb/typec/tcpm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 72996cc..e7c0b95 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -4500,6 +4500,7 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
>  	char *psy_name;
>  
>  	psy_cfg.drv_data = port;
> +	psy_cfg.of_node = port->dev->of_node;
>  	psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
>  	if (!psy_name)
>  		return -ENOMEM;

Would it be possible to use fwnode here instead? It would mean that
you add a member for it to the struct power_supply_config, and handle
it separately in power_supply_core.c. You could just convert it to
of_node there for now.

That is just a request, I'm fine with this, but it would prepare this
driver for all types of platforms, so less patching would be needed
once we add ACPI support to the power_supply_core.c.


Thanks,

-- 
heikki

RE: [PATCH] typec: tcpm: Provide of_node pointer as part of psy_cfg

[Adam Thomson]

Hi Heikki,

On 21 May 2018 14:20, Heikki Krogerus wrote:

> On Wed, May 16, 2018 at 05:00:46PM +0100, Adam Thomson wrote:
> > For supply registration, provide of_node pointer of the port device,
> > via the power_supply_config structure, to allow other psy drivers
> > to add us as a supplier using the 'power-supplies' DT property.
> >
> > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > ---
> >  drivers/usb/typec/tcpm.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > index 72996cc..e7c0b95 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -4500,6 +4500,7 @@ static int devm_tcpm_psy_register(struct tcpm_port
> *port)
> >  	char *psy_name;
> >
> >  	psy_cfg.drv_data = port;
> > +	psy_cfg.of_node = port->dev->of_node;
> >  	psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
> >  	if (!psy_name)
> >  		return -ENOMEM;
> 
> Would it be possible to use fwnode here instead? It would mean that
> you add a member for it to the struct power_supply_config, and handle
> it separately in power_supply_core.c. You could just convert it to
> of_node there for now.
> 
> That is just a request, I'm fine with this, but it would prepare this
> driver for all types of platforms, so less patching would be needed
> once we add ACPI support to the power_supply_core.c.

Would the following commit from Hans, already present in power_supply_core.c,
not fit the bill:

[58a36bb06891ee779074db6ef84e98347c634d38]
power: supply: core: Add support for supplied-from device-property

Or was that just meant as a stop gap for something more?

I have no problems adding something further, but I don't have a means to verify
anything ACPI based, beyond a simple build test, so would ideally want someone
to verify that path through the code.

Re: [PATCH] typec: tcpm: Provide of_node pointer as part of psy_cfg

[Heikki Krogerus]

On Mon, May 21, 2018 at 01:58:16PM +0000, Adam Thomson wrote:
> Hi Heikki,
> 
> On 21 May 2018 14:20, Heikki Krogerus wrote:
> 
> > On Wed, May 16, 2018 at 05:00:46PM +0100, Adam Thomson wrote:
> > > For supply registration, provide of_node pointer of the port device,
> > > via the power_supply_config structure, to allow other psy drivers
> > > to add us as a supplier using the 'power-supplies' DT property.
> > >
> > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > > ---
> > >  drivers/usb/typec/tcpm.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > > index 72996cc..e7c0b95 100644
> > > --- a/drivers/usb/typec/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm.c
> > > @@ -4500,6 +4500,7 @@ static int devm_tcpm_psy_register(struct tcpm_port
> > *port)
> > >  	char *psy_name;
> > >
> > >  	psy_cfg.drv_data = port;
> > > +	psy_cfg.of_node = port->dev->of_node;
> > >  	psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
> > >  	if (!psy_name)
> > >  		return -ENOMEM;
> > 
> > Would it be possible to use fwnode here instead? It would mean that
> > you add a member for it to the struct power_supply_config, and handle
> > it separately in power_supply_core.c. You could just convert it to
> > of_node there for now.
> > 
> > That is just a request, I'm fine with this, but it would prepare this
> > driver for all types of platforms, so less patching would be needed
> > once we add ACPI support to the power_supply_core.c.
> 
> Would the following commit from Hans, already present in power_supply_core.c,
> not fit the bill:
> 
> [58a36bb06891ee779074db6ef84e98347c634d38]
> power: supply: core: Add support for supplied-from device-property
> 
> Or was that just meant as a stop gap for something more?

I think the main idea with that patch was that it allows us to take
advantage of build-in device properties, but I think we could actually
improve also it if we had the fwnode handle assigned to the psy. We
would not have to assume the parent has those device properties then.

But ACPI actually defines a specific object called _PCL (power
consumer list) for power source objects that we should one day check
in power_supply_core.c. That's what I meant by ACPI support.

> I have no problems adding something further, but I don't have a means to verify
> anything ACPI based, beyond a simple build test, so would ideally want someone
> to verify that path through the code.

This is not really about ACPI. By using fwnode handle instead of
of_node, we would continue to keep tcpm.c agnostic about the type of
hw description.

So I don't expect any ACPI support to be added to the
power_supply_code.c at this point. You can use to_of_node(cfg->fwnode)
in __power_supply_register() for now:

        psy->of_node = cfg->of_node;
        if (cfg->fwnode)
                psy->of_node = to_of_node(cfg->fwnode);


Thanks,

-- 
heikki

RE: [PATCH] typec: tcpm: Provide of_node pointer as part of psy_cfg

[Adam Thomson]

On 21 May 2018 15:56, Heikki Krogerus wrote:

> On Mon, May 21, 2018 at 01:58:16PM +0000, Adam Thomson wrote:
> > Hi Heikki,
> >
> > On 21 May 2018 14:20, Heikki Krogerus wrote:
> >
> > > On Wed, May 16, 2018 at 05:00:46PM +0100, Adam Thomson wrote:
> > > > For supply registration, provide of_node pointer of the port device,
> > > > via the power_supply_config structure, to allow other psy drivers
> > > > to add us as a supplier using the 'power-supplies' DT property.
> > > >
> > > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > > > ---
> > > >  drivers/usb/typec/tcpm.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > > > index 72996cc..e7c0b95 100644
> > > > --- a/drivers/usb/typec/tcpm.c
> > > > +++ b/drivers/usb/typec/tcpm.c
> > > > @@ -4500,6 +4500,7 @@ static int devm_tcpm_psy_register(struct tcpm_port
> > > *port)
> > > >  	char *psy_name;
> > > >
> > > >  	psy_cfg.drv_data = port;
> > > > +	psy_cfg.of_node = port->dev->of_node;
> > > >  	psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
> > > >  	if (!psy_name)
> > > >  		return -ENOMEM;
> > >
> > > Would it be possible to use fwnode here instead? It would mean that
> > > you add a member for it to the struct power_supply_config, and handle
> > > it separately in power_supply_core.c. You could just convert it to
> > > of_node there for now.
> > >
> > > That is just a request, I'm fine with this, but it would prepare this
> > > driver for all types of platforms, so less patching would be needed
> > > once we add ACPI support to the power_supply_core.c.
> >
> > Would the following commit from Hans, already present in power_supply_core.c,
> > not fit the bill:
> >
> > [58a36bb06891ee779074db6ef84e98347c634d38]
> > power: supply: core: Add support for supplied-from device-property
> >
> > Or was that just meant as a stop gap for something more?
> 
> I think the main idea with that patch was that it allows us to take
> advantage of build-in device properties, but I think we could actually
> improve also it if we had the fwnode handle assigned to the psy. We
> would not have to assume the parent has those device properties then.
> 
> But ACPI actually defines a specific object called _PCL (power
> consumer list) for power source objects that we should one day check
> in power_supply_core.c. That's what I meant by ACPI support.

Ok, fair enough. Makes sense to me. Thanks for clarifying.

> > I have no problems adding something further, but I don't have a means to verify
> > anything ACPI based, beyond a simple build test, so would ideally want someone
> > to verify that path through the code.
> 
> This is not really about ACPI. By using fwnode handle instead of
> of_node, we would continue to keep tcpm.c agnostic about the type of
> hw description.
> 
> So I don't expect any ACPI support to be added to the
> power_supply_code.c at this point. You can use to_of_node(cfg->fwnode)
> in __power_supply_register() for now:
> 
>         psy->of_node = cfg->of_node;
>         if (cfg->fwnode)
>                 psy->of_node = to_of_node(cfg->fwnode);

OK, no problem. Will take a look and add that in.

Re: [PATCH] typec: tcpm: Provide of_node pointer as part of psy_cfg

[Heikki Krogerus]

On Mon, May 21, 2018 at 03:34:53PM +0000, Adam Thomson wrote:
> On 21 May 2018 15:56, Heikki Krogerus wrote:
> 
> > On Mon, May 21, 2018 at 01:58:16PM +0000, Adam Thomson wrote:
> > > Hi Heikki,
> > >
> > > On 21 May 2018 14:20, Heikki Krogerus wrote:
> > >
> > > > On Wed, May 16, 2018 at 05:00:46PM +0100, Adam Thomson wrote:
> > > > > For supply registration, provide of_node pointer of the port device,
> > > > > via the power_supply_config structure, to allow other psy drivers
> > > > > to add us as a supplier using the 'power-supplies' DT property.
> > > > >
> > > > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > > > > ---
> > > > >  drivers/usb/typec/tcpm.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > > > > index 72996cc..e7c0b95 100644
> > > > > --- a/drivers/usb/typec/tcpm.c
> > > > > +++ b/drivers/usb/typec/tcpm.c
> > > > > @@ -4500,6 +4500,7 @@ static int devm_tcpm_psy_register(struct tcpm_port
> > > > *port)
> > > > >  	char *psy_name;
> > > > >
> > > > >  	psy_cfg.drv_data = port;
> > > > > +	psy_cfg.of_node = port->dev->of_node;
> > > > >  	psy_name = devm_kzalloc(port->dev, psy_name_len, GFP_KERNEL);
> > > > >  	if (!psy_name)
> > > > >  		return -ENOMEM;
> > > >
> > > > Would it be possible to use fwnode here instead? It would mean that
> > > > you add a member for it to the struct power_supply_config, and handle
> > > > it separately in power_supply_core.c. You could just convert it to
> > > > of_node there for now.
> > > >
> > > > That is just a request, I'm fine with this, but it would prepare this
> > > > driver for all types of platforms, so less patching would be needed
> > > > once we add ACPI support to the power_supply_core.c.
> > >
> > > Would the following commit from Hans, already present in power_supply_core.c,
> > > not fit the bill:
> > >
> > > [58a36bb06891ee779074db6ef84e98347c634d38]
> > > power: supply: core: Add support for supplied-from device-property
> > >
> > > Or was that just meant as a stop gap for something more?
> > 
> > I think the main idea with that patch was that it allows us to take
> > advantage of build-in device properties, but I think we could actually
> > improve also it if we had the fwnode handle assigned to the psy. We
> > would not have to assume the parent has those device properties then.
> > 
> > But ACPI actually defines a specific object called _PCL (power
> > consumer list) for power source objects that we should one day check
> > in power_supply_core.c. That's what I meant by ACPI support.
> 
> Ok, fair enough. Makes sense to me. Thanks for clarifying.
> 
> > > I have no problems adding something further, but I don't have a means to verify
> > > anything ACPI based, beyond a simple build test, so would ideally want someone
> > > to verify that path through the code.
> > 
> > This is not really about ACPI. By using fwnode handle instead of
> > of_node, we would continue to keep tcpm.c agnostic about the type of
> > hw description.
> > 
> > So I don't expect any ACPI support to be added to the
> > power_supply_code.c at this point. You can use to_of_node(cfg->fwnode)
> > in __power_supply_register() for now:
> > 
> >         psy->of_node = cfg->of_node;
> >         if (cfg->fwnode)
> >                 psy->of_node = to_of_node(cfg->fwnode);
> 
> OK, no problem. Will take a look and add that in.

Thanks Adam!

-- 
heikki