Re: [PATCH v3 2/2] clk: reset: Modify reset-controller driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting Yong Liang (2019-04-30 00:04:48)
> From: "yong.liang" <yong.liang@mediatek.com>
> 
> Set reset signal by a register and clear reset signal by another register for 8183.

Wrap this at the appropriate line length. Read the submitting patches
doc for more help with this.

> 
> Signed-off-by: yong.liang <yong.liang@mediatek.com>
> ---
> 
> Base on https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git branch clk-next and https://patchwork.kernel.org/patch/10856987/

You can use git format-patch --base=<commit> with newer git to indicate
this stuff. Please consider using that.

> diff --git a/drivers/clk/mediatek/clk-mt8183.c b/drivers/clk/mediatek/clk-mt8183.c
> index 9d86510..ea28587 100644
> --- a/drivers/clk/mediatek/clk-mt8183.c
> +++ b/drivers/clk/mediatek/clk-mt8183.c
> @@ -1204,13 +1204,21 @@ static int clk_mt8183_infra_probe(struct platform_device *pdev)
>  {
>         struct clk_onecell_data *clk_data;
>         struct device_node *node = pdev->dev.of_node;
> +       int r;
>  
>         clk_data = mtk_alloc_clk_data(CLK_INFRA_NR_CLK);
>  
>         mtk_clk_register_gates(node, infra_clks, ARRAY_SIZE(infra_clks),
>                 clk_data);
>  
> -       return of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> +       r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> +       if (r)
> +               dev_err(&pdev->dev,
> +                       "%s(): could not register clock provider: %d\n",__func__, r);
> +
> +       mtk_register_reset_controller_set_clr(node, 4, 0x120);

We still do this if we can't register the clks? Why?

> +
> +       return r;
>  }
>  
>  static int clk_mt8183_mcu_probe(struct platform_device *pdev)
> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> index 33ab173..7a74a54 100644
> --- a/drivers/clk/mediatek/clk-mtk.h
> +++ b/drivers/clk/mediatek/clk-mtk.h
> @@ -248,4 +248,7 @@ struct clk *mtk_clk_register_ref2usb_tx(const char *name,
>  void mtk_register_reset_controller(struct device_node *np,
>                         unsigned int num_regs, int regofs);
>  
> +void mtk_register_reset_controller_set_clr(struct device_node *np,
> +       unsigned int num_regs, int regofs);
> +
>  #endif /* __DRV_CLK_MTK_H */
> diff --git a/drivers/clk/mediatek/reset.c b/drivers/clk/mediatek/reset.c
> index d3551d5..ee70798 100644
> --- a/drivers/clk/mediatek/reset.c
> +++ b/drivers/clk/mediatek/reset.c
> @@ -27,6 +27,21 @@ struct mtk_reset {
>         struct reset_controller_dev rcdev;
>  };
>  
> +static int mtk_reset_assert_set_clr(struct reset_controller_dev *rcdev,
> +       unsigned long id)
> +{
> +       struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
> +       return regmap_write(data->regmap, data->regofs + ((id / 32) << 4), 1);
> +}
> +
> +static int mtk_reset_deassert_set_clr(struct reset_controller_dev *rcdev,
> +       unsigned long id)
> +{
> +       struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
> +       return regmap_write(data->regmap,
> +               data->regofs + ((id / 32) << 4) + 0x4, 1);
> +}

Sorry, the above two are hard to read. Can you add some newlines?

static int mtk_reset_assert_set_clr(struct reset_controller_dev *rcdev,
       unsigned long id)
{
       struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
       unsigned int reg = data->regofs + ((id / 32) << 4);

       return regmap_write(data->regmap, reg, 1);
}

static int mtk_reset_deassert_set_clr(struct reset_controller_dev *rcdev,
       unsigned long id)
{
       struct mtk_reset *data = container_of(rcdev, struct mtk_reset, rcdev);
       unsigned int reg = data->regofs + ((id / 32) << 4) + 0x4;

       return regmap_write(data->regmap, reg, 1);
}


> +
>  static int mtk_reset_assert(struct reset_controller_dev *rcdev,
>                               unsigned long id)
>  {
> @@ -57,14 +72,31 @@ static int mtk_reset(struct reset_controller_dev *rcdev,
>         return mtk_reset_deassert(rcdev, id);
>  }
>  
> +static int mtk_reset_set_clr(struct reset_controller_dev *rcdev,
> +       unsigned long id)
> +{
> +       int ret;

Nitpick: Add a newline between local variables and code.

> +       ret = mtk_reset_assert_set_clr(rcdev, id);
> +       if (ret)
> +               return ret;
> +       return mtk_reset_deassert_set_clr(rcdev, id);
> +}
> +
>  static const struct reset_control_ops mtk_reset_ops = {
>         .assert = mtk_reset_assert,
>         .deassert = mtk_reset_deassert,
>         .reset = mtk_reset,
>  };
>  
> -void mtk_register_reset_controller(struct device_node *np,
> -                       unsigned int num_regs, int regofs)
> +static const struct reset_control_ops mtk_reset_ops_set_clr = {
> +       .assert = mtk_reset_assert_set_clr,
> +               .deassert = mtk_reset_deassert_set_clr,
> +               .reset = mtk_reset_set_clr,

Something weird happened to the tabs and spaces here.

> +};
> +
> +void mtk_register_reset_controller_common(struct device_node *np,
> +                       unsigned int num_regs, int regofs,
> +                       const struct reset_control_ops *reset_ops)
>  {
>         struct mtk_reset *data;
>         int ret;
> diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
> new file mode 100644
> index 0000000..f0d92af
> --- /dev/null
> +++ b/include/dt-bindings/reset-controller/mt8183-resets.h
> @@ -0,0 +1,89 @@
> +/*
> + * Copyright (c) 2017 MediaTek Inc.
> + * Author: Yong Liang, MediaTek
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Please use SPDX tags instead of this boilerplate.