Hi Mark, On Tue, 13 Nov 2018 at 23:51, Mark Brown <broonie@xxxxxxxxxx> wrote: > > On Thu, Nov 08, 2018 at 01:49:33PM +0100, Clément Péron wrote: > > This looks mostly good, a few small things below but nothing too major: > > > --- /dev/null > > +++ b/sound/soc/codecs/ak4118.c > > @@ -0,0 +1,449 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * ak4118.c -- Asahi Kasei ALSA Soc Audio driver > > + * > > Please make the entire comment a C++ one so this looks more intentional. I check from other files and also this article says : "For normal C source files, the string will be a comment using the "//" syntax; header files, instead, use traditional (/* */) comments for reasons related to tooling" https://lwn.net/Articles/739183/ > > > +static const char * const ak4118_iec958_fs_texts[] = { > > + "44100", > > + "Reserved", > > If you use a _VALUE_ENUM_SINGLE you can hide the reserved/invalid values > from userspace which is easier to use. Ok will change to SOC_VALUE_ENUM_SINGLE_DECL > > > + ret = request_threaded_irq(gpiod_to_irq(ak4118->irq), NULL, > > + ak4118_irq_handler, IRQF_TRIGGER_RISING | > > + IRQF_ONESHOT, "ak4118-irq", ak4118); > > + if (ret < 0) { > > + dev_err(component->dev, "Fail to request_irq: %d\n", ret); > > + return ret; > > + } > > You should request resources in the device level probe, there's no point > in requesting and releasing the interrupt like this. Ok, will change to devm_request_threaded_irq and remove the irq_free Thanks for the review, Regards, Clément _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
