Hi Charles,
Thank you for your excellent review. Anything not replied to will be adopted as-is in the next version.
On Jan 5, 2024, at 8:04 AM, Charles Keepax ckeepax@opensource.cirrus.com wrote:
On Thu, Jan 04, 2024 at 10:36:36PM +0000, James Ogletree wrote
+static int cs40l50_dsp_init(struct cs40l50 *cs40l50) +{
- int err;
- cs40l50->dsp.num = 1;
- cs40l50->dsp.type = WMFW_HALO;
- cs40l50->dsp.dev = cs40l50->dev;
- cs40l50->dsp.regmap = cs40l50->regmap;
- cs40l50->dsp.base = CS40L50_CORE_BASE;
- cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
- cs40l50->dsp.mem = cs40l50_dsp_regions;
- cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
- cs40l50->dsp.no_core_startstop = true;
- err = cs_dsp_halo_init(&cs40l50->dsp);
- if (err)
- return err;
- return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove,
- &cs40l50->dsp);
Hmm... I notice you use this for both dsp_remove and dsp_power_down. Are you sure devm will guarantee those are called in the right order? Its not immediately clear to me that would be have to be the case.
On my inspection of the devm code, actions are always added to the tail, and played back from head to tail on driver detach.
+static int cs40l50_power_up_dsp(struct cs40l50 *cs40l50) +{
- int err;
- mutex_lock(&cs40l50->lock);
- if (cs40l50->patch) {
- /* Stop core if loading patch file */
- err = regmap_multi_reg_write(cs40l50->regmap, cs40l50_stop_core,
ARRAY_SIZE(cs40l50_stop_core));- if (err)
- goto err_mutex;
- }
- err = cs_dsp_power_up(&cs40l50->dsp, cs40l50->patch, "cs40l50.wmfw",
cs40l50->bin, "cs40l50.bin", "cs40l50");- if (err)
- goto err_mutex;
- err = devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_power_down,
&cs40l50->dsp);- if (err)
- goto err_mutex;
- if (cs40l50->patch) {
- /* Resume core after loading patch file */
- err = regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
- CS40L50_CLOCK_ENABLE);
This feels like this needs a comment, why are we skipping the normal DSP init and doing it manually (this appears to be the same writes start_core would have done)? I assume its something to do with what you are really doing is you don't want lock_memory to run?
The dsp struct uses cs_dsp_halo_ao_ops, made for self-booting DSPs, which has none of the ops used in cs_dsp_run(). The manual stop is because it is self-booting (already running you could say) but we need to stop the clock to patch the firmware. Please correct me if that is not right.
+static int cs40l50_configure_dsp(struct cs40l50 *cs40l50) +{
- u32 nwaves;
- int err;
- if (cs40l50->bin) {
- /* Log number of effects if wavetable was loaded */
- err = regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves);
- if (err)
- return err;
- dev_info(cs40l50->dev, "Loaded with %u RAM waveforms\n", nwaves);
Kinda nervous about the fact we access all these DSP controls directly through address, rather than using the DSP control accessors, we have the accessors for a reason. They manage things like access permissions etc. and historically, the firmware guys have not been able to guarantee these remain in consistent locations between firmware versions.
I guess this is so you can access them even in the case of the ROM firmware, but you could have a meta-data only firmware file that you load in that case to give you the controls. I don't feel the need to NAK the driver based on this but please think about this very carefully it's a strange way to use the DSP controls, and feels likely to cause problems to me. It is also quite hostile to fixing it in the future since as you are not using the controls no one will be checking that things like the access flags in the firmware are set correctly, which is annoying if the decision has to be reversed later since there will likely be a bunch of broken firmwares already in the field.
Noting here that we discussed this offline. The driver is going to stay with a static register design for now, but the write sequence interface is being modified to be control based.
Best, James