Re: [PATCH 1/3] usb: typec: Add QCOM PMIC typec detection driver

[Wesley Cheng <wcheng@codeaurora.org>]

On 6/9/2020 7:27 PM, Jun Li wrote:
>> +static int qcom_pmic_typec_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct qcom_pmic_typec *qcom_usb;
>> +       struct typec_capability *cap;
>> +       const char *buf;
>> +       int ret, irq, role;
>> +
>> +       qcom_usb = kzalloc(sizeof(*qcom_usb), GFP_KERNEL);
> 
> devm_kzalloc().
> 

Hi Jun,

Thanks for the input.  I have replaced with devm_kzalloc() as you
recommended.

>> +       if (!qcom_usb)
>> +               return -ENOMEM;
>> +
>> +       qcom_usb->dev = dev;
>> +
>> +       qcom_usb->regmap = dev_get_regmap(dev->parent, NULL);
>> +       if (!qcom_usb->regmap) {
>> +               dev_err(dev, "Failed to get regmap\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq < 0) {
>> +               dev_err(dev, "Failed to get CC irq\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       ret = devm_request_irq(qcom_usb->dev, irq, qcom_pmic_typec_interrupt,
>> +                              IRQF_SHARED, "qcom-pmic-typec", qcom_usb);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Could not request IRQ\n");
>> +               return ret;
>> +       }
>> +
>> +       qcom_usb->fwnode = device_get_named_child_node(dev, "connector");
>> +       if (!qcom_usb->fwnode)
>> +               return -EINVAL;
>> +
>> +       cap = kzalloc(sizeof(*cap), GFP_KERNEL);
> 
> devm_kzalloc().
> 
>> +       if (!cap) {
>> +               ret = -ENOMEM;
>> +               goto err_put_node;
>> +       }
>> +
>> +       ret = fwnode_property_read_string(qcom_usb->fwnode, "power-role", &buf);
>> +       if (!ret)
>> +               role = typec_find_port_power_role(buf);
> 
> if (role < 0)
> 


Added a check for role <0, if so, set to SNK as well
>> +       else
>> +               role = TYPEC_PORT_SNK;
>> +       cap->type = role;
>> +
>> +       ret = fwnode_property_read_string(qcom_usb->fwnode, "data-role", &buf);
>> +       if (!ret)
>> +               role = typec_find_port_data_role(buf);
> 
> if (role < 0)
> 

Added a check for role <0, if so, set to UFP as well
>> +       else
>> +               role = TYPEC_PORT_UFP;
>> +       cap->data = role;
>> +
>> +       cap->prefer_role = -1;
> 
> TYPEC_NO_PREFERRED_ROLE
> 

Done.
>> +       cap->fwnode = qcom_usb->fwnode;
>> +       qcom_usb->port = typec_register_port(dev, cap);
>> +       if (IS_ERR(qcom_usb->port)) {
>> +               dev_err(dev, "Failed to register type c port %d\n",
>> +                       IS_ERR(qcom_usb->port));
>> +               ret = PTR_ERR(qcom_usb->port);
> 
> dev_err(dev, , ret)?
> 
> Li Jun

Agreed.  Thanks!

>> +               goto err_put_node;
>> +       }
>> +
>> +       qcom_usb->cap = cap;
>> +
>> +       qcom_usb->role_sw = fwnode_usb_role_switch_get(qcom_usb->fwnode);
>> +       if (IS_ERR(qcom_usb->role_sw)) {
>> +               if (PTR_ERR(qcom_usb->role_sw) != -EPROBE_DEFER)
>> +                       dev_err(dev, "failed to get role switch\n");
>> +               ret = PTR_ERR(qcom_usb->role_sw);
>> +               goto err_typec_port;
>> +       }
>> +
>> +       INIT_WORK(&qcom_usb->bh_work, qcom_pmic_typec_bh_work);
>> +       platform_set_drvdata(pdev, qcom_usb);
>> +       qcom_pmic_typec_typec_hw_init(qcom_usb);
>> +
>> +       queue_work(system_power_efficient_wq, &qcom_usb->bh_work);
>> +
>> +       return 0;
>> +
>> +err_typec_port:
>> +       typec_unregister_port(qcom_usb->port);
>> +err_put_node:
>> +       fwnode_handle_put(qcom_usb->fwnode);
>> +
>> +       return ret;
>> +}
>> +
>> +static int qcom_pmic_typec_remove(struct platform_device *pdev)
>> +{
>> +       struct qcom_pmic_typec *qcom_usb = platform_get_drvdata(pdev);
>> +
>> +       cancel_work_sync(&qcom_usb->bh_work);
>> +       usb_role_switch_set_role(qcom_usb->role_sw, USB_ROLE_NONE);
>> +       qcom_pmic_typec_vbus_disable(qcom_usb);
>> +
>> +       typec_unregister_port(qcom_usb->port);
>> +       usb_role_switch_put(qcom_usb->role_sw);
>> +       fwnode_handle_put(qcom_usb->fwnode);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_pmic_typec_table[] = {
>> +       { .compatible = "qcom,pm8150b-usb-typec" },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_pmic_typec_table);
>> +
>> +static struct platform_driver qcom_pmic_typec = {
>> +       .driver = {
>> +               .name = "qcom,pmic-typec",
>> +               .of_match_table = qcom_pmic_typec_table,
>> +       },
>> +       .probe = qcom_pmic_typec_probe,
>> +       .remove = qcom_pmic_typec_remove,
>> +};
>> +
>> +module_platform_driver(qcom_pmic_typec);
>> +
>> +MODULE_DESCRIPTION("QCOM PMIC USB type C driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

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