Re: [PATCH 04/12] usb: common: Small class for USB role switches

From: Hans de Goede
Date: Mon Feb 19 2018 - 04:32:26 EST


Hi,

On 16-02-18 15:22, Heikki Krogerus wrote:
On Fri, Feb 16, 2018 at 04:07:59PM +0200, Andy Shevchenko wrote:
On Fri, Feb 16, 2018 at 12:47 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:

USB role switch is a device that can be used to choose the
data role for USB connector. With dual-role capable USB
controllers, the controller itself will be the switch, but
on some platforms the USB host and device controllers are
separate IPs and there is a mux between them and the
connector. On those platforms the mux driver will need to
register the switch.

With USB Type-C connectors, the host-to-device relationship
is negotiated over the Configuration Channel (CC). That
means the USB Type-C drivers need to be in control of the
role switch. The class provides a simple API for the USB
Type-C drivers for the control.

For other types of USB connectors (mainly microAB) the class
provides user space control via sysfs attribute file that
can be used to request role swapping from the switch.

+static int __switch_match(struct device *dev, const void *name)

bool?

No, this is callback for class_find_device(). It takes int so int it
is.

+{
+ return !strcmp((const char *)name, dev_name(dev));
+}


+static ssize_t role_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct usb_role_switch *sw = to_role_switch(dev);
+ int ret;
+
+ ret = sysfs_match_string(usb_roles, buf);
+ if (ret < 0) {
+ bool res;
+
+ /* Extra check if the user wants to disable the switch */
+ ret = kstrtobool(buf, &res);
+ if (ret || res)
+ return -EINVAL;
+ }
+

+ ret = usb_role_switch_set_role(sw, ret);
+ if (!ret)
+ ret = size;
+
+ return ret;

Perhaps

ret = ...
if (ret)
return ret;

return size;

Sure. Hans, can you clean-up this as well?

Yes I can, not sure when exactly I will get around to this, but I will
try to get out a v2 addressing all comments made this week.

And thank you for your reviews.

Andy, thank you for all the reviews too.

Regards,

Hans


+}

+struct usb_role_switch *
+usb_role_switch_register(struct device *parent,
+ const struct usb_role_switch_desc *desc)
+{
+ struct usb_role_switch *sw;
+ int ret;
+
+ if (!desc || !desc->set)
+ return ERR_PTR(-EINVAL);
+
+ sw = kzalloc(sizeof(*sw), GFP_KERNEL);
+ if (!sw)
+ return ERR_PTR(-ENOMEM);

+ ret = device_register(&sw->dev);
+ if (ret) {
+ put_device(&sw->dev);

Memory leak?

No. Check usb_role_switch_release().


Thanks,