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

[Andy Shevchenko]

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@xxxxxxxxxxxxxxx/

It selects REGMAP_IRQ which selects necessary bits from regmap API.

> >> +	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.

> >> +	/*
> >> +	 * 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.

> >> +		.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.

-- 
With Best Regards,
Andy Shevchenko