Re: [PATCH v4 4/5] clk: microchip: add PolarFire SoC fabric clock support

[Conor.Dooley]

On 06/09/2022 08:28, Claudiu Beznea - M18063 wrote:
> On 30.08.2022 15:28, Conor Dooley wrote:
>> Add a driver to support the PLLs in PolarFire SoC's Clock Conditioning
>> Circuitry, an instance of which is located in each ordinal corner of
>> the FPGA. Only get_rate() is supported as these clocks are intended to
>> be statically configured by the FPGA design. Currently, the DLLs are
>> not supported by this driver. For more information on the hardware, see
>> "PolarFire SoC FPGA Clocking Resources" in the link below.
>>
>> Link: https://onlinedocs.microchip.com/pr/GUID-8F0CC4C0-0317-4262-89CA-CE7773ED1931-en-US-1/index.html
>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>

>> +
>> +#define CLK_CCC_PLL(_id, _parents, _shift, _width, _flags, _offset) {		\
>> +	.id = _id,									\
>> +	.shift = _shift,								\
>> +	.width = _width,								\
>> +	.reg_offset = _offset,								\
>> +	.flags = _flags,								\
>> +	.parents = _parents,\
> 
> There is a bit of missalignment of \ in this macro.
> 

Eww, there is...

>> +static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clock *pll_hws,
>> +				  unsigned int num_clks, struct mpfs_ccc_data *data)
>> +{
>> +	int ret;
>> +
>> +	for (unsigned int i = 0; i < num_clks; i++) {
>> +		struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
>> +		char *name = devm_kzalloc(dev, 18, GFP_KERNEL);
>> +
>> +		pll_hw->base = data->pll_base[i];
>> +		snprintf(name, 18, "ccc%s_pll%u", strchrnul(dev->of_node->full_name, '@'), i);
>> +		pll_hw->name = (const char *)name;
>> +		pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
>> +								      pll_hw->parents,
>> +								      &mpfs_ccc_pll_ops, 0);
>> +
>> +		ret = devm_clk_hw_register(dev, &pll_hw->hw);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret, "failed to register ccc id: %d\n",
>> +					     pll_hw->id);
>> +
>> +		data->hw_data.hws[pll_hw->id] = &pll_hw->hw;
>> +
>> +		ret = mpfs_ccc_register_outputs(dev, mpfs_ccc_pllout_clks[i],
>> +						MPFS_CCC_OUTPUTS_PER_PLL, data, pll_hw);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mpfs_ccc_probe(struct platform_device *pdev)
>> +{
>> +	struct mpfs_ccc_data *clk_data;
>> +	void __iomem *pll_base[ARRAY_SIZE(mpfs_ccc_pll_clks)];
>> +	unsigned int num_clks;
>> +	int ret;
>> +
>> +	num_clks = ARRAY_SIZE(mpfs_ccc_pll_clks) + ARRAY_SIZE(mpfs_ccc_pll0out_clks)
>> +		+ ARRAY_SIZE(mpfs_ccc_pll1out_clks);
> 
> Usually, as far as I can tell, there are no (or not too much) lines
> starting with arithmetic signs, but these goes at the end of the previous
> lines. This allows you to also align the ARRAY_SIZE() macros to each others.
> 
> Other than that:
> 
> Reviewed-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
> 

I think I just started the line with the operator as I find it
easier to read that way around, but not a big deal.

Thanks,
Conor.