Thank you Mark for the constructive feedback ! A few follow-up questions below.
On Thu, Apr 16, 2020 at 8:42 AM Mark Brown broonie@kernel.org wrote:
+++ b/sound/soc/codecs/zl38060.c @@ -0,0 +1,643 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Codec driver for Microsemi ZL38060 Connected Home Audio Processor.
Please make the entire comment a C++ one so things look more intentional.
The 'weird' combination of // SPDX and /* Description/copyright */ seems to be a kernel-wide standard (for C files, at least) ?
E.g.: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
Ok to keep?
+skip_setup:
if (priv->amp_en_gpio && tx) {/* enable the external amplifier before playback starts */gpiod_set_value_cansleep(priv->amp_en_gpio, 1);if (priv->amp_startup_delay_ms)msleep(priv->amp_startup_delay_ms);}This external amplifier support shouldn't be here, if there's other devices in the system then they will have their own drivers and the machine driver will take care of linking things together.
In our application, the amp is a "dumb" class-D amp with a single enable line: https://www.onsemi.com/pub/Collateral/FAB3103-D.pdf
I am not sure how I could make this more general. Could you point me to an example somewhere in the tree?
priv->regmap = devm_regmap_init(dev, &zl38_regmap_bus, spi,&zl38_regmap_conf);if (IS_ERR(priv->regmap))return PTR_ERR(priv->regmap);devm_regmap_init_spi()
I wish !! This chip has complex SPI addressing, using an "address" which: - is variable length, depending on the page of the register being accessed; - contains a field with the length of the data to follow.
Unfortunately, during firmware programming, multi-writes are mandatory (usually address header + 32 data bytes).
Implementing my own regmap_bus looked like the only way out.