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

\
 
 \ /
  Last update: 2019-07-24 18:40    [W:0.040 / U:0.516 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site