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

[Johan Hovold <johan@xxxxxxxxxx>]

On Mon, Oct 02, 2023 at 07:35:06AM -0700, Doug Anderson wrote:
> On Mon, Oct 2, 2023 at 5:09 AM Johan Hovold <johan@xxxxxxxxxx> wrote:

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

Ok, thanks for the pointer.

> > > > 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?

I get your point, but with runtime pm suspending the touchpad after a
timeout it seems that would still be the most flexible alternative
which allows deferring the decision whether to support wakeup on
touch events to the user.

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

I hear you, but users sometimes want do things with their hardware which
may not have originally been intended (e.g. your kiosk example).

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

Ok, but this isn't just about style as that initial_power_on() function
which does all the magic needs to be broken up to fix the regression
(unless you want to convolute the driver and defer resource lookups
until panel power-on).

I'll respin a v2 with that panel-property lookup change I mentioned and
hopefully we can get this fixed this week.

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

If you need to start to thinking about rewriting your interrupt handler,
I'd say that qualifies as "less right". ;)

Johan