Re: [PATCH v4 4/7] include: fpga: adi-axi-common: add new helper macros

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2025-05-05 at 12:21 -0500, David Lechner wrote:
> On 5/5/25 11:41 AM, Nuno Sá via B4 Relay wrote:
> > From: Nuno Sá <nuno.sa@analog.com>
> > 
> > Add new helper macros and enums to help identifying the platform and some
> > characteristics of it at runtime.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  include/linux/adi-axi-common.h | 35 +++++++++++++++++++++++++++++++++++
> 
> Since this file was moved in the previous patch, should we drop "fpga:" from the
> subject of this patch?

sure

> 
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/include/linux/adi-axi-common.h b/include/linux/adi-axi-common.h
> > index
> > 141ac3f251e6f256526812b9d55cd440a2a46e76..a832ef9b37473ca339a2a2ff8a4a5716d428fd2
> > 9 100644
> > --- a/include/linux/adi-axi-common.h
> > +++ b/include/linux/adi-axi-common.h
> > @@ -12,6 +12,8 @@
> >  #define ADI_AXI_COMMON_H_
> >  
> >  #define ADI_AXI_REG_VERSION			0x0000
> > +#define ADI_AXI_REG_FPGA_INFO			0x001C
> > +#define ADI_AXI_REG_FPGA_VOLTAGE		0x0140
> 
> Doesn't the voltage register only apply to AXI CLKGEN and therefore would
> belong in the clock driver rather than here? 0x1C seems to be the last of the
> defined "common to all IP cores" address before we possibly get into
> core-specific register definitions starting at 0x40.
> 
> I guess there are 1 or 2 other cores that define it at the same place, but it
> still seems not-global.


Hmm, to be honest I did not checked. This patch was out of tree and I blindly pushed
it (as its straightforward). Will drop the above.

> 
> >  
> >  #define ADI_AXI_PCORE_VER(major, minor, patch)	\
> >  	(((major) << 16) | ((minor) << 8) | (patch))
> > @@ -20,4 +22,37 @@
> >  #define ADI_AXI_PCORE_VER_MINOR(version)	(((version) >> 8) & 0xff)
> >  #define ADI_AXI_PCORE_VER_PATCH(version)	((version) & 0xff)
> >  
> > +#define ADI_AXI_INFO_FPGA_TECH(info)            (((info) >> 24) & 0xff)
> > +#define ADI_AXI_INFO_FPGA_FAMILY(info)          (((info) >> 16) & 0xff)
> > +#define ADI_AXI_INFO_FPGA_SPEED_GRADE(info)     (((info) >> 8) & 0xff)
> 
> I guess we don't care about the DEV_PACKAGE field?

ack

> 
> > +#define ADI_AXI_INFO_FPGA_VOLTAGE(val)          ((val) & 0xffff)
> 
> This VOLTAGE also goes applies to the first comment.
> 

ack