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

[Michal Wilczynski <m.wilczynski@samsung.com>]

Hi Vinod,

On 10/3/26 16:22, Vinod Koul wrote:
> 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...

The phy patches don't depend on anything else so they apply cleanly
from this series alone. The helpers and the Rockchip conversion are
self contained and the JH7110 phy driver only needs its parent at
runtime not at build time.

I kept everything in one series so a single posting applies and boots on
its own. The patches already target three separate trees and everyone
testing this has been applying the full set splitting would force them
to assemble two postings in the correct order, given that the JH7110 phy
driver build depends on the helpers. The v5 cover letter lists which
tree each patch goes to.

That being said, the helpers and the Rockchip conversion are the two
patches independent of the StarFive work - happy to send those as a
separate series if you'd rather take them that way.

> 
>> +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?

The same IP block is used by both Rockchip and the JH7110 but on the
JH7110 the registers sit at a 0x100 offset.That is what inno_reg() is
for.

Dropping the error is wrong though - nd comes from one of those reads
and then becomes the divisor in do_div(vco, nd * 5 ) so a failed read
feeds in 0. I will make it return int and have recalc_rate check.

> 
>> +/**
>> + * 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.

Fair. For now both config tables end with a sentinel, so it cannot
happen. I can add the count of the bounds though.

> 
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>