Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller support

From: andy.shevchenko@gmail.com
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: broonie@kernel.org, lee@kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	linus.walleij@linaro.org, vkoul@kernel.org, lgirdwood@gmail.com,
	yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
	pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org, patches@opensource.cirrus.com,
	devicetree@vger.kernel.org, linux-gpio@vger.kernel.org,
	linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller support
Date: Thu, 18 Jan 2024 19:06:13 +0200	[thread overview]
Message-ID: <ZalahZkCrBm-BXwz@surfacebook.localdomain> (raw)
In-Reply-To: <20230804104602.395892-6-ckeepax@opensource.cirrus.com>

Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti:
> From: Lucas Tanure <tanureal@opensource.cirrus.com>
> 
> 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 SPI component incorporates a SPI controller interface for
> communication with other peripheral components.

...

> +		while (buf < block) {
> +			const u8 *word = min(buf + sizeof(u32), block);
> +			int pad = (buf + sizeof(u32)) - word;
> +
> +			while (buf < word) {
> +				val >>= BITS_PER_BYTE;
> +				val |= FIELD_PREP(GENMASK(31, 24), *buf);
> +
> +				buf++;
> +			}

Is this a reinvented way of get_unaligned_*() APIs?

> +			val >>= pad * BITS_PER_BYTE;
> +
> +			regmap_write(regmap, CS42L43_TX_DATA, val);
> +		}

...

> +			while (buf < word) {
> +				*buf = FIELD_GET(GENMASK(7, 0), val);
> +
> +				val >>= BITS_PER_BYTE;
> +				buf++;
> +			}

put_unaligned_*() ?

...

> +	/* select another internal CS, which doesn't exist, so CS 0 is not used */
> +	if (spi_get_csgpiod(spi, 0))
> +		spi_config1 |= 1 << CS42L43_SPI_SS_SEL_SHIFT;

BIT() ?

> +	if (spi->mode & SPI_CPOL)
> +		spi_config1 |= CS42L43_SPI_CPOL_MASK;
> +	if (spi->mode & SPI_CPHA)
> +		spi_config1 |= CS42L43_SPI_CPHA_MASK;
> +	if (spi->mode & SPI_3WIRE)
> +		spi_config1 |= CS42L43_SPI_THREE_WIRE_MASK;

...

> +	if (is_of_node(fwnode))
> +		fwnode = fwnode_get_named_child_node(fwnode, "spi");

You can actually drop these is_of_node() tests and use another variable. In
ACPI there can't be child node in small letters.

But main problem here (and in another driver where the similar is used) that
you bumped reference count for fwnode. I haven't seen where you drop it back.
Have you tested rmmod/modprobe in a loop?

...

> +	devm_pm_runtime_enable(priv->dev);

No error check? Why?

...

> +	ret = devm_spi_register_controller(priv->dev, priv->ctlr);
> +	if (ret) {
> +		pm_runtime_disable(priv->dev);

Ah! Are you sure you properly simulated faults when testing this code?

> +		dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret);
> +	}
> +
> +	return ret;

-- 
With Best Regards,
Andy Shevchenko