| Message ID | 20190705042623.129541-1-cychiang@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add HDMI jack support on RK3288 | expand |
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h > index 7fea496f1f34..26c02abb8eba 100644 > --- a/include/sound/hdmi-codec.h > +++ b/include/sound/hdmi-codec.h > @@ -47,6 +47,9 @@ struct hdmi_codec_params { > int channels; > }; > > +typedef void (*hdmi_codec_plugged_cb)(struct platform_device *dev, > + bool plugged); > + The callback prototype is "weird" by struct platform_device. Is it possible to having snd_soc_component instead of platform_device? > struct hdmi_codec_pdata; > struct hdmi_codec_ops { > /* > @@ -88,6 +91,13 @@ struct hdmi_codec_ops { > */ > int (*get_dai_id)(struct snd_soc_component *comment, > struct device_node *endpoint); > + > + /* > + * Hook callback function to handle connector plug event. > + * Optional > + */ > + int (*hook_plugged_cb)(struct device *dev, void *data, > + hdmi_codec_plugged_cb fn); > }; The first parameter dev could be removed. Not used.
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c > index c5fc24675a33..195309d1225a 100644 > --- a/sound/soc/rockchip/rockchip_max98090.c > +++ b/sound/soc/rockchip/rockchip_max98090.c > static int rk_aif1_hw_params(struct snd_pcm_substream *substream, > @@ -92,38 +95,59 @@ static int rk_aif1_hw_params(struct snd_pcm_substream *substream, > > ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk, > SND_SOC_CLOCK_OUT); > - if (ret < 0) { > - dev_err(codec_dai->dev, "Can't set codec clock %d\n", ret); > + if (ret && ret != -ENOTSUPP) { > + dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n", ret); > return ret; > } > > ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk, > SND_SOC_CLOCK_IN); > - if (ret < 0) { > - dev_err(codec_dai->dev, "Can't set codec clock %d\n", ret); > + if (ret && ret != -ENOTSUPP) { > + dev_err(codec_dai->dev, "Can't set codec dai clock %d\n", ret); > return ret; > } Does it imply: it is acceptable even if they are "not supported"? > > - return ret; > + return 0; > } > > static const struct snd_soc_ops rk_aif1_ops = { > .hw_params = rk_aif1_hw_params, > }; > > -SND_SOC_DAILINK_DEFS(hifi, > +SND_SOC_DAILINK_DEFS(analog, > DAILINK_COMP_ARRAY(COMP_EMPTY()), > DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "HiFi")), > DAILINK_COMP_ARRAY(COMP_EMPTY())); > > -static struct snd_soc_dai_link rk_dailink = { > - .name = "max98090", > - .stream_name = "Audio", > - .ops = &rk_aif1_ops, > - /* set max98090 as slave */ > - .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > - SND_SOC_DAIFMT_CBS_CFS, > - SND_SOC_DAILINK_REG(hifi), > +SND_SOC_DAILINK_DEFS(hdmi, > + DAILINK_COMP_ARRAY(COMP_EMPTY()), > + DAILINK_COMP_ARRAY(COMP_CODEC("hdmi-audio-codec.3.auto", "i2s-hifi")), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > + > +enum { > + DAILINK_MAX98090, > + DAILINK_HDMI, > +}; > + > +/* max98090 and HDMI codec dai_link */ > +static struct snd_soc_dai_link rk_dailinks[] = { > + [DAILINK_MAX98090] = { > + .name = "max98090", > + .stream_name = "Analog", > + .ops = &rk_aif1_ops, > + /* set max98090 as slave */ > + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > + SND_SOC_DAIFMT_CBS_CFS, > + SND_SOC_DAILINK_REG(analog), > + }, > + [DAILINK_HDMI] = { > + .name = "HDMI", > + .stream_name = "HDMI", > + .ops = &rk_aif1_ops, > + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > + SND_SOC_DAIFMT_CBS_CFS, > + SND_SOC_DAILINK_REG(hdmi), > + } > }; > > static int rk_98090_headset_init(struct snd_soc_component *component); > @@ -136,8 +160,8 @@ static struct snd_soc_aux_dev rk_98090_headset_dev = { > static struct snd_soc_card snd_soc_card_rk = { > .name = "ROCKCHIP-I2S", > .owner = THIS_MODULE, > - .dai_link = &rk_dailink, > - .num_links = 1, > + .dai_link = rk_dailinks, > + .num_links = ARRAY_SIZE(rk_dailinks), > .aux_dev = &rk_98090_headset_dev, > .num_aux_devs = 1, > .dapm_widgets = rk_dapm_widgets, > @@ -173,27 +197,48 @@ static int snd_rk_mc_probe(struct platform_device *pdev) > int ret = 0; > struct snd_soc_card *card = &snd_soc_card_rk; > struct device_node *np = pdev->dev.of_node; > + struct device_node *np_analog; > + struct device_node *np_cpu; > + struct of_phandle_args args; > > /* register the soc card */ > card->dev = &pdev->dev; > > - rk_dailink.codecs->of_node = of_parse_phandle(np, > - "rockchip,audio-codec", 0); > - if (!rk_dailink.codecs->of_node) { > + np_analog = of_parse_phandle(np, "rockchip,audio-codec", 0); > + if (!np_analog) { > dev_err(&pdev->dev, > "Property 'rockchip,audio-codec' missing or invalid\n"); > return -EINVAL; > } > + rk_dailinks[DAILINK_MAX98090].codecs->of_node = np_analog; > + > + ret = of_parse_phandle_with_fixed_args(np, "rockchip,audio-codec", > + 0, 0, &args); > + if (ret) { > + dev_err(&pdev->dev, > + "Unable to parse property 'rockchip,audio-codec'\n"); > + return ret; > + } > + > + ret = snd_soc_get_dai_name( > + &args, &rk_dailinks[DAILINK_MAX98090].codecs->dai_name); > + if (ret) { > + dev_err(&pdev->dev, "Unable to get codec dai_name\n"); > + return ret; > + } > + > + np_cpu = of_parse_phandle(np, "rockchip,i2s-controller", 0); > > - rk_dailink.cpus->of_node = of_parse_phandle(np, > - "rockchip,i2s-controller", 0); > - if (!rk_dailink.cpus->of_node) { > + if (!np_cpu) { > dev_err(&pdev->dev, > "Property 'rockchip,i2s-controller' missing or invalid\n"); > return -EINVAL; > } > > - rk_dailink.platforms->of_node = rk_dailink.cpus->of_node; > + rk_dailinks[DAILINK_MAX98090].cpus->of_node = np_cpu; > + rk_dailinks[DAILINK_MAX98090].platforms->of_node = np_cpu; > + rk_dailinks[DAILINK_HDMI].cpus->of_node = np_cpu; > + rk_dailinks[DAILINK_HDMI].platforms->of_node = np_cpu; > > rk_98090_headset_dev.codec_of_node = of_parse_phandle(np, > "rockchip,headset-codec", 0); > -- > 2.22.0.410.gd8fdbe21b5-goog >
On Fri, Jul 05, 2019 at 12:26:19PM +0800, Cheng-Yi Chiang wrote: > This patch series supports HDMI jack reporting on RK3288, which uses > DRM dw-hdmi driver and hdmi-codec codec driver. > > The previous discussion about reporting jack status using hdmi-notifier > and drm_audio_component is at > > https://lore.kernel.org/patchwork/patch/1083027/ > > The new approach is to use a callback mechanism that is > specific to hdmi-codec. I think this looks reasonable. There's the entire question of getting rid of the platform_device in hdmi_codec an roll our own devices (so that it all looks a bit cleaner, like e.g. the cec stuff does). But that can also be done in a follow-up (if you can convince reviewers of that). -Daniel > Cheng-Yi Chiang (4): > ASoC: hdmi-codec: Add an op to set callback function for plug event > drm: bridge: dw-hdmi: Report connector status using callback > ASoC: rockchip_max98090: Add dai_link for HDMI > ASoC: rockchip_max98090: Add HDMI jack support > > .../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 3 + > .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10 ++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 32 ++++- > include/sound/hdmi-codec.h | 16 +++ > sound/soc/codecs/hdmi-codec.c | 52 ++++++++ > sound/soc/rockchip/rockchip_max98090.c | 112 ++++++++++++++---- > 6 files changed, 201 insertions(+), 24 deletions(-) > > -- > 2.22.0.410.gd8fdbe21b5-goog >
On Fri, Jul 05, 2019 at 03:08:37PM +0800, Tzung-Bi Shih wrote: > On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > > +typedef void (*hdmi_codec_plugged_cb)(struct platform_device *dev, > > + bool plugged); > > + > The callback prototype is "weird" by struct platform_device. Is it > possible to having snd_soc_component instead of platform_device? Or if it's got to be a device why not just a generic device so we're not tied to a particular bus here?
On Fri, Jul 5, 2019 at 3:10 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > > diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c > > index c5fc24675a33..195309d1225a 100644 > > --- a/sound/soc/rockchip/rockchip_max98090.c > > +++ b/sound/soc/rockchip/rockchip_max98090.c > > static int rk_aif1_hw_params(struct snd_pcm_substream *substream, > > @@ -92,38 +95,59 @@ static int rk_aif1_hw_params(struct snd_pcm_substream *substream, > > > > ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk, > > SND_SOC_CLOCK_OUT); > > - if (ret < 0) { > > - dev_err(codec_dai->dev, "Can't set codec clock %d\n", ret); > > + if (ret && ret != -ENOTSUPP) { > > + dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n", ret); I should remove this change because cpu dai should support sysclk ops. > > return ret; > > } > > > > ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk, > > SND_SOC_CLOCK_IN); > > - if (ret < 0) { > > - dev_err(codec_dai->dev, "Can't set codec clock %d\n", ret); > > + if (ret && ret != -ENOTSUPP) { > > + dev_err(codec_dai->dev, "Can't set codec dai clock %d\n", ret); > > return ret; > > } > Does it imply: it is acceptable even if they are "not supported"? Thank you for this good catch. Here machine driver has the knowledge of deciding whether it is expected to see the ops is not supported. For HDMI path using hdmi-codec driver, it is expected to see -ENOTSUPP. But it is not expected for codec DAI of max98090. I should distinguish them. > > > > > > - return ret; > > + return 0; > > } > > > > static const struct snd_soc_ops rk_aif1_ops = { > > .hw_params = rk_aif1_hw_params, > > }; > > > > -SND_SOC_DAILINK_DEFS(hifi, > > +SND_SOC_DAILINK_DEFS(analog, > > DAILINK_COMP_ARRAY(COMP_EMPTY()), > > DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "HiFi")), > > DAILINK_COMP_ARRAY(COMP_EMPTY())); > > > > -static struct snd_soc_dai_link rk_dailink = { > > - .name = "max98090", > > - .stream_name = "Audio", > > - .ops = &rk_aif1_ops, > > - /* set max98090 as slave */ > > - .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > > - SND_SOC_DAIFMT_CBS_CFS, > > - SND_SOC_DAILINK_REG(hifi), > > +SND_SOC_DAILINK_DEFS(hdmi, > > + DAILINK_COMP_ARRAY(COMP_EMPTY()), > > + DAILINK_COMP_ARRAY(COMP_CODEC("hdmi-audio-codec.3.auto", "i2s-hifi")), > > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > > + > > +enum { > > + DAILINK_MAX98090, > > + DAILINK_HDMI, > > +}; > > + > > +/* max98090 and HDMI codec dai_link */ > > +static struct snd_soc_dai_link rk_dailinks[] = { > > + [DAILINK_MAX98090] = { > > + .name = "max98090", > > + .stream_name = "Analog", > > + .ops = &rk_aif1_ops, > > + /* set max98090 as slave */ > > + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > > + SND_SOC_DAIFMT_CBS_CFS, > > + SND_SOC_DAILINK_REG(analog), > > + }, > > + [DAILINK_HDMI] = { > > + .name = "HDMI", > > + .stream_name = "HDMI", > > + .ops = &rk_aif1_ops, > > + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > > + SND_SOC_DAIFMT_CBS_CFS, > > + SND_SOC_DAILINK_REG(hdmi), > > + } > > }; > > > > static int rk_98090_headset_init(struct snd_soc_component *component); > > @@ -136,8 +160,8 @@ static struct snd_soc_aux_dev rk_98090_headset_dev = { > > static struct snd_soc_card snd_soc_card_rk = { > > .name = "ROCKCHIP-I2S", > > .owner = THIS_MODULE, > > - .dai_link = &rk_dailink, > > - .num_links = 1, > > + .dai_link = rk_dailinks, > > + .num_links = ARRAY_SIZE(rk_dailinks), > > .aux_dev = &rk_98090_headset_dev, > > .num_aux_devs = 1, > > .dapm_widgets = rk_dapm_widgets, > > @@ -173,27 +197,48 @@ static int snd_rk_mc_probe(struct platform_device *pdev) > > int ret = 0; > > struct snd_soc_card *card = &snd_soc_card_rk; > > struct device_node *np = pdev->dev.of_node; > > + struct device_node *np_analog; > > + struct device_node *np_cpu; > > + struct of_phandle_args args; > > > > /* register the soc card */ > > card->dev = &pdev->dev; > > > > - rk_dailink.codecs->of_node = of_parse_phandle(np, > > - "rockchip,audio-codec", 0); > > - if (!rk_dailink.codecs->of_node) { > > + np_analog = of_parse_phandle(np, "rockchip,audio-codec", 0); > > + if (!np_analog) { > > dev_err(&pdev->dev, > > "Property 'rockchip,audio-codec' missing or invalid\n"); > > return -EINVAL; > > } > > + rk_dailinks[DAILINK_MAX98090].codecs->of_node = np_analog; > > + > > + ret = of_parse_phandle_with_fixed_args(np, "rockchip,audio-codec", > > + 0, 0, &args); > > + if (ret) { > > + dev_err(&pdev->dev, > > + "Unable to parse property 'rockchip,audio-codec'\n"); > > + return ret; > > + } > > + > > + ret = snd_soc_get_dai_name( > > + &args, &rk_dailinks[DAILINK_MAX98090].codecs->dai_name); > > + if (ret) { > > + dev_err(&pdev->dev, "Unable to get codec dai_name\n"); > > + return ret; > > + } > > + > > + np_cpu = of_parse_phandle(np, "rockchip,i2s-controller", 0); > > > > - rk_dailink.cpus->of_node = of_parse_phandle(np, > > - "rockchip,i2s-controller", 0); > > - if (!rk_dailink.cpus->of_node) { > > + if (!np_cpu) { > > dev_err(&pdev->dev, > > "Property 'rockchip,i2s-controller' missing or invalid\n"); > > return -EINVAL; > > } > > > > - rk_dailink.platforms->of_node = rk_dailink.cpus->of_node; > > + rk_dailinks[DAILINK_MAX98090].cpus->of_node = np_cpu; > > + rk_dailinks[DAILINK_MAX98090].platforms->of_node = np_cpu; > > + rk_dailinks[DAILINK_HDMI].cpus->of_node = np_cpu; > > + rk_dailinks[DAILINK_HDMI].platforms->of_node = np_cpu; > > > > rk_98090_headset_dev.codec_of_node = of_parse_phandle(np, > > "rockchip,headset-codec", 0); > > -- > > 2.22.0.410.gd8fdbe21b5-goog > >
On Fri, Jul 5, 2019 at 8:12 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Jul 05, 2019 at 03:08:37PM +0800, Tzung-Bi Shih wrote: > > On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > > > > +typedef void (*hdmi_codec_plugged_cb)(struct platform_device *dev, > > > + bool plugged); > > > + > > > The callback prototype is "weird" by struct platform_device. Is it > > possible to having snd_soc_component instead of platform_device? > > Or if it's got to be a device why not just a generic device so > we're not tied to a particular bus here? My intention was to invoke the call in dw-hdmi.c like this: hdmi->plugged_cb(hdmi->audio, result == connector_status_connected); Here hdmi->audio is a platform_device. I think dw-hdmi can not get snd_soc_component easily. I can use a generic device here so the ops is more general. The calling will be like hdmi->plugged_cb(&hdmi->audio->dev, result == connector_status_connected); I will update this in v2. Thanks!
On Mon, Jul 8, 2019 at 1:03 PM Cheng-yi Chiang <cychiang@chromium.org> wrote: > > On Fri, Jul 5, 2019 at 8:12 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Fri, Jul 05, 2019 at 03:08:37PM +0800, Tzung-Bi Shih wrote: > > > On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote: > > > > > > +typedef void (*hdmi_codec_plugged_cb)(struct platform_device *dev, > > > > + bool plugged); > > > > + > > > > > The callback prototype is "weird" by struct platform_device. Is it > > > possible to having snd_soc_component instead of platform_device? > > > > Or if it's got to be a device why not just a generic device so > > we're not tied to a particular bus here? > > My intention was to invoke the call in dw-hdmi.c like this: > > hdmi->plugged_cb(hdmi->audio, > result == connector_status_connected); > > Here hdmi->audio is a platform_device. > I think dw-hdmi can not get snd_soc_component easily. > I can use a generic device here so the ops is more general. > The calling will be like > hdmi->plugged_cb(&hdmi->audio->dev, > result == connector_status_connected); > I will update this in v2. > Thanks! I have thought about this a bit more. And I think the more proper interface is to pass in a generic struct device* for codec. This way, the user of hdmi-codec driver on the DRM side is not limited to the relation chain of audio platform device -> codec platform device, which is just a special case in dw-hdmi driver. As long as DRM side can get hdmi-codec device pointer through drv_data, it can use this callback. Hope this makes the interface more generic.