Hi Stephen, Thank you for your time. Please see my comments below.
On 2019/1/10 4:48, Stephen Boyd wrote: > Quoting Jianxin Pan (2018-12-17 08:24:10) >> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c >> new file mode 100644 >> index 0000000..2582a98 >> --- /dev/null >> +++ b/drivers/clk/meson/mmc-clkc.c >> @@ -0,0 +1,304 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Amlogic Meson MMC Sub Clock Controller Driver >> + * >> + * Copyright (c) 2017 Baylibre SAS. >> + * Author: Jerome Brunet <[email protected]> >> + * >> + * Copyright (c) 2018 Amlogic, inc. >> + * Author: Yixun Lan <[email protected]> >> + * Author: Jianxin Pan <[email protected]> >> + */ >> + >> +#include <linux/clk.h> > > Do you need this include? > Yes, It's also a clock-consumer. devm_clk_get() gets clock from dts as the parent of mux. Thank you for your review. >> +#include <linux/clk-provider.h> >> +#include <linux/module.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> >> +#include <linux/of_device.h> >> +#include <linux/mfd/syscon.h> > > Is this used? Yes, syscon_node_to_regmap() is used in mmc_clkc_probe(). > >> +#include <linux/platform_device.h> > [...] >> + init.num_parents = 1; >> + >> + clk = mmc_clkc_register_clk(dev, map, &init, suffix, data); >> + if (IS_ERR(clk)) >> + dev_err(dev, "%s clock registration failed\n", suffix); >> + >> + return clk; >> +} >> + >> +static int mmc_clkc_probe(struct platform_device *pdev) >> +{ >> + struct clk_hw_onecell_data *onecell_data; >> + struct device *dev = &pdev->dev; >> + struct mmc_clkc_data *data; >> + struct regmap *map; >> + struct clk_regmap *clk, *core; >> + struct meson_sclk_div_data *div_data; >> + >> + /*cast to drop the const in match->data*/ >> + data = (struct mmc_clkc_data *)of_device_get_match_data(dev); > > Is that necessary? Maybe the users of this should take a const argument > and copy things? OK, I can alloc two extra memory for PHASE_RX and PHASE_TX data, and then copy from const data to them. > >> + if (!data) >> + return -ENODEV; >> + >> + map = syscon_node_to_regmap(dev->of_node); >> + if (IS_ERR(map)) { >> + dev_err(dev, "could not find mmc clock controller\n"); >> + return PTR_ERR(map); >> + } >> + >> + onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) + >> + sizeof(*onecell_data->hws) * >> MMC_MAX_CLKS, > > struct_size()? > OK, I will change it in the next version. Thank you for your review. > . >
