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
