Re: [PATCH v6 07/13] usb: typec: qcom: Add Qualcomm PMIC Type-C driver

[Caleb Connolly <caleb.connolly@linaro.org>]

On 01/05/2023 13:11, Bryan O'Donoghue wrote:
> This commit adds a QCOM PMIC TCPM driver with an initial pm8150b
> block.
> 
> The driver is layered as follows:
> 
> qcom_pmic_typec.c : Responsible for registering with TCPM and arbitrates
>                     access to the Type-C and PDPHY hardware blocks in one
>                     place.  This presents a single TCPM device to device to
>                     the Linux TCPM layer.
> 
> qcom_pmic_typec_pdphy.c: Responsible for interfacing with the PDPHY hardware and
>                          processing power-delivery related calls from TCPM.
>                          This hardware binding can be extended to
>                          facilitate similar hardware in different PMICs.
> 
> qcom_pmic_typec_port.c: Responsible for notifying and processing Type-C
>                         related calls from TCPM. Similar to the pdphy this
>                         layer can be extended to handle the specifics of
>                         different Qualcomm PMIC Type-C port managers.
> 
> This code provides all of the same functionality as the existing
> qcom typec driver plus power-delivery as well.
> 
> As a result commit 6c8cf3695176 ("usb: typec: Add QCOM PMIC typec detection
> driver") can be deleted entirely.
> 
> References code from Jonathan Marek, Jack Pham, Wesley Cheng, Hemant Kumar,
> Guru Das Srinagesh and Ashay Jaiswal.
> 
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>

Just a few additional nits:

[...]

> +
> +static struct platform_driver qcom_pmic_typec_platform_driver = {

This could be renamed to qcom_pmic_typec_driver, following the trend of
the other tcpm drivers.
> +	.driver = {
> +		.name = "qcom,pmic-typec",
> +		.of_match_table = qcom_pmic_typec_table,
> +	},
> +	.probe = qcom_pmic_typec_probe,
> +	.remove = qcom_pmic_typec_remove,
> +};
> +
> +static int __init qcom_pmic_typec_module_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&qcom_pmic_typec_platform_driver);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +module_init(qcom_pmic_typec_module_init);
> +
> +static void __exit qcom_pmic_typec_module_exit(void)
> +{
> +	platform_driver_unregister(&qcom_pmic_typec_platform_driver);
> +}
> +module_exit(qcom_pmic_typec_module_exit);

Can't this be simplified to just:

module_platform_driver(qcom_pmic_typec_platform_driver);

[...]
> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.h b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.h
> new file mode 100644
> index 0000000000000..ebd33c9ae0606
> --- /dev/null
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.h
> @@ -0,0 +1,115 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023, Linaro Ltd. All rights reserved.
> + */
> +#ifndef __QCOM_PMIC_PDPHY_H__
> +#define __QCOM_PMIC_PDPHY_H__

Missing a few headers:

#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/usb/tcpm.h>

[...]

> +static irqreturn_t pmic_typec_port_isr(int irq, void *dev_id)
> +{
> +	struct pmic_typec_port_irq_data *irq_data = dev_id;
> +	struct pmic_typec_port *pmic_typec_port = irq_data->pmic_typec_port;
> +	u32 misc_stat;
> +	bool vbus_change = false;
> +	bool cc_change = false;
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&pmic_typec_port->lock, flags);
> +
> +	ret = regmap_read(pmic_typec_port->regmap,
> +			  pmic_typec_port->base + TYPEC_MISC_STATUS_REG,
> +			  &misc_stat);
> +	if (ret)
> +		goto done;
> +
> +	switch (irq_data->virq) {
> +	case PMIC_TYPEC_VBUS_IRQ:
> +		/* Incoming vbus assert/de-assert detect */

This comment can probably be dropped
> +		vbus_change = true;
> +		break;
> +	case PMIC_TYPEC_CC_STATE_IRQ:
> +		if (!pmic_typec_port->debouncing_cc)
> +			cc_change = true;
> +		break;
> +	case PMIC_TYPEC_ATTACH_DETACH_IRQ:
> +		if (!pmic_typec_port->debouncing_cc)
> +			cc_change = true;
> +		break;
> +	}

The middle case can just fall through:

	case PMIC_TYPEC_CC_STATE_IRQ:
	case PMIC_TYPEC_ATTACH_DETACH_IRQ:
		if (!pmic_typec_port->debouncing_cc)
			cc_change = true;
		break;
	}

[...]

> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_port.h b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_port.h
> new file mode 100644
> index 0000000000000..5a9c47373c614
> --- /dev/null
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_port.h
> @@ -0,0 +1,194 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023, Linaro Ltd. All rights reserved.
> + */
> +#ifndef __QCOM_PMIC_TYPEC_H__
> +#define __QCOM_PMIC_TYPEC_H__
> +

Also missing some headers:

#include <linux/platform_device.h>
#include <linux/regmap.h>
> +#include <linux/usb/tcpm.h>
> +

-- 
Kind Regards,
Caleb (they/them)