Received-SPF: pass (google.com: domain of linux-kernel+bounces-56490-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <374f5069-9f3d-4ad2-a6da-b38a143a0c99@collabora.com>
Date: Wed, 7 Feb 2024 14:39:36 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] phy: rockchip: Add Samsung HDMI/eDP Combo PHY
 driver
Content-Language: en-US
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
References: <20240205-phy-hdptx-v2-0-a7150814c047@collabora.com>
 <20240205-phy-hdptx-v2-2-a7150814c047@collabora.com>
 <ZcNYlFgT0rXiCGbo@matsya>
From: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
In-Reply-To: <ZcNYlFgT0rXiCGbo@matsya>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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