On Thu, Jan 18, 2024 at 06:42:26PM +0200, andy.shevchenko@gmail.com wrote:
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.
Full disclosure probably not going to fix everything here, few almost entirely stylistic things and the patch was like merged 4 month ago. But happy to spin through and do a few fix ups.
...
+#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).
...
Thanks for spotting these will spin through and tidy the headers up.
+#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.
Fair enough, but what is the expected alternative here? Is it now preferred to just always include both in the driver? That does come at a small cost in driver size, but it doesn't really bother me.
+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
...
- 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);
Oops, yeah looks like I missed the ones in sdw and i2c, will fix those up.
+#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).
Can add those, does make it clearer.
...
+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.
Sure.
- 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?
I mean... no... but I might need more to go on. The chip generates an active low IRQ by default so it seems reasonable if nothing is specified to assume the chip is doing what it normally would.
- default:
irq_flags = IRQF_TRIGGER_LOW;break;- }
...
+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().
Yeah fixed that up already in an earlier fixup patch.
Thanks, Charles