Re: [PATCH v8 1/2] ASoC: hdmi: Add a transmitter interface to the HDMI CODEC

[Mark Brown]

On Thu, Oct 23, 2014 at 09:09:35AM +0200, Jean-Francois Moine wrote:

> +struct hdmi_data {
> +	int (*get_audio)(struct device *dev,
> +			 int *max_channels,
> +			 int *rate_mask,
> +			 int *fmt);
> +	void (*audio_switch)(struct device *dev,
> +			     int port_index,
> +			     unsigned sample_rate,
> +			     int sample_format);

I can't really tell from these function names what these functions are
supposed to do.  I think get_audio() should be get_audio_caps() or
similar since it reads the capabilities and audio_switch() is supposed
to be set_audio_params() or similar to set the settings for the running
stream.

> +	snd_pcm_hw_constraint_list(runtime, 0,
> +				   SNDRV_PCM_HW_PARAM_RATE,
> +				   rate_constraints);
> +
> +	formats = 0;
> +	if (fmt & 1)
> +		formats |= SNDRV_PCM_FMTBIT_S16_LE;
> +	if (fmt & 2)
> +		formats |= SNDRV_PCM_FMTBIT_S20_3LE;
> +	if (fmt & 4)
> +		formats |= SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S24_3LE |
> +			   SNDRV_PCM_FMTBIT_S32_LE;

Magic numbers here - can't we have some constants defined?

> +	priv->hdmi_data.audio_switch(dai->dev, dai->id,
> +				     params_rate(params),
> +				     params_format(params));

I'd be happier if this were able to return an error; even if the
constraints are satisfied perhaps something changed or some operation
fails for some reason.

> +static void hdmi_shutdown(struct snd_pcm_substream *substream,
> +			  struct snd_soc_dai *dai)
> +{
> +	struct device *cdev;
> +	struct hdmi_priv *priv;
> +
> +	cdev = hdmi_get_cdev(dai->dev);
> +	if (!cdev)
> +		return;
> +	priv = dev_get_drvdata(cdev);
> +
> +	priv->hdmi_data.audio_switch(dai->dev, -1, 0, 0);	/* stop */
> +}

So the set parameters operation has to support these magic values as
being "off" - why not have an explicit shutdown operation here?

> +static int hdmi_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct hdmi_priv *priv;
> +	struct device *dev = codec->dev;	/* encoder device */
> +	struct device *cdev;			/* codec device */
> +
> +	cdev = hdmi_get_cdev(dev);
> +	if (!cdev)
> +		return -ENODEV;

This is (I think) only called for cases where the driver is being used
from a parent device with ops but the name makes it sound like it should
be called all the time so errors like that shouldn't happen.  This
should be clearer, or perhaps we should just have a separate device
(perhaps rename the existing one to say that it's for a dumb device with
no control?).

Attachment: signature.asc
Description: Digital signature