Messages in this thread | | | Date | Wed, 24 Jun 2020 20:41:21 +0530 | From | Vinod Koul <> | Subject | Re: [PATCH v8 2/3] phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver |
| |
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
| |