Re: [PATCH 2/2] New driver: Xillybus generic interface for FPGA(programmable logic)

[Greg KH]

On Fri, Nov 30, 2012 at 04:50:39PM +0200, Eli Billauer wrote:
> Thanks for the remarks.
> 
> I'm sending the updated patches in a minute. Basically, I divided
> the module into three (one core, one for PCIe and one for OF) and
> made several corrections.
> 
> On 11/28/2012 06:57 PM, Greg KH wrote:
> >What is the user/kernel interface for this driver?  Is it documented
> >anywhere?
> There's a rather extensive documentation for download at the site.
> The docs for the host side mostly instruct common UNIX programming
> techniques: The device files are just data pipes to FIFOs in the
> FPGA, behaving like one would expect.

As we need to review the user/kernel api here, putting the docs as part
of the driver submission is a good idea :)

I didn't know, nor do I trust, that a random web site would have the
correct documentation for a kernel driver.

> >>+#if (PAGE_SIZE<  4096)
> >>+#error Your processor architecture has a page size smaller than 4096
> >>+#endif
> >That can never happen.  Even if it does, you don't care about that in
> >the driver.
> >
> I removed this check because it can't happen. But the driver *does*
> care about this, since it creates a lot of buffers with different
> alignments, hence depending on the pages' alignment.

Alignment is different than the size of a page.  What happens if your
driver runs on a machine with a page size bigger than 4K?  You need to
be able to handle that properly, so perhaps you should check that?

> >>+static struct class *xillybus_class;
> >Why not just use the misc interface instead of your own class?
> When Xillybus is used, the whole system's mission is usually around
> it (e.g. it's a computer doing data acquisition through the Xillybus
> pipes). So giving it a high profile makes sense, I believe. Besides,
> a dozen of device files are not rare.

It is no problem to create dozens of misc devices.  It makes your driver
smaller, contain less code that I have to audit and you have to ensure
you got right, and it removes another user of 'struct class' which we
are trying to get rid of anyway.  So please, move to use a misc device.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/