Re: [RFC][PATCH 6/7] ASoC: hisilicon: Add hi6210 i2s audio driver for hdmi audio

[Mark Brown]

On Fri, Jul 15, 2016 at 07:13:26PM -0700, John Stultz wrote:

>  sound/soc/Kconfig                      |   1 +
>  sound/soc/Makefile                     |   1 +
>  sound/soc/hisilicon/Kconfig            |   5 +
>  sound/soc/hisilicon/Makefile           |   2 +
>  sound/soc/hisilicon/hi6210-hdmi-card.c | 131 +++++++
>  sound/soc/hisilicon/hi6210-i2s.c       | 641 +++++++++++++++++++++++++++++++++
>  sound/soc/hisilicon/hi6210-i2s.h       | 276 ++++++++++++++
>  7 files changed, 1057 insertions(+)

This is adding several different drivers so should be at least one
patch per driver.  Combining many different drivers into one patch makes
it more difficult to review as the patch gets larger and more fatiguing
and causes review problems in one driver to block others.  Look at how
existing drivers are added to the same subsystem.

> +static int hdmi_hw_params(struct snd_pcm_substream *substream,
> +			  struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	int ret;
> +
> +	ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_I2S |
> +			SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
> +	if (ret)
> +		return ret;
> +
> +	/* set i2s system clock */
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, 0, 24576000, SND_SOC_CLOCK_IN);
> +	if (ret)
> +		return ret;

Neither of these depends at all on the parameters so they should be
being set on init, not at hw_params time.  In the case of the DAI format
this should be done with dai_fmt in the dai_link.

> +	.cpu_dai_name = "f7118000.hi6210_i2s",

Why is this not connected up in DT - as things stand the card has zero
reuse potential.  Though given that this has 

> +	.codec_name = "0.hi6210_hdmi_card",

If you've got a CODEC with hdmi_card in the name that suggests there's a
serious abstraction problem going on somewhere.  

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

This isn't the usual pattern for read/modify/write operations in the
kernel and doesn't seem like it's going to be great for legibility - the
reader has to remember which order clear and set are and the name
doesn't give many clues.

> +	hi6210_bits(i2s, HII2S_DIG_FILTER_MODULE_CFG,
> +		    HII2S_DIG_FILTER_MODULE_CFG__DACR_MIXER_IN2_MUTE |
> +		    HII2S_DIG_FILTER_MODULE_CFG__DACL_MIXER_IN2_MUTE,
> +		    0
> +	);

Coding style, the final ); is just weird.  These register names make
this look like there's some routing control going on here which should
be being exposed to users.

> +	for (n = 0; n < i2s->clocks; n++) {
> +		ret = clk_prepare_enable(i2s->clk[n]);
> +		if (ret)
> +			return ret;
> +	}

This won't unwind things on error.

> +	ret = clk_set_rate(i2s->clk[1], 49152000);
> +	if (ret) {
> +		dev_err(i2s->dev, "%s: setting 49.152MHz base rate failed %d\n",
> +			__func__, ret);
> +		return ret;
> +	}

Magic array index there isn't great.

> +static void hi6210_i2s_txctrl(struct snd_soc_dai *cpu_dai, int on)
> +{
> +	struct hi6210_i2s *i2s = dev_get_drvdata(cpu_dai->dev);
> +
> +	spin_lock(&i2s->lock);
> +
> +	if (on) {
> +		/* enable S2 TX */
> +		hi6210_bits(i2s, HII2S_I2S_CFG, 0, HII2S_I2S_CFG__S2_IF_TX_EN);
> +	} else
> +		/* disable S2 TX */
> +		hi6210_bits(i2s, HII2S_I2S_CFG, HII2S_I2S_CFG__S2_IF_TX_EN, 0);

Coding style, { } on both side or neither :(

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

Remove empty operations, if the operation can reasonably be ignored the
frameworks will handle that well.

> +static int hi6210_i2s_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
> +	struct hi6210_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;

We're validating this elsewhere rather than when the user sets it.

> +	switch (i2s->rate) {
> +	case 8000:
> +		u = HII2S_FS_RATE_8KHZ;
> +		break;
> +	case 16000:
> +		u = HII2S_FS_RATE_16KHZ;
> +		break;
> +	case 32000:
> +		u = HII2S_FS_RATE_32KHZ;
> +		break;
> +	case 48000:
> +		u = HII2S_FS_RATE_48KHZ;
> +		break;
> +	case 96000:
> +		u = HII2S_FS_RATE_96KHZ;
> +		break;
> +	case 192000:
> +		u = HII2S_FS_RATE_192KHZ;
> +		break;
> +	};

Should have validation on the default case.  I'm not super convinced
that u is a helpful variable name for the temporaries in this driver,
it's not a convention I've ever seen before so I had to go and check
what was going on.

> +};
> +
> +#include <sound/dmaengine_pcm.h>
> +
> +static const struct snd_pcm_hardware snd_hi6210_hardware = {

Why are we including files in the middle of the driver?

> +	.info                   = SNDRV_PCM_INFO_MMAP |
> +				  SNDRV_PCM_INFO_MMAP_VALID |
> +				  SNDRV_PCM_INFO_PAUSE |
> +				  SNDRV_PCM_INFO_RESUME |
> +				  SNDRV_PCM_INFO_INTERLEAVED |
> +				  SNDRV_PCM_INFO_HALF_DUPLEX,
> +	.period_bytes_min       = 4096,
> +	.period_bytes_max       = 4096,
> +	.periods_min            = 4,
> +	.periods_max            = UINT_MAX,
> +	.buffer_bytes_max       = SIZE_MAX,
> +};

Why are we open coding dmaengine configuration here?  The core code
should be able to discover this by querying the dmaengine driver.

> +static int hi6210_i2s_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hi6210_i2s *i2s;
> +	struct resource *res;
> +	int ret;
> +
> +	i2s = kzalloc(sizeof(*i2s), GFP_KERNEL);
> +	if (!i2s)
> +		return -ENOMEM;

devm_kzalloc().

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {

No need to check, devm_ioremap_resource() will do this for you.

> +	i2s->base = devm_ioremap_resource(dev, res);
> +	if (i2s->base == NULL) {
> +		dev_err(&pdev->dev, "ioremap failed\n");

devm_ioremap_resource() is quite noisy when it fails, probably no need
for another error especially one that's not saying what we failed to
map.

> +	do {
> +		i2s->clk[i2s->clocks] = of_clk_get(pdev->dev.of_node,
> +						   i2s->clocks);
> +		if (IS_ERR_OR_NULL(i2s->clk[i2s->clocks]))
> +			break;
> +		i2s->clocks++;
> +	} while (i2s->clocks < ARRAY_SIZE(i2s->clk));
> +	if (!i2s->clocks) {
> +		ret = PTR_ERR(i2s->clk[0]);
> +		dev_err(&pdev->dev, "Failed to get clock\n");
> +		goto err2;
> +	}

So we grab some random clocks until we hit an error, hope that the
second one we find is whatever one we were explicitly setting rates on
and ignore errors other than possibly a failure to find any clocks at
all.  This isn't great at all, it's not going to be very robust.  We
should know what clocks we're looking for, we should use the regular
clock API to get them (so we're not tied to DT and so we get a devm
version) and we should care about errors on all the clocks.

> +	dev_info(&pdev->dev, "Registered as %s\n", i2s->dai.name);

This is just noise, remove it.

> +static const struct of_device_id hi6210_i2s_dt_ids[] = {
> +	{ .compatible = "hisilicon,hi6210-i2s" },
> +	{ /* sentinel */ }
> +};

The code makes this look like it's not just an I2S controller.

Attachment: signature.asc
Description: PGP signature