Re: [PATCH v5 3/5] mfd: cs40l50: Add support for CS40L50 core driver

[James Ogletree <James.Ogletree@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxx> 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