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 <[email protected]> >> --- >> 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 <[email protected]> >> + */ >> + >> +#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 <[email protected]>"); >> +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
