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

Thanks Adam!

-- 
heikki