Hi Marek, On Sat, 21 Mar 2020 at 11:00, Marek Vasut <[email protected]> wrote: > > On 3/21/20 5:42 PM, Simon Glass wrote: > > Hi Marek, > > Hi, > > > On Sat, 21 Mar 2020 at 09:09, Marek Vasut <[email protected]> wrote: > >> > >> On 3/21/20 3:12 PM, Simon Glass wrote: > >>> Hi Marek, > >>> > >>> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <[email protected]> wrote: > >>>> > >>>> On 3/11/20 7:50 AM, Chunfeng Yun wrote: > >>>> [...] > >>>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are > >>>>> used > >>>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are > >>>>> used > >>>>> + * @u2_phy_pll: usb2 phy pll control register > >>>>> + */ > >>>>> +struct mtk_ippc_regs { > >>>>> + __le32 ip_pw_ctr0; > >>>>> + __le32 ip_pw_ctr1; > >>>>> + __le32 ip_pw_ctr2; > >>>> > >>>> Please define the registers with #define macros , this struct-based > >>>> approach doesn't scale. > >>>> > >>> > >>> What does this mean? I much prefer the struct approach, unless the > >>> registers move around in future generations. > >> > >> This means I want to see #define MTK_XHCI_<register name> 0xf00 > >> > >> The struct based approach doesn't scale and on e.g. altera is causing us > >> a massive amount of problems now. It looked like a good idea back then, > >> but now it's a massive burden, so I don't want that to proliferate. And > >> here I would expect the registers to move around, just like everywhere > >> else. > > > > That sounds like something that is easily avoided. For example, > > Designware doesn't seem to move things around. > > > > Perhaps MediaTek has a policy of not doing this either? > > Such policies change as the hardware evolves. I got burned by this > struct-based approach more often then it was beneficial, if it ever > really was beneficial, hence I don't want to see it used.
Some benefits: - using C struct instead of 'assembler-like' addition is less error-prone - you can pass the regs struct around between functions in the driver, particularly helpful in large drivers - lower-case letters are reasier to read - you can specify the data size and endianness in the struct using C types If the hardware changes enough, then you need a new driver anyway, or perhaps separate C implementations of the low-level register access if the high-level flow is similar. In general you can't always determine this sort of thing at compile time since the version of the hardware may not be apparent until run-time. You end up with variables holding the offset of each register. Sometimes is it better to have an umbrella driver with a common core. So I would push back against a move in this direction in general. It depends on the circumstances, and anyway, converting from struct to offsets if desirable later is not that hard. Regards, Simon
