| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50f8e238-c9f5-4af2-adec-deee98dbb5e4@kernel.org>
Date: Wed, 12 Feb 2025 07:19:58 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Fred Treven <ftreven@...nsource.cirrus.com>, Lee Jones <lee@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Simon Trimmer <simont@...nsource.cirrus.com>,
Charles Keepax <ckeepax@...nsource.cirrus.com>,
Richard Fitzgerald <rf@...nsource.cirrus.com>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
James Ogletree <jogletre@...nsource.cirrus.com>,
Ben Bright <ben.bright@...rus.com>, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, David Rhodes <david.rhodes@...rus.com>,
Jeff LaBundy <jeff@...undy.com>, Heiko Stuebner <heiko@...ech.de>,
Karel Balej <balejk@...fyz.cz>, Igor Prusov <ivprusov@...utedevices.com>,
Jack Yu <jack.yu@...ltek.com>, Weidong Wang <wangweidong.a@...nic.com>,
Binbin Zhou <zhoubinbin@...ngson.cn>,
Prasad Kumpatla <quic_pkumpatl@...cinc.com>,
Paul Handrigan <paulha@...nsource.cirrus.com>,
Masahiro Yamada <masahiroy@...nel.org>, Nuno Sa <nuno.sa@...log.com>
Cc: alsa-devel@...a-project.org, patches@...nsource.cirrus.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-input@...r.kernel.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH RESEND 5/7] mfd: cs40l26: Add support for CS40L26 core
driver
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
Powered by blists - more mailing lists