Re: [PATCH v4 16/20] phy: Add common Innosilicon HDMI PHY helpers

[Vinod Koul <vkoul@kernel.org>]

On 15-09-26, 17:32, Michal Wilczynski wrote:
> The Innosilicon HDMI PHY IP is used by several SoCs. They differ in
> where the PHY register block sits in the register space and in which
> pixel clocks they support, but the pre-PLL programming sequence and the
> layout of its registers are the same.
> 
> Add a small library holding that shared part: the pre-PLL configuration
> table format, a lookup, clk_ops determine_rate and recalc_rate helpers,
> and the pre-PLL register programming. Callers pass a regmap, a register
> offset for the PHY block, and their own pixel clock table.
> 
> No driver uses it yet; the users are converted separately.

Is there anything in phy patches here that has dependency with rest? If
not consider splitting it up...

> +static u8 inno_read(const struct inno_hdmi_phy_pre_pll *pll, unsigned int reg)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(pll->regmap, inno_reg(pll, reg), &val);
> +	if (ret)
> +		return 0;
> +
> +	return val;

why not just return regmap_read()

There is no logic here, nothing. This is really not ideal.
Also why add a wrapper and not use regmap directly?

> +/**
> + * inno_hdmi_phy_pre_pll_determine_rate - clk_ops.determine_rate helper
> + * @pll: pre-PLL instance
> + * @req: rate request, updated with the rate the PHY would produce
> + *
> + * The PHY can only generate the pixel clocks described by its table, so a
> + * request that does not appear there is rejected rather than rounded.
> + *
> + * Return: 0 on success, -EINVAL if the rate is not supported.
> + */
> +int inno_hdmi_phy_pre_pll_determine_rate(const struct inno_hdmi_phy_pre_pll *pll,
> +					 struct clk_rate_request *req)
> +{
> +	const struct inno_hdmi_phy_pre_pll_config *cfg;
> +	unsigned long rate = rounddown(req->rate, 1000);
> +
> +	for (cfg = pll->table; cfg->pixclock != 0; cfg++) {

Is there a chance of null/garbage if we reach end of table? You dont
know the size of table, and incrementing pointer without bounds does not
look good to me.


-- 
~Vinod

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip