On Sun, Jun 02, 2013 at 04:58:49PM +0200, Oliver Schinagl wrote: > +#include <asm/io.h>
We have an include file called linux/io.h. Please use linux/*.h files which include asm/*.h files in preference to directly using asm/*.h. In fact, no driver should include an asm/*.h header. > +#include <linux/compiler.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/errno.h> > +#include <linux/export.h> > +#include <linux/fs.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/kobject.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/random.h> > +#include <linux/stat.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > + > +#define DRV_NAME "sunxi-sid" > +#define DRV_VERSION "1.0" > + > +/* There are 4 32-bit keys */ > +#define SID_KEYS 4 > +/* and 4 byte sized keys per 32-bit key */ > +#define SID_SIZE (SID_KEYS * 4) > + > +static void __iomem *p_sid_reg_base; > + > +/* We read the entire key, but only return the requested byte. This is of > + * course slower then it could be and uses 4 times more reads as needed but > + * keeps code a simpler. > + */ > +u8 sunxi_sid_read_byte(const int offset) > +{ > + u32 sid_key; > + u8 ret; > + > + ret = 0; > + > + if (likely((SID_SIZE))) { > + sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4)); > + sid_key >>= (offset % 4) * 8; > + ret = sid_key & 0xff; > + } What happens if you unbind the device in sysfs and then try and use this function? > +static int sunxi_sid_remove(struct platform_device *pdev) > +{ > + device_remove_bin_file(&pdev->dev, &sid_bin_attr); > + dev_info(&pdev->dev, "Sunxi SID driver unloaded successfully.\n"); Maybe you want to set p_sid_reg_base to NULL here? > + > + return 0; > +} > + > +static int __init sunxi_sid_probe(struct platform_device *pdev) > +{ > + int entropy[SID_SIZE], i, ret; > + struct device *dev; > + struct resource *res; > + void __iomem *sid_reg_base; > + > + dev = &pdev->dev; > + if (unlikely(!pdev->dev.of_node)) { > + dev_err(dev, "No devicetree data available\n"); > + ret = -EFAULT; > + goto exit; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(sid_reg_base)) { > + dev_err(dev, "Unable to obtain resource\n"); > + ret = PTR_ERR(sid_reg_base); > + goto exit; > + } > + platform_set_drvdata(pdev, sid_reg_base); > + p_sid_reg_base = sid_reg_base; So what happens if you have two of these devices? Maybe you want to check whether p_sid_reg_base is already set? > + > + ret = device_create_bin_file(dev, &sid_bin_attr); > + if (unlikely(ret)) { > + dev_err(dev, "Unable to create sysfs bin entry\n"); > + goto exit; > + } > + > + for (i = 0; i < SID_SIZE; i++) > + entropy[i] = sunxi_sid_read_byte(i); > + add_device_randomness(entropy, SID_SIZE); > + > + dev_info(dev, "Sunxi SID driver loaded successfully.\n"); Do we really need to report that the driver "loaded successfully" ? Do we need lots of lines in the kernel log telling us simply that random driver X was built into the kernel or the module was loaded? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/