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

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

> > +
> > +int eswin_clk_register_divider(struct device *dev,
> > +			       struct eswin_divider_clock *clks,
> > +			       int nums, struct eswin_clock_data *data)
> > +{
> > +	struct clk_hw *clk_hw;
> > +	int i;
> > +
> > +	for (i = 0; i < nums; i++) {
> > +		clk_hw = clk_hw_register_divider_parent_data
> > +				(dev, clks[i].name, clks[i].parent_data,
> > +				clks[i].flags, data->base + clks[i].offset,
> > +				clks[i].shift, clks[i].width, clks[i].div_flags,
> > +				&data->lock);
> > +
> > +		if (IS_ERR(clk_hw)) {
> > +			while (i--)
> > +				clk_hw_unregister_divider
> > +					(data->clk_data.hws[clks[i].id]);
> 
> All of the other places you are using the devm_ variant to automate the
> cleanup, such as devm_clk_hw_register_gate_parent_data(),
> devm_clk_hw_register_mux_parent_data_table(), and 
> devm_clk_hw_register_divider_parent_hw(). What do you think about adding
> a devm_clk_hw_register_divider_parent_data() for consistency?
> 

Hi Brian and Stephen,

Thank you for the suggestions. We agree that implementing
devm_clk_hw_register_divider_parent_data() is a good approach.
In v14, we'll add this function in clk-provider.h as a separate preparatory patch.
The ESWIN clock driver will then switch to using
devm_clk_hw_register_divider_parent_data() instead of
clk_hw_register_divider_parent_data(), with the driver patch depending on the former.
Does this approach better align with upstream conventions?

> > +			return PTR_ERR(clk_hw);
> > +		}
> > +
> > +		clks[i].hw = *clk_hw;
> > +		data->clk_data.hws[clks[i].id] = clk_hw;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(eswin_clk_register_divider);
> > +
> > +int eswin_clk_register_gate(struct device *dev, struct eswin_gate_clock *clks,
> > +			    int nums, struct eswin_clock_data *data)
> > +{
> > +	struct clk_hw *clk_hw;
> > +	int i;
> > +
> > +	for (i = 0; i < nums; i++) {
> > +		clk_hw = devm_clk_hw_register_gate_parent_data
> > +				(dev, clks[i].name, clks[i].parent_data,
> > +				clks[i].flags, data->base + clks[i].offset,
> > +				clks[i].bit_idx, clks[i].gate_flags,
> > +				&data->lock);
> > +
> > +		if (IS_ERR(clk_hw))
> > +			return PTR_ERR(clk_hw);
> > +
> > +		clks[i].hw = *clk_hw;
> > +		data->clk_data.hws[clks[i].id] = clk_hw;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(eswin_clk_register_gate);
> > +
> > +int eswin_clk_register_clks(struct device *dev, struct eswin_clk_info *clks,
> > +			    int nums, struct eswin_clock_data *data)
> > +{
> > +	struct eswin_clk_info *info;
> > +	const struct clk_hw *phw = NULL;
> > +	struct clk_hw *hw;
> > +	int i;
> > +
> > +	for (i = 0; i < nums; i++) {
> > +		info = &clks[i];
> > +		switch (info->type) {
> > +		case CLK_FIXED_FACTOR: {
> > +			const struct eswin_fixed_factor_clock *factor;
> > +
> > +			factor = &info->data.factor;
> > +			phw = data->clk_data.hws[info->pid];
> > +			hw = devm_clk_hw_register_fixed_factor_parent_hw
> > +					(dev, factor->name, phw, factor->flags,
> > +					factor->mult, factor->div);
> > +			break;
> > +		}
> > +		case CLK_MUX: {
> > +			const struct eswin_mux_clock *mux = &info->data.mux;
> > +
> > +			hw = devm_clk_hw_register_mux_parent_data_table
> > +				(dev, mux->name, mux->parent_data,
> > +				mux->num_parents, mux->flags,
> > +				data->base + mux->offset, mux->shift,
> > +				mux->width, mux->mux_flags, mux->table,
> > +				&data->lock);
> > +			break;
> > +		}
> > +		case CLK_DIVIDER: {
> > +			const struct eswin_divider_clock *div = &info->data.div;
> > +
> > +			phw = data->clk_data.hws[info->pid];
> > +			if (div->priv_flag)
> > +				hw = eswin_register_clkdiv
> > +				(dev, div->id, div->name, phw,
> > +				div->flags, data->base + div->offset,
> > +				div->shift, div->width, div->div_flags,
> > +				div->priv_flag, &data->lock);
> 
> You can now go out to 100 characters for the line lengths instead of 80, however,
> I'm not sure how Stephen feels about that. Personally I think it'd make this
> block, plus some others in this series a bit cleaner. Taking into account the
> current indentation, this block could become this with 100 characters as the max:
> 
>     hw = eswin_register_clkdiv(dev, div->id, div->name, phw,
>                                div->flags, data->base + div->offset,
>                                div->shift, div->width, div->div_flags,
>                                div->priv_flag, &data->lock);
> 

Stephen,
Brian's feedback on line length was very helpful. 
For v14, we've kept lines within 80 characters wherever possible.
In a few cases, such as function calls with long parameter lists, we've kept 
slightly longer lines to preserve readability, 
but we're happy to rewrap them if preferred.
Does this approach work for the clk subsystem?

Regards,
Xuyang Dong