Re: [alsa-devel] [PATCH 1/2] ASoC: ak4118: Add support for AK4118 S/PDIF transceiver

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.

+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.

• 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.