On Tue, 11 Feb 2025, Fred Treven wrote:
On 2/5/25 04:34, Krzysztof Kozlowski wrote:
On 05/02/2025 00:18, Fred Treven wrote:
Introduce support for Cirrus Logic Device CS40L26: A boosted haptic driver with integrated DSP and waveform memory with advanced closed loop algorithms and LRA protection.
Please wrap commit message according to Linux coding style / submission process (neither too early nor over the limit): https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submi...
+#include <linux/cleanup.h> +#include <linux/mfd/core.h> +#include <linux/mfd/cs40l26.h> +#include <linux/property.h> +#include <linux/regulator/consumer.h>
+static const struct mfd_cell cs40l26_devs[] = {
- { .name = "cs40l26-codec", },
- { .name = "cs40l26-vibra", },
+};
+const struct regmap_config cs40l26_regmap = {
- .reg_bits = 32,
- .val_bits = 32,
- .reg_stride = 4,
- .reg_format_endian = REGMAP_ENDIAN_BIG,
- .val_format_endian = REGMAP_ENDIAN_BIG,
- .max_register = CS40L26_LASTREG,
- .cache_type = REGCACHE_NONE,
+}; +EXPORT_SYMBOL_GPL(cs40l26_regmap);
+static const char *const cs40l26_supplies[] = {
- "va", "vp",
+};
+inline void cs40l26_pm_exit(struct device *dev)
Exported function and inlined? This feels odd. Anyway, don't use any inline keywords in C units.
+{
- pm_runtime_mark_last_busy(dev);
- pm_runtime_put_autosuspend(dev);
+} +EXPORT_SYMBOL_GPL(cs40l26_pm_exit);
+static int cs40l26_fw_write_raw(struct cs_dsp *dsp, const char *const name,
const unsigned int algo_id, const u32 offset_words,const size_t len_words, u32 *buf)+{
- struct cs_dsp_coeff_ctl *ctl;
- __be32 *val;
- 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;- }
- val = kzalloc(len_words * sizeof(u32), GFP_KERNEL);
Looks like an array, so kcalloc
- 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.
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.
FWIW, I agree with Krzysztof.
There's a bunch of functionality in here that should be exported out to leaf drivers which should reside in their associated subsystems. From just a quick glance that looks to include, but not necessary limited to; IRQs, GPIOs and PLLs (Clocks).
MFD has been used for a dumping ground under the premise of "core functionality" before. Tolerance for those arguments are now fairly low.