Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 Aug 2022 10:54:47 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     James Clark <james.clark@arm.com>
Cc:     suzuki.poulose@arm.com, coresight@lists.linaro.org,
        mike.leach@linaro.org, leo.yan@linaro.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/4] coresight: Simplify sysfs accessors by using
 csdev_access abstraction
Message-ID: <20220825165447.GA1910926@p14s>
References: <20220725145221.517776-1-james.clark@arm.com>
 <20220725145221.517776-3-james.clark@arm.com>
 <20220822173735.GC1583519@p14s>
 <f6623ca8-227f-9f92-5b7b-5dbb151b0fb1@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f6623ca8-227f-9f92-5b7b-5dbb151b0fb1@arm.com>
Precedence: bulk

[...]

> >>  
> >>  static struct attribute *coresight_tmc_mgmt_attrs[] = {
> >>  	&dev_attr_rsz.attr,
> >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> >> index 9f445f09fcfe..c1bb93c7c1de 100644
> >> --- a/include/linux/coresight.h
> >> +++ b/include/linux/coresight.h
> >> @@ -372,6 +372,24 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
> >>  	return csa->read(offset, true, false);
> >>  }
> >>  
> >> +static inline u64 csdev_access_relaxed_read_pair(struct csdev_access *csa,
> >> +						 u32 lo_offset, u32 hi_offset)
> > 
> > Parameters lo_offset and hi_offset are s32 in coresight_read_reg_pair()...
> 
> Hi Mathieu,
> 
> I probably should have mentioned this in the commit message. You're
> right that the previous version used signed values, but the csdev
> accessors in include/linux/coresight.h all use u32 and I had to add a
> new one in there for 'csdev_access_relaxed_read_pair()' which would have
> looked very out of place if it was the only one to used signed values.
> 
> Because of this I also changed the 'not set' test for hi_offset from '<
> -1' to '== -1' which would also work with unsigned values. So although
> it looks different, it is still working the same way as before.
> 
> I can think of some possible options to make it better:
> 
>   * Have csdev_access_relaxed_read_pair() be the only csdev_access_
>     function to take signed values

That part is not a big deal for me.

> 
>   * Keep the unsigned type but change the unset value of -1 to be
>     UINT_MAX

I find this counterintuitive and error prone.  And sparse will likely yell
at you profusely.  

> 
>   * Split the accessors into ones that are 64 bit pairs, and ones that
>     are a single read. It's always known when it's defined whether it's
>     a 'pair' or not, so technically this if statement with the 'not set'
>     value isn't actually needed, you just use a different accessor type
> 

That would work.

> I was tempted to do the 3rd one during the refactor, but I wanted to
> keep it more like the original than not. I'm not a fan of the first
> option, I think that would be confusing to read the code and would look
> like a mistake. So I'm more in favour of 2 or 3. What are your thoughts?

Let's meet in the middle and go with option 3.

Thanks,
Mathieu

> 
> > 
> >> +{
> >> +	u64 val;
> >> +
> >> +	if (likely(csa->io_mem)) {
> >> +		val = readl_relaxed(csa->base + lo_offset);
> >> +		val |= (hi_offset == -1) ? 0 :
> >> +		       (u64)readl_relaxed(csa->base + hi_offset) << 32;
> >> +		return val;
> >> +	}
> >> +
> >> +	val = csa->read(lo_offset, true, false);
> >> +	val |= (hi_offset == -1) ? 0 :
> > 
> > ... and hi_offset can't take on a negative value.
> 
> This is just shorthand for UINT_MAX. I could change it to be more
> explicit (option 2 above)?
> 
> > 
> >> +	       (u64)csa->read(hi_offset, true, false) << 32;
> >> +	return val;
> >> +}
> >> +
> >>  static inline u32 csdev_access_read32(struct csdev_access *csa, u32 offset)
> >>  {
> >>  	if (likely(csa->io_mem))
> >> -- 
> >> 2.28.0
> >>