Re: [PATCH 2/4] usb: typec: Add typec_port_register_altmodes_from_fwnode()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 7/27/20 3:05 PM, Heikki Krogerus wrote:
> Hi Hans,
> 
> On Tue, Jul 14, 2020 at 01:36:15PM +0200, Hans de Goede wrote:
>> This can be used by Type-C controller drivers which use a standard
>> usb-connector fwnode, with altmodes sub-node, to describe the available
>> altmodes.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/class.c | 56 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/usb/typec.h |  7 +++++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index c9234748537a..47de2b2e3d54 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c
>> @@ -1607,6 +1607,62 @@ typec_port_register_altmode(struct typec_port *port,
>>   }
>>   EXPORT_SYMBOL_GPL(typec_port_register_altmode);
>>   
>> +void typec_port_register_altmodes_from_fwnode(struct typec_port *port,
>> +	const struct typec_altmode_ops *ops, void *drvdata,
>> +	struct typec_altmode **altmodes, size_t n,
>> +	struct fwnode_handle *fwnode)
>> +{
>> +	struct fwnode_handle *altmodes_node, *child;
>> +	struct typec_altmode_desc desc;
>> +	struct typec_altmode *alt;
>> +	size_t index = 0;
>> +	u32 svid, vdo;
>> +	int ret;
>> +
>> +	altmodes_node = fwnode_get_named_child_node(fwnode, "altmodes");
>> +	if (!altmodes_node)
>> +		return;
> 
> Do we need that? Why not just make the sub-nodes describing the
> alternate modes direct children of the connector node instead of
> grouping them under a special sub-node?

If you envision how this will look in e.g. DTS sources then I think
you will see that this grouping keeps the DTS source code more
readable. Grouping things together like this is somewhat normal in
devicetree files. E.g. PMIC's or other regulator providers typical
have a "regulators" node grouping all their regulators; and also the OF
graph bindings which are used in the USB-connector node start with a
"ports" parent / grouping node.

> If the child node of the connector has device properties "svid" and
> "vdo" then it is an alt mode that the connector supports, and it can't
> be anything else, no?

If you want to get rid of the altmodes parent/grouping node, then the
usual way to do this would be to add a compatible string to the nodes,
rather then check for the existence of some properties.

Regards,

Hans


> 
> 
>> +	child = NULL;
>> +	while ((child = fwnode_get_next_child_node(altmodes_node, child))) {
>> +		ret = fwnode_property_read_u32(child, "svid", &svid);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading svid for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		ret = fwnode_property_read_u32(child, "vdo", &vdo);
>> +		if (ret) {
>> +			dev_err(&port->dev, "Error reading vdo for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		if (index >= n) {
>> +			dev_err(&port->dev, "Error not enough space for altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		desc.svid = svid;
>> +		desc.vdo = vdo;
>> +		desc.mode = index + 1;
>> +		alt = typec_port_register_altmode(port, &desc);
>> +		if (IS_ERR(alt)) {
>> +			dev_err(&port->dev, "Error registering altmode %s\n",
>> +				fwnode_get_name(child));
>> +			continue;
>> +		}
>> +
>> +		alt->ops = ops;
>> +		typec_altmode_set_drvdata(alt, drvdata);
>> +		altmodes[index] = alt;
>> +		index++;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(typec_port_register_altmodes_from_fwnode);
>> +
>>   /**
>>    * typec_register_port - Register a USB Type-C Port
>>    * @parent: Parent device
>> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
>> index 5daa1c49761c..fbe4bccb3a98 100644
>> --- a/include/linux/usb/typec.h
>> +++ b/include/linux/usb/typec.h
>> @@ -17,6 +17,7 @@ struct typec_partner;
>>   struct typec_cable;
>>   struct typec_plug;
>>   struct typec_port;
>> +struct typec_altmode_ops;
>>   
>>   struct fwnode_handle;
>>   struct device;
>> @@ -121,6 +122,12 @@ struct typec_altmode
>>   struct typec_altmode
>>   *typec_port_register_altmode(struct typec_port *port,
>>   			     const struct typec_altmode_desc *desc);
>> +
>> +void typec_port_register_altmodes_from_fwnode(struct typec_port *port,
>> +	const struct typec_altmode_ops *ops, void *drvdata,
>> +	struct typec_altmode **altmodes, size_t n,
>> +	struct fwnode_handle *fwnode);
>> +
>>   void typec_unregister_altmode(struct typec_altmode *altmode);
>>   
>>   struct typec_port *typec_altmode2port(struct typec_altmode *alt);
>> -- 
>> 2.26.2
> 
> thanks,
>