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@xxxxxxxxxxxxxxx>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
Reviewed-by: Caleb Connolly <caleb.connolly@xxxxxxxxxx>
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)
