lkml.org 
[lkml]   [2019]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC
From
Date
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@linux.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@linux.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@linux.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

\
 
 \ /
  Last update: 2019-03-18 11:39    [W:0.088 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site