Re: [PATCH] drivers: extcon: Add support for ptn5150

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Vijai,

On 19. 1. 22. 오후 1:42, Vijai Kumar K wrote:
> Hi Chanwoo Choi,
> 
> This is the first time I am sending a driver to LKML. I have a few doubts. Can
> you please clarify them when you are free?
> 
> 1. I have developed and tested this patch on 4.14.89 kernel. When trying to 
> mainline the driver should I rebase and send the patch on top of current tree(v5.0-rc2)?

You better to rebase your patch on extcon-next branch (v5.0-rc1).
because the extcon.git[1] keep the v5.0-rc1 version. But, if you use over v5.0-rc1 version,
it doesn't matter. Maybe, this patch doesn't get the any impact from the modification
of v5.0-rcX.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

> 
> 2. Is there any specific git on which I should test this driver on? Like below?
> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

Yes.

> 
> Thanks,
> Vijai Kumar K
> 
> On Tue, Jan 22, 2019 at 08:05:33AM +0800, kbuild test robot wrote:
>> Hi Vijai,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on chanwoo-extcon/extcon-next]
>> [also build test ERROR on v5.0-rc2]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url:    https://github.com/0day-ci/linux/commits/Vijai-Kumar-K/drivers-extcon-Add-support-for-ptn5150/20190122-041153
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
>> config: sh-allyesconfig (attached as .config)
>> compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
>> reproduce:
>>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>         chmod +x ~/bin/make.cross
>>         # save the attached .config to linux build tree
>>         GCC_VERSION=8.2.0 make.cross ARCH=sh 
>>
>> All error/warnings (new ones prefixed by >>):
>>
>>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_irq_work':
>>>> drivers//extcon/extcon-ptn5150.c:93:5: error: implicit declaration of function 'extcon_set_state_sync'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
>>         extcon_set_state_sync(info->edev,
>>         ^~~~~~~~~~~~~~~~~~~~~
>>         extcon_get_state
>>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_i2c_probe':
>>>> drivers//extcon/extcon-ptn5150.c:255:15: error: implicit declaration of function 'devm_extcon_dev_allocate'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
>>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>>                   ^~~~~~~~~~~~~~~~~~~~~~~~
>>                   extcon_get_state
>>>> drivers//extcon/extcon-ptn5150.c:255:13: warning: assignment to 'struct extcon_dev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
>>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>>                 ^
>>>> drivers//extcon/extcon-ptn5150.c:262:8: error: implicit declaration of function 'devm_extcon_dev_register'; did you mean 'devm_pinctrl_register'? [-Werror=implicit-function-declaration]
>>      ret = devm_extcon_dev_register(info->dev, info->edev);
>>            ^~~~~~~~~~~~~~~~~~~~~~~~
>>            devm_pinctrl_register
>>    cc1: some warnings being treated as errors
>>
>> vim +93 drivers//extcon/extcon-ptn5150.c
>>
>>     51	
>>     52	static void ptn5150_irq_work(struct work_struct *work)
>>     53	{
>>     54		struct ptn5150_info *info = container_of(work,
>>     55				struct ptn5150_info, irq_work);
>>     56		int ret = 0;
>>     57		unsigned int reg_data;
>>     58		unsigned int port_status;
>>     59		unsigned int vbus;
>>     60		unsigned int cable_attach;
>>     61		unsigned int int_status;
>>     62	
>>     63		if (!info->edev)
>>     64			return;
>>     65	
>>     66		mutex_lock(&info->mutex);
>>     67	
>>     68		ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
>>     69		if (ret) {
>>     70			dev_err(info->dev,
>>     71				"failed to read CC STATUS %d\n", ret);
>>     72			mutex_unlock(&info->mutex);
>>     73			return;
>>     74		}
>>     75		/* Clear interrupt. Read would clear the register */
>>     76		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
>>     77		if (ret) {
>>     78			dev_err(info->dev,
>>     79				"failed to read INT STATUS %d\n", ret);
>>     80			mutex_unlock(&info->mutex);
>>     81			return;
>>     82		}
>>     83	
>>     84		if (int_status) {
>>     85			cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
>>     86			if (cable_attach) {
>>     87				port_status = ((reg_data &
>>     88						PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
>>     89						PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
>>     90	
>>     91				switch (port_status) {
>>     92				case PTN5150_DFP_ATTACHED:
>>   > 93					extcon_set_state_sync(info->edev,
>>     94							      EXTCON_USB_HOST, false);
>>     95					gpiod_set_value(info->vbus_gpiod, 0);
>>     96					extcon_set_state_sync(info->edev, EXTCON_USB,
>>     97							      true);
>>     98					break;
>>     99				case PTN5150_UFP_ATTACHED:
>>    100					extcon_set_state_sync(info->edev, EXTCON_USB,
>>    101							      false);
>>    102					vbus = ((reg_data &
>>    103						PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
>>    104						PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
>>    105					if (vbus)
>>    106						gpiod_set_value(info->vbus_gpiod, 0);
>>    107					else
>>    108						gpiod_set_value(info->vbus_gpiod, 1);
>>    109	
>>    110					extcon_set_state_sync(info->edev,
>>    111							      EXTCON_USB_HOST, true);
>>    112					break;
>>    113				default:
>>    114					dev_err(info->dev,
>>    115						"Unknown Port status : %x\n",
>>    116						port_status);
>>    117					break;
>>    118				}
>>    119			} else {
>>    120				extcon_set_state_sync(info->edev,
>>    121						      EXTCON_USB_HOST, false);
>>    122				extcon_set_state_sync(info->edev,
>>    123						      EXTCON_USB, false);
>>    124				gpiod_set_value(info->vbus_gpiod, 0);
>>    125			}
>>    126		}
>>    127	
>>    128		/* Clear interrupt. Read would clear the register */
>>    129		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
>>    130					&int_status);
>>    131		if (ret) {
>>    132			dev_err(info->dev,
>>    133				"failed to read INT REG STATUS %d\n", ret);
>>    134			mutex_unlock(&info->mutex);
>>    135			return;
>>    136		}
>>    137	
>>    138	
>>    139		mutex_unlock(&info->mutex);
>>    140	}
>>    141	
>>    142	
>>    143	static irqreturn_t ptn5150_irq_handler(int irq, void *data)
>>    144	{
>>    145		struct ptn5150_info *info = data;
>>    146	
>>    147		schedule_work(&info->irq_work);
>>    148	
>>    149		return IRQ_HANDLED;
>>    150	}
>>    151	
>>    152	static int ptn5150_init_dev_type(struct ptn5150_info *info)
>>    153	{
>>    154		unsigned int reg_data, vendor_id, version_id;
>>    155		int ret;
>>    156	
>>    157		ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
>>    158		if (ret) {
>>    159			dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
>>    160			return -EINVAL;
>>    161		}
>>    162	
>>    163		vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
>>    164					PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
>>    165		version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
>>    166					PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
>>    167	
>>    168		dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
>>    169				    version_id, vendor_id);
>>    170	
>>    171		/* Clear any existing interrupts */
>>    172		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
>>    173		if (ret) {
>>    174			dev_err(info->dev,
>>    175				"failed to read PTN5150_REG_INT_STATUS %d\n",
>>    176				ret);
>>    177			return -EINVAL;
>>    178		}
>>    179	
>>    180		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
>>    181		if (ret) {
>>    182			dev_err(info->dev,
>>    183				"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
>>    184			return -EINVAL;
>>    185		}
>>    186	
>>    187		return 0;
>>    188	}
>>    189	
>>    190	static int ptn5150_i2c_probe(struct i2c_client *i2c,
>>    191					 const struct i2c_device_id *id)
>>    192	{
>>    193		struct device *dev = &i2c->dev;
>>    194		struct device_node *np = i2c->dev.of_node;
>>    195		struct ptn5150_info *info;
>>    196		int ret;
>>    197	
>>    198		if (!np)
>>    199			return -EINVAL;
>>    200	
>>    201		info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
>>    202		if (!info)
>>    203			return -ENOMEM;
>>    204		i2c_set_clientdata(i2c, info);
>>    205	
>>    206		info->dev = &i2c->dev;
>>    207		info->i2c = i2c;
>>    208		info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
>>    209		if (!info->int_gpiod) {
>>    210			dev_err(dev, "failed to get INT GPIO\n");
>>    211			return -EINVAL;
>>    212		}
>>    213		info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
>>    214		if (!info->vbus_gpiod) {
>>    215			dev_err(dev, "failed to get VBUS GPIO\n");
>>    216			return -EINVAL;
>>    217		}
>>    218		ret = gpiod_direction_output(info->vbus_gpiod, 0);
>>    219		if (ret) {
>>    220			dev_err(dev, "failed to set VBUS GPIO direction\n");
>>    221			return -EINVAL;
>>    222		}
>>    223	
>>    224		mutex_init(&info->mutex);
>>    225	
>>    226		INIT_WORK(&info->irq_work, ptn5150_irq_work);
>>    227	
>>    228		info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
>>    229		if (IS_ERR(info->regmap)) {
>>    230			ret = PTR_ERR(info->regmap);
>>    231			dev_err(info->dev, "failed to allocate register map: %d\n",
>>    232					   ret);
>>    233			return ret;
>>    234		}
>>    235	
>>    236		if (info->int_gpiod) {
>>    237			info->irq = gpiod_to_irq(info->int_gpiod);
>>    238			if (info->irq < 0) {
>>    239				dev_err(dev, "failed to get INTB IRQ\n");
>>    240				return info->irq;
>>    241			}
>>    242	
>>    243			ret = devm_request_threaded_irq(dev, info->irq, NULL,
>>    244							ptn5150_irq_handler,
>>    245							IRQF_TRIGGER_FALLING |
>>    246							IRQF_ONESHOT,
>>    247							i2c->name, info);
>>    248			if (ret < 0) {
>>    249				dev_err(dev, "failed to request handler for INTB IRQ\n");
>>    250				return ret;
>>    251			}
>>    252		}
>>    253	
>>    254		/* Allocate extcon device */
>>  > 255		info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>>    256		if (IS_ERR(info->edev)) {
>>    257			dev_err(info->dev, "failed to allocate memory for extcon\n");
>>    258			return -ENOMEM;
>>    259		}
>>    260	
>>    261		/* Register extcon device */
>>  > 262		ret = devm_extcon_dev_register(info->dev, info->edev);
>>    263		if (ret) {
>>    264			dev_err(info->dev, "failed to register extcon device\n");
>>    265			return ret;
>>    266		}
>>    267	
>>    268		/* Initialize PTN5150 device and print vendor id and version id */
>>    269		ret = ptn5150_init_dev_type(info);
>>    270		if (ret)
>>    271			return -EINVAL;
>>    272	
>>    273		return 0;
>>    274	}
>>    275	
>>
>> ---
>> 0-DAY kernel test infrastructure                Open Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics