On 11/02/2025 22:16, Fred Treven wrote:
>>
>>> + if (!val)
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; i < len_words; i++)
>>> + val[i] = cpu_to_be32(buf[i]);
>>> +
>>> + ret = cs_dsp_coeff_write_ctrl(ctl, offset_words, val, len_words * sizeof(u32));
>>> + if (ret < 0)
>>> + dev_err(dsp->dev, "Failed to write FW control %s\n", name);
>>> +
>>> + kfree(val);
>>> +
>>> + return (ret < 0) ? ret : 0;
>>> +}
>>> +
>>> +inline int cs40l26_fw_write(struct cs_dsp *dsp, const char *const name, const unsigned int algo_id,
>>> + u32 val)
>>> +{
>>> + return cs40l26_fw_write_raw(dsp, name, algo_id, 0, 1, &val);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cs40l26_fw_write);
>>> +
>>> +static int cs40l26_fw_read_raw(struct cs_dsp *dsp, const char *const name,
>>> + const unsigned int algo_id, const unsigned int offset_words,
>>> + const size_t len_words, u32 *buf)
>>> +{
>>> + struct cs_dsp_coeff_ctl *ctl;
>>> + int i, ret;
>>> +
>>> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
>>> + if (!ctl) {
>>> + dev_err(dsp->dev, "Failed to find FW control %s\n", name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = cs_dsp_coeff_read_ctrl(ctl, offset_words, buf, len_words * sizeof(u32));
>>> + if (ret) {
>>> + dev_err(dsp->dev, "Failed to read FW control %s\n", name);
>>> + return ret;
>>> + }
>>> +
>>> + for (i = 0; i < len_words; i++)
>>> + buf[i] = be32_to_cpu(buf[i]);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +inline int cs40l26_fw_read(struct cs_dsp *dsp, const char *const name, const unsigned int algo_id,
>>
>> All your exported functions should have kerneldoc.
>
> I'm happy to add this, but I don't know where this directive comes from.
> Could you share where in the kernel style guide (or elsewhere) this is stated?
> There are also hundreds of examples in MFD in which exported functions
> do not have kerneldoc which is why I'm curious.
You are not looking hard enough. It's explicitly mentioned in kernel doc
documentation.
>
>>
>>> + u32 *buf)
>>> +{
>>> + return cs40l26_fw_read_raw(dsp, name, algo_id, 0, 1, buf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cs40l26_fw_read);
>>> +
>>> +static struct cs40l26_irq *cs40l26_get_irq(struct cs40l26 *cs40l26, const int num, const int bit);
>>> +
>>> +static int cs40l26_gpio1_rise_irq(void *data)
>>> +{
>>> + struct cs40l26 *cs40l26 = data;
>>> +
>>> + if (cs40l26->wksrc_sts & CS40L26_WKSRC_STS_EN)
>>> + dev_dbg(cs40l26->dev, "GPIO1 Rising Edge Detected\n");
>>> +
>>> + cs40l26->wksrc_sts |= CS40L26_WKSRC_STS_EN;
>>> +
>>> + return 0;
>>> +}
>>
>>
>> ...
>>
>>> +err:
>>> + dev_err(cs40l26->dev, "Invalid revision 0x%02X for device 0x%06X\n", cs40l26->revid,
>>> + cs40l26->devid);
>>> + return -EINVAL;
>>> +}
>>> +
>>> +int cs40l26_set_pll_loop(struct cs40l26 *cs40l26, const u32 pll_loop)
>>> +{
>>> + int i;
>>> +
>>> + /* Retry in case DSP is hibernating */
>>> + for (i = 0; i < CS40L26_PLL_NUM_SET_ATTEMPTS; i++) {
>>> + if (!regmap_update_bits(cs40l26->regmap, CS40L26_REFCLK_INPUT,
>>> + CS40L26_PLL_REFCLK_LOOP_MASK,
>>> + pll_loop << CS40L26_PLL_REFCLK_LOOP_SHIFT))
>>> + break;
>>> + }
>>> +
>>> + if (i == CS40L26_PLL_NUM_SET_ATTEMPTS) {
>>> + dev_err(cs40l26->dev, "Failed to configure PLL\n");
>>> + return -ETIMEDOUT;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(cs40l26_set_pll_loop);
>>> +
>>
>> This looks way past simple MFD driver. Not only this - entire file. You
>> configure there quite a lot and for example setting PLLs is not job for
>> MFD. This should be placed in appropriate subsystem.
>>
> I disagree here because the configuration being done in this file
> is essential to the core operation of the part. For instance,
> setting the PLL to open-loop here is required to prevent any
> external interference (e.g. GPIO events) from interrupting
> the part while loading firmware.
>
> The other hardware configuration being done here is required for
> both the Input and ASoC operations of the part.
>
> Lastly, these need to be done in order and independently of which
> child driver (ASoC or input) the user adds. If this is moved
> to cs40l26-vibra.c (the input driver), for instance,
> and that module is then not added, it will disturb the
> required setup for use by the ASoC driver.
>
> I would really like to get Lee's opinion here because it does not
> make sense to me why this is inappropriate when the configuration
> done in the core MFD driver is required for use by all of its
> children.
Sure.
...
>
>
>>
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, cs40l26_id_spi);
>>> +
>>> +static const struct of_device_id cs40l26_of_match[] = {
>>> + { .compatible = "cirrus,cs40l26a" },
>>> + { .compatible = "cirrus,cs40l27b" },
>>
>> So devices are compatible? Or rather this is unsynced with other ID table.
> I'm not sure what you mean by this.
Lack of driver data means devices are compatible or some sort of other
problem (e.g. ID tables not being in sync, because they are supposed to
be always in sync). Choose, but it is almost never correct. Either
correct the issue or mention why exception is justified.
Best regards,
Krzysztof
