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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Apr 2020 13:42:39 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] ASoC: Add initial ZL38060 driver

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)