Re: [PATCH 4/5] Input: cs40l26 - Load multiple coefficient files

[Jeff LaBundy]

Hi Fred,

On Thu, May 25, 2023 at 07:04:30PM -0500, Fred Treven wrote:
> Enable the driver to load all necessary coefficient tuning
> files:	cs40l26.bin (wavetable)
> 	cs40l26-svc.bin (Sensorless Velocity Control)
> 	cs40l26-dvl.bin (Dynamic Voltage Limiter)
> 
> Signed-off-by: Fred Treven <fred.treven@xxxxxxxxxx>
> ---

Thanks for sending; glad to see this series continue to move along. Just a
few notes about best practice for reviews:

1. For the purpose of mainline submission, patches 2 and 4 must be squashed,
and then come after their dependency (patch 3 in this case). Otherwise, we
end up reviewing code that subsequently disappears or changes in a subsequent
patch.

2. All patches in the same thread must have the same version number (e.g. v2).
If a patch was not present in a previous iteration, the change log can simply
say so. Speaking of which...

3. Please include a change log below the '---' so that it is clear what has
changed since the last review.

4. This is a matter of personal taste, but for a new driver that touches a
few different areas such as this, it's nice to have a cover letter to explain
the general layout and motivation. This is also a good place to put a change
log.

>  drivers/input/misc/cs40l26.c  | 31 +++++++++++++++++++++++--------
>  include/linux/input/cs40l26.h |  3 ++-
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/misc/cs40l26.c b/drivers/input/misc/cs40l26.c
> index 1959438dfe31..12c29cbd4ff0 100644
> --- a/drivers/input/misc/cs40l26.c
> +++ b/drivers/input/misc/cs40l26.c
> @@ -2006,6 +2006,12 @@ static const struct cs_dsp_client_ops cs40l26_cs_dsp_client_ops = {
>  	.post_run		= cs40l26_cs_dsp_post_run,
>  };
>  
> +static struct cs_dsp_coeff_desc cs40l26_coeffs[] = {
> +	{ .coeff_firmware = NULL,	.coeff_filename = "cs40l26.bin",	.optional = false },
> +	{ .coeff_firmware = NULL,	.coeff_filename =  "cs40l26-svc.bin",	.optional = true },
> +	{ .coeff_firmware = NULL,	.coeff_filename = "cs40l26-dvl.bin",	.optional = true },
> +};
> +
>  static int cs40l26_cs_dsp_init(struct cs40l26_private *cs40l26)
>  {
>  	int error;
> @@ -2035,14 +2041,16 @@ static int cs40l26_cs_dsp_init(struct cs40l26_private *cs40l26)
>  static void cs40l26_dsp_start(struct cs40l26_private *cs40l26)
>  {
>  	struct device *dev = cs40l26->dev;
> +	int i;
>  
>  	if (cs40l26_dsp_pre_config(cs40l26))
>  		goto err_rls_fw;
>  
>  	mutex_lock(&cs40l26->lock);
>  
> -	if (cs_dsp_power_up(&cs40l26->dsp, cs40l26->wmfw, "cs40l26.wmfw",
> -				cs40l26->bin, "cs40l26.bin", "cs40l26"))
> +	if (cs_dsp_power_up_multiple(&cs40l26->dsp, cs40l26->wmfw, "cs40l26.wmfw",
> +					cs40l26_coeffs, CS40L26_NUM_COEFF_FILES,
> +					"cs40l26"))
>  		goto err_mutex;
>  
>  	if (cs40l26->dsp.fw_id != CS40L26_FW_ID) {
> @@ -2062,7 +2070,9 @@ static void cs40l26_dsp_start(struct cs40l26_private *cs40l26)
>  	mutex_unlock(&cs40l26->lock);
>  
>  err_rls_fw:
> -	release_firmware(cs40l26->bin);
> +	for (i = 0; i < CS40L26_NUM_COEFF_FILES; i++)
> +		release_firmware(cs40l26_coeffs[i].coeff_firmware);
> +
>  	release_firmware(cs40l26->wmfw);
>  
>  	cs40l26_pm_runtime_setup(cs40l26);
> @@ -2074,17 +2084,20 @@ static void cs40l26_coeff_upload(const struct firmware *bin, void *context)
>  
>  	if (!bin) {
>  		dev_err(cs40l26->dev, "Failed to request coefficient file\n");
> +		cs40l26->ncoeffs++;
>  		return;
>  	}
>  
> -	cs40l26->bin = bin;
> +	cs40l26_coeffs[cs40l26->ncoeffs++].coeff_firmware = bin;
>  
> -	cs40l26_dsp_start(cs40l26);
> +	if (cs40l26->ncoeffs == CS40L26_NUM_COEFF_FILES)
> +		cs40l26_dsp_start(cs40l26);
>  }
>  
>  static void cs40l26_fw_upload(const struct firmware *wmfw, void *context)
>  {
>  	struct cs40l26_private *cs40l26 = (struct cs40l26_private *)context;
> +	int i;
>  
>  	if (!wmfw) {
>  		dev_err(cs40l26->dev, "Failed to request firmware file\n");
> @@ -2098,9 +2111,11 @@ static void cs40l26_fw_upload(const struct firmware *wmfw, void *context)
>  
>  	cs40l26->wmfw = wmfw;
>  
> -	request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> -				"cs40l26.bin", cs40l26->dev, GFP_KERNEL,
> -				cs40l26, cs40l26_coeff_upload);
> +	for (i = 0; i < CS40L26_NUM_COEFF_FILES; i++)
> +		request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> +					"cs40l26.bin", cs40l26->dev,
> +					GFP_KERNEL, cs40l26,
> +					cs40l26_coeff_upload);
>  }
>  
>  static inline int cs40l26_worker_init(struct cs40l26_private *cs40l26)
> diff --git a/include/linux/input/cs40l26.h b/include/linux/input/cs40l26.h
> index bc0acec9cf23..2f74ef61b952 100644
> --- a/include/linux/input/cs40l26.h
> +++ b/include/linux/input/cs40l26.h
> @@ -231,6 +231,7 @@
>  /* Firmware Handling */
>  #define CS40L26_FW_ID				0x1800D4
>  #define CS40L26_FW_ID_VERSION_MIN		0x070237
> +#define CS40L26_NUM_COEFF_FILES			3
>  
>  /* Algorithms */
>  #define CS40L26_BUZZGEN_ALGO_ID			0x0004F202
> @@ -473,8 +474,8 @@ struct cs40l26_private {
>  	unsigned int vbst_uv;
>  	bool exploratory_mode_enabled;
>  	const struct firmware *wmfw;
> -	const struct firmware *bin;
>  	bool dsp_initialized;
> +	int ncoeffs;
>  };
>  
>  int cs40l26_probe(struct cs40l26_private *cs40l26);
> -- 
> 2.7.4
> 

Kind regards,
Jeff LaBundy