Re: [PATCH 2/3] platform/x86: intel_cht_int33fe: use inline reference properties

[Dmitry Torokhov]

On Fri, Sep 06, 2019 at 02:22:43PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Sep 05, 2019 at 09:38:08PM -0700, Dmitry Torokhov wrote:
> > Now that static device properties allow defining reference properties
> > together with all other types of properties, instead of managing them
> > separately, let's adjust the driver.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > ---
> > 
> > Heikki, I do not have this hardware, so if you could try this out
> > it would be really great.
> > 
> >  drivers/platform/x86/intel_cht_int33fe.c | 46 ++++++++++++------------
> >  1 file changed, 22 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> > index 4fbdff48a4b5..91f3c8840fd8 100644
> > --- a/drivers/platform/x86/intel_cht_int33fe.c
> > +++ b/drivers/platform/x86/intel_cht_int33fe.c
> > @@ -50,28 +50,8 @@ struct cht_int33fe_data {
> >  
> >  static const struct software_node nodes[];
> 
> I think you can remove that.

Not really, as there is still circular dependency between nodes and
properties. I moved it down closer to properties though.

> 
> > -static const struct software_node_ref_args pi3usb30532_ref = {
> > -	&nodes[INT33FE_NODE_PI3USB30532]
> > -};
> > -
> > -static const struct software_node_ref_args dp_ref = {
> > -	&nodes[INT33FE_NODE_DISPLAYPORT]
> > -};
> > -
> >  static struct software_node_ref_args mux_ref;
> 
> I'm pretty sure you should now drop that one.

I ended up reworking things a bit and still using a form of this to
reach in and change data, as properties ended up being constants.

> 
> > -static const struct software_node_reference usb_connector_refs[] = {
> > -	{ "orientation-switch", 1, &pi3usb30532_ref},
> > -	{ "mode-switch", 1, &pi3usb30532_ref},
> > -	{ "displayport", 1, &dp_ref},
> > -	{ }
> > -};
> > -
> > -static const struct software_node_reference fusb302_refs[] = {
> > -	{ "usb-role-switch", 1, &mux_ref},
> > -	{ }
> > -};
> > -
> >  /*
> >   * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates
> >   * the max17047 both through the INT33FE ACPI device (it is right there
> > @@ -107,7 +87,13 @@ static const struct property_entry max17047_props[] = {
> >  	{ }
> >  };
> >  
> > -static const struct property_entry fusb302_props[] = {
> > +/* Not const because we need to update "usb-role-switch" property. */
> > +static struct property_entry fusb302_props[] = {
> > +	/*
> > +	 * usb-role-switch property must be first as we rely on fixed
> > +	 * position to adjust it once we know the real node.
> > +	 */
> > +	PROPERTY_ENTRY_REF("usb-role-switch", NULL),
> >  	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
> >  	{ }
> >  };
> > @@ -131,16 +117,22 @@ static const struct property_entry usb_connector_props[] = {
> >  	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
> >  	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
> >  	PROPERTY_ENTRY_U32("op-sink-microwatt", 2500000),
> > +	PROPERTY_ENTRY_REF("orientation-switch",
> > +			   &nodes[INT33FE_NODE_PI3USB30532]),
> > +	PROPERTY_ENTRY_REF("mode-switch",
> > +			   &nodes[INT33FE_NODE_PI3USB30532]),
> > +	PROPERTY_ENTRY_REF("displayport",
> > +			   &nodes[INT33FE_NODE_DISPLAYPORT]),
> >  	{ }
> >  };
> >  
> >  static const struct software_node nodes[] = {
> > -	{ "fusb302", NULL, fusb302_props, fusb302_refs },
> > +	{ "fusb302", NULL, fusb302_props },
> >  	{ "max17047", NULL, max17047_props },
> >  	{ "pi3usb30532" },
> >  	{ "displayport" },
> >  	{ "usb-role-switch" },
> > -	{ "connector", &nodes[0], usb_connector_props, usb_connector_refs },
> > +	{ "connector", &nodes[0], usb_connector_props },
> >  	{ }
> >  };
> >  
> > @@ -174,7 +166,13 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data)
> >  
> >  	data->mux = fwnode_handle_get(dev->fwnode);
> >  	put_device(dev);
> > -	mux_ref.node = to_software_node(data->mux);
> > +
> > +	/*
> > +	 * Update "usb-role-switch" property with real node. Note that we
> > +	 * rely on software_node_register_nodes() to use the original
> > +	 * instance of properties instead of copying them.
> > +	 */
> > +	fusb302_props[0].value.ref.node = to_software_node(data->mux);
> 
> There are other changes to this driver and to swnode.c in Rafael's
> tree, so if you send v2 soon, then please rebase on top of his devprop
> branch, or alternatively linux-next.

Yep, did that, will re-post series shortly.

-- 
Dmitry