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
