Re: Re: [PATCH 2/3] clk: eswin: Add eic7700 HSP clock driver

["Xuyang Dong" <dongxuyang@eswincomputing.com>]

> > +
> > +static struct clk_parent_data hsp_usb_sata[] = {
> > +	{ .fw_name = "hsp_sata" }
> > +};
> > +
> > +static struct eswin_fixed_factor_clock eic7700_hsp_factor_clks[] = {
> 
> More places for static const? I'll leave out the others.
> 

Hi Brian,

When registering a clock, the 'hw' field in the structure will be assigned, 
so these clock structures cannot be declared const.

Other comments will be fixed in the next version.

Best regards,
Xuyang Dong

> > +	ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_CFG_DIV2, "factor_hsp_cfg_div2",
> > +		     hsp_cfg, 1, 2, 0),
> > +	ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_CFG_DIV4, "factor_hsp_cfg_div4",
> > +		     hsp_cfg, 1, 4, 0),
> > +	ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_MMC_DIV10, "factor_hsp_mmc_div10",
> > +		     hsp_mmc, 1, 10, 0),
> > +};
> > +
> > +static struct eswin_gate_clock eic7700_hsp_gate_clks[] = {
> > +	ESWIN_GATE(EIC7700_HSP_CLK_GATE_SATA, "gate_clk_hsp_sata", hsp_usb_sata,
> > +		   CLK_SET_RATE_PARENT, EIC7700_HSP_SATA_REG, 28, 0),
> > +	ESWIN_GATE(EIC7700_HSP_CLK_GATE_MSHC0_TMR, "gate_clk_hsp_mshc0_tmr",
> > +		   hsp_mmc, CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC0_REG, 8, 0),
> > +	ESWIN_GATE(EIC7700_HSP_CLK_GATE_MSHC1_TMR, "gate_clk_hsp_mshc1_tmr",
> > +		   hsp_mmc, CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC1_REG, 8, 0),
> > +	ESWIN_GATE(EIC7700_HSP_CLK_GATE_MSHC2_TMR, "gate_clk_hsp_mshc2_tmr",
> > +		   hsp_mmc, CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC2_REG, 8, 0),
> > +};
> > +
> > +static struct eic7700_hsp_clk_gate eic7700_hsp_spec_gate_clks[] = {
> > +	EIC7700_HSP_GATE(EIC7700_HSP_CLK_GATE_USB0, "gate_clk_hsp_usb0",
> > +			 hsp_usb_sata, CLK_SET_RATE_PARENT,
> > +			 EIC7700_HSP_USB0_REG, 28, EIC7700_HSP_USB0_REF_REG),
> > +	EIC7700_HSP_GATE(EIC7700_HSP_CLK_GATE_USB1, "gate_clk_hsp_usb1",
> > +			 hsp_usb_sata, CLK_SET_RATE_PARENT,
> > +			 EIC7700_HSP_USB1_REG, 28, EIC7700_HSP_USB1_REF_REG),
> > +};
> > +
> > +static const struct clk_parent_data mux_mmc_3mux1_p[] = {
> > +	{ .fw_name = "hsp_cfg" },
> > +	{ .hw = &eic7700_hsp_factor_clks[0].hw },
> > +	{ .hw = &eic7700_hsp_factor_clks[1].hw },
> > +};
> > +
> > +static const struct clk_parent_data mux_mmc_2mux1_p[] = {
> > +	{ .fw_name = "hsp_mmc" },
> > +	{ .hw = &eic7700_hsp_factor_clks[2].hw },
> > +};
> > +
> > +static u32 mux_mmc_3mux1_tbl[] = { 0x0, 0x1, 0x3 };
> > +
> > +static struct eswin_mux_clock eic7700_hsp_mux_clks[] = {
> > +	ESWIN_MUX_TBL(EIC7700_HSP_CLK_MUX_EMMC_3MUX1, "mux_hsp_emmc_3mux1",
> > +		      mux_mmc_3mux1_p, ARRAY_SIZE(mux_mmc_3mux1_p),
> > +		      CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC0_REG, 16, 2, 0,
> > +		      mux_mmc_3mux1_tbl),
> > +	ESWIN_MUX_TBL(EIC7700_HSP_CLK_MUX_SD0_3MUX1, "mux_hsp_sd0_3mux1",
> > +		      mux_mmc_3mux1_p, ARRAY_SIZE(mux_mmc_3mux1_p),
> > +		      CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC1_REG, 16, 2, 0,
> > +		      mux_mmc_3mux1_tbl),
> > +	ESWIN_MUX_TBL(EIC7700_HSP_CLK_MUX_SD1_3MUX1, "mux_hsp_sd1_3mux1",
> > +		      mux_mmc_3mux1_p, ARRAY_SIZE(mux_mmc_3mux1_p),
> > +		      CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC2_REG, 16, 2, 0,
> > +		      mux_mmc_3mux1_tbl),
> > +	ESWIN_MUX(EIC7700_HSP_CLK_MUX_EMMC_CQE_2MUX1, "mux_hsp_emmc_cqe_2mux1",
> > +		  mux_mmc_2mux1_p, ARRAY_SIZE(mux_mmc_2mux1_p),
> > +		  CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC0_REG, 0, 1, 0),
> > +	ESWIN_MUX(EIC7700_HSP_CLK_MUX_SD0_CQE_2MUX1, "mux_hsp_sd0_cqe_2mux1",
> > +		  mux_mmc_2mux1_p, ARRAY_SIZE(mux_mmc_2mux1_p),
> > +		  CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC1_REG, 0, 1, 0),
> > +	ESWIN_MUX(EIC7700_HSP_CLK_MUX_SD1_CQE_2MUX1, "mux_hsp_sd1_cqe_2mux1",
> > +		  mux_mmc_2mux1_p, ARRAY_SIZE(mux_mmc_2mux1_p),
> > +		  CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC2_REG, 0, 1, 0),
> > +};
> > +
> > +static struct eswin_clk_info eic7700_hsp_clks[] = {
> > +	ESWIN_GATE_TYPE(EIC7700_HSP_CLK_GATE_EMMC, "gate_clk_hsp_emmc",
> > +			EIC7700_HSP_CLK_MUX_EMMC_3MUX1,
> > +			CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> > +			EIC7700_HSP_MSHC0_REG, 24, 0),
> > +	ESWIN_GATE_TYPE(EIC7700_HSP_CLK_GATE_SD0, "gate_clk_hsp_sd0",
> > +			EIC7700_HSP_CLK_MUX_SD0_3MUX1,
> > +			CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> > +			EIC7700_HSP_MSHC1_REG, 24, 0),
> > +	ESWIN_GATE_TYPE(EIC7700_HSP_CLK_GATE_SD1, "gate_clk_hsp_sd1",
> > +			EIC7700_HSP_CLK_MUX_SD1_3MUX1,
> > +			CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> > +			EIC7700_HSP_MSHC2_REG, 24, 0),
> > +};
> > +
> > +static int eic7700_hsp_clk_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct auxiliary_device *adev;
> > +	struct eswin_clock_data *data;
> > +	struct clk_hw *hw;
> > +	int i, ret;
> > +
> > +	data = eswin_clk_init(pdev, EIC7700_HSP_NR_CLKS);
> > +	if (IS_ERR(data))
> > +		return dev_err_probe(dev, PTR_ERR(data),
> > +				     "failed to get clk data!\n");
> > +
> > +	ret = eswin_clk_register_fixed_factor
> > +		(dev, eic7700_hsp_factor_clks,
> > +		ARRAY_SIZE(eic7700_hsp_factor_clks), data);
> 
> The first two lines can be combined together to improve the formatting:
> 
>      ret = eswin_clk_register_fixed_factor(dev, eic7700_hsp_factor_clks,
> 
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to register fixed factor clock\n");
> > +
> > +	ret = eswin_clk_register_gate(dev, eic7700_hsp_gate_clks,
> > +				      ARRAY_SIZE(eic7700_hsp_gate_clks), data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to register gate clock\n");
> > +
> > +	ret = eswin_clk_register_mux(dev, eic7700_hsp_mux_clks,
> > +				     ARRAY_SIZE(eic7700_hsp_mux_clks),
> > +				     data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to register mux clock\n");
> > +
> > +	ret = eswin_clk_register_clks(dev, eic7700_hsp_clks,
> > +				      ARRAY_SIZE(eic7700_hsp_clks), data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to register clock\n");
> > +
> > +	for (i = 0; i < ARRAY_SIZE(eic7700_hsp_spec_gate_clks); i++) {
> > +		struct eic7700_hsp_clk_gate *gate;
> > +
> > +		gate = &eic7700_hsp_spec_gate_clks[i];
> > +		hw = hsp_clk_register_gate(dev, gate->id, gate->name,
> > +					   gate->parent_data, gate->flags,
> > +					   data->base + gate->offset,
> > +					   data->base + gate->ref_offset,
> > +					   gate->bit_idx, 0, &data->lock);
> > +		if (IS_ERR(hw))
> > +			return dev_err_probe(dev, PTR_ERR(hw),
> > +					     "failed to register gate clock\n");
> > +
> > +		data->clk_data.hws[gate->id] = hw;
> > +	}
> > +
> > +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> > +					  &data->clk_data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "add clk provider failed\n");
> > +
> > +	adev = devm_auxiliary_device_create(dev, "hsp-reset",
> > +					    (__force void *)data->base);
> 
> So this driver is sharing the same register space with the reset driver,
> and the reset driver calls devm_regmap_init_mmio(). What do you think
> about having a shared regmap between the two drivers so that the __force
> is not needed?
> 
> Brian
> 
> 
> > +	if (!adev)
> > +		return dev_err_probe(dev, -ENODEV,
> > +				     "register hsp-reset device failed\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id eic7700_hsp_clock_dt_ids[] = {
> > +	{ .compatible = "eswin,eic7700-hspcrg", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, eic7700_hsp_clock_dt_ids);
> > +
> > +static struct platform_driver eic7700_hsp_clock_driver = {
> > +	.probe	= eic7700_hsp_clk_probe,
> > +	.driver = {
> > +		.name	= "eic7700-hsp-clock",
> > +		.of_match_table	= eic7700_hsp_clock_dt_ids,
> > +	},
> > +};
> > +
> > +module_platform_driver(eic7700_hsp_clock_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Xuyang Dong <dongxuyang@eswincomputing.com>");
> > +MODULE_DESCRIPTION("ESWIN EIC7700 HSP clock controller driver");
> > -- 
> > 2.34.1
> >