Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool <vwool@ru.mvista.com>]

Russell King wrote:

>>+iii. struct spi_driver
>>+~~~~~~~~~~~~~~~~~~~~~~
>>+
>>+struct spi_driver {
>>+    	void* (*alloc)( size_t, int );
>>+    	void  (*free)( const void* );
>>+    	unsigned char* (*get_buffer)( struct spi_device*, void* );
>>+    	void (*release_buffer)( struct spi_device*, unsigned char*);
>>+    	void (*control)( struct spi_device*, int mode, u32 ctl );
>>+        struct device_driver driver;
>>+};
>>    
>>
>
>This doesn't appear to have been updated.
>
>  
>
>>+formed spi_driver structure:
>>+	extern struct bus_type spi_bus;
>>+	static struct spi_driver pnx4008_eeprom_driver = {
>>+        	.driver = {
>>+                   	.bus = &spi_bus,
>>+                   	.name = "pnx4008-eeprom",
>>+                   	.probe = pnx4008_spiee_probe,
>>+                   	.remove = pnx4008_spiee_remove,
>>+                   	.suspend = pnx4008_spiee_suspend,
>>+                   	.resume = pnx4008_spiee_resume,
>>+               	},
>>+};
>>    
>>
>
>Ditto.
>
>  
>
>>+iv. struct spi_bus_driver
>>+~~~~~~~~~~~~~~~~~~~~~~~~~
>>+To handle transactions over the SPI bus, the spi_bus_driver structure must
>>+be prepared and registered with core. Any transactions, either synchronous
>>+or asynchronous, go through spi_bus_driver->xfer function.
>>+
>>+struct spi_bus_driver
>>+{
>>+        int (*xfer)( struct spi_msg* msgs );
>>+        void (*select) ( struct spi_device* arg );
>>+        void (*deselect)( struct spi_device* arg );
>>+
>>+	struct spi_msg *(*retrieve)( struct spi_bus_driver *this, struct spi_bus_data *bd);
>>+	void (*reset)( struct spi_bus_driver *this, u32 context);
>>+
>>+        struct device_driver driver;
>>+};
>>    
>>
>
>Does this need updating as well?
>
>  
>
>>+spi_bus_driver structure:
>>+	static struct spi_bus_driver spipnx_driver = {
>>+        .driver = {
>>+                   .bus = &platform_bus_type,
>>+                   .name = "spipnx",
>>+                   .probe = spipnx_probe,
>>+                   .remove = spipnx_remove,
>>+                   .suspend = spipnx_suspend,
>>+                   .resume = spipnx_resume,
>>+                   },
>>+        .xfer = spipnx_xfer,
>>+};
>>    
>>
>
>From this it looks like it does.
>
>  
>
Yep, thanks for pointing that out. The Documentation part needs to be 
updated. Will do soon.

>  
>
>>+
>>+int spi_driver_suspend (struct device *dev, pm_message_t pm)
>>+{
>>+	struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
>>+	struct spi_device *sdev = TO_SPI_DEV(dev);
>>+
>>+	return (sdrv && sdrv->suspend) ?  sdrv->suspend(sdev, pm) : -EFAULT;
>>+}
>>+
>>+int spi_driver_resume (struct device *dev)
>>+{
>>+	struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
>>+	struct spi_device *sdev = TO_SPI_DEV(dev);
>>+
>>+	return (sdrv && sdrv->resume) ? sdrv->resume(sdev) : -EFAULT;
>>+}
>>    
>>
>
>If the bus_type does not call the device_driver suspend/resume methods,
>these are not necessary.
>
>Another oddity I notice is that if there isn't a driver or method, you're
>returning -EFAULT - seems odd since if there isn't a driver do you really
>want to prevent suspend/resume?
>  
>
Yep, I must admit here that it's really better to do something like
if (!dev->driver)
return 0;
else if (sdrv->suspend)
return sdrv->suspend(...)

Does that sound better to you?

>  
>
>>+static inline int spi_driver_add(struct spi_driver *drv)
>>+{
>>+	drv->driver.bus = &spi_bus;
>>+	drv->driver.probe = spi_driver_probe;
>>+	drv->driver.remove = spi_driver_remove;
>>+	drv->driver.shutdown = spi_driver_shutdown;
>>    
>>
>
>  
>
>>+	drv->driver.suspend = spi_driver_suspend;
>>+	drv->driver.resume = spi_driver_resume;
>>    
>>
>
>As a result of the comment above, you don't need these two initialisers.
>
>  
>
Yup. Thanks.

Vitaly