Date:   Mon, 18 Mar 2019 19:50:20 +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,

On 19. 3. 18. 오후 7:38, Chanwoo Choi wrote:
> 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.

Please rebase the extcon-next branch instead of 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