Received-SPF: pass (google.com: domain of linux-kernel+bounces-41667-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 28 Jan 2024 17:11:22 +0200
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Shenghao Ding <shenghao-ding@ti.com>
Cc: broonie@kernel.org, conor+dt@kernel.org, krzysztof.kozlowski@linaro.org,
	robh+dt@kernel.org, kevin-lu@ti.com, baojun.xu@ti.com,
	devicetree@vger.kernel.org, lgirdwood@gmail.com, perex@perex.cz,
	pierre-louis.bossart@linux.intel.com, 13916275206@139.com,
	linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org,
	liam.r.girdwood@intel.com, soyer@irl.hu, jkhuang3@ti.com,
	tiwai@suse.de, pdjuandi@ti.com, j-mcpherson@ti.com, navada@ti.com
Subject: Re: [PATCH v1 1/4] ASoc: pcm6240: Create pcm6240 codec driver code
Message-ID: <ZbZumnfISSojuA80@smile.fi.intel.com>
References: <20240123111411.850-1-shenghao-ding@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240123111411.850-1-shenghao-ding@ti.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Tue, Jan 23, 2024 at 07:14:07PM +0800, Shenghao Ding wrote:
> PCM6240 driver implements a flexible and configurable setting for register
> and filter coefficients, to one, two or even multiple PCM6240 Family Audio
> chips.

..

> +#include <linux/of_gpio.h>


No new code is supposed to use this header.
Also the header inclusion here is a mess, please use IWYU principle
(Include What You Use).

..

> +static struct pcmdevice_mixer_control adc5120_analog_gain_ctl[] = {
> +	{
> +		.shift = 1,
> +		.reg = ADC5120_REG_CH1_ANALOG_GAIN,
> +		.max = 0x54,
> +		.invert = 0,

Strictly speaking assignments to 0, false, NULL are unnecessary for static
variables, but I'm not against this as it might make cleaner to see what's
going on here. Btw, shouldn't these global variables be const?

> +	},
> +	{
> +		.shift = 1,
> +		.reg = ADC5120_REG_CH2_ANALOG_GAIN,
> +		.max = 0x54,
> +		.invert = 0,
> +	}
> +};

..

> +static int pcmdev_change_dev(struct pcmdevice_priv *pcm_priv,
> +	unsigned short dev_no)
> +{
> +	struct i2c_client *client = (struct i2c_client *)pcm_priv->client;

Why do you need casting? Is that variable is not void *?

> +	struct regmap *map = pcm_priv->regmap;

> +	int ret = 0;

Unneeded assignment, just return 0 directly.

> +
> +	if (client->addr != pcm_priv->addr[dev_no]) {
> +		client->addr = pcm_priv->addr[dev_no];
> +		/* All pcmdevices share the same regmap, clear the page
> +		 * inside regmap once switching to another pcmdevice.
> +		 * Register 0 at any pages inside pcmdevice is the same
> +		 * one for page-switching.
> +		 */
> +		ret = regmap_write(map, PCMDEVICE_PAGE_SELECT, 0);
> +		if (ret < 0)
> +			dev_err(pcm_priv->dev, "%s, E=%d\n", __func__, ret);
> +	}
> +
> +	return ret;
> +}

..

> +static int pcmdev_dev_read(struct pcmdevice_priv *pcm_dev,
> +	unsigned int dev_no, unsigned int reg, unsigned int *val)
> +{

> +	int ret = -EINVAL;

Unneeded assignment, use this value directly.

> +	if (dev_no < pcm_dev->ndev) {
> +		struct regmap *map = pcm_dev->regmap;
> +
> +		ret = pcmdev_change_dev(pcm_dev, dev_no);
> +		if (ret < 0) {
> +			dev_err(pcm_dev->dev, "%s, E=%d\n", __func__, ret);
> +			goto out;
> +		}
> +
> +		ret = regmap_read(map, reg, val);
> +		if (ret < 0)
> +			dev_err(pcm_dev->dev, "%s, E=%d\n", __func__, ret);
> +	} else

Besides broken style, this 'else' becomes redundant after proposed changes.

> +		dev_err(pcm_dev->dev, "%s, no such channel(%d)\n", __func__,
> +			dev_no);
> +
> +
> +out:

Useless label.

> +	return ret;
> +}

I believe you may reduce code size by ~2-3% just by refactoring it in a better
way. (Some examples are given above)

..

> +static int pcmdev_dev_bulk_write(struct pcmdevice_priv *pcm_dev,
> +	unsigned int dev_no, unsigned int reg, unsigned char *data,
> +	unsigned int len)

Ditto and so on!

..

> +	struct snd_soc_component *codec
> +		= snd_soc_kcontrol_component(kcontrol);

= on the previous line, but you may just put all on one line.

..

> +	struct pcmdevice_priv *pcm_dev =
> +		snd_soc_component_get_drvdata(codec);

Ditto, and so on...


> +	ucontrol->value.integer.value[0] = pcm_dev->cur_conf;
> +
> +	return 0;
> +}

..

> +{
> +	struct snd_soc_component *codec
> +		= snd_soc_kcontrol_component(kcontrol);
> +	struct pcmdevice_priv *pcm_dev =
> +		snd_soc_component_get_drvdata(codec);

> +	int ret = 0;

Useless variable, return number directly.

> +
> +	if (pcm_dev->cur_conf != ucontrol->value.integer.value[0]) {
> +		pcm_dev->cur_conf = ucontrol->value.integer.value[0];
> +		ret = 1;
> +	}
> +
> +	return ret;
> +}

..

> +	unsigned int mask = (1 << fls(max)) - 1;

BIT() ?

..

> +	mutex_lock(&pcm_dev->codec_lock);

Why not using cleanup.h?

> +	rc = pcmdev_dev_read(pcm_dev, dev_no, reg, &val);
> +	if (rc) {
> +		dev_err(pcm_dev->dev, "%s:read, ERROR, E=%d\n",
> +			__func__, rc);
> +		goto out;
> +	}
> +
> +	val = (val >> shift) & mask;
> +	val = (val > max) ? max : val;
> +	val = mc->invert ? max - val : val;
> +	ucontrol->value.integer.value[0] = val;

> +out:
> +	mutex_unlock(&pcm_dev->codec_lock);

With cleanup.h becomes redundant.

> +	return rc;

Be consistent with name, ret, rc, what else?

..

> +};
> +

Unnecessary blank line.

> +module_i2c_driver(pcmdevice_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko