[alsa-devel] [PATCH 1/3] sound: Add hikey960 i2s audio driver

[Mark Brown]

On Thu, Feb 28, 2019 at 09:57:00PM +0800, Pengcheng Li wrote:
> From: Youlin Wang <wangyoulin1 at hisilicon.com>

Please use subject lines matching the style for the subsystem.  This
makes it easier for people to identify relevant patches.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.

>  config SND_I2S_HI6210_I2S
> -	tristate "Hisilicon I2S controller"
> +	tristate "Hisilicon Hi6210 I2S controller"
> +	select SND_SOC_GENERIC_DMAENGINE_PCM
> +	help
> +	  Hisilicon I2S
> +

This should really be a separate patch as it's an update to the
existing driver.

> --- /dev/null
> +++ b/sound/soc/hisilicon/hi3660-i2s.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * linux/sound/soc/hisilicon/hi3660-i2s.c
> + *

Please make the entire comment a C++ one so it looks intentional.

> + * I2S IP driver for hi3660.
> + *
> + * Copyright (c) 2001-2021, Huawei Tech. Co., Ltd.
> + */

Given that it's currently 2019 I'm not sure that the years copyright is
claimed for here are accurate...

> +static void update_bits(struct hi3660_i2s *i2s, u32 ofs, u32 reset, u32 set)
> +{
> +	u32 val = readl(i2s->base + ofs) & ~reset;
> +
> +	writel(val | set, i2s->base + ofs);
> +}

It'd be better to namespace the functions in the driver to avoid
collisons with anything that gets added to headers.  Look at how other
drivers name their functions for examples.

It's also a bit confusing to have an _update_bits() with the set/reset
pattern given that we've got _update_bits() functions with the
mask/values pattern at both the ASoC and regmap levels in Linux.

> +static void shutdown(struct snd_pcm_substream *substream,
> +		       struct snd_soc_dai *cpu_dai)
> +{
> +	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
> +
> +	if (!IS_ERR_OR_NULL(i2s->asp_subsys_clk))
> +		clk_disable_unprepare(i2s->asp_subsys_clk);
> +}

Can the driver actually function usefully without this clock?

> +static int set_sysclk(struct snd_soc_dai *cpu_dai,
> +			     int clk_id, unsigned int freq, int dir)
> +{
> +	return 0;
> +}

Don't implement empty operations; if it's safe to not have an operation
(like this one) then the framework will handle it being missing.

> +static int set_format(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
> +	struct hi3660_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
> +
> +	i2s->format = fmt;
> +	i2s->master = (i2s->format & SND_SOC_DAIFMT_MASTER_MASK) ==
> +		      SND_SOC_DAIFMT_CBS_CFS;
> +
> +	return 0;
> +}

There should be some validation in this function to make sure that the
format is valid.

> +	case SNDRV_PCM_FORMAT_U24_LE:
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		i2s->bits = 32;
> +		dma_data->addr_width = 4;
> +		break;

Does the hardware not support 32 bit samples?

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		ret = -ENODEV;
> +		return ret;
> +	}
> +	i2s->base_syscon = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(i2s->base_syscon)) {
> +		dev_err(&pdev->dev, "ioremap failed\n");
> +		ret = PTR_ERR(i2s->base_syscon);
> +		return ret;
> +	}

If there's a system controller involved shouldn't we be using the
standard system controller support in drivers/mfd/syscon.c?  Other
HiSilicon devices seem to use it.

> +	i2s->pctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(i2s->pctrl)) {
> +		dev_err(dev, "could not get pinctrl\n");
> +		ret = -EIO;
> +		return ret;
> +	}

This is discarding the error code returned by the API which is going to
make debuggging harder and will break deferred probe - you should use
PTR_ERR() to get the error and both print it and return it as the error
code.

> +static int hi3660_i2s_remove(struct platform_device *pdev)
> +{
> +	struct hi3660_i2s *i2s = dev_get_drvdata(&pdev->dev);
> +
> +	snd_soc_unregister_component(&pdev->dev);
> +	dev_set_drvdata(&pdev->dev, NULL);
> +
> +	pinctrl_put(i2s->pctrl);

The driver used devm_ functions on probe so no need to explicitly undo
things, the devm_ code will handle that for you.
-------------- 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/20190410/0533b3fb/attachment.sig>