On 12/05/2023 14:28, Charles Keepax wrote:
The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed for portable applications. It provides a high dynamic range, stereo DAC for headphone output, two integrated Class D amplifiers for loudspeakers, and two ADCs for wired headset microphone input or stereo line input. PDM inputs are provided for digital microphones.
The MFD component registers and initialises the device and provides PM/system power management.
Thank you for your patch. There is something to discuss/improve.
+static const char * const cs42l43_core_supplies[] = {
- "VDD_A", "VDD_IO", "VDD_CP",
+};
+static const char * const cs42l43_parent_supplies[] = { "VDD_AMP" };
+static const struct mfd_cell cs42l43_devs[] = {
- { .name = "cs42l43-pinctrl", },
- { .name = "cs42l43-irq", },
- { .name = "cs42l43-spi", },
- {
.name = "cs42l43-codec",
.parent_supplies = cs42l43_parent_supplies,
.num_parent_supplies = ARRAY_SIZE(cs42l43_parent_supplies),
- },
+};
+static int cs42l43_soft_reset(struct cs42l43 *cs42l43) +{
- static const struct reg_sequence reset[] = {
{ CS42L43_SFT_RESET, 0x5A000000 },
- };
- unsigned long time;
- dev_dbg(cs42l43->dev, "Soft resetting\n");
Drop simple debug statements for function entry/exit. There are other tools in kernel to do such debugging.
- reinit_completion(&cs42l43->device_detach);
- /* apply cache only as the device will also fall off the soundwire bus */
- regcache_cache_only(cs42l43->regmap, true);
- regmap_multi_reg_write_bypassed(cs42l43->regmap, reset, ARRAY_SIZE(reset));
- msleep(20);
- if (cs42l43->sdw) {
time = wait_for_completion_timeout(&cs42l43->device_detach,
msecs_to_jiffies(100));
if (!time) {
dev_err(cs42l43->dev, "Timed out waiting for device detach\n");
return -ETIMEDOUT;
}
- }
- return -EAGAIN;
+}
+static int cs42l43_wait_for_attach(struct cs42l43 *cs42l43) +{
- unsigned long time;
- if (!cs42l43->attached) {
time = wait_for_completion_timeout(&cs42l43->device_attach,
msecs_to_jiffies(500));
if (!time) {
dev_err(cs42l43->dev, "Timed out waiting for device re-attach\n");
return -ETIMEDOUT;
}
- }
- regcache_cache_only(cs42l43->regmap, false);
- // Must enable OSC_DIV before doing any SoundWire reads
- if (cs42l43->sdw)
regmap_write(cs42l43->regmap, CS42L43_OSC_DIV_SEL, 0x1);
- return 0;
+}
+static int cs42l43_mcu_stage_2_3(struct cs42l43 *cs42l43, bool shadow) +{
- unsigned int need_reg = CS42L43_NEED_CONFIGS;
- unsigned int val;
- int ret;
- dev_dbg(cs42l43->dev, "Moving firmware to stage 3\n");
Drop simple debug statements for function entry/exit. There are other tools in kernel to do such debugging.
- if (shadow)
need_reg = CS42L43_FW_SH_BOOT_CFG_NEED_CONFIGS;
- regmap_write(cs42l43->regmap, need_reg, 0x0);
- ret = regmap_read_poll_timeout(cs42l43->regmap, CS42L43_BOOT_STATUS,
val, (val == 3), 5000, 20000);
- if (ret) {
dev_err(cs42l43->dev, "Failed to move to stage 3: %d, 0x%x\n", ret, val);
return ret;
- }
- return -EAGAIN;
+}
+static int cs42l43_mcu_stage_3_2(struct cs42l43 *cs42l43) +{
- dev_dbg(cs42l43->dev, "Returning firmware to stage 2\n");
- regmap_write(cs42l43->regmap, CS42L43_FW_CTRL_NEED_CONFIGS,
CS42L43_FW_PATCH_NEED_CFG_MASK);
- regmap_write(cs42l43->regmap, CS42L43_FW_CTRL_HAVE_CONFIGS, 0x0);
- return cs42l43_soft_reset(cs42l43);
+}
+static int cs42l43_mcu_disable(struct cs42l43 *cs42l43) +{
- unsigned int val;
- int ret;
- dev_dbg(cs42l43->dev, "Disabling firmware\n");
Drop simple debug statements for function entry/exit. There are other tools in kernel to do such debugging.
- regmap_write(cs42l43->regmap, CS42L43_FW_CTRL_MM_MCU_CFG_REG, 0xF05AA50F);
- regmap_write(cs42l43->regmap, CS42L43_FW_CTRL_MM_CTRL_SELECTION, 0x1);
- regmap_write(cs42l43->regmap, CS42L43_MCU_SW_INTERRUPT, CS42L43_CONTROL_IND_MASK);
- regmap_write(cs42l43->regmap, CS42L43_MCU_SW_INTERRUPT, 0);
- ret = regmap_read_poll_timeout(cs42l43->regmap, CS42L43_SOFT_INT_SHADOW, val,
(val & CS42L43_CONTROL_APPLIED_INT_MASK),
5000, 20000);
- if (ret) {
dev_err(cs42l43->dev, "Failed to disable firmware: %d, 0x%x\n", ret, val);
return ret;
- }
- /* Soft reset to clear any register state the firmware left behind */
- return cs42l43_soft_reset(cs42l43);
+}
+struct cs42l43_patch_header {
- __le16 version;
- __le16 size;
- u8 reserved;
- u8 secure;
- __le16 bss_size;
- __le32 apply_addr;
- __le32 checksum;
- __le32 sha;
- __le16 swrev;
- __le16 patchid;
- __le16 ipxid;
- __le16 romver;
- __le32 load_addr;
+} __packed;
Put all structs together at the top.
+static void cs42l43_mcu_load_firmware(const struct firmware *firmware, void *context) +{
- struct cs42l43 *cs42l43 = context;
- struct cs42l43_patch_header *hdr;
- unsigned int loadaddr, val;
- int ret;
- if (!firmware) {
dev_err(cs42l43->dev, "Failed to load firmware\n");
cs42l43->firmware_error = -ENODEV;
goto err;
- }
- dev_dbg(cs42l43->dev, "Updating firmware\n");
Drop simple debug statements for function entry/exit. There are other tools in kernel to do such debugging.
- hdr = (void *)&firmware->data[0];
Aren't you dropping here const? Why? That's not recommended programming.
- loadaddr = le32_to_cpu(hdr->load_addr);
- if (le16_to_cpu(hdr->version) != 0x3) {
dev_err(cs42l43->dev, "Bad firmware file format: %d\n", hdr->version);
cs42l43->firmware_error = -EINVAL;
goto err_release;
- }
- regmap_write(cs42l43->regmap, CS42L43_PATCH_START_ADDR, loadaddr);
- regmap_bulk_write(cs42l43->regmap, loadaddr + 0x100000,
&firmware->data[0], firmware->size / sizeof(u32));
- regmap_write(cs42l43->regmap, CS42L43_MCU_SW_INTERRUPT, CS42L43_PATCH_IND_MASK);
- regmap_write(cs42l43->regmap, CS42L43_MCU_SW_INTERRUPT, 0);
- ret = regmap_read_poll_timeout(cs42l43->regmap, CS42L43_SOFT_INT_SHADOW, val,
(val & CS42L43_PATCH_APPLIED_INT_MASK),
5000, 500000);
- if (ret) {
dev_err(cs42l43->dev, "Failed to update firmware: %d, 0x%x\n", ret, val);
cs42l43->firmware_error = ret;
goto err_release;
- }
+err_release:
- release_firmware(firmware);
+err:
- complete(&cs42l43->firmware_download);
+}
+static int cs42l43_mcu_update_step(struct cs42l43 *cs42l43) +{
- unsigned int mcu_rev, bios_rev, boot_status, secure_cfg;
- bool patched, shadow;
- int ret;
- // Clear any stale software interrupt bits
- regmap_read(cs42l43->regmap, CS42L43_SOFT_INT, &mcu_rev);
- ret = regmap_read(cs42l43->regmap, CS42L43_BOOT_STATUS, &boot_status);
- if (ret) {
dev_err(cs42l43->dev, "Failed to read boot status: %d\n", ret);
return ret;
- }
- ret = regmap_read(cs42l43->regmap, CS42L43_MCU_SW_REV, &mcu_rev);
- if (ret) {
dev_err(cs42l43->dev, "Failed to read firmware revision: %d\n", ret);
return ret;
- }
- bios_rev = ((mcu_rev & CS42L43_BIOS_MAJOR_REV_MASK) << 12) |
((mcu_rev & CS42L43_BIOS_MINOR_REV_MASK) << 4) |
((mcu_rev & CS42L43_BIOS_SUBMINOR_REV_MASK) >> 8);
- mcu_rev = ((mcu_rev & CS42L43_FW_MAJOR_REV_MASK) << 12) |
((mcu_rev & CS42L43_FW_MINOR_REV_MASK) << 4) |
((mcu_rev & CS42L43_FW_SUBMINOR_REV_MASK) >> 8);
- patched = mcu_rev >= 0x2105 || bios_rev > 0x0000;
- shadow = mcu_rev >= 0x2200;
- ret = regmap_read(cs42l43->regmap, CS42L43_BOOT_CONTROL, &secure_cfg);
- if (ret) {
dev_err(cs42l43->dev, "Failed to read security settings: %d\n", ret);
return ret;
- }
- cs42l43->hw_lock = secure_cfg & CS42L43_LOCK_HW_STS_MASK;
- if (!patched && cs42l43->hw_lock) {
dev_err(cs42l43->dev, "Unpatched secure device\n");
return -EPERM;
- }
- dev_dbg(cs42l43->dev, "Firmware(0x%x) in boot stage %d\n", mcu_rev, boot_status);
- switch (boot_status) {
- case 2:
if (!patched) {
ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
"cs42l43.bin", cs42l43->dev,
GFP_KERNEL, cs42l43,
cs42l43_mcu_load_firmware);
if (ret) {
dev_err(cs42l43->dev, "Failed to request firmware: %d\n", ret);
return ret;
}
wait_for_completion(&cs42l43->firmware_download);
if (cs42l43->firmware_error)
return cs42l43->firmware_error;
return -EAGAIN;
} else {
return cs42l43_mcu_stage_2_3(cs42l43, shadow);
}
- case 3:
if (patched)
return cs42l43_mcu_disable(cs42l43);
else
return cs42l43_mcu_stage_3_2(cs42l43);
- case 4:
return 0;
- default:
dev_err(cs42l43->dev, "Invalid boot status: %d\n", boot_status);
return -EINVAL;
- }
+}
+static int cs42l43_mcu_update(struct cs42l43 *cs42l43) +{
- const int update_retries = 5;
- int i, ret;
- for (i = 0; i < update_retries; i++) {
ret = cs42l43_mcu_update_step(cs42l43);
if (ret != -EAGAIN)
return ret;
ret = cs42l43_wait_for_attach(cs42l43);
if (ret)
return ret;
- }
- dev_err(cs42l43->dev, "Failed retrying update\n");
- return -ETIMEDOUT;
+}
+static void cs42l43_boot_work(struct work_struct *work) +{
- struct cs42l43 *cs42l43 = container_of(work, struct cs42l43, boot_work);
- unsigned int devid, revid, otp;
- int ret;
- dev_dbg(cs42l43->dev, "Boot work running\n");
Drop simple debug statements for function entry/exit. There are other tools in kernel to do such debugging.
- ret = cs42l43_wait_for_attach(cs42l43);
- if (ret)
goto err;
- if (cs42l43->sdw)
cs42l43->irq = cs42l43->sdw->irq;
- ret = regmap_read(cs42l43->regmap, CS42L43_DEVID, &devid);
- if (ret) {
dev_err(cs42l43->dev, "Failed to read devid: %d\n", ret);
goto err;
- }
- switch (devid) {
- case 0x42a43:
break;
- default:
dev_err(cs42l43->dev, "Unrecognised devid: 0x%06x\n", devid);
goto err;
- }
- ret = regmap_read(cs42l43->regmap, CS42L43_REVID, &revid);
- if (ret) {
dev_err(cs42l43->dev, "Failed to read rev: %d\n", ret);
goto err;
- }
- ret = regmap_read(cs42l43->regmap, CS42L43_OTP_REVISION_ID, &otp);
- if (ret) {
dev_err(cs42l43->dev, "Failed to read otp rev: %d\n", ret);
goto err;
- }
- dev_info(cs42l43->dev,
"devid: 0x%06x, rev: 0x%02x, otp: 0x%02x\n", devid, revid, otp);
- ret = cs42l43_mcu_update(cs42l43);
- if (ret)
goto err;
- ret = regmap_register_patch(cs42l43->regmap, cs42l43_reva_patch,
ARRAY_SIZE(cs42l43_reva_patch));
- if (ret) {
dev_err(cs42l43->dev, "Failed to apply register patch: %d\n", ret);
goto err;
- }
- pm_runtime_mark_last_busy(cs42l43->dev);
- pm_runtime_put_autosuspend(cs42l43->dev);
- ret = devm_mfd_add_devices(cs42l43->dev, PLATFORM_DEVID_NONE,
cs42l43_devs, ARRAY_SIZE(cs42l43_devs),
I don't why adding devices is not in probe. They use the same regmap right? So there will be no problem in probing them from MFD probe.
NULL, 0, NULL);
- if (ret) {
dev_err(cs42l43->dev, "Failed to add subdevices: %d\n", ret);
goto err;
- }
- dev_dbg(cs42l43->dev, "Successfully initialised\n");
Drop simple debug statements for function entry/exit. There are other tools in kernel to do such debugging.
- return;
+err:
- pm_runtime_put_sync(cs42l43->dev);
- cs42l43_dev_remove(cs42l43);
+}
+static int cs42l43_power_up(struct cs42l43 *cs42l43) +{
- int ret;
- ret = regulator_enable(cs42l43->vdd_p);
- if (ret) {
dev_err(cs42l43->dev, "Failed to enable VDD_P: %d\n", ret);
return ret;
- }
- usleep_range(50, 100); /* VDD_P must be on for 50uS before any other supply */
- gpiod_set_value_cansleep(cs42l43->reset, 1);
- ret = regulator_bulk_enable(CS42L43_N_SUPPLIES, cs42l43->core_supplies);
- if (ret) {
dev_err(cs42l43->dev, "Failed to enable core supplies: %d\n", ret);
goto err_reset;
- }
- ret = regulator_enable(cs42l43->vdd_d);
- if (ret) {
dev_err(cs42l43->dev, "Failed to enable VDD_D: %d\n", ret);
goto err_core_supplies;
- }
- usleep_range(1000, 2000);
- dev_dbg(cs42l43->dev, "Powered up\n");
- return 0;
+err_core_supplies:
- regulator_bulk_disable(CS42L43_N_SUPPLIES, cs42l43->core_supplies);
+err_reset:
- gpiod_set_value_cansleep(cs42l43->reset, 0);
- regulator_disable(cs42l43->vdd_p);
- return ret;
+}
+static int cs42l43_power_down(struct cs42l43 *cs42l43) +{
- int ret;
- ret = regulator_disable(cs42l43->vdd_d);
- if (ret) {
dev_err(cs42l43->dev, "Failed to disable VDD_D: %d\n", ret);
return ret;
- }
- ret = regulator_bulk_disable(CS42L43_N_SUPPLIES, cs42l43->core_supplies);
- if (ret) {
dev_err(cs42l43->dev, "Failed to disable core supplies: %d\n", ret);
return ret;
- }
- gpiod_set_value_cansleep(cs42l43->reset, 0);
- ret = regulator_disable(cs42l43->vdd_p);
- if (ret) {
dev_err(cs42l43->dev, "Failed to disable VDD_P: %d\n", ret);
return ret;
- }
- dev_dbg(cs42l43->dev, "Powered down\n");
Drop simple debug statements for function entry/exit. There are other tools in kernel to do such debugging.
- return 0;
+}
+int cs42l43_dev_probe(struct cs42l43 *cs42l43) +{
- int i, ret;
- dev_set_drvdata(cs42l43->dev, cs42l43);
- mutex_init(&cs42l43->pll_lock);
- init_completion(&cs42l43->device_attach);
- init_completion(&cs42l43->device_detach);
- init_completion(&cs42l43->firmware_download);
- INIT_WORK(&cs42l43->boot_work, cs42l43_boot_work);
- regcache_cache_only(cs42l43->regmap, true);
- cs42l43->reset = devm_gpiod_get_optional(cs42l43->dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(cs42l43->reset)) {
ret = PTR_ERR(cs42l43->reset);
dev_err(cs42l43->dev, "Failed to get reset: %d\n", ret);
return dev_err_probe
In other places as well
return ret;
- }
- cs42l43->vdd_p = devm_regulator_get(cs42l43->dev, "VDD_P");
Why these are not part of bulk get?
- if (IS_ERR(cs42l43->vdd_p)) {
ret = PTR_ERR(cs42l43->vdd_p);
dev_err(cs42l43->dev, "Failed to get VDD_P: %d\n", ret);
return ret;
- }
- cs42l43->vdd_d = devm_regulator_get(cs42l43->dev, "VDD_D");
- if (IS_ERR(cs42l43->vdd_d)) {
ret = PTR_ERR(cs42l43->vdd_d);
dev_err(cs42l43->dev, "Failed to get VDD_D: %d\n", ret);
return ret;
- }
- BUILD_BUG_ON(ARRAY_SIZE(cs42l43_core_supplies) != CS42L43_N_SUPPLIES);
- for (i = 0; i < CS42L43_N_SUPPLIES; i++)
cs42l43->core_supplies[i].supply = cs42l43_core_supplies[i];
- ret = devm_regulator_bulk_get(cs42l43->dev, CS42L43_N_SUPPLIES,
cs42l43->core_supplies);
- if (ret) {
dev_err(cs42l43->dev, "Failed to get core supplies: %d\n", ret);
return ret;
- }
- ret = cs42l43_power_up(cs42l43);
- if (ret)
return ret;
- pm_runtime_set_autosuspend_delay(cs42l43->dev, 250);
- pm_runtime_use_autosuspend(cs42l43->dev);
- pm_runtime_set_active(cs42l43->dev);
- pm_runtime_get_noresume(cs42l43->dev);
- pm_runtime_enable(cs42l43->dev);
- queue_work(system_long_wq, &cs42l43->boot_work);
- return 0;
+} +EXPORT_SYMBOL_NS_GPL(cs42l43_dev_probe, MFD_CS42L43);
+void cs42l43_dev_remove(struct cs42l43 *cs42l43) +{
- pm_runtime_disable(cs42l43->dev);
- cs42l43_power_down(cs42l43);
+} +EXPORT_SYMBOL_NS_GPL(cs42l43_dev_remove, MFD_CS42L43);
+static int __maybe_unused cs42l43_suspend(struct device *dev) +{
- struct cs42l43 *cs42l43 = dev_get_drvdata(dev);
- int ret;
- dev_dbg(cs42l43->dev, "System suspend\n");
Drop simple debug statements. There are other tools in kernel to do such debugging.
- /*
* Don't care about being resumed here, but we do want force_resume to
* always trigger an actual resume, so that register state for the
* MCU/GPIOs is returned as soon as possible after system resume
*/
- pm_runtime_get_noresume(dev);
- ret = pm_runtime_force_suspend(dev);
- if (ret) {
dev_err(cs42l43->dev, "Failed to force suspend: %d\n", ret);
return ret;
- }
- pm_runtime_put_noidle(dev);
- ret = cs42l43_power_down(cs42l43);
- if (ret)
return ret;
- return 0;
+}
+static int __maybe_unused cs42l43_resume(struct device *dev) +{
- struct cs42l43 *cs42l43 = dev_get_drvdata(dev);
- int ret;
- dev_dbg(cs42l43->dev, "System resume\n");
Ditto
- ret = cs42l43_power_up(cs42l43);
- if (ret)
return ret;
- ret = pm_runtime_force_resume(dev);
- if (ret) {
dev_err(cs42l43->dev, "Failed to force resume: %d\n", ret);
return ret;
- }
- return 0;
+}
+static int __maybe_unused cs42l43_runtime_suspend(struct device *dev) +{
- struct cs42l43 *cs42l43 = dev_get_drvdata(dev);
- dev_dbg(cs42l43->dev, "Runtime suspend\n");
Ditto
- /*
* Whilst we don't power the chip down here, going into runtime
* suspend lets the SoundWire bus power down, which means we can't
* communicate with the device any more.
*/
- regcache_cache_only(cs42l43->regmap, true);
- return 0;
+}
+static int __maybe_unused cs42l43_runtime_resume(struct device *dev) +{
- struct cs42l43 *cs42l43 = dev_get_drvdata(dev);
- unsigned int reset_canary;
- int ret;
- dev_dbg(cs42l43->dev, "Runtime resume\n");
Ditto
Best regards, Krzysztof