Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Feb 2022 13:55:38 +0200
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Samuel Holland <samuel@sholland.org>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/4] usb: typec: Support the WUSB3801 port controller
Message-ID: <YgOruoTJSybLweNC@kuha.fi.intel.com>
References: <20220202221948.5690-1-samuel@sholland.org>
 <20220202221948.5690-5-samuel@sholland.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220202221948.5690-5-samuel@sholland.org>
Precedence: bulk

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