Thanks Magnus... Re: SPI I've been pondering some ideas along this line, but I haven't done anything concrete. There are other cases where it makes sense to have some information about the board-level design of the system (e.g. ethernet PHYs.).
Steve > -----Original Message----- > From: Magnus Hjorth [mailto:[EMAIL PROTECTED] > Sent: Thursday, March 13, 2008 1:34 AM > To: Stephen Neuendorffer; git > Cc: [email protected]; 'Grant Likely' > Subject: RE: [PATCH] Ported Xilinx GPIO driver to OpenFirmware. > > Hi, > > Thanks for the feedback. I'll have a look into refining the patch in a few weeks when I get some > more time. > > I have also been tinkering a little with the SPI driver, and that got me thinking. Wouldn't it be > great if SPI controllers and devices could be specified in the OF device tree and registered on boot > time? Even better if SPI worked as a true bus in EDK, with placeholder IP-cores for each slave > device, so such device entries could be autogenerated. > > Cheers, > Magnus > > > -----Original Message----- > > From: Stephen Neuendorffer [mailto:[EMAIL PROTECTED] > > Sent: den 11 mars 2008 18:36 > > To: Magnus Hjorth; git > > Cc: [email protected]; Grant Likely > > Subject: RE: [PATCH] Ported Xilinx GPIO driver to OpenFirmware. > > > > > > 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 [email protected] https://ozlabs.org/mailman/listinfo/linuxppc-embedded
