Re: [PATCH v2 2/2] Embedded USB Debugger (EUD) driver

From: Manu Gautam
Date: Wed Sep 05 2018 - 05:31:37 EST


Hi,


On 9/5/2018 3:04 AM, Prakruthi Deepak Heragu wrote:
> Add support for control peripheral of EUD (Embedded USB Debugger) to
> listen to events such as USB attach/detach, charger enable/disable, pet
> EUD to indicate software is functional.
>
> Signed-off-by: Satya Durga Srinivasu Prabhala <satyap@xxxxxxxxxxxxxx>
> Signed-off-by: Prakruthi Deepak Heragu <pheragu@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/Kconfig | 12 ++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/eud.c | 338 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 351 insertions(+)
> create mode 100644 drivers/soc/qcom/eud.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
[snip]
> +
> +#define EUD_ENABLE_CMD 1
> +#define EUD_DISABLE_CMD 0

Why not use module param as boolean? I mean zero to disable and non-zero to enable?
> +
> +#define EUD_REG_INT1_EN_MASK 0x0024
> +#define EUD_REG_INT_STATUS_1 0x0044
> +#define EUD_REG_CTL_OUT_1 0x0074
> +#define EUD_REG_VBUS_INT_CLR 0x0080
> +#define EUD_REG_CHGR_INT_CLR 0x0084
> +#define EUD_REG_CSR_EUD_EN 0x1014
> +#define EUD_REG_SW_ATTACH_DET 0x1018
> +
> +#define EUD_INT_VBUS BIT(2)
> +#define EUD_INT_CHGR BIT(3)
> +#define EUD_INT_SAFE_MODE BIT(4)
> +
> +struct eud_chip {
> + struct device *dev;
> + int eud_irq;
> + unsigned int extcon_id;
> + unsigned int int_status;
> + bool usb_attach;
> + bool chgr_enable;
> + void __iomem *eud_reg_base;
> + struct extcon_dev *extcon;
> + struct work_struct eud_work;
> +};
> +
> +static const unsigned int eud_extcon_cable[] = {
> + EXTCON_USB,
> + EXTCON_CHG_USB_SDP,
> + EXTCON_NONE,
> +};
> +
> +/*
> + * On the kernel command line specify eud.enable=1 to enable EUD.
> + * EUD is disabled by default.
> + */
> +static int enable;
> +static bool eud_ready;
> +static struct eud_chip *eud_private;
> +
> +static void enable_eud(struct eud_chip *priv)
> +{
> + struct power_supply *usb_psy = NULL;
> + union power_supply_propval pval = {0};
> + union power_supply_propval tval = {0};
> + int ret;
> +
> + usb_psy = power_supply_get_by_name("usb");
> + if (!usb_psy)
> + return;
> +
> + ret = power_supply_get_property(usb_psy,
> + POWER_SUPPLY_PROP_PRESENT, &pval);
> + if (ret)
> + return;
> +
> + ret = power_supply_get_property(usb_psy,
> + POWER_SUPPLY_PROP_REAL_TYPE, &tval);
> + if (ret)
> + return;
> +

if(!pval.intval || (tval.intval != POWER_SUPPLY_TYPE_USB &&
tval.intval != POWER_SUPPLY_TYPE_USB_CDP))
 return;

/* following if-else can be removed */
Â

> + if (pval.intval && (tval.intval == POWER_SUPPLY_TYPE_USB ||
> + tval.intval == POWER_SUPPLY_TYPE_USB_CDP)) {
> + /* write into CSR to enable EUD */
> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
> + /* Enable vbus, chgr & safe mode warning interrupts */
> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
> +
> + /* Ensure Register Writes Complete */
> + wmb();
> +
> + /*
> + * Set the default cable state to usb connect and charger
> + * enable
> + */
> + ret = extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
> + if (ret)
> + return;
> + ret = extcon_set_state_sync(priv->extcon,
> + EXTCON_CHG_USB_SDP, true);
> + if (ret)
> + return;
> + } else {
> + return;
> + }
> +
> +}
> +
> +static void disable_eud(struct eud_chip *priv)
> +{
> + /* write into CSR to disable EUD */
> + writel_relaxed(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
> +}
> +
> +static int param_eud_set(const char *val, const struct kernel_param *kp)
> +{
> + int enable = 0;
> +
> + if (sscanf(val, "%du", &enable) != 1)
> + return -EINVAL;
> +
> + if (enable != EUD_ENABLE_CMD && enable != EUD_DISABLE_CMD)
> + return -EINVAL;
Do we really need to worry about it? 'enable' could just be treated as bool

[snip]
> +
> +
> +static irqreturn_t handle_eud_irq(int irq, void *data)
> +{
> + struct eud_chip *chip = data;
> + u32 reg;
> +
> + /* read status register and find out which interrupt triggered */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_INT_STATUS_1);
> + if (reg & EUD_INT_VBUS) {
> + chip->int_status = EUD_INT_VBUS;
> + usb_attach_detach(chip);
> + } else if (reg & EUD_INT_CHGR) {
> + chip->int_status = EUD_INT_CHGR;
> + chgr_enable_disable(chip);
> + } else if (reg & EUD_INT_SAFE_MODE) {
> + pet_eud(chip);
> + } else {
> + return IRQ_NONE;
> + }

switch-case instead?

> +
> + return IRQ_HANDLED;
> +}
> +
> +static int msm_eud_probe(struct platform_device *pdev)
> +{
> + struct eud_chip *chip;
> + struct resource *res;
> + int ret;
> +
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip) {
> + ret = -ENOMEM;
> + return ret;
return -ENOMEM;
> + }
> +
> + platform_set_drvdata(pdev, chip);
> +
> + chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);
> + if (IS_ERR(chip->extcon))
> + return PTR_ERR(chip->extcon);
> +
> + ret = devm_extcon_dev_register(&pdev->dev, chip->extcon);
> + if (ret)
> + return ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + ret = -ENOMEM;
> + return ret;
same here
> + }
> +
> + chip->eud_reg_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(chip->eud_reg_base))
> + return PTR_ERR(chip->eud_reg_base);
> +
> + chip->eud_irq = platform_get_irq(pdev, 0);
> +
> + ret = devm_request_irq(&pdev->dev, chip->eud_irq, handle_eud_irq,
> + IRQF_TRIGGER_HIGH, NULL, chip);
> + if (ret)
> + return ret;
> +
> + device_init_wakeup(&pdev->dev, true);
> + enable_irq_wake(chip->eud_irq);
> +
> + INIT_WORK(&chip->eud_work, eud_event_notifier);
> +
> + eud_private = chip;
> + eud_ready = true;
> +
> + /* Enable EUD */
> + if (enable)
> + enable_eud(eud_private);
> +
> + return 0;
> +}
> +
> +static int msm_eud_remove(struct platform_device *pdev)
> +{
> + struct eud_chip *chip = platform_get_drvdata(pdev);
> +
> + if (enable)
> + disable_eud(eud_private);
> + device_init_wakeup(&pdev->dev, false);
> + disable_irq_wake(chip->eud_irq);
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id msm_eud_dt_match[] = {
> + {.compatible = "qcom,msm-eud"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, msm_eud_dt_match);
> +
> +static struct platform_driver msm_eud_driver = {
> + .probe = msm_eud_probe,
> + .remove = msm_eud_remove,
> + .driver = {
> + .name = "msm-eud",
> + .of_match_table = msm_eud_dt_match,
> + },
> +};
> +module_platform_driver(msm_eud_driver);
> +
> +MODULE_DESCRIPTION("QTI EUD driver");
> +MODULE_LICENSE("GPL v2");

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project