Hi Charles,
On Mon, Mar 11, 2024 at 10:30:53AM +0000, Charles Keepax wrote:
> On Sun, Mar 10, 2024 at 06:40:04PM -0500, Jeff LaBundy wrote:
> > On Fri, Mar 08, 2024 at 10:24:19PM +0000, James Ogletree wrote:
> > > +static void cs40l50_start_dsp(const struct firmware *bin, void *context)
> > > +{
> > > + struct cs40l50 *cs40l50 = context;
> > > + int err;
> > > +
> > > + /* Wavetable is optional; start DSP regardless */
> > > + cs40l50->bin = bin;
> > > +
> > > + mutex_lock(&cs40l50->lock);
> >
> > It seems the mutex is used only to prevent interrupt handling while the DSP
> > is being configured; can't we just call disable_irq() and enable_irq() here?
> >
>
> The trouble with diabling the IRQ is it masks the IRQ line. That
> means if the IRQ is shared with other devices you will be
> silencing their IRQs for the duration as well, which is slightly
> rude. Especially given the driver requests the IRQ with the
> SHARED flag I would be inclinded to stick with the mutex unless
> there are other reasons not to.
Ah, yes; I see we're using IRQF_SHARED here. Shared interrupts seem
like they made more sense for L+R audio amplifiers, and less for a
haptic driver; but if customers are indeed sharing this interrupt,
then the mutex is OK.
>
> > > +static int cs40l50_irq_init(struct cs40l50 *cs40l50)
> > > +{
> > > + struct device *dev = cs40l50->dev;
> > > + int err, i, virq;
> > > +
> > > + err = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> > > + IRQF_ONESHOT | IRQF_SHARED, 0,
> > > + &cs40l50_irq_chip, &cs40l50->irq_data);
> > > + if (err) {
> > > + dev_err(dev, "Failed adding IRQ chip\n");
> >
> > I don't see any need for individual prints in this function, since the call
> > to cs40l50_irq_init() is already followed by a call to dev_err_probe().
>
> I would probably suggest keeping these individual prints here and
> removing the one in cs40l50_probe, it is nicer to know exactly
> what failed when something goes wrong. That said at least the
> devm_request_threaded_irq can probe defer so that print should be
> a dev_err_probe since this function is only called on the probe
> path.
Makes sense to me.
>
> > > + dev_info(cs40l50->dev, "Cirrus Logic CS40L50 rev. %02X\n", cs40l50->revid);
> >
> > This should be dev_dbg().
> >
>
> Not sure what the concenus is on this, but personally I greatly
> prefer these device ID prints being infos. Nice to always have
> some indication of the device and its version.
Right, but every other silicon vendor feels the same way too, and many
customers' dmesg is typically quite messy for the first couple minutes.
This is only one print, but my stance is that this type of information
belongs in debugfs; in fact for this case, we can get this information
from regmap if we know what we're looking for.
>
> Thanks,
> Charles
Kind regards,
Jeff LaBundy
