Re: [PATCHv2] phy: cpcap-usb: Add CPCAP PMIC USB support

[Kishon Vijay Abraham I]

Hi Tony,

On Monday 27 March 2017 08:35 PM, Tony Lindgren wrote:
> * Kishon Vijay Abraham I <kishon@xxxxxx> [170326 23:27]:
>> On Thursday 23 March 2017 05:16 AM, Tony Lindgren wrote:
>>> +static const struct phy_ops ops = {
>>> +	.owner		= THIS_MODULE,
>>> +};
>>
>> Given that this phy doesn't have any phy_ops, Is there a reason for registering
>> this phy with the phy framework? Is it because this driver uses the phy_core's
>> pm_runtime feature?
> 
> Well there's also an ULPI PHY on CPCAP, but it's not used in the
> motorola-mapphone configuration. I think some other configurations
> use it though, so probably best to keep it.
> 
> And yeah, for PM runtime, I was thinking that we can have the
> UART mode be the idle state.
> 
>>> +static void cpcap_usb_detect(struct work_struct *work)
>>> +{
>>> +	struct cpcap_phy_ddata *ddata;
>>> +	struct cpcap_usb_ints_state s;
>>> +	bool vbus = false;
>>> +	int error;
>>> +
>>> +	ddata = container_of(work, struct cpcap_phy_ddata, detect_work.work);
>>> +
>>> +	error = cpcap_phy_get_ints_state(ddata, &s);
>>> +	if (error)
>>> +		return;
>>> +
>>> +	/* See also cpcap-charger.c phy_companion for VBUS handling */
>>
>> I think this companion should have ideally used extcon framework. Then we could
>> have used extcon_get_state() here.
> 
> OK I'll take a look at that. I'd like to do that as a follow-up
> patches if that's OK with you.
> 
> FYI in case you're wondering, there are ADCs on the PMIC available
> via IIO, and the charger is also using IIO framework for VBUS and
> battery voltage. And the VBUS is set up as reverse mode in the
> charger registers and the charging needs to be disabled for VBUS.
> 
> I guess Ideally the VBUS would be just a regulator from Linux point
> of view that also allows negative current :)
> 
>>> +	ddata->vusb = devm_regulator_get(&pdev->dev, "vusb");
>>> +	if (IS_ERR(ddata->vusb))
>>> +		return PTR_ERR(ddata->vusb);
>>> +
>>> +	error = regulator_enable(ddata->vusb);
>>> +	if (error)
>>> +		return error;
>>
>> Maybe we should create power_on ops and do regulator enable there?
> 
> This regulator is also needed for the UART mode, so it would have
> to be disabled conditionally based on maybe a module parameter.
> But yeah thanks for the pointer I did not even realize you have
> power_on ops :)
> 
> Seems this can be also done when implementing PM runtime handling.
> If you want some of these changes done for the initial patch,
> please let me know.

I think it's better we get extcon stuff in the initial patch so that we don't
have to maintain some piece of code for legacy dt. Others can be added later.

Thanks
Kishon