[PATCH v1] ona10iv: add ona10iv smart PA kernel driver

[Mark Brown]

On Mon, Nov 16, 2020 at 02:40:07PM +0000, Jamie Meacham wrote:
> From: Jamie Meacham <jamie.meacham at onsemi.com>
> 
> add ona10iv smart PA kernel driver

There's quite a few comments here but I think they should be relatively
straightforward to address - a lot of them are fairly simple stylistic
issues and while it looks like the power management is fairly confused
the fix should just be to remove most of that code as the issue is that
the driver is doing the same thing in a lot of places.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

> Reported-by: kernel test robot <lkp at intel.com>
> Signed-off-by: Jamie Meacham <jamie.meacham at onsemi.com>

This Reported-by doesn't make much sense - are you *sure* that the bot
asked for this driver?

> +++ b/sound/soc/codecs/ona10iv.c
> @@ -0,0 +1,699 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ALSA SoC ON Semiconductor ONA10IV 16W Digital Input Mono Class-D
> + * Audio Amplifier with Speaker I/V Sense

Please make the entire comment a C++ one to make things look more
intentional.

> +///////////////////////////////////////////////////////
> +// Local function prototypes
> +///////////////////////////////////////////////////////
> +static void param_errcheck(int retval, struct device *dev,
> +				const char *errmsg, int param_val);
> +
> +///////////////////////////////////////////////////////
> +// Local structure definitions
> +///////////////////////////////////////////////////////

These comments don't resemble the usual kernel coding style, please try
to do something more consistent with the usual style.

> +static int ona10iv_set_bias_level(struct snd_soc_component *component,
> +					enum snd_soc_bias_level level)
> +{

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Or just return the value?  This is a repeating pattern throughout the
driver.

> +#ifdef CONFIG_PM
> +static int ona10iv_codec_suspend(struct snd_soc_component *component)
> +{
> +	int from_state;
> +	int ret;
> +
> +	dev_dbg(component->dev, "ONA10IV-->suspend (standby)\n");
> +	from_state = snd_soc_component_get_bias_level(component);

You should never be suspending from a non-idle state.

> +	ret = snd_soc_component_update_bits(component,
> +			ONA10IV_REG_PWR_CTRL, ONA10IV_PWR_STATE_MASK,
> +			ONA10IV_SD_N_NORMAL | ONA10IV_STBY);
> +
> +	if (from_state == SND_SOC_BIAS_ON)
> +		msleep(255);	// pause for volume ramp to complete

...and the implementation appears to duplicate set_bias_level() anyway?

> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

The delay here doesn't appear to actually be waiting to do anything?

> +#else
> +#define ona10iv_codec_suspend NULL
> +#define ona10iv_codec_resume NULL
> +#endif

Use DEV_PM_OPS()

> +static int ona10iv_dac_event(struct snd_soc_dapm_widget *w,
> +				struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component =
> +			snd_soc_dapm_to_component(w->dapm);
> +	int ret = 0;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		dev_dbg(component->dev, "ONA10IV-->post power-up\n");
> +		ret = snd_soc_component_update_bits(component,
> +				ONA10IV_REG_PWR_CTRL, ONA10IV_PWR_STATE_MASK,
> +				ONA10IV_SD_N_NORMAL | ONA10IV_STBY_RELEASE);
> +		break;

This also appears to be duplicating things done in set_bias_level() -
I'm not sure what the goal is here but it looks like this entire event
handler should just be removed.

> +static int ona10iv_mute(struct snd_soc_dai *dai, int mute, int dir)
> +{
> +	int ret;
> +	struct snd_soc_component *component = dai->component;
> +	struct ona10iv_priv *ona10iv;
> +
> +	ona10iv = snd_soc_component_get_drvdata(component);
> +
> +	/* using "mute" bit can cause a pop. Instead store volume setting
> +	 * and set volume to minimum allowing a smooth ramp-down.
> +	 */

If your device doesn't have a useful mute control just don't implement
this operation.

> +static int ona10iv_set_bitwidth(struct ona10iv_priv *ona10iv, int format)
> +{

> +static int ona10iv_set_samplerate(struct ona10iv_priv *ona10iv,
> +					int samplerate)
> +{

Both these functions are very simple, linear functions with exactly one
user - it would be cleaner to just inline them in hw_params().

> +static void param_errcheck(int retval, struct device *dev,
> +				const char *errmsg, int param_val)
> +{
> +	if (retval < 0)
> +		dev_dbg(dev, "Error %d: %s = %d\n", retval, errmsg, param_val);
> +}

It would be much better to just have dev_err() messages in the error
paths where this is used, it's confusing when reading the code as it's
not idiomatic to have this and it's hard to see what it's achieving -
it's duplicating the ret check which follows it immediately anyway.

Also note that the logging style here does not really reflect the usual
kernel style for log messages.

> +static struct snd_soc_dai_ops ona10iv_dai_ops = {
> +	.mute_stream = ona10iv_mute,
> +	.hw_params  = ona10iv_hw_params,
> +	.set_fmt    = NULL,
> +	.set_tdm_slot = ona10iv_set_dai_tdm_slot,
> +	.startup = NULL,
> +	.shutdown = NULL,
> +	.prepare = NULL,
> +};

There is no need to assign NULL to unused function pointers, just omit
them.

> +static int ona10iv_codec_probe(struct snd_soc_component *component)
> +{
> +	struct ona10iv_priv *ona10iv =
> +			snd_soc_component_get_drvdata(component);
> +
> +	ona10iv->component = component;
> +
> +	dev_dbg(component->dev, "ONA10IV-->device probe\n");
> +
> +	ona10iv_reset(ona10iv);

Better to do the reset on I2C probe so we get the device into a known
good state as early as possible, you can then omit this function
entirely.

> +static const struct snd_kcontrol_new ona10iv_snd_controls[] = {
> +	SOC_SINGLE_TLV("Playback Volume", ONA10IV_REG_MAX_VOL, 0, 255, 1,
> +					ona10iv_playback_volume),
> +	SOC_SINGLE_TLV("Amp Gain", ONA10IV_REG_GAIN_CTRL1, 0, 31, 1,
> +					ona10iv_digital_tlv),

Volume controls should end in Volume so userspace knows how to handle
them, see control-names.rst for details on the naming conventions. 

> +static bool ona10iv_volatile(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ONA10IV_REG_PWR_CTRL:	/* reset bit is self clearing */
> +	case ONA10IV_REG_INT_FLAG:
> +	case ONA10IV_REG_ERR_STAT:
> +	case ONA10IV_REG_T_SENSE_OUT1:
> +	case ONA10IV_REG_T_SENSE_OUT2:/* Sticky interrupt flags */

Missing spaces after the : here.

> +static int ona10iv_i2c_remove(struct i2c_client *client)
> +{
> +	return 0;
> +}

Remove empty functions.

> +static const struct of_device_id ona10iv_of_match[] = {
> +	{ .compatible = "onnn,ona10iv" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ona10iv_of_match);

New device tree bindings need to have binding documentation added.

> +#define ONA10IV_REG_MAX_VOL		(0x09)
> +#define		ONA10IV_VOL_0DB			(0x00)
> +#define		ONA10IV_VOL_MINUS_0_375DB	(0x01)

There's rather a lot of these volume defines and the values are already
documented much more compactly by the TLV for the volume controls - are
these really required?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20201119/da0e8808/attachment.sig>