Message-ID: <3af26666-8913-c8bb-d2fb-64bd9ea0ec69@samsung.com>
Date:   Mon, 18 Mar 2019 19:38:26 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        linux-kernel@...r.kernel.org, Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin
 Cove PMIC

Hi Andy,

Thanks for comment. I add my comments
and then you have to rebase it on latest v5.0-rc1
because the merge conflict happen on v5.0-rc1.

On 19. 3. 18. 오후 7:11, Andy Shevchenko wrote:
> On Mon, Mar 18, 2019 at 12:52:25PM +0300, Andy Shevchenko wrote:
>> TBD
> 
> Oops.
> I though I have written it already.
> 
> I will wait for other comments today and sent a new version with commit message
> filled as follows:
> 
> On Intel Merrifield the Basin Cove PMIC provides a feature to detect
> the USB connection type. This driver utilizes the feature in order to support
> the USB dual role detection.
> 
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> ---
>>  drivers/extcon/Kconfig              |   7 +
>>  drivers/extcon/Makefile             |   1 +
>>  drivers/extcon/extcon-intel-mrfld.c | 256 ++++++++++++++++++++++++++++
>>  3 files changed, 264 insertions(+)
>>  create mode 100644 drivers/extcon/extcon-intel-mrfld.c
>>
>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>> index 8e17149655f0..75349c6cc89e 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -60,6 +60,13 @@ config EXTCON_INTEL_CHT_WC
>>  	  Say Y here to enable extcon support for charger detection / control
>>  	  on the Intel Cherrytrail Whiskey Cove PMIC.
>>  
>> +config EXTCON_INTEL_MRFLD
> 
>> +	tristate "Intel MErrifield Basin Cove PMIC extcon driver"
> 
> ME -> Me (will be fixed)
> 
>> +	depends on INTEL_SOC_PMIC_MRFLD

This driver uses the regmap interface. So, you better to add
following dependency?
- select REGMAP_I2C or REGMAP_SPI

But, if 'INTEL_SOC_PMIC_MRFLE' selects already REGMAP_*
configuration. It is not necessary.

>> +	help
>> +	  Say Y here to enable extcon support for charger detection / control
>> +	  on the Intel Merrifiel Basin Cove PMIC.

What is correct word?
- Merrifield? is used on above
- Merrifiel?


>> +
>>  config EXTCON_MAX14577
>>  	tristate "Maxim MAX14577/77836 EXTCON Support"
>>  	depends on MFD_MAX14577
>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> index 261ce4cfe209..d3941a735df3 100644
>> --- a/drivers/extcon/Makefile
>> +++ b/drivers/extcon/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
>>  obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
>>  obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o
>>  obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o
>> +obj-$(CONFIG_EXTCON_INTEL_MRFLD) += extcon-intel-mrfld.o
>>  obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
>>  obj-$(CONFIG_EXTCON_MAX3355)	+= extcon-max3355.o
>>  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>> diff --git a/drivers/extcon/extcon-intel-mrfld.c b/drivers/extcon/extcon-intel-mrfld.c
>> new file mode 100644
>> index 000000000000..d45db4975b5f
>> --- /dev/null
>> +++ b/drivers/extcon/extcon-intel-mrfld.c
>> @@ -0,0 +1,256 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Extcon driver for Basin Cove PMIC
>> + *
>> + * Copyright (c) 2018, Intel Corporation.
> 
> 2019 I suppose :-)

Right.

> 
>> + * Author: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> + */
>> +
>> +#include <linux/extcon-provider.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/mfd/intel_soc_pmic_mrfld.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "extcon-intel.h"
>> +
>> +#define BCOVE_USBIDCTRL			0x19
>> +#define BCOVE_USBIDCTRL_ID		BIT(0)
>> +#define BCOVE_USBIDCTRL_ACA		BIT(1)
>> +#define BCOVE_USBIDCTRL_ALL	(BCOVE_USBIDCTRL_ID | BCOVE_USBIDCTRL_ACA)
>> +
>> +#define BCOVE_USBIDSTS			0x1a
>> +#define BCOVE_USBIDSTS_GND		BIT(0)
>> +#define BCOVE_USBIDSTS_RARBRC_MASK	GENMASK(2, 1)
>> +#define BCOVE_USBIDSTS_RARBRC_SHIFT	1
>> +#define BCOVE_USBIDSTS_NO_ACA		0
>> +#define BCOVE_USBIDSTS_R_ID_A		1
>> +#define BCOVE_USBIDSTS_R_ID_B		2
>> +#define BCOVE_USBIDSTS_R_ID_C		3
>> +#define BCOVE_USBIDSTS_FLOAT		BIT(3)
>> +#define BCOVE_USBIDSTS_SHORT		BIT(4)
>> +
>> +#define BCOVE_CHGRIRQ_ALL	(BCOVE_CHGRIRQ_VBUSDET | BCOVE_CHGRIRQ_DCDET | \
>> +				 BCOVE_CHGRIRQ_BATTDET | BCOVE_CHGRIRQ_USBIDDET)
>> +
>> +#define BCOVE_CHGRCTRL0			0x4b
>> +#define BCOVE_CHGRCTRL0_CHGRRESET	BIT(0)
>> +#define BCOVE_CHGRCTRL0_EMRGCHREN	BIT(1)
>> +#define BCOVE_CHGRCTRL0_EXTCHRDIS	BIT(2)
>> +#define BCOVE_CHGRCTRL0_SWCONTROL	BIT(3)
>> +#define BCOVE_CHGRCTRL0_TTLCK		BIT(4)
>> +#define BCOVE_CHGRCTRL0_BIT_5		BIT(5)
>> +#define BCOVE_CHGRCTRL0_BIT_6		BIT(6)
>> +#define BCOVE_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
>> +
>> +struct mrfld_extcon_data {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct extcon_dev *edev;
>> +	unsigned int status;
>> +	unsigned int id;
>> +};
>> +
>> +static const unsigned int mrfld_extcon_cable[] = {
>> +	EXTCON_USB,
>> +	EXTCON_USB_HOST,
>> +	EXTCON_CHG_USB_SDP,
>> +	EXTCON_CHG_USB_CDP,
>> +	EXTCON_CHG_USB_DCP,
>> +	EXTCON_CHG_USB_ACA,
>> +	EXTCON_NONE,
>> +};
>> +
>> +static int mrfld_extcon_clear(struct mrfld_extcon_data *data, unsigned int reg,
>> +			      unsigned int mask)
>> +{
>> +	return regmap_update_bits(data->regmap, reg, mask, 0x00);
>> +}
>> +
>> +static int mrfld_extcon_set(struct mrfld_extcon_data *data, unsigned int reg,
>> +			    unsigned int mask)
>> +{
>> +	return regmap_update_bits(data->regmap, reg, mask, 0xff);
>> +}

mrfld_extcon_clear() and mrfld_extcon_set() are just wrapper function
for regmap interface. I think that you better to define
the meaningful defintion for '0x00' and '0xff' as following:

(just example, you may make the more correct name)
#define INTEL_MRFLD_RESET	0x00
#define INTEL_MRFLD_SET		0xff

And then you better to use the 'regmap_update_bits()' function
directly instead of mrfld_extcon_clear/set'.

Also, you should handle the exception hanlding when using regmap function.

