Message-ID: <20230123170739.GZ36097@ediswmail.ad.cirrus.com>
Date:   Mon, 23 Jan 2023 17:07:39 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
CC:     <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <linux-kernel@...r.kernel.org>, <vkoul@...nel.org>,
        <sanyog.r.kale@...el.com>, <yung-chuan.liao@...ux.intel.com>
Subject: Re: [PATCH 2/2] soundwire: bus: Allow SoundWire peripherals to
 register IRQ handlers

On Mon, Jan 23, 2023 at 09:50:15AM -0600, Pierre-Louis Bossart wrote:
> On 1/23/23 08:53, Charles Keepax wrote:
> > On Fri, Jan 20, 2023 at 10:20:50AM -0600, Pierre-Louis Bossart wrote:
> >> On 1/20/23 03:59, Charles Keepax wrote:
> >>> On Thu, Jan 19, 2023 at 11:12:04AM -0600, Pierre-Louis Bossart wrote:
> >>>> There should be an explanation and something checking that both are not
> >>>> used concurrently.
> >>>
> >>> I will try to expand the explanation a litte, but I dont see any
> >>> reason to block calling both handlers, no ill effects would come
> >>> for a driver having both and it is useful if any soundwire
> >>> specific steps are needed that arn't on other control buses.
> >>
> >> I think it's problematic if the peripheral tries to wake-up the manager
> >> from clock-stop with both an in-band wake (i.e. drive the data line
> >> high) and a separate GPIO-based interrupt. It's asking for trouble IMHO.
> >> We spent hours in the MIPI team to make sure there were no races between
> >> the manager-initiated restarts and peripheral-initiated restarts, adding
> >> a 3rd mechanism in the mix gives me a migraine already.
> > 
> > Apologies but I am struggling see why this has any bearing on
> > the case of a device that does both an in-band and out-of-band
> > wake. The code we are adding in this patch will only be called in the
> > in-band case. handle_nested_irq doesn't do any hardware magic or
> > schedule any threads, it just calls a function that was provided
> > when the client called request_threaded_irq. The only guarantee
> > of atomicity you have on the interrupt_callback is sdw_dev_lock
> > and that is being held across both calls after the patch.
> > 
> > Could you be a little more specific on what you mean by this
> > represents a 3rd mechanism, to me this isn't a new mechanism just
> > an extra callback? Say for example this patch added an
> > interrupt_callback_early to sdw_slave_ops that is called just
> > before interrupt_callback.
> 
> Well, the main concern is exiting the clock-stop. That is handled by the
> manager and could be done
> a) as the result of the framework deciding that something needs to be
> done (typically as a result of user/applications starting a stream)
> b) by the device with an in-band wake in case of e.g. jack detection or
> acoustic events detected
> c) same as b) but with a separate out-of-band interrupt.
> 
> I'd like to make sure b) and c) are mutually-exclusive options, and that
> the device will not throw BOTH an in-band wake and an external interrupt.

I think this is the bit I don't follow why does this patch have
anything to do with whether b) and c) are mutually-exclusive
options?

This patch lets you register an IRQ handler to the in-band IRQs,
it has nothing to do with whether you register an IRQ handler to
some out of band IRQ and there is nothing in the current
framework that will prevent someone doing that. Adding a
check that forces someone to choose between using
handle_nested_irq or interrupt_callback will also have no
bearing on whether they attach an IRQ handler to an out-of-band
IRQ.

Thanks,
Charles

Powered by blists - more mailing lists