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@xxxxxxxxxxx>
> > > > ---
> > > >  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.