Re: [PATCH v2 2/2] phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver

From: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
To: Vinod Koul <vkoul@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Johan Jonker <jbx6244@gmail.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Andy Yan <andy.yan@rock-chips.com>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Algea Cao <algea.cao@rock-chips.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-phy@lists.infradead.org, kernel@collabora.com
Subject: Re: [PATCH v2 2/2] phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver
Date: Wed, 7 Feb 2024 14:39:36 +0200	[thread overview]
Message-ID: <374f5069-9f3d-4ad2-a6da-b38a143a0c99@collabora.com> (raw)
In-Reply-To: <ZcNYlFgT0rXiCGbo@matsya>

On 2/7/24 12:16, Vinod Koul wrote:
> On 05-02-24, 13:24, Cristian Ciocaltea wrote:
>> Add driver for the HDMI/eDP TX Combo PHY found on Rockchip RK3588 SoC.

[...]

>> +static const struct reg_sequence hdtpx_common_cmn_init_seq[] = {
>> +	REG_SEQ0(CMN_REG(0009), 0x0c),
>> +	REG_SEQ0(CMN_REG(000A), 0x83),
>> +	REG_SEQ0(CMN_REG(000B), 0x06),
>> +	REG_SEQ0(CMN_REG(000C), 0x20),
>> +	REG_SEQ0(CMN_REG(000D), 0xb8),
>> +	REG_SEQ0(CMN_REG(000E), 0x0f),
>> +	REG_SEQ0(CMN_REG(000F), 0x0f),
> 
> Any reason for these to be mixed case, lets have lower case pls
> everywhere

These were initially part of the register name defines, as specified in
the RK3588 Technical Reference Manual, e.g. HDPTXPHY_CMN_REG000A.  But
consistency is more important, I assume, hence I will do a lower case
conversion.

[...]

>> +static int hdptx_write(struct rockchip_hdptx_phy *hdptx, u32 reg, u8 val)
>> +{
>> +	return regmap_write(hdptx->regmap, reg, val);
>> +}
>> +
>> +#define hdptx_multi_reg_write(hdptx, seq) \
>> +	regmap_multi_reg_write((hdptx)->regmap, seq, ARRAY_SIZE(seq))
>> +
>> +static int hdptx_update_bits(struct rockchip_hdptx_phy *hdptx, u32 reg,
>> +			     u8 mask, u8 val)
>> +{
>> +	return regmap_update_bits(hdptx->regmap, reg, mask, val);
>> +}
>> +
>> +static int hdptx_grf_write(struct rockchip_hdptx_phy *hdptx, u32 reg, u32 val)
>> +{
>> +	return regmap_write(hdptx->grf, reg, val);
>> +}
>> +
>> +static u8 hdptx_grf_read(struct rockchip_hdptx_phy *hdptx, u32 reg)
>> +{
>> +	u32 val;
>> +
>> +	regmap_read(hdptx->grf, reg, &val);
>> +
>> +	return val;
>> +}
> 
> why use wrappers, why not call regmap_ apis directly

Agree, no real benefit, will drop them, except probably
hdptx_multi_reg_write() for the extra savings.

So I'd keep using that one if there's no strong reason against.

[...]

>> +	hdptx_write(hdptx, CMN_REG(0059), (cfg->pms_pdiv << 4) | cfg->pms_refdiv);
>> +	hdptx_write(hdptx, CMN_REG(005A), cfg->pms_sdiv << 4);
> 
> small case here as well pls

Yes, will make sure to handle them all.

Thanks for the review,
Cristian

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy