Thread (8 messages) 8 messages, 2 authors, 2019-07-24

Re: [PATCH net-next v1 1/4] enetc: Clean up local mdio bus allocation

From: Andrew Lunn <andrew@lunn.ch>
Date: 2019-07-24 16:39:14
Also in: linux-arm-kernel, linux-devicetree, lkml

quoted
All the horrible casts go away, the driver is structured like every
other driver, sparse is probably happy, etc.
This looks more like a matter cosmetic preferences.  I mean, I didn't
notice anything "horrible" in the code so far.
#define bus_to_enetc_regs(bus)  (struct enetc_mdio_regs __iomem *)((bus)->priv)

You should not need a cast here, bus->priv is a void *. But bus->priv
is being abused to hold a __iomem pointer.

enetc_wr_reg(&regs->mdio_cfg, mdio_cfg);

This is also rather odd, passing the address of something to an IO
operator? I also don't know the C standard well enough to know if it
is guaranteed that:

struct enetc_mdio_regs {
        u32     mdio_cfg;       /* MDIO configuration and status */
        u32     mdio_ctl;       /* MDIO control */
        u32     mdio_data;      /* MDIO data */
        u32     mdio_addr;      /* MDIO address */
};

actually works. On a 64bit system is the compiler allowed to put in
padding to keep the u32 64 bit aligned?
I actually find it more
ugly to define a new structure with only one element inside, like:
struct enetc_mdio_priv {
       struct enetc_hw *hw;
}
One advantage of this is that struct enetc_hw correctly has all the
__iomem attributes. All the casts to __iomem go away, and sparse is
happy.
Anyway, if others already did this in the kernel, what can I do?
Clean it up. Make the code more readable and easy to maintain.

      Andrew
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help