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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Mar 2017 08:05:19 -0700
From:   Tony Lindgren <tony@...mide.com>
To:     Kishon Vijay Abraham I <kishon@...com>
Cc:     linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-omap@...r.kernel.org, devicetree@...r.kernel.org,
        Marcel Partap <mpartap@....net>,
        Michael Scott <michael.scott@...aro.org>
Subject: Re: [PATCHv2] phy: cpcap-usb: Add CPCAP PMIC USB support

* Kishon Vijay Abraham I <kishon@...com> [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.

Regards,

Tony