Re: [PATCH v4 3/5] PCI: cadence: Add header support for PCIe HPA controller

["Peter Chen (CIX)" <peter.chen@kernel.org>]

On 25-04-24 09:04:42, hans.zhang@cixtech.com wrote:
> From: Manikandan K Pillai <mpillai@cadence.com>
> 
> +/**
> + * struct cdns_plat_pcie_of_data - Register bank offset for a platform
> + * @is_rc: controller is a RC
> + * @is_hpa: Controller architecture is HPA
> + * @ip_reg_bank_off: ip register bank start offset
> + * @ip_cfg_ctrl_reg_off: ip config control register start offset
> + * @axi_mstr_common_off: AXI master common register start
> + * @axi_slave_off: AXI slave offset start
> + * @axi_master_off: AXI master offset start
> + * @axi_hls_off: AXI HLS offset start
> + * @axi_ras_off: AXI RAS offset
> + * @axi_dti_off: AXI DTI offset

The variable's suffix _off may confuse the reader, since off stands for
something is turned off and also used at device driver commonly,
suggest using _offset and align with your other code.

> + */
> +struct cdns_plat_pcie_of_data {
> +	u32 is_rc:1;
> +	u32 is_hpa:1;
> +	u32 ip_reg_bank_off;
> +	u32 ip_cfg_ctrl_reg_off;
> +	u32 axi_mstr_common_off;
> +	u32 axi_slave_off;
> +	u32 axi_master_off;
> +	u32 axi_hls_off;
> +	u32 axi_ras_off;
> +	u32 axi_dti_off;
>  };
>  
> +static inline void cdns_pcie_hpa_writel(struct cdns_pcie *pcie,
> +					enum cdns_pcie_reg_bank bank,
> +					u32 reg,
> +					u32 value)
> +{
> +	u32 offset =  cdns_reg_bank_to_off(pcie, bank);

More than one blank space after "=".

> +}
> +
> +static inline u32 cdns_pcie_hpa_readl(struct cdns_pcie *pcie,
> +				      enum cdns_pcie_reg_bank bank,
> +				      u32 reg)
> +{
> +	u32 offset =  cdns_reg_bank_to_off(pcie, bank);

ditto

-- 

Best regards,
Peter