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?).