Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 May 2022 13:57:07 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Takashi Iwai <tiwai@suse.de>
CC:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, Mark Brown <broonie@kernel.org>,
        <alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
        <linux-kernel@vger.kernel.org>,
        Stefan Binding <sbinding@opensource.cirrus.com>
Subject: Re: [PATCH v2 10/26] ALSA: hda: hda_cs_dsp_ctl: Add Library to
 support CS_DSP ALSA controls
Message-ID: <20220511135707.GH38351@ediswmail.ad.cirrus.com>
References: <20220509214703.4482-1-vitalyr@opensource.cirrus.com>
 <20220509214703.4482-11-vitalyr@opensource.cirrus.com>
 <s5hzgjofacn.wl-tiwai@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <s5hzgjofacn.wl-tiwai@suse.de>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Wed, May 11, 2022 at 03:49:12PM +0200, Takashi Iwai wrote:
> On Mon, 09 May 2022 23:46:47 +0200,
> Vitaly Rodionov wrote:
> > 
> > From: Stefan Binding <sbinding@opensource.cirrus.com>
> > 
> > The cs35l41 part contains a DSP which is able to run firmware.
> > The cs_dsp library can be used to control the DSP.
> > These controls can be exposed to userspace using ALSA controls.
> > This library adds apis to be able to interface between
> > cs_dsp and hda drivers and expose the relevant controls as
> > ALSA controls.
> 
> Hmm, quite lots of things aren't explained here.
> 
> First off, as far as I see, the control elements that are implemented
> in this patch are pretty unique, they don't follow the standard way.
> Admittedly, ASoC core (ab)uses the TLV read/write for the arbitrary
> data bytes, and this seems following that instead.  If so, it needs
> more clear explanation in the comments add/or commit logs.
> 

Hm... yes I think we should be very clear on if the TLV support
is needed here. This should only be needed if the firmware has
controls (and we need to use those controls) over 512 bytes, and
most firmwares don't. Can we check this and remove the TLV
support if we can? We probably don't want to add more users of
that stuff if we can help it.

Thanks,
Charles