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

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