Re: [PATCH v8 2/3] phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver

[Vinod Koul]

Hi Laurent,

Mostly this looks fine to me, some minor nitpicks below:

On 13-05-20, 20:22, Laurent Pinchart wrote:
> +config PHY_XILINX_ZYNQMP
> +	tristate "Xilinx ZynqMP PHY driver"
> +	depends on ARCH_ZYNQMP

Can we add COMPILE_TEST here so that this driver gets wider compile
coverage?

> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -0,0 +1,995 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * phy-zynqmp.c - PHY driver for Xilinx ZynqMP GT.
> + *
> + * Copyright (C) 2018-20 Xilinx Inc.

2018-2020 please

> +/* Number of GT lanes */
> +#define NUM_LANES			4

Should this be coded in driver like this? Maybe future versions of
hardware will have more lanes..? Why not describe this in DT?
> +
> +/* SIOU SATA control register */
> +#define SATA_CONTROL_OFFSET		0x0100
> +
> +/* Total number of controllers */
> +#define CONTROLLERS_PER_LANE		5

Same question for this as well..

> +/*
> + * I/O Accessors
> + */
> +
> +static inline u32 xpsgtr_read(struct xpsgtr_dev *gtr_dev, u32 reg)
> +{
> +	return readl(gtr_dev->serdes + reg);
> +}
> +
> +static inline void xpsgtr_write(struct xpsgtr_dev *gtr_dev, u32 reg, u32 value)
> +{
> +	writel(value, gtr_dev->serdes + reg);
> +}
> +
> +static inline void xpsgtr_clr_set(struct xpsgtr_dev *gtr_dev, u32 reg,
> +				  u32 clr, u32 set)

wouldn't it be apt to rename this to xpsgtr_modify() and with args as
value and mask, somehow I find that more simpler...

Also, please align second line with opening brace of preceding line

-- 
~Vinod