Messages in this thread | | | Date | Wed, 24 Jul 2019 18:39:06 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next v1 1/4] enetc: Clean up local mdio bus allocation |
| |
> >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(®s->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
| |