On Tue, 2005-05-31 at 16:32 -0700, Greg KH wrote:
Thanks for you comments; my answers follow. This patch will be reworked
in short time.
> Is this code intergrated into the driver model?
It will be.
> What does the /sys/ tree look like?
> Why are you using a char device node?
Because it's not a block device :) I assume that's common practice to
access low-level devices, isn't it ?
>
> > +/**
> > + * spi_add_adapter - register a new SPI bus adapter
> > + * @adap: spi_adapter structure for the registering adapter
> > + *
> > + * Make the adapter available for use by clients using name adap->name.
> > + * The adap->adapters list is initialised by this function.
> > + *
> > + * Returns 0;
>
> You have this a lot. If the function can not fail, just make it a void
> type :)
I'd like to keep this of type int because of possible future
enhancements
> > + list_for_each(l, &driver_list) {
>
> list_for_each_entry() please.
Looks reasonable, thank you
>
> > +/**
> > + * spi_get_adapter - get a reference to an adapter
> > + * @id: driver id
> > + *
> > + * Obtain a spi_adapter structure for the specified adapter. If the adapter
> > + * is not currently load, then load it. The adapter will be locked in core
> > + * until all references are released via spi_put_adapter.
> > + */
>
> Hm, that comment is not correct. Please fix it as nothing is "loaded".
OK
> > +void spi_put_adapter(struct spi_adapter *adap)
> > +{
> > + if (adap && adap->owner)
> > + module_put(adap->owner);
> > +}
>
> Then why not use the traditional kref style of reference counting? That
> ensures that if you try to use the reference, bad things will happen?
> Right now all you are doing is relying on module references, and you
> aren't cleaning up the memory.
I'll consider this during rework
> > +EXPORT_SYMBOL(spi_transfer);
> > +EXPORT_SYMBOL(spi_write);
> > +EXPORT_SYMBOL(spi_read);
>
> EXPORT_SYMBOL_GPL() perhaps?
Yup.
> Please use dev_dbg() and friends instead of your own debugging macros.
> The error log people will thank you (along with your users...)
I'd rather use pr_debug
> > +#include <linux/spi/spi.h>
>
> Why a separate subdir for spi.h?
Due to historical reasons :) And functional drivers can pput their
public headers to separate directory.
>
> > +static int spidev_ioctl(struct inode *inode, struct file *file,
> > + unsigned int cmd, unsigned long arg)
This function will be removed, because its functionality is duplicated by spidev_read/spidev_write.
> > +static int spidev_attach_adapter(struct spi_adapter *adap)
> > +{
> > + struct spi_dev *spi_dev;
> > + int retval;
> > +
> > + spi_dev = get_free_spi_dev(adap);
> > + if (IS_ERR(spi_dev))
> > + return PTR_ERR(spi_dev);
> > +
> > +#if defined( CONFIG_DEVFS_FS )
> > + devfs_mk_cdev(MKDEV(SPI_MAJOR, spi_dev->minor),
> > + S_IFCHR | S_IRUSR | S_IWUSR, "spi/%d", spi_dev->minor);
> > +#endif
>
> No #if needed. You do this a lot.
As devfs is going to become obsolete, maybe just drop all the
devfs-related code?
> > +/*
> > + * spi_adapter is the structure used to identify a physical SPI bus along
> > + * with the access algorithms necessary to access it.
> > + */
> > +struct spi_adapter {
>
> <snip> Ick, don't copy the mess I did in the i2c core for i2c adapter
> structures please. It was a hack then, and I regret it still. Please
> fix it up properly.
>
> This code is _very_ close to just a copy of the i2c core code. Why
> duplicate it and not work with the i2c people instead?
Well, those two are both simple serial interfaces, so their similarity
is just reflected in driver structures.