>> +
>> +static int mrfld_extcon_get_id(struct mrfld_extcon_data *data)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	unsigned int id;
>> +	bool ground;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, BCOVE_USBIDSTS, &id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (id & BCOVE_USBIDSTS_FLOAT)
>> +		return INTEL_USB_ID_FLOAT;
>> +
>> +	switch ((id & BCOVE_USBIDSTS_RARBRC_MASK) >> BCOVE_USBIDSTS_RARBRC_SHIFT) {
>> +	case BCOVE_USBIDSTS_R_ID_A:
>> +		return INTEL_USB_RID_A;
>> +	case BCOVE_USBIDSTS_R_ID_B:
>> +		return INTEL_USB_RID_B;
>> +	case BCOVE_USBIDSTS_R_ID_C:
>> +		return INTEL_USB_RID_C;

Please add 'default' statement for exception handling.

>> +	}
>> +
>> +	/*
>> +	 * PMIC A0 reports USBIDSTS_GND = 1 for ID_GND,
>> +	 * but PMIC B0 reports USBIDSTS_GND = 0 for ID_GND.
>> +	 * Thus we must check this bit at last.
>> +	 */
>> +	ground = id & BCOVE_USBIDSTS_GND;
>> +	switch ('A' + BCOVE_MAJOR(data->id)) {
>> +	case 'A':
>> +		return ground ? INTEL_USB_ID_GND : INTEL_USB_ID_FLOAT;
>> +	case 'B':
>> +		return ground ? INTEL_USB_ID_FLOAT : INTEL_USB_ID_GND;
>> +	}
>> +
>> +	/* Unknown or unsupported type */
>> +	return INTEL_USB_ID_FLOAT;
>> +}
>> +
>> +static int mrfld_extcon_role_detect(struct mrfld_extcon_data *data)
>> +{
>> +	unsigned int id;
>> +	bool usb_host;
>> +	int ret;>> +
>> +	ret = mrfld_extcon_get_id(data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	id = ret;
>> +
>> +	usb_host = (id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A);
>> +	extcon_set_state_sync(data->edev, EXTCON_USB_HOST, usb_host);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mrfld_extcon_cable_detect(struct mrfld_extcon_data *data)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	unsigned int status;
>> +	int ret;
>> +
>> +	/*
>> +	 * It seems SCU firmware clears the content of BCOVE_CHGRIRQ1
>> +	 * and makes it useless for OS. Instead we compare a previously
>> +	 * stored status to the current one, provided by BCOVE_SCHGRIRQ1.
>> +	 */
>> +	ret = regmap_read(regmap, BCOVE_SCHGRIRQ1, &status);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!(status ^ data->status))
>> +		return -ENODATA;
>> +
>> +	if ((status ^ data->status) & BCOVE_CHGRIRQ_USBIDDET)
>> +		ret = mrfld_extcon_role_detect(data);
This line gets the return value from mrfld_extcon_role_detect(data)
without any error handling and then the below line just saves 'status'
to 'data->status' regardless of 'ret' value.

I think that you have to handle the error case of
'ret = mrfld_extcon_role_detect(data)'.

>> +
>> +	data->status = status;

nitpick. better to add one blank line.

>> +	return ret;
>> +}
>> +
>> +static irqreturn_t mrfld_extcon_interrupt(int irq, void *dev_id)
>> +{
>> +	struct mrfld_extcon_data *data = dev_id;
>> +	int ret;
>> +
>> +	ret = mrfld_extcon_cable_detect(data);
>> +
>> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
>> +
>> +	return ret ? IRQ_NONE: IRQ_HANDLED;
>> +}
>> +
>> +static int mrfld_extcon_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
>> +	struct regmap *regmap = pmic->regmap;
>> +	struct mrfld_extcon_data *data;
>> +	unsigned int id;
>> +	int irq, ret;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->dev = dev;
>> +	data->regmap = regmap;
>> +
>> +	data->edev = devm_extcon_dev_allocate(dev, mrfld_extcon_cable);
>> +	if (IS_ERR(data->edev))
>> +		return -ENOMEM;
>> +
>> +	ret = devm_extcon_dev_register(dev, data->edev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "can't register extcon device: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(dev, irq, NULL, mrfld_extcon_interrupt,
>> +					IRQF_ONESHOT | IRQF_SHARED, pdev->name,
>> +					data);
>> +	if (ret)
>> +		return ret;

You better add the error log with dev_err.

>> +
>> +	ret = regmap_read(regmap, BCOVE_ID, &id);
>> +	if (ret)
>> +		return ret;

ditto for error log.

>> +
>> +	data->id = id;
>> +
>> +	mrfld_extcon_set(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
>> +
>> +	/* Get initial state */
>> +	mrfld_extcon_role_detect(data);

Please handle the return value for exception handling with log.

>> +
>> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
>> +	mrfld_extcon_clear(data, BCOVE_MCHGRIRQ1, BCOVE_CHGRIRQ_ALL);
>> +
>> +	mrfld_extcon_set(data, BCOVE_USBIDCTRL, BCOVE_USBIDCTRL_ALL);
>> +
>> +	platform_set_drvdata(pdev, data);

nitpick. better to add one blank line.

>> +	return 0;
>> +}
>> +
>> +static int mrfld_extcon_remove(struct platform_device *pdev)
>> +{
>> +	struct mrfld_extcon_data *data = platform_get_drvdata(pdev);
>> +
>> +	mrfld_extcon_clear(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);

nitpick. better to add one blank line.

>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id mrfld_extcon_id_table[] = {
>> +	{ .name = "mrfld_bcove_extcon" },

I think that it is not proper to use 'extcon' word for the compatible name
because 'extcon' word is linuxium. So, I recommend that you remove
the 'extcon' word. Instead, you better to use new word related to h/w.

>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(platform, mrfld_extcon_id_table);
>> +
>> +static struct platform_driver mrfld_extcon_driver = {
>> +	.driver = {
>> +		.name	= KBUILD_MODNAME,

Where is the definition of KBUILD_MODNAME? Are you missing?

>> +	},
>> +	.probe		= mrfld_extcon_probe,
>> +	.remove		= mrfld_extcon_remove,
>> +	.id_table	= mrfld_extcon_id_table,
>> +};
>> +module_platform_driver(mrfld_extcon_driver);
>> +
>> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@...ux.intel.com>");
>> +MODULE_DESCRIPTION("Extcon driver for Basin Cove PMIC");

Add the 'Merrifield' word in front of 'Basin Cove PMIC' as following:
- Merrifield Basic Cove PMIC

>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.20.1
>>
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Powered by blists - more mailing lists