Re: [PATCH v1 2/2] ASoC: Add initial ZL38060 driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

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);
> +		} else
> +			err = -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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]