Fri, Aug 04, 2023 at 11:45:59AM +0100, Charles Keepax kirjoitti:
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.
...
+#include <linux/err.h> +#include <linux/errno.h>
It seems errno is not used (as Linux kernel error codes, otherwise err.h already includes necessary header).
Also seems array_size.h, mod_devicetable.h are missing (at least).
...
+#if IS_ENABLED(CONFIG_OF)
We are trying hard to get rid of this ugly ifdefferies (ACPI as well) along with respective macros that are often the PITA for CIs.
+static const struct of_device_id cs42l43_of_match[] = {
- { .compatible = "cirrus,cs42l43", },
- {}
+}; +MODULE_DEVICE_TABLE(of, cs42l43_of_match); +#endif
+#if IS_ENABLED(CONFIG_ACPI) +static const struct acpi_device_id cs42l43_acpi_match[] = {
- { "CSC4243", 0 },
- {}
+}; +MODULE_DEVICE_TABLE(acpi, cs42l43_acpi_match); +#endif
+static struct i2c_driver cs42l43_i2c_driver = {
- .driver = {
.name = "cs42l43",.pm = pm_ptr(&cs42l43_pm_ops),.of_match_table = of_match_ptr(cs42l43_of_match),.acpi_match_table = ACPI_PTR(cs42l43_acpi_match),- },
- .probe = cs42l43_i2c_probe,
- .remove = cs42l43_i2c_remove,
+};
...
+#include <linux/err.h> +#include <linux/errno.h>
As per above.
+#include <linux/mfd/cs42l43-regs.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/soundwire/sdw.h> +#include <linux/soundwire/sdw_registers.h> +#include <linux/soundwire/sdw_type.h>
...
- mutex_lock(&cs42l43->pll_lock);
This can be converted using cleanup.h.
- mutex_unlock(&cs42l43->pll_lock);
...
- cs42l43->regmap = devm_regmap_init_sdw(sdw, &cs42l43_sdw_regmap);
- if (IS_ERR(cs42l43->regmap)) {
ret = PTR_ERR(cs42l43->regmap);dev_err(cs42l43->dev, "Failed to allocate regmap: %d\n", ret);return ret;- }
Can be simplified as
cs42l43->regmap = devm_regmap_init_sdw(sdw, &cs42l43_sdw_regmap); if (IS_ERR(cs42l43->regmap)) dev_err_probe(cs42l43->dev, PTR_ERR(cs42l43->regmap), "Failed to allocate regmap: %d\n", ret);
...
+#include <linux/err.h> +#include <linux/errno.h>
As per above.
...
+#define CS42L43_RESET_DELAY 20
+#define CS42L43_SDW_ATTACH_TIMEOUT 500 +#define CS42L43_SDW_DETACH_TIMEOUT 100
+#define CS42L43_MCU_POLL 5000 +#define CS42L43_MCU_CMD_TIMEOUT 20000
+#define CS42L43_MCU_UPDATE_TIMEOUT 500000
+#define CS42L43_VDDP_DELAY 50 +#define CS42L43_VDDD_DELAY 1000
+#define CS42L43_AUTOSUSPEND_TIME 250
Usually we use units for the macro names as suffixes... E.g., _US (for microseconds).
...
+struct cs42l43_patch_header {
- __le16 version;
- __le16 size;
- u8 reserved;
- u8 secure;
Seems to me that __u8 is appropriate as patch is external to the kernel and it's kinda firmware interface.
- __le16 bss_size;
- __le32 apply_addr;
- __le32 checksum;
- __le32 sha;
- __le16 swrev;
- __le16 patchid;
- __le16 ipxid;
- __le16 romver;
- __le32 load_addr;
+} __packed;
...
+static const struct mfd_cell cs42l43_devs[] = {
- { .name = "cs42l43-pinctrl", },
- { .name = "cs42l43-spi", },
Inner commas are not needed
- {
.name = "cs42l43-codec",.parent_supplies = cs42l43_parent_supplies,.num_parent_supplies = ARRAY_SIZE(cs42l43_parent_supplies),- },
+};
- case CS42L43_MCU_BOOT_STAGE2:
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 {
Redundant 'else' and
return cs42l43_mcu_stage_2_3(cs42l43, shadow);}
why not positive conditional as below?
if (patched) return ... ...
- case CS42L43_MCU_BOOT_STAGE3:
if (patched)return cs42l43_mcu_disable(cs42l43);elsereturn cs42l43_mcu_stage_3_2(cs42l43);
...
- irq_flags = irqd_get_trigger_type(irq_data);
- switch (irq_flags) {
- case IRQF_TRIGGER_LOW:
- case IRQF_TRIGGER_HIGH:
- case IRQF_TRIGGER_RISING:
- case IRQF_TRIGGER_FALLING:
break;- case IRQ_TYPE_NONE:
Are you sure it's a right place to interpret no type flags as a default?
- default:
irq_flags = IRQF_TRIGGER_LOW;break;- }
...
+static int cs42l43_suspend(struct device *dev) +{
- struct cs42l43 *cs42l43 = dev_get_drvdata(dev);
- int ret;
- /*
* Don't care about being resumed here, but the driver does 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. force_resume will resume if the reference count is resumed on* suspend hence the get_noresume.*/- pm_runtime_get_noresume(dev);
- ret = pm_runtime_force_suspend(dev);
- if (ret) {
dev_err(cs42l43->dev, "Failed to force suspend: %d\n", ret);pm_runtime_put_noidle(dev);return ret;- }
- pm_runtime_put_noidle(dev);
- ret = cs42l43_power_down(cs42l43);
- if (ret)
return ret;- return 0;
return cs42l43_power_down(cs42l43);
+}
...
+EXPORT_NS_GPL_DEV_PM_OPS(cs42l43_pm_ops, MFD_CS42L43) = {
- SET_SYSTEM_SLEEP_PM_OPS(cs42l43_suspend, cs42l43_resume)
- SET_RUNTIME_PM_OPS(cs42l43_runtime_suspend, cs42l43_runtime_resume, NULL)
+};
Why do you need SET_*() versions of those macros? They are not supposed to be used with the new macros such as EXPORT_NS_GPL_DEV_PM_OPS().
...
+++ b/drivers/mfd/cs42l43.h
+#include <linux/mfd/cs42l43.h>
How is this header being used? Wouldn't the forward declaration fulfill the need?
+#include <linux/pm.h> +#include <linux/regmap.h>
+#ifndef CS42L43_CORE_INT_H +#define CS42L43_CORE_INT_H
Why you don't guard other headers with this? It will slow down the build.
+#define CS42L43_N_DEFAULTS 176
+extern const struct dev_pm_ops cs42l43_pm_ops; +extern const struct reg_default cs42l43_reg_default[CS42L43_N_DEFAULTS];
Missing forward declaration for
struct device;
+bool cs42l43_readable_register(struct device *dev, unsigned int reg); +bool cs42l43_precious_register(struct device *dev, unsigned int reg); +bool cs42l43_volatile_register(struct device *dev, unsigned int reg);
+int cs42l43_dev_probe(struct cs42l43 *cs42l43); +void cs42l43_dev_remove(struct cs42l43 *cs42l43);
+#endif /* CS42L43_CORE_INT_H */
...
+#define CS42L43_MIXER_VOL_MASK 0x00FE0000 +#define CS42L43_MIXER_VOL_SHIFT 17 +#define CS42L43_MIXER_SRC_MASK 0x000001FF +#define CS42L43_MIXER_SRC_SHIFT 0
This header would benefit from bits.h... (the above is just a little example).
...
+++ b/include/linux/mfd/cs42l43.h
+#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/soundwire/sdw.h>
Missing forward declarations (instead of inclusions).
struct device; struct gpio_desc; struct sdw_slave;
Missing types.h.
...
+#ifndef CS42L43_CORE_EXT_H +#define CS42L43_CORE_EXT_H
Same questions as per another header.