Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 18 Mar 2020 20:23:57 +0000
From:   Will Deacon <will@kernel.org>
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc:     Tuan Phan <tuanphan@os.amperecomputing.com>,
        Mark Rutland <mark.rutland@arm.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] driver/perf: Add PMU driver for the ARM DMC-620 memory
 controller.
Message-ID: <20200318202356.GB7463@willie-the-truck>
References: <1584491381-31492-1-git-send-email-tuanphan@os.amperecomputing.com>
 <20200318160202.0000032c@Huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200318160202.0000032c@Huawei.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 18, 2020 at 04:02:02PM +0000, Jonathan Cameron wrote:
> On Tue, 17 Mar 2020 17:29:38 -0700
> Tuan Phan <tuanphan@os.amperecomputing.com> wrote:
> > +/* User ABI */
> > +#define ATTR_CFG_FLD_mask_CFG		config
> > +#define ATTR_CFG_FLD_mask_LO		0
> > +#define ATTR_CFG_FLD_mask_HI		44
> > +#define ATTR_CFG_FLD_match_CFG		config1
> > +#define ATTR_CFG_FLD_match_LO		0
> > +#define ATTR_CFG_FLD_match_HI		44
> > +#define ATTR_CFG_FLD_invert_CFG		config2
> > +#define ATTR_CFG_FLD_invert_LO		0
> > +#define ATTR_CFG_FLD_invert_HI		0
> > +#define ATTR_CFG_FLD_incr_CFG		config2
> > +#define ATTR_CFG_FLD_incr_LO		1
> > +#define ATTR_CFG_FLD_incr_HI		2
> > +#define ATTR_CFG_FLD_event_CFG		config2
> > +#define ATTR_CFG_FLD_event_LO		3
> > +#define ATTR_CFG_FLD_event_HI		8
> > +
> > +#define __GEN_PMU_FORMAT_ATTR(cfg, lo, hi)			\
> > +	(lo) == (hi) ? #cfg ":" #lo "\n" : #cfg ":" #lo "-" #hi
> > +
> > +#define _GEN_PMU_FORMAT_ATTR(cfg, lo, hi)			\
> > +	__GEN_PMU_FORMAT_ATTR(cfg, lo, hi)
> > +
> > +#define GEN_PMU_FORMAT_ATTR(name)				\
> > +	PMU_FORMAT_ATTR(name,					\
> > +	_GEN_PMU_FORMAT_ATTR(ATTR_CFG_FLD_##name##_CFG,		\
> > +			     ATTR_CFG_FLD_##name##_LO,		\
> > +			     ATTR_CFG_FLD_##name##_HI))
> > +
> > +#define _ATTR_CFG_GET_FLD(attr, cfg, lo, hi)			\
> > +	((((attr)->cfg) >> lo) & GENMASK(hi - lo, 0))
> 
> Hmm. I see this came form SPE pmu.
> 
> Personally I'd argue this makes the code harder to read than doing
> most of it long hand.  Ah well.

I agree that it's harder to read, but I did it this way in the SPE driver
so that the user ABI is always in sync with what the driver thinks, because
the accessors and the sysfs bits are all generated from the same constants.
If you screw that up, then it's really hard to fix without breaking
userspace.

Will