Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 24 May 2020 08:59:22 +0200
Message-ID: <s5hftbpx1h1.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Vasily Khoruzhick <anarsoul@gmail.com>
Cc:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Allison Randal <allison@lohutok.net>,
        Pavel Machek <pavel@denx.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Kai-Heng Feng <kai.heng.feng@canonical.com>,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] ALSA: line6: add hw monitor volume control for POD HD500
In-Reply-To: <20200523174957.6294-1-anarsoul@gmail.com>
References: <20200523174957.6294-1-anarsoul@gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, 23 May 2020 19:49:57 +0200,
Vasily Khoruzhick wrote:
> 
> Add hw monitor volume control for POD HD500. The same change may
> work for HD500X but I don't have it to test.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> v2: clamp volume value to [0, ARRAY_SIZE() -1] in
>     podhd_set_monitor_level()

Thanks for the patch.
The basic implementation looks good, but I see a few issues.
Below are the comments:

> @@ -132,6 +132,7 @@ static int line6_send_raw_message(struct usb_line6 *line6, const char *buffer,
>  
>  	return done;
>  }
> +EXPORT_SYMBOL(line6_send_raw_message);

Let's use EXPORT_SYMBOL_GPL() instead.


> +static const unsigned int float_zero_to_one_lookup[] = {
> +0x00000000, 0x3C23D70A, 0x3CA3D70A, 0x3CF5C28F, 0x3D23D70A, 0x3D4CCCCD,
> +0x3D75C28F, 0x3D8F5C29, 0x3DA3D70A, 0x3DB851EC, 0x3DCCCCCD, 0x3DE147AE,
> +0x3DF5C28F, 0x3E051EB8, 0x3E0F5C29, 0x3E19999A, 0x3E23D70A, 0x3E2E147B,
> +0x3E3851EC, 0x3E428F5C, 0x3E4CCCCD, 0x3E570A3D, 0x3E6147AE, 0x3E6B851F,
> +0x3E75C28F, 0x3E800000, 0x3E851EB8, 0x3E8A3D71, 0x3E8F5C29, 0x3E947AE1,
> +0x3E99999A, 0x3E9EB852, 0x3EA3D70A, 0x3EA8F5C3, 0x3EAE147B, 0x3EB33333,
> +0x3EB851EC, 0x3EBD70A4, 0x3EC28F5C, 0x3EC7AE14, 0x3ECCCCCD, 0x3ED1EB85,
> +0x3ED70A3D, 0x3EDC28F6, 0x3EE147AE, 0x3EE66666, 0x3EEB851F, 0x3EF0A3D7,
> +0x3EF5C28F, 0x3EFAE148, 0x3F000000, 0x3F028F5C, 0x3F051EB8, 0x3F07AE14,
> +0x3F0A3D71, 0x3F0CCCCD, 0x3F0F5C29, 0x3F11EB85, 0x3F147AE1, 0x3F170A3D,
> +0x3F19999A, 0x3F1C28F6, 0x3F1EB852, 0x3F2147AE, 0x3F23D70A, 0x3F266666,
> +0x3F28F5C3, 0x3F2B851F, 0x3F2E147B, 0x3F30A3D7, 0x3F333333, 0x3F35C28F,
> +0x3F3851EC, 0x3F3AE148, 0x3F3D70A4, 0x3F400000, 0x3F428F5C, 0x3F451EB8,
> +0x3F47AE14, 0x3F4A3D71, 0x3F4CCCCD, 0x3F4F5C29, 0x3F51EB85, 0x3F547AE1,
> +0x3F570A3D, 0x3F59999A, 0x3F5C28F6, 0x3F5EB852, 0x3F6147AE, 0x3F63D70A,
> +0x3F666666, 0x3F68F5C3, 0x3F6B851F, 0x3F6E147B, 0x3F70A3D7, 0x3F733333,
> +0x3F75C28F, 0x3F7851EC, 0x3F7AE148, 0x3F7D70A4, 0x3F800000

Just nitpick: better to align with lower hex letters (a-f).


> +};
> +
> +static void podhd_set_monitor_level(struct usb_line6_podhd *podhd, int value)
> +{
> +	unsigned int fl;
> +	static const unsigned char msg[16] = {
> +		/* Chunk is 0xc bytes (without first word) */
> +		0x0c, 0x00,
> +		/* First chunk in the message */
> +		0x01, 0x00,
> +		/* Message size is 2 4-byte words */
> +		0x02, 0x00,
> +		/* Unknown */
> +		0x04, 0x41,
> +		/* Unknown */
> +		0x04, 0x00, 0x13, 0x00,
> +		/* Volume, LE float32, 0.0 - 1.0 */
> +		0x00, 0x00, 0x00, 0x00
> +	};
> +	unsigned char *buf;
> +
> +	buf = kmalloc(sizeof(msg), GFP_ATOMIC);

Is this function called from the irq context like the urb callback?
I saw that it's called from the mixer put callback, and it's in the
normal sleepable context, hence GFP_KERNEL can be used safely.


thanks,

Takashi