Thanks Magnus!
Generally speaking this looks reasonable. Some comments:
> struct xgpio_instance {
> struct list_head link;
> unsigned long base_phys; /* GPIO base address - physical
*/
> unsigned long remap_size;
> - u32 device_id;
> + u32 device_id; /* Dev ID for platform devices, 0 for OF
devices */
> + void *of_id; /* of_dev pointer for OF devices, NULL
for plat devices */
Why have separate ids? I don't think the of_dev needs to be kept around
here. This driver seems seems awkwardly written to have a local list of
all the devices, rather than simply attaching the xgpio_instance as the
private data of the file.
For instance, in drivers/char/xilinx_hwicap.c:
static ssize_t
hwicap_read(struct file *file, char __user *buf, size_t count, loff_t
*ppos)
{
struct hwicap_drvdata *drvdata = file->private_data;
and the drvdata is set in open:
static int hwicap_open(struct inode *inode, struct file *file)
{
struct hwicap_drvdata *drvdata;
int status;
drvdata = container_of(inode->i_cdev, struct hwicap_drvdata,
cdev);
...
file->private_data = drvdata;
Which would work if xgpio_instance directly contains the struct
miscdevice.
I think this is a much cleaner pattern (although it took me a while to
figure out the magic that makes it work... )
> +static struct of_device_id xgpio_of_match[] = {
> + {.compatible = "xlnx,xps-gpio-1.00.a"},
This should also probably contain the corresponding strings for the
following as well:
opb_gpio_v1_00_a
opb_gpio_v2_00_a
opb_gpio_v3_01_a
opb_gpio_v3_01_b
plb_gpio_v1_00_b
This would seem to be a relatively easy driver to clean up (by pulling
it all into one file and converting the other code to the kernel style)
and submit to mainline, if you're interested?
Steve
_______________________________________________
Linuxppc-embedded mailing list
Linuxppc-embedded@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-embedded
