Re: [PATCH v3 03/10] ASoC: mediatek: mt8196: support audio clock control

["Darren Ye (叶飞)" <Darren.Ye@mediatek.com>]

On Thu, 2025-05-15 at 16:50 +0800, Chen-Yu Tsai wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, May 15, 2025 at 4:40 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> > 
> > Il 14/05/25 10:11, Darren.Ye ha scritto:
> > > From: Darren Ye <darren.ye@mediatek.com>
> > > 
> > > Add audio clock wrapper and audio tuner control.
> > > 
> > > Signed-off-by: Darren Ye <darren.ye@mediatek.com>
> > > ---
> > >   sound/soc/mediatek/mt8196/mt8196-afe-clk.c    | 723
> > > ++++++++++++++++++
> > >   sound/soc/mediatek/mt8196/mt8196-afe-clk.h    | 142 ++++
> > >   sound/soc/mediatek/mt8196/mt8196-audsys-clk.c | 252 ++++++
> > >   sound/soc/mediatek/mt8196/mt8196-audsys-clk.h |  14 +
> > >   .../soc/mediatek/mt8196/mt8196-audsys-clkid.h |  78 ++
> > >   5 files changed, 1209 insertions(+)
> > >   create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-clk.c
> > >   create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-clk.h
> > >   create mode 100644 sound/soc/mediatek/mt8196/mt8196-audsys-
> > > clk.c
> > >   create mode 100644 sound/soc/mediatek/mt8196/mt8196-audsys-
> > > clk.h
> > >   create mode 100644 sound/soc/mediatek/mt8196/mt8196-audsys-
> > > clkid.h
> > > 
> > > diff --git a/sound/soc/mediatek/mt8196/mt8196-afe-clk.c
> > > b/sound/soc/mediatek/mt8196/mt8196-afe-clk.c
> > > new file mode 100644
> > > index 000000000000..83b5ee9d30ef
> > > --- /dev/null
> > > +++ b/sound/soc/mediatek/mt8196/mt8196-afe-clk.c
> > > @@ -0,0 +1,723 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + *  mt8196-afe-clk.c  --  Mediatek 8196 afe clock ctrl
> > 
> > mt8196-afe-clk.c - MediaTek MT8196 AFE Clock Control
> > 
> > > + *
> > > + *  Copyright (c) 2024 MediaTek Inc.
> > > + *  Author: Darren Ye <darren.ye@mediatek.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include "mt8196-afe-common.h"
> > > +#include "mt8196-audsys-clk.h"
> > > +#include "mt8196-afe-clk.h"
> > > +
> > 
> > > +int mt8196_audsys_clk_register(struct mtk_base_afe *afe)
> > > +{
> > > +     struct mt8196_afe_private *afe_priv = afe->platform_priv;
> > > +     struct clk *clk;
> > > +     struct clk_lookup *cl;
> > > +     int i;
> > > +
> > > +     afe_priv->lookup = devm_kcalloc(afe->dev, CLK_AFE_NR_CLK,
> > > +                                     sizeof(*afe_priv->lookup),
> > > +                                     GFP_KERNEL);
> > > +
> > > +     if (!afe_priv->lookup)
> > > +             return -ENOMEM;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(aud_clks); i++) {
> > > +             const struct afe_gate *gate = &aud_clks[i];
> > > +
> > > +             clk = clk_register_gate(afe->dev, gate->name, gate-
> > > >parent_name,
> > > +                                     gate->flags, afe->base_addr 
> > > + gate->reg,
> > > +                                     gate->bit, gate->cg_flags,
> > > NULL);
> > > +
> > > +             if (IS_ERR(clk)) {
> > > +                     dev_err(afe->dev, "Failed to register clk
> > > %s: %ld\n",
> > > +                             gate->name, PTR_ERR(clk));
> > > +                     continue;
> > > +             }
> > > +
> > 
> > All of the above, until...
> > 
> > > +             /* add clk_lookup for
> > > devm_clk_get(SND_SOC_DAPM_CLOCK_SUPPLY) */
> > > +             cl = kzalloc(sizeof(*cl), GFP_KERNEL);
> > > +             if (!cl)
> > > +                     return -ENOMEM;
> > > +
> > > +             cl->clk = clk;
> > > +             cl->con_id = gate->name;
> > > +             cl->dev_id = dev_name(afe->dev);
> > > +             cl->clk_hw = NULL;
> > > +             clkdev_add(cl);
> > 
> > 
> > ...here, can be simplified with a single call to
> > 
> > clk_register_clkdev(clk, gate->name, dev_name(afe->dev))
> > 
> > or alternatively, you could simplify it even more:
> > 
> > 
> > static void mt8196_audsys_clk_unregister(void *data)
> > {
> >         /* nothing to do here, remove this function */
> > }
> > 
> > int mt8196_audsys_clk_register(struct mtk_base_afe *afe)
> > {
> >         struct mt8196_afe_private *afe_priv = afe->platform_priv;
> >         int i, ret;
> > 
> >         for (i = 0; i < ARRAY_SIZE(aud_clks); i++) {
> >                 const struct afe_gate *gate = &aud_clks[i];
> >                 struct clk_hw *hw;
> > 
> >                 hw = devm_clk_hw_register_gate(afe->dev, gate-
> > >name, gate->parent_name,
> >                                                gate->flags, afe-
> > >base_addr + gate->reg,
> >                                                gate->bit, gate-
> > >cg_flags, NULL);
> >                 if (IS_ERR(clk)) {
> >                         dev_err(afe->dev, "Failed to register clk
> > %s: %ld\n",
> >                                 gate->name, PTR_ERR(clk));
> >                         continue;
> >                 }
> > 
> >                 ret = devm_clk_hw_register_clkdev(afe->dev, hw,
> > gate->name, dev_name(afe->dev));
> >                 if (ret)
> >                         return ret;
> >         }
> > 
> >         return 0;
> > }
> 
> There is no need to involve the clk subsystem. These are simply
> supply
> gates, be them for power or clks, one per bit. Simply model them as
> ASoC
> supply widgets, add appropriate routes for each so that the
> dependencies
> are correct, and ASoC will deal with them for you. No code is needed,
> just descriptions.
> 
> _That_ is why I asked for he audio clocks to be integrated into the
> AFE driver.
> 
> ChenYu
> 

OK, I misunderstood. The next version will use widgets and route to
control the afe clk gate.

Best regards,
Darren

> > > +
> > > +             afe_priv->lookup[i] = cl;
> > > +     }
> > > +
> > > +     return devm_add_action_or_reset(afe->dev,
> > > mt8196_audsys_clk_unregister, afe);
> > > +}
> > 
> > Cheers,
> > Angelo
> >