Re: [PATCH v4 09/16] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Gabor Juhos <juhosg@openwrt.org>]

2011.01.03. 18:02 keltezéssel, Grant Likely írta:
> On Sun, Jan 02, 2011 at 07:56:22PM +0100, Gabor Juhos wrote:
>> The Atheros AR71XX/AR724X/AR913X SoCs have a built-in SPI controller. This
>> patch implements a driver for that.
>>
> 
> Mostly looks okay to me.  A few comments below.  Do you want this one
> merged via my spi tree, or does it depend on other patches in this
> series?  I'm also okay with it going in with the rest of this series.

The driver won't be usable without the preceding patches in the series.
Additionally, further patches depends on this one so it should go via the MIPS tree.

> After addressing comments, feel free to add:
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Thank you!

>> <...>
>> +struct ath79_spi {
>> +	struct	spi_bitbang	bitbang;
>> +	u32			ioc_base;
>> +	u32			reg_ctrl;
>> +
>> +	void __iomem		*base;
>> +
>> +	struct platform_device	*pdev;
> 
> This field doesn't seem to actually be used anywhere.

True, i will remove that.

> 
>> +};
>> +
>> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
>> +{
>> +	return ioread32(sp->base + reg);
>> +}
>> +
>> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
>> +{
>> +	iowrite32(val, sp->base + reg);
>> +}
>> +
>> +static inline struct ath79_spi *spidev_to_sp(struct spi_device *spi)
> 
> should be ath79_spidev_to_sp()

Ok.

> 
>> +{
>> +	return spi_master_get_devdata(spi->master);
>> +}
>> +
>> +static void ath79_spi_chipselect(struct spi_device *spi, int is_active)
>> +{
>> +	struct ath79_spi *sp = spidev_to_sp(spi);
>> +	int cs_high = (spi->mode & SPI_CS_HIGH) ? is_active : !is_active;
>> +
>> +	if (is_active) {
>> +		/* set initial clock polarity */
>> +		if (spi->mode & SPI_CPOL)
>> +			sp->ioc_base |= AR71XX_SPI_IOC_CLK;
>> +		else
>> +			sp->ioc_base &= ~AR71XX_SPI_IOC_CLK;
>> +
>> +		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
>> +	}
>> +
>> +	if (spi->chip_select) {
>> +		unsigned long gpio = (unsigned long) spi->controller_data;
>> +
>> +		/* SPI is normally active-low */
>> +		gpio_set_value(gpio, cs_high);
>> +	} else {
>> +		if (cs_high)
>> +			sp->ioc_base |= AR71XX_SPI_IOC_CS0;
>> +		else
>> +			sp->ioc_base &= ~AR71XX_SPI_IOC_CS0;
>> +
>> +		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
>> +	}
>> +
>> +}
>> +
>> +static int ath79_spi_setup_cs(struct spi_device *spi)
>> +{
>> +	struct ath79_spi *sp = spidev_to_sp(spi);
>> +
>> +	/* enable GPIO mode */
>> +	ath79_spi_wr(sp, AR71XX_SPI_REG_FS, AR71XX_SPI_FS_GPIO);
>> +
>> +	/* save CTRL register */
>> +	sp->reg_ctrl = ath79_spi_rr(sp, AR71XX_SPI_REG_CTRL);
>> +	sp->ioc_base = ath79_spi_rr(sp, AR71XX_SPI_REG_IOC);
>> +
>> +	/* TODO: setup speed? */
>> +	ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, 0x43);
>> +
>> +	if (spi->chip_select) {
>> +		unsigned long gpio = (unsigned long) spi->controller_data;
> 
> Casting here is bad practice.  

It seems that the spi-gpio driver was a bad example. :)

> It would be better to have an explicit ath97_spi_controller_data structure.

I will add the explicit structure. Expect an updated version soon.

Thanks,
Gabor