Re: [PATCH] HID: i2c-hid: fix handling of unpopulated devices

[Doug Anderson <dianders@xxxxxxxxxxxx>]

Hi,

On Mon, Oct 2, 2023 at 5:09 AM Johan Hovold <johan@xxxxxxxxxx> wrote:
>
> > > > > I skimmed the thread were you added this, but I'm not sure I saw any
> > > > > reason for why powering on the panel follower temporarily during probe
> > > > > would not work?
> > > >
> > > > My first instinct says we can't do this, but let's think about it...
> > > >
> > > > In general the "panel follower" API is designed to give all the
> > > > decision making about when to power things on and off to the panel
> > > > driver, which is controlled by DRM.
> > > >
> > > > The reason for this is from experience I had when dealing with the
> > > > Samsung ATNA33XC20 panel that's on "sc7180-trogdor-homestar". The TCON
> > > > on that panel tended to die if you didn't sequence it just right.
> > > > Specifically, if you were sending pixels to the panel and then stopped
> > > > then you absolutely needed to power the panel off and on again. Folks
> > > > I talked to even claimed that the panel was working "to spec" since,
> > > > in the "Power Sequencing" section of the eDP spec it clearly shows
> > > > that you _must_ turn the panel off and on again after you stop giving
> > > > it bits. ...this is despite the fact that no other panel I've worked
> > > > with cares. ;-)
> > > >
> > > > On homestar, since we didn't have the "panel follower" API, we ended
> > > > up adding cost to the hardware and putting the panel and touchscreens
> > > > on different power rails. However, I wanted to make sure that if we
> > > > ran into a similar situation in the future (or maybe if we were trying
> > > > to make hardware work that we didn't have control over) that we could
> > > > solve it.
>
> Out of curiosity, are there any machines that actually need this
> "panel-follower" API today, or are saying above that this is just
> something that may be needed one day?

Yes. See commit de0874165b83 ("drm/panel: Add a way for other devices
to follow panel state") where I point to Cong Yang's original patch
[1]. In that patch Cong was trying to make things work by assuming
probe ordering and manually taking some of the power sequencing stuff
out of some of the drivers in order to get things to work.

[1] https://lore.kernel.org/r/20230519032316.3464732-1-yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx



> > > > The other reason for giving full control to the panel driver is just
> > > > how userspace usually works. Right now userspace tends to power off
> > > > panels if they're not used (like if a lid is closed on a laptop) but
> > > > doesn't necessarily power off the touchscreen. Thus if the touchscreen
> > > > has the ability to keep things powered on then we'd never get to a low
> > > > power state.
> > >
> > > Don't you need to keep the touchscreen powered to support wakeup events
> > > (e.g. when not closing the lid)?
> >
> > No. The only reason you'd use panel follower is if the hardware was
> > designed such that the touchscreen needed to be power sequenced with
> > the panel. If the touchscreen can stay powered when the panel is off
> > then it is, by definition, not a panel follower.
> >
> > For a laptop I don't think most people expect the touchscreen to stay
> > powered when the screen is off. I certainly wouldn't expect it. If the
> > screen was off and I wanted to interact with the device, I would hit a
> > key on the keyboard or touch the trackpad. When the people designing
> > sc7180-trogdor chose to have the display and touchscreen share a power
> > rail they made a conscious choice that they didn't need the
> > touchscreen active when the screen was off.
>
> Sure, but that's a policy decision and not something that should be
> hard-coded in our drivers.

If the touchscreen and panel can be powered separately then, sure,
it's a policy decision.

In the cases where the touchscreen and panel need to be powered
together I'd say it's more than a policy decision. Even if it wasn't,
you have to make _some_ decision in the kernel. One could also argue
that if you say that you're going to force the panel to be powered on
whenever the touchscreen is on then that's just as much of a policy
decision, isn't it?

In any case, the fact that there is a shared power rail / shared power
sequence is because the hardware designer intended them to either be
both off or both on. Whenever I asked the EEs that designed these
boards about leaving the touchscreen on while turning the panel power
off they always looked at me incredulously and asked why I would ever
do that. Although we can work around the hardware by powering the
panel in order to allow the touchscreen to be on, it's just not the
intention.


> > > But the main reason is still that requesting resources belongs in
> > > probe() and should not be deferred to some later random time where you
> > > cannot inform driver core of failures (e.g. for probe deferral if the
> > > interrupt controller is not yet available).
> >
> > OK, I guess the -EPROBE_DEFER is technically possible though probably
> > not likely in practice. ...so that's a good reason to make sure we
> > request the IRQ in probe even in the "panel follower" case. I still
> > beleive Benjamin would prefer that this was abstracted out and not in
> > the actual probe() routine, but I guess we can wait to hear from him.
>
> I talked to Benjamin at Kernel Recipes last week and I don't think he
> has any fundamental objections to the fix I'm proposing.

Sure. I don't either though I'm hoping that we can come up with a more
complete solution long term.


> I prefer it as it makes the code easier to reason about and clearly
> marks the code paths that differ in case the device is a "panel
> follower". And since you said it also makes the code look more like what
> you originally intended, then I guess you should be ok with it too?

It looks OK to me. The biggest objection I have is just that I dislike
it when code churns because two people disagree what the nicer style
is. It just makes for bigger diffs and more work to review things.


> > One last idea I had while digging would be to wonder if we could
> > somehow solve this case with "IRQF_PROBE_SHARED". I guess that doesn't
> > work well together with "IRQF_NO_AUTOEN", but conceivably we could
> > have the interrupt handler return "IRQ_NONE" if the initial power up
> > never happened? I haven't spent much time poking with shared
> > interrupts though, so I don't know if there are other side effects...
>
> Yeah, that doesn't seem right, though. The interrupt line is not really
> shared, it's just that we need to check whether the device is populated
> before requesting the interrupt.

I'm not convinced that marking it as shared is any "less right" than
extra work to request the interrupt after we've probed the device.
Fundamentally both are taking into account that another touchscreen
might be trying to probe with the same interrupt line.

-Doug