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
