On Wed, Feb 02, 2022 at 04:19:47PM -0600, Samuel Holland wrote:
> WUSB3801 features a configurable port type, accessory detection, and
> plug orientation detection. It provides a hardware "ID" pin output for
> compatibility with USB 2.0 OTG PHYs. Add a typec class driver for it.
>
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - License the driver as GPL 2 only; probably best anyway as I used a
> lot of other drivers/usb/typec code as inspiration
> - Don't try to be clever; use `default` instead of `unreachable`
> - Free the IRQ before unregistering the partner/port
>
> drivers/usb/typec/Kconfig | 10 +
> drivers/usb/typec/Makefile | 1 +
> drivers/usb/typec/wusb3801.c | 445 +++++++++++++++++++++++++++++++++++
> 3 files changed, 456 insertions(+)
> create mode 100644 drivers/usb/typec/wusb3801.c
This looked mostly OK to me. One nitpick below.
> +static int wusb3801_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct fwnode_handle *connector;
> + unsigned int device_id, test01;
> + struct wusb3801 *wusb3801;
> + const char *cap_str;
> + int ret;
> +
> + wusb3801 = devm_kzalloc(dev, sizeof(*wusb3801), GFP_KERNEL);
> + if (!wusb3801)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, wusb3801);
> +
> + wusb3801->dev = dev;
> +
> + wusb3801->regmap = devm_regmap_init_i2c(client, &config);
> + if (IS_ERR(wusb3801->regmap))
> + return PTR_ERR(wusb3801->regmap);
> +
> + regmap_read(wusb3801->regmap, WUSB3801_REG_DEVICE_ID, &device_id);
> + regmap_read(wusb3801->regmap, WUSB3801_REG_TEST01, &test01);
> + dev_info(dev, "Vendor ID: %ld, Version ID: %ld, Vendor SubID: 0x%02lx\n",
> + device_id & WUSB3801_DEVICE_ID_VENDOR_ID,
> + (device_id & WUSB3801_DEVICE_ID_VERSION_ID) >> 3,
> + test01 & WUSB3801_TEST01_VENDOR_SUB_ID);
That is just noise.
> + wusb3801->vbus_supply = devm_regulator_get(dev, "vbus");
> + if (IS_ERR(wusb3801->vbus_supply))
> + return PTR_ERR(wusb3801->vbus_supply);
> +
> + connector = device_get_named_child_node(dev, "connector");
> + if (!connector)
> + return -ENODEV;
> +
> + ret = typec_get_fw_cap(&wusb3801->cap, connector);
One note here: Don't use fw_devlink_purge_absent_suppliers() here
either unless you really see some problem yourself!
That function is broken like I said. What ever it's fixing, it's doing
it wrong. That function seems to be just a broken hack that most
likely covered some individual case that was reported at the time.
Instead of hacks like that, we need to figure out a solution for the
core problem, what ever that might be.
> + if (ret)
> + goto err_put_connector;
> + wusb3801->port_type = wusb3801->cap.type;
> +
> + ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str);
> + if (ret)
> + goto err_put_connector;
> +
> + ret = typec_find_pwr_opmode(cap_str);
> + if (ret < 0 || ret == TYPEC_PWR_MODE_PD)
> + goto err_put_connector;
> + wusb3801->pwr_opmode = ret;
> +
> + /* Initialize the hardware with the devicetree settings. */
> + ret = wusb3801_hw_init(wusb3801);
> + if (ret)
> + return ret;
> +
> + wusb3801->cap.revision = USB_TYPEC_REV_1_2;
> + wusb3801->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO;
> + wusb3801->cap.accessory[1] = TYPEC_ACCESSORY_DEBUG;
> + wusb3801->cap.orientation_aware = true;
> + wusb3801->cap.driver_data = wusb3801;
> + wusb3801->cap.ops = &wusb3801_typec_ops;
> +
> + wusb3801->port = typec_register_port(dev, &wusb3801->cap);
> + if (IS_ERR(wusb3801->port)) {
> + ret = PTR_ERR(wusb3801->port);
> + goto err_put_connector;
> + }
> +
> + /* Initialize the port attributes from the hardware state. */
> + wusb3801_hw_update(wusb3801);
> +
> + ret = request_threaded_irq(client->irq, NULL, wusb3801_irq,
> + IRQF_ONESHOT, dev_name(dev), wusb3801);
> + if (ret)
> + goto err_unregister_port;
> +
> + fwnode_handle_put(connector);
> +
> + return 0;
> +
> +err_unregister_port:
> + typec_unregister_port(wusb3801->port);
> +err_put_connector:
> + fwnode_handle_put(connector);
> +
> + return ret;
> +}
thanks,
--
heikki
