Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Hans de Goede]

HI,

On 15-01-18 10:08, Chanwoo Choi wrote:
> On 2018ë 01ì 15ì 17:36, Hans de Goede wrote:
> > Hi,
> >
> > On 15-01-18 06:22, Chanwoo Choi wrote:
> > > On 2018ë 01ì 15ì 00:10, Hans de Goede wrote:
> > > > The only misdetection which can happen at boot due to data-lines mux issues
> > > > is detecting a non SDP as SDP, so we only need to retry if we detect a SDP
> > > > on our first detection.
> > > >
> > > > Note Vbus misdetection is not a problem, as soon as the drivers controlling
> > > > the Vbus path set it correctly we will get an interrupt which reschedules
> > > > the charger-detection.
> > > >
> > > > Also update the comment about the re-detection to reflect this.
> > > >
> > > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > > ---
> > > >    drivers/extcon/extcon-axp288.c | 14 +++++++-------
> > > >    1 file changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> > > > index 63b99d5becd7..17e6808af0d1 100644
> > > > --- a/drivers/extcon/extcon-axp288.c
> > > > +++ b/drivers/extcon/extcon-axp288.c
> > > > @@ -161,16 +161,16 @@ static void axp288_chrg_detect_complete(struct axp288_extcon_info *info,
> > > >        /*
> > > >         * We depend on other drivers to do things like mux the data lines,
> > > >         * enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has
> > > > -     * not set these things up correctly resulting in the initial charger
> > > > -     * cable type detection giving a wrong result and we end up not charging
> > > > -     * or charging at only 0.5A.
> > > > +     * not set these things up correctly resulting in a wrong result for the
> > > > +     * initial charger type detection and we end up charging at only 0.5A.
> > > >         *
> > > > -     * So we schedule a second cable type detection after 2 seconds to
> > > > -     * give the other drivers time to load and do their thing.
> > > > +     * If our first detect detects an SDP charger-type, we try again after
> > > > +     * 2 seconds to give the other drivers time to load and do their thing.
> > > >         */
> > > >        if (!info->first_detect_done) {
> > > > -        queue_delayed_work(system_wq, &info->det_work,
> > > > -                   msecs_to_jiffies(2000));
> > > > +        if (info->previous_cable == EXTCON_CHG_USB_SDP)
> > > > +            queue_delayed_work(system_wq, &info->det_work,
> > > > +                       msecs_to_jiffies(2000));
> > > >            info->first_detect_done = true;
> > > >        }
> > > >   
> > >
> > > I understand why you add the second delayed_work because of dependency
> > > of other consumer driver. But, this patch is not proper method. It looks
> > > like the workaround.
> > >
> > > We need to consider the fundamental solution such as using OF graph
> > > or sending the pending notification when consumer driver is probed.
> >
> > I agree that having some sort of proper probe ordering here would be
> > better. But on these ACPI systems that is going to be quite tricky todo,
> > since we've no control over the firmware there.
> >
> > Note that you've already merged the workaround, this patch merely changes
> > the workaround to avoid it in cases where it is not necessary, so I would
> > really like to see this get merged.
>
> I merged your patch because I knew this issue related to dependency.
>
> But, I don't want to merge this patch until developing the fundamental
> method. All extcon provider driver have the same issue. I'll try to
> resolve this issue thro extcon framework.

I really don't see how holding this very simple patch hostage is going to
help (or deter) finding a better solution for this.

In the mean time my original patch seems to cause mis-detection of CDP ports
as SDP ports which this fixes.

Regards,

Hans