[PATCH] [XILINX][HWICAP] Xilinx Internal Configuration Access Port device driver.

[Stephen Neuendorffer]

Peter,

Sorry, this has taken so long...  I've undertaken some significant
refactorings, and included support for the new EDK9.2 xps_hwicap.
Responses below.

> Please don't put the HWICAP option in the middle of the HVC options.

oops!  fixed.

> 
> > +config XILINX_HWICAP
> > +	tristate "Xilinx OPB HWICAP Support"
> > +	depends on XILINX_VIRTEX
> > +	help
> > +	This option enables support for Xilinx Internal 
> Configuration Access Port (ICAP) driver.
> 
> Line too long.

fixed.  checkpatch passes now.

> > +obj-$(CONFIG_XILINX_HWICAP) += xilinx_hwicap_m.o 
> > + 
> > +xilinx_hwicap_m-y := xilinx_hwicap.o xhwicap_srp.o
> 
> Those files are both quite small, couldn't you merge them and get rid
> of the global symbols and the xilinx_hwicap directory?

Its somewhat an artifact of the original EDK code that it is derived
from,
but your probably right.  I've refactored the code to look a little bit
nicer, and now there is a new C file for the fifo-based xps_hwicap.  I
think it really does make sense to have multiple files, now.

> Could you please you a smaller / standard GPL header instead?

Although most copyrights are shorter, in poking through the code, there
others which are of comparable size/language.

> Please use std kerneldoc format.

Fixed.

> No CamelCase, Uppercase parameters.

Fixed.

> > +	if (XHwIcap_mGetDoneReg(InstancePtr->baseAddress) == 
> XHI_NOT_FINISHED) {
> > +		return -EBUSY;
> > +	}
> 
> No curly brackets around single statement. In general, please run the
> patch through checkpatch.pl and fixup stuff.

Gah... force of habit.  Does anyone have an emacs mode that comes close
to the linux coding style?

> Why don't you request official major/minor numbers?
> (Documentation/devices.txt)

I sent a request to lanana over amonth ago, but haven't received a
response.  For embedded targets the dynamically allocated device is
annoying, but without a static number I don't see what else to do.  In
any event, it is easy to change when a static device number is
allocated.

> > +static struct device_driver xhwicap_module_driver = {
> > +	.name = DRIVER_NAME,
> > +	.bus = &platform_bus_type,
> > +
> > +	.probe = xhwicap_drv_probe,
> > +	.remove = xhwicap_drv_remove,
> > +};
> 
> Please use struct platform_driver instead.

That's what I get for copying old code.  I also realized that there is a
better way of dealing with the of_platform_device part.  It's been
rewritten to follow the pattern that Grant and I have been following.

> > +
> > +static int __init xhwicap_module_init(void)
> > +{
> > +	dev_t devt;
> > +	int retval;
> > +
> > +	icap_class = class_create(THIS_MODULE, "xilinx_config");
> 
> What's that for?

To get this:

-bash-3.00# pwd
/sys/class
-bash-3.00# find xilinx_config
xilinx_config
xilinx_config/xilinx_icap
xilinx_config/xilinx_icap/subsystem
xilinx_config/xilinx_icap/uevent
xilinx_config/xilinx_icap/dev

I started trying to do more with this, but I could never quite figure
out how to get sysfs to do what I wanted to.  There's a couple of things
(like the IDCODE) that would be interesting to see here, I think.

> > +#ifdef CONFIG_XILINX_VIRTEX_4_FX
> > +#define XHI_FAMILY virtex4
> > +#else
> > +#define XHI_FAMILY virtex2
> > +#endif
> 
> So having a single kernel with v2p/v4 support is not an option?

I've fixed this to add a level of indirection, and to select the right
value based on the device tree.  This required a slight modification to
the mhs->dts generator to make the 'xlnx,family' attribute visible.
Note that we currently can't build a real v2/v4 kernel because of some
v2pro errata.  Currently, I enable the errata even in v4, which is not
so nice.

> > +#define XHwIcap_mGetSizeReg(BaseAddress) \
> > +    (in_be32((u32 *)((BaseAddress) + XHI_SIZE_REG_OFFSET)))
> > +
> > 
> +/************************************************************
> ****************/
> 
> Why not a single getter with a offset/register parameter instead of
> all these? And use inline functions instead of macros.

Historical artifact to some extent, but I think it makes cleaner code
than trying to read the constants, and in some cases, it's not a
straightforward register read.  In any event, they've been changed to be
static inlines.  In general, I cleaned the code up alot.

> Bye, Peter Korsgaard

Thanks!  An update will be coming around shortly.

Steve