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/submitting-patches.rst#L597 > > > > > > > + > > > +#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. -- Lee Jones [李琼斯]
