Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code

[Krzysztof Kozlowski <krzk@kernel.org>]

On 04/12/2024 14:54, Michal Wilczynski wrote:
> 
> 
> On 12/3/24 20:56, Stephen Boyd wrote:
>> Quoting Michal Wilczynski (2024-12-03 05:41:24)
>>> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
>>> index 7ee0bec1f251..d7cf88390b69 100644
>>> --- a/drivers/clk/thead/Makefile
>>> +++ b/drivers/clk/thead/Makefile
>>> @@ -1,2 +1,2 @@
>>>  # SPDX-License-Identifier: GPL-2.0
>>> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
>>> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
>>
>> Can the -ap driver be extended instead? Or are the clks in a different
>> IO region?
> 
> The Video Output (VO) clocks reside in a different address space as
> defined in the T-HEAD manual 4.4.1 [1]. Therefore, creating a separate
> driver made sense to maintain clarity and adhere to the existing

There is no such rule, no convention even. But there is a rule and
convention of re-using drivers.

> convention of having one driver per subsystem, similar to the

You have here two drivers per subsystem, so even if there was such a
rule, you just broke it.

> AP-specific driver.
> 
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
>>
>>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>>> index 17e32ae08720..a6015805b859 100644
>>> --- a/drivers/clk/thead/clk-th1520-ap.c
>>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>>> @@ -5,297 +5,9 @@
>>>   *  Authors: Yangtao Li <frank.li@vivo.com>
>>>   */
>>>  
>>> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>
>>
>> Presumably this should stay here.
>>
>>> -#include <linux/bitfield.h>
>>> -#include <linux/clk-provider.h>
>>> -#include <linux/device.h>
>>> -#include <linux/module.h>
>>> -#include <linux/platform_device.h>
>>> -#include <linux/regmap.h>
>>

...

>>> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>>> +{
>>> +       return container_of(hw, struct ccu_common, hw);
>>> +}
>>> +
>>> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_mux, common);
>>> +}
>>> +
>>> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_pll, common);
>>> +}
>>> +
>>> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_div, common);
>>> +}
>>> +
>>> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>>> +{
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> +       return container_of(common, struct ccu_gate, common);
>>> +}
>>> +
>>> +extern const struct clk_ops ccu_div_ops;
>>> +extern const struct clk_ops clk_pll_ops;
>>> +extern const struct regmap_config th1520_clk_regmap_config;
>>
>> Why is the regmap config exported?
> 
> The regmap_config is exported to allow reuse across multiple drivers.
> Initially, I passed the clock VOSYS address space using the reg property
> and created the regmap from it, enabling other drivers to utilize the
> same configuration. Later, I switched to a regmap-based syscon approach
> but haven’t moved the regmap_config back to the AP driver.


It anyway in your original code cannot be exported. If you want to use
syscon, then use syscon, not exporting regmaps manually.




Best regards,
Krzysztof