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.
> > +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.
> > + 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.
Thanks,
Charles
