On 7/29/2020 1:33 AM, Stephen Boyd wrote:
> Quoting Wesley Cheng (2020-07-29 00:13:37)
>> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
>> index 559dd06117e7..3e375f82849d 100644
>> --- a/drivers/usb/typec/Kconfig
>> +++ b/drivers/usb/typec/Kconfig
>> @@ -73,6 +73,18 @@ config TYPEC_TPS6598X
>> If you choose to build this driver as a dynamically linked module, the
>> module will be called tps6598x.ko.
>>
>> +config TYPEC_QCOM_PMIC
>> + tristate "Qualcomm PMIC USB Type-C driver"
>> + depends on ARCH_QCOM
>
> Can you add || COMPILE_TEST here?
>
Sure, will do.
>> +#include <linux/err.h>
>> +#include <linux/regmap.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_irq.h>
>
> Is this include used?
>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_device.h>
>
> Is this include used?
>
Reviewed which includes I used, and removed the ones that were not needed.
>> +static void qcom_pmic_typec_enable_vbus_regulator(struct qcom_pmic_typec
>> + *qcom_usb, bool enable)
>> +{
>> + int ret = 0;
>
> Please don't assign and then reassign before testing this variable.
>
I will just remove the assignment here.
>> + if (stat & CC_ATTACHED) {
>> + orientation = ((stat & CC_ORIENTATION) >> 1) ?
>
> Do we really need to shift >> by 1? Seems useless for a test.
>
Agreed, we can remove the shift.
>> + ret = of_property_read_u32(dev->of_node, "reg", ®);
>> + if (ret < 0) {
>> + dev_err(dev, "missing base address");
>
> Please add newlines at the end of printk messages.
>
Done.
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + dev_err(dev, "Failed to get CC irq\n");
>
> platform_get_irq() already prints an error message. Please remove this.
>
Got it.
>> +static const struct of_device_id qcom_pmic_typec_table[] = {
>> + { .compatible = "qcom,pm8150b-usb-typec" },
>> + { },
>
> Nitpick: Drop the comma here so nothing can come after without causing a
> compile error.
>
Sure.
>> +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,
>> +};
>> +
>
> Another nitpick: Drop the newline and make module_platform_driver()
> follow directly after the driver.
>
Ok, will do.
Thanks for the review/feedback, Stephen.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
