Re: [PATCH 4/5] ASoC: codecs: Add WCD939x Soundwire slave driver

[Neil Armstrong]

On 28/11/2023 13:47, Mark Brown wrote:
> On Tue, Nov 28, 2023 at 10:09:29AM +0100, Neil Armstrong wrote:
> > On 23/11/2023 18:43, Mark Brown wrote:
>
> > > > +static int wcd9390_interrupt_callback(struct sdw_slave *slave,
> > > > +				      struct sdw_slave_intr_status *status)
> > > > +{
> > > > +	struct wcd939x_sdw_priv *wcd = dev_get_drvdata(&slave->dev);
> > > > +	struct irq_domain *slave_irq = wcd->slave_irq;
> > > > +	u32 sts1, sts2, sts3;
> > > > +
> > > > +	do {
> > > > +		handle_nested_irq(irq_find_mapping(slave_irq, 0));
> > > > +		regmap_read(wcd->regmap, WCD939X_DIGITAL_INTR_STATUS_0, &sts1);
> > > > +		regmap_read(wcd->regmap, WCD939X_DIGITAL_INTR_STATUS_1, &sts2);
> > > > +		regmap_read(wcd->regmap, WCD939X_DIGITAL_INTR_STATUS_2, &sts3);
> > > > +
> > > > +	} while (sts1 || sts2 || sts3);
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
>
> > > We do this in the other Qualcomm drivers but it doesn't seem ideal to
> > > just ignore the interrupts.
>
> > It seems we simply ignore IRQs that are not mapped in the regmap_irq,
> > what would be the ideal way to handle this ?
>
> I don't understnad what "this" is here.  Why even register an interrupt
> handler here?  What is the regmap_irq you are referring to here and why
> would an interrupt handler here be related to it?

OK, I'm analyzing it, basically the Soundwire device interrupt event is
passed out-of-band and the device gets such interrupt_callback().

The codec part setups a linear single-irq domain where the regmap_irq is
attached, and this code generates irq events to the single-irq domain
thus triggering the regmap_irq chip handlers on the unique irq.

Seems the looping makes sure no interrupts were missed in the process,
we do not ignore interrupts in any case.

This design was introduced with wcd939x to make full usage or regmap_irq,
on previous drivers (wcd934x.c/wcd9335.c) it was manually done for slimbus.

> > > > +static int wcd939x_sdw_component_bind(struct device *dev, struct device *master,
> > > > +				      void *data)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void wcd939x_sdw_component_unbind(struct device *dev,
> > > > +					 struct device *master, void *data)
> > > > +{
> > > > +}
> > > > +
> > > > +static const struct component_ops wcd939x_sdw_component_ops = {
> > > > +	.bind = wcd939x_sdw_component_bind,
> > > > +	.unbind = wcd939x_sdw_component_unbind,
> > > > +};
>
> > > Do these need to be provided if they can legitimately be empty?
>
> > AFAIK yes, component code will crash if those are not defined.
> > I'll add a comment explaining whey they are no-op.
>
> If the framework can genuninely have empty callbacks here the framework
> should be updated to make the callbacks optional.

Sure, I'll add it to my todo list...

Thanks,
Neil