RE: [PATCH net-next 1/3] Add LAN743X driver

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, August 11, 2017 6:12 PM
To: Bryan Whitehead - C21958
Cc: netdev@vger.kernel.org; UNGLinuxDriver
Subject: Re: [PATCH net-next 1/3] Add LAN743X driver

From: <Bryan.Whitehead@microchip.com>
Date: Fri, 11 Aug 2017 19:47:57 +0000

quoted

+static int lan743x_pci_init(struct lan743x_adapter *adapter,
+			    struct pci_dev *pdev)
+{
+	int ret = -ENODEV;
+	int bars = 0;
+	struct lan743x_pci *pci = &adapter->pci;

Please always order local variable declarations from longest to shortest line
(reverse christmas tree format).

quoted

+	pci_set_master(pdev);
+
+clean_up:
+	if (ret) {

It is more intuitive to structure this like:

	return 0;

clean_up:
 ...

quoted

+static u8 __iomem *lan743x_pci_get_bar_address(struct lan743x_adapter

*adapter,

quoted

+					       int bar_index)
+{
+	u8 __iomem *result = NULL;
+	struct lan743x_pci *pci = &adapter->pci;

Reverse christmas tree ordering please.

quoted

+static int lan743x_csr_light_reset(struct lan743x_adapter *adapter) {
+	int result = -EIO;
+	u32 data;
+	unsigned long timeout;

Likewise.

quoted

+static inline void lan743x_csr_write(
+	struct lan743x_adapter *adapter, int offset, u32 data)

Don't do the argument formatting like this please, it looks terrible.

This:

static inline void lan743x_csr_write(struct lan743x_adapter *adapter,
				     int offset, u32 data)

works much better.

quoted

+static void lan743x_intr_union_isr(void *context, u32 int_sts) {
+	struct lan743x_adapter *adapter = (struct lan743x_adapter

*)context;

Casts from void pointers are never necessary, that's the whole point of void
pointers.  Please remove this cast.

quoted

+static irqreturn_t lan743x_vector_isr(int irq, void *ptr) {
+	irqreturn_t result = IRQ_NONE;
+	struct lan743x_vector *vector = (struct lan743x_vector *)ptr;
+	struct lan743x_adapter *adapter = NULL;
+	u32 int_sts;
+	u32 mask;

Reverse christmas tree ordering please.

quoted

+static int lan743x_intr_open(struct lan743x_adapter *adapter) {
+	int ret = -ENODEV;
+	struct lan743x_intr *intr = &adapter->intr;
+	int index = 0;

Likewise.

quoted

+static int lan743x_dp_wait_till_not_busy(struct lan743x_adapter
+*adapter) {
+	int i;
+	u32 dp_sel = 0;

Likewise.

quoted

+static int lan743x_dp_write(struct lan743x_adapter *adapter,
+			    u32 select, u32 addr, u32 length, u32 *buf) {
+	struct lan743x_dp *dp = &adapter->dp;
+	int ret = -EIO;
+	int i;
+	u32 dp_sel;

Likewise.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static int lan743x_gpio_reserve_ptp_output(struct lan743x_adapter

*adapter,

quoted

+					   int bit, int ptp_channel)
+{
+	struct lan743x_gpio *gpio = &adapter->gpio;
+	int ret = -EBUSY;
+	unsigned long irq_flags = 0;
+	int bit_mask = BIT(bit);

Likewise.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static int lan743x_ptpci_adjfreq(struct ptp_clock_info *ptpci, s32
+delta_ppb) {
+	u32 u32_delta = 0;
+	u64 u64_delta = 0;
+	u32 lan743x_rate_adj = 0;
+	bool positive = true;
+	struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
+	struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;

Likewise.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static int lan743x_ptpci_adjtime(struct ptp_clock_info *ptpci, s64
+delta) {
+	struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
+	struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;

Likewise.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static int lan743x_ptpci_gettime64(struct ptp_clock_info *ptpci,
+				   struct timespec64 *ts)
+{
+	struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
+	struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;

Likewise.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static int lan743x_ptpci_settime64(struct ptp_clock_info *ptpci,
+				   const struct timespec64 *ts)
+{
+	struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
+	struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;

Likewise.

Also, these X_TO_Y macros are terrible.  If you aren an accessor like that,
make it a nice inline function declared in a header file with lowercase letter
which actually does type checking on the pointer variable which is
dereferenced.

quoted

+	if (ts) {
+		u32 seconds = 0;
+		u32 nano_seconds = 0;

Reverse christmas tree ordering please.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter) {
+	struct lan743x_ptp *ptp = &adapter->ptp;
+	int result = -ENODEV;
+	u32 current_seconds = 0;
+	u32 target_seconds = 0;
+	u32 general_config = 0;

Likewise.

quoted

+static void lan743x_ptp_isr(void *context) {
+	int enable_flag = 1;
+	u32 ptp_int_sts = 0;
+	struct lan743x_adapter *adapter = (struct lan743x_adapter

*)context;

quoted

+	struct lan743x_ptp *ptp = NULL;

Likewise.  And again please remove the void pointer cast.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static int lan743x_ptp_reserve_event_ch(struct lan743x_adapter
+*adapter) {
+	struct lan743x_ptp *ptp = &adapter->ptp;
+	int index = 0;
+	int result = -ENODEV;

Likewise.

quoted

+#ifdef CONFIG_PTP_1588_CLOCK
+static void lan743x_ptp_clock_step(struct lan743x_adapter *adapter,
+				   s64 time_step_ns)
+{
+	struct lan743x_ptp *ptp = &adapter->ptp;
+	u64 abs_time_step_ns = 0;
+	s32 seconds = 0;
+	u32 nano_seconds = 0;

Likewise.

This thing is huge, I'm not reviewing any more of this enormous submission.

David,

Thanks for the feedback. 
I will work on your suggestions, and submit again later.

Bryan