Messages in this thread | | | Date | Thu, 25 Aug 2022 10:54:47 -0600 | From | Mathieu Poirier <> | Subject | Re: [PATCH v1 2/4] coresight: Simplify sysfs accessors by using csdev_access abstraction |
| |
[...]
> >> > >> 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 > >>
| |