Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 08/15] clock: milbeaut: Add Milbeaut M10V clock
 controller
To:     Stephen Boyd <sboyd@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Michael Turquette <mturquette@baylibre.com>,
        Takao Orito <orito.takao@socionext.com>,
        Kazuhiro Kasai <kasai.kazuhiro@socionext.com>,
        Shinji Kanematsu <kanematsu.shinji@socionext.com>,
        Jassi Brar <jaswinder.singh@linaro.org>,
        Masami Hiramatsu <masami.hiramatsu@linaro.org>
References: <1549628837-31574-1-git-send-email-sugaya.taichi@socionext.com>
 <155087986877.77512.2765555413921453918@swboyd.mtv.corp.google.com>
From:   "Sugaya, Taichi" <sugaya.taichi@socionext.com>
Message-ID: <7b7fbbd8-42cc-a4d3-ecfe-694716509476@socionext.com>
Date:   Mon, 25 Feb 2019 21:13:07 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:60.0) Gecko/20100101
 Thunderbird/60.5.0
MIME-Version: 1.0
In-Reply-To: <155087986877.77512.2765555413921453918@swboyd.mtv.corp.google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

Thank you for your comments.

On 2019/02/23 8:57, Stephen Boyd wrote:
> Quoting Sugaya Taichi (2019-02-08 04:27:17)
>> diff --git a/drivers/clk/clk-milbeaut.c b/drivers/clk/clk-milbeaut.c
>> new file mode 100644
>> index 0000000..f798939
>> --- /dev/null
>> +++ b/drivers/clk/clk-milbeaut.c
>> @@ -0,0 +1,626 @@
> [....]
>> +struct m10v_clk_div_fixed_data {
>> +       const char      *name;
>> +       const char      *parent_name;
>> +       u8              div;
>> +       u8              mult;
>> +       int             onecell_idx;
>> +};
>> +struct m10v_clk_mux_factors {
>> +       const char              *name;
>> +       const char * const      *parent_names;
>> +       u8                      num_parents;
>> +       u32                     offset;
>> +       u8                      shift;
>> +       u8                      mask;
>> +       u32                     *table;
>> +       unsigned long           mux_flags;
>> +       int                     onecell_idx;
>> +};
> 
> Please add newlines between struct definitions. It also wouldn't hurt to
> have kernel-doc on these.
> 

I got it.

>> +
>> +static const struct clk_div_table emmcclk_table[] = {
>> +       { .val = 0, .div = 8 },
>> +       { .val = 1, .div = 9 },
>> +       { .val = 2, .div = 10 },
>> +       { .val = 3, .div = 15 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table mclk400_table[] = {
>> +       { .val = 1, .div = 2 },
>> +       { .val = 3, .div = 4 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table mclk200_table[] = {
>> +       { .val = 3, .div = 4 },
>> +       { .val = 7, .div = 8 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table aclk400_table[] = {
>> +       { .val = 1, .div = 2 },
>> +       { .val = 3, .div = 4 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table aclk300_table[] = {
>> +       { .val = 0, .div = 2 },
>> +       { .val = 1, .div = 3 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table aclk_table[] = {
>> +       { .val = 3, .div = 4 },
>> +       { .val = 7, .div = 8 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table aclkexs_table[] = {
>> +       { .val = 3, .div = 4 },
>> +       { .val = 4, .div = 5 },
>> +       { .val = 5, .div = 6 },
>> +       { .val = 7, .div = 8 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table hclk_table[] = {
>> +       { .val = 7, .div = 8 },
>> +       { .val = 15, .div = 16 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table hclkbmh_table[] = {
>> +       { .val = 3, .div = 4 },
>> +       { .val = 7, .div = 8 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table pclk_table[] = {
>> +       { .val = 15, .div = 16 },
>> +       { .val = 31, .div = 32 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table rclk_table[] = {
>> +       { .val = 0, .div = 8 },
>> +       { .val = 1, .div = 16 },
>> +       { .val = 2, .div = 24 },
>> +       { .val = 3, .div = 32 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table uhs1clk0_table[] = {
>> +       { .val = 0, .div = 2 },
>> +       { .val = 1, .div = 3 },
>> +       { .val = 2, .div = 4 },
>> +       { .val = 3, .div = 8 },
>> +       { .val = 4, .div = 16 },
>> +       { .div = 0 },
>> +};
>> +static const struct clk_div_table uhs2clk_table[] = {
>> +       { .val = 0, .div = 9 },
>> +       { .val = 1, .div = 10 },
>> +       { .val = 2, .div = 11 },
>> +       { .val = 3, .div = 12 },
>> +       { .val = 4, .div = 13 },
>> +       { .val = 5, .div = 14 },
>> +       { .val = 6, .div = 16 },
>> +       { .val = 7, .div = 18 },
>> +       { .div = 0 },
>> +};
> 
> Same comment applies here. Newlines between tables please.
> 

OK.

>> +
>> +static u32 spi_mux_table[] = {0, 1, 2};
>> +static const char * const spi_mux_names[] = {
>> +       M10V_SPI_PARENT0, M10V_SPI_PARENT1, M10V_SPI_PARENT2
>> +};
>> +
>> +static u32 uhs1clk2_mux_table[] = {2, 3, 4, 8};
>> +static const char * const uhs1clk2_mux_names[] = {
>> +       M10V_UHS1CLK2_PARENT0, M10V_UHS1CLK2_PARENT1,
>> +       M10V_UHS1CLK2_PARENT2, M10V_PLL6DIV2
>> +};
>> +
>> +static u32 uhs1clk1_mux_table[] = {3, 4, 8};
>> +static const char * const uhs1clk1_mux_names[] = {
>> +       M10V_UHS1CLK1_PARENT0, M10V_UHS1CLK1_PARENT1, M10V_PLL6DIV2
>> +};
>> +
> [...]
>> +
>> +static const struct m10v_clk_mux_factors m10v_mux_factor_data[] = {
>> +       {"spi", spi_mux_names, ARRAY_SIZE(spi_mux_names),
>> +               CLKSEL(8), 3, 7, spi_mux_table, 0, M10V_SPICLK_ID},
>> +       {"uhs1clk2", uhs1clk2_mux_names, ARRAY_SIZE(uhs1clk2_mux_names),
>> +               CLKSEL(1), 13, 31, uhs1clk2_mux_table, 0, -1},
>> +       {"uhs1clk1", uhs1clk1_mux_names, ARRAY_SIZE(uhs1clk1_mux_names),
>> +               CLKSEL(1), 8, 31, uhs1clk1_mux_table, 0, -1},
>> +       {"nfclk", nfclk_mux_names, ARRAY_SIZE(nfclk_mux_names),
>> +               CLKSEL(1), 22, 127, nfclk_mux_table, 0, M10V_NFCLK_ID},
>> +};
>> +
>> +static u8 m10v_mux_get_parent(struct clk_hw *hw)
>> +{
>> +       struct clk_mux *mux = to_clk_mux(hw);
>> +       u32 val;
>> +
>> +       val = clk_readl(mux->reg) >> mux->shift;
> 
> Please don't use clk_readl() unless you absolutely need it.
> 

OK, I try to use "readl()" simply.

>> +       val &= mux->mask;
>> +
>> +       return clk_mux_val_to_index(hw, mux->table, mux->flags, val);
>> +}
>> +
> [...]
>> +static struct clk_hw *m10v_clk_hw_register_divider(struct device *dev,
>> +               const char *name, const char *parent_name, unsigned long flags,
>> +               void __iomem *reg, u8 shift, u8 width,
>> +               u8 clk_divider_flags, const struct clk_div_table *table,
>> +               spinlock_t *lock, void __iomem *write_valid_reg)
>> +{
>> +       struct m10v_clk_divider *div;
>> +       struct clk_hw *hw;
>> +       struct clk_init_data init;
>> +       int ret;
>> +
>> +       div = kzalloc(sizeof(*div), GFP_KERNEL);
>> +       if (!div)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       init.name = name;
>> +       if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
> 
> Is this used?
> 

Now there are no factors to use "CLK_DIVIDER_READ_ONLY".
I will drop this statement and ro_ops too.

>> +               init.ops = &m10v_clk_divider_ro_ops;
>> +       else
>> +               init.ops = &m10v_clk_divider_ops;
>> +       init.flags = flags;
>> +       init.parent_names = &parent_name;
>> +       init.num_parents = 1;
>> +
>> +       div->reg = reg;
>> +       div->shift = shift;
>> +       div->width = width;
>> +       div->flags = clk_divider_flags;
>> +       div->lock = lock;
>> +       div->hw.init = &init;
>> +       div->table = table;
>> +       div->write_valid_reg = write_valid_reg;
>> +
>> +       /* register the clock */
>> +       hw = &div->hw;
>> +       ret = clk_hw_register(dev, hw);
>> +       if (ret) {
>> +               kfree(div);
>> +               hw = ERR_PTR(ret);
>> +       }
>> +
>> +       return hw;
>> +}
>> +
>> +static int m10v_clk_probe(struct platform_device *pdev)
>> +{
> [...]
>> +       for (id = 0; id < ARRAY_SIZE(m10v_div_fixed_data); ++id) {
>> +               const struct m10v_clk_div_fixed_data *dfd =
>> +                               &m10v_div_fixed_data[id];
>> +               const char *pn = dfd->parent_name ?
>> +                               dfd->parent_name : parent_name;
>> +               hw = clk_hw_register_fixed_factor(NULL, dfd->name,
>> +                                       pn, 0, dfd->mult, dfd->div);
>> +               if (dfd->onecell_idx >= 0)
>> +                       m10v_clk_data->hws[dfd->onecell_idx] = hw;
>> +       }
>> +       for (id = 0; id < ARRAY_SIZE(m10v_mux_factor_data); ++id) {
>> +               const struct m10v_clk_mux_factors *mfd =
>> +                               &m10v_mux_factor_data[id];
>> +               hw = m10v_clk_hw_register_mux(NULL, mfd->name,
>> +                                       mfd->parent_names, mfd->num_parents,
>> +                                       CLK_SET_RATE_PARENT,
>> +                                       base + mfd->offset, mfd->shift,
>> +                                       mfd->mask, mfd->mux_flags, mfd->table,
>> +                                       &m10v_crglock);
>> +               if (mfd->onecell_idx >= 0)
>> +                       m10v_clk_data->hws[mfd->onecell_idx] = hw;
>> +       }
> 
> Similar style nitpick here. Add newlines between for loops. It may also
> make sense to make functions for each of those so that we don't need to
> put all the local variables interspersed throughout the function in each
> for loop.
> 

Yeah OK, I think these statements are bit of ugly too..
I try to make it easy to see.

>> +
>> +       for (id = 0; id < M10V_NUM_CLKS; id++) {
>> +               if (IS_ERR(m10v_clk_data->hws[id]))
>> +                       return PTR_ERR(m10v_clk_data->hws[id]);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id m10v_clk_dt_ids[] = {
>> +       { .compatible = "socionext,milbeaut-m10v-ccu", },
>> +       { },
> 
> Drop the , on the sentinel please. That way nothing can ever go after it
> without causing a compilation error.
> 

OK, drop it.

Thanks,
Sugaya Taichi

>> +};