On Wed, 05 Mar 2025 08:41:28 +0100
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
Hi Thomas,
thanks for having a look!
> On Tue, Mar 04 2025 at 22:23, Andre Przywara wrote:
> >
> > -struct sunxi_sc_nmi_reg_offs {
> > +struct sunxi_sc_nmi_data {
> > u32 ctrl;
> > u32 pend;
> > u32 enable;
> > + u32 enable_val;
>
> The data structure name and the corresponding variable/argument name
> were making the code pretty obvious, but now this is opaque and
> incomprehensible.
>
> data::ctrl does not even give the slightest hint what this is about. You
> need to read up in the code to figure out what it means. Something like:
>
> struct sunxi_sc_nmi_data {
> u32 reg_offs_ctrl;
> u32 reg_offs_pend;
> u32 reg_offs_enable;
> u32 enable_val;
> };
>
> or even better:
>
> struct sunxi_sc_nmi_data {
> struct {
> u32 ctrl;
> u32 pend;
> u32 enable;
> } reg_offs;
> u32 enable_val;
> };
>
> makes it clear and obvious, no?
Sure, will change it, it was just the usual decision between reworking the
existing code or just adding my small change in. 50% chance of getting that
right, I guess ;-)
>
> > +static const struct sunxi_sc_nmi_data sun55i_a523_data __initconst = {
> > + .ctrl = SUN9I_NMI_CTRL,
> > + .pend = SUN9I_NMI_PENDING,
> > + .enable = SUN9I_NMI_ENABLE,
> > + .enable_val = BIT(31),
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
Oops, missed that.
Thanks,
Andre
>
> Thanks,
>
> tglx
