lkml.org 
[lkml]   [2019]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5 8/9] clk: mediatek: Add MT8183 clock support
On Fri, Mar 8, 2019 at 2:42 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> )
> On Tue, Mar 5, 2019 at 1:05 PM Weiyi Lu <weiyi.lu@mediatek.com> wrote:
> >
> > Add MT8183 clock support, include topckgen, apmixedsys,
> > infracfg, mcucfg and subsystem clocks.
> >
> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
>
> In v1 a while back (https://patchwork.kernel.org/patch/10669765/) I
> was complaining about code duplication between these many files, and
> wondering if we can make simplify a lot of this code.

Okay, we had a discussion on this gerrit:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1510921,
and I didn't know the history that having separate clock domains that
we can enable/disable with config options. And the same pattern
already exists on clk-mt2712-*.c, for example.

So, for now:
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>

Some comment below about how this could be improved, potentially:

> Apart from that:
> Tested-by: Nicolas Boichat <drinkcat@chromium.org>
>
[snip]
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8183-audio.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +// Author: Weiyi Lu <weiyi.lu@mediatek.com>
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-gate.h"
> > +
> > +#include <dt-bindings/clock/mt8183-clk.h>
> > +
> > +static const struct mtk_gate_regs audio0_cg_regs = {
> > + .set_ofs = 0x0,
> > + .clr_ofs = 0x0,
> > + .sta_ofs = 0x0,
> > +};
> > +
> > +static const struct mtk_gate_regs audio1_cg_regs = {
> > + .set_ofs = 0x4,
> > + .clr_ofs = 0x4,
> > + .sta_ofs = 0x4,
> > +};
> > +
> > +#define GATE_AUDIO0(_id, _name, _parent, _shift) \
> > + GATE_MTK(_id, _name, _parent, &audio0_cg_regs, _shift, \
> > + &mtk_clk_gate_ops_no_setclr)

This macro (or variants that end up being equivalent) is repeated 103
times in drivers/clk/mediatek/*. We can probably do better. My
suggestion is to do something like this:
#define GATE_MTK_CLK(reg, _id, _name, _parent, _shift) \
GATE_MTK(_id, _name, _parent, &reg##_cg_regs, _shift, \
&mtk_clk_gate_ops_setclr_inv)

and use GATE_MTK_CLK(audio0, ...) in the gate clock arrays.

> > +static int clk_mt8183_audio_probe(struct platform_device *pdev)
> > +{
> > + struct clk_onecell_data *clk_data;
> > + int r;
> > + struct device_node *node = pdev->dev.of_node;
> > +
> > + clk_data = mtk_alloc_clk_data(CLK_AUDIO_NR_CLK);
> > +
> > + mtk_clk_register_gates(node, audio_clks, ARRAY_SIZE(audio_clks),
> > + clk_data);
> > +
> > + r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> > + if (r)
> > + return r;
> > +
> > + r = devm_of_platform_populate(&pdev->dev);
> > + if (r)
> > + of_clk_del_provider(node);
> > +
> > + return r;
> > +}

This (almost exact) function is now repeated 33 times in
drivers/clk/mediatek, I think it's really time for a cleanup... Maybe
there should be a common helper in clk-gate.c (or another file, not
sure), that fetches the clocks (and number of clocks from .data field
in the structure below).

> > +static const struct of_device_id of_match_clk_mt8183_audio[] = {
> > + { .compatible = "mediatek,mt8183-audiosys", },
> > + {}
> > +};
[snip]

\
 
 \ /
  Last update: 2019-03-08 15:47    [W:0.044 / U:1.952 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site