On Wed, Apr 15, 2020 at 08:14:14PM -0400, Sven Van Asbroeck 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.
err = zl38_fw_send_data(regmap, addr, rec->data, len);} else if (len == 4) {/* execution address ihex record */err = zl38_fw_send_xaddr(regmap, rec->data);} elseerr = -EINVAL;if (err)
If any part of an if/else has { } then all of them should.
+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.
- /* take chip out of reset, if a reset gpio is provided */
- reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(reset_gpio))
return PTR_ERR(reset_gpio);- if (reset_gpio) {
/* according to the datasheet, chip needs 3ms for its digital* section to become stable.*/usleep_range(3000, 10000);- }
It would be better to explicitly put the chip into reset and then bring it out of reset if there's a GPIO, that way the chip is in a known state.
- 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()
- if (device_property_present(dev, "mscc,load-firmware")) {
err = zl38_load_firmware(dev, priv->regmap);if (err)return err;- }
I'm assuming this is for the case where the device has a flash attached to the master SPI port I can see described on the web site and can boot off it - if that's the case I think it'd be clearer for the DT to explicitly say that rather than this indirected property.