lkml.org 
[lkml]   [2020]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2020-06-24 17:12    [W:0.754 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site