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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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

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