Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC

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

Hi Andy,

On 19. 3. 18. 오후 9:46, Andy Shevchenko wrote:
> On Mon, Mar 18, 2019 at 07:38:26PM +0900, Chanwoo Choi wrote:
> 
>> 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.
> 
> Thanks for review, see my answers below.
> Non-answered items will be fixed accordingly.
> 
>>>> +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
> 
> None of them fits this or MFD driver. See below.
> 
>> But, if 'INTEL_SOC_PMIC_MRFLE' selects already REGMAP_*
>> configuration. It is not necessary.
> 
> https://lore.kernel.org/lkml/20190318095316.69278-1-andriy.shevchenko@linux.intel.com/
> 
> It selects REGMAP_IRQ which selects necessary bits from regmap API.

OK.

> 
>>>> +	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?
> 
> Merrifield is a correct one. Thanks for spotting this.
> 
>>>> +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
> 
> It makes a little sense here, the idea is to reduce parameters.
> 
> I could ideally write
> (..., mask, ~mask) for clear
> and
> (..., mask, mask) for set
> 
>> And then you better to use the 'regmap_update_bits()' function
>> directly instead of mrfld_extcon_clear/set'.
> 
> It will bring duplication of long definitions and reduce readability of the
> code.

Actually, it is not critical issue. If you don't agree my comments,
you keep your style on next version. I have no any strong objection.

> 
>>>> +	/*
>>>> +	 * 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)'.
> 
> I'm not sure of the consequences of such change.
> I will give it some tests, and then will proceed accordingly.

OK. Thanks.

> 
>>>> +		.name	= KBUILD_MODNAME,
>>
>> Where is the definition of KBUILD_MODNAME? Are you missing?
> 
> In the Makefile.
> Nothing is missed here.
> 
> But I could put its content explicitly here.

OK. Thanks.

> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics