Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Wesley Cheng <wcheng@codeaurora.org>
To: Stephen Boyd <sboyd@kernel.org>,
	agross@kernel.org, bjorn.andersson@linaro.org,
	gregkh@linuxfoundation.org, heikki.krogerus@linux.intel.com,
	robh+dt@kernel.org
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	jackp@codeaurora.org
Subject: Re: [PATCH v6 1/4] usb: typec: Add QCOM PMIC typec detection driver
Date: Fri, 31 Jul 2020 16:31:33 -0700
Message-ID: <e11fc325-7f82-b13f-3e8a-f444ddc50257@codeaurora.org> (raw)
In-Reply-To: <159601160976.1360974.10172804658083744292@swboyd.mtv.corp.google.com>



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", &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

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29  7:13 [PATCH v6 0/4] Introduce PMIC based USB type C detection Wesley Cheng
2020-07-29  7:13 ` [PATCH v6 1/4] usb: typec: Add QCOM PMIC typec detection driver Wesley Cheng
2020-07-29  8:33   ` Stephen Boyd
2020-07-31 23:31     ` Wesley Cheng [this message]
2020-07-29  7:13 ` [PATCH v6 2/4] dt-bindings: usb: Add Qualcomm PMIC type C controller dt-binding Wesley Cheng
2020-07-29  7:13 ` [PATCH v6 3/4] arm64: boot: dts: qcom: pm8150b: Add node for USB type C block Wesley Cheng
2020-07-29  7:13 ` [PATCH v6 4/4] arm64: boot: dts: qcom: pm8150b: Add DTS node for PMIC VBUS booster Wesley Cheng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e11fc325-7f82-b13f-3e8a-f444ddc50257@codeaurora.org \
    --to=wcheng@codeaurora.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=jackp@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git