Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2

[Stephen Warren <swarren@xxxxxxxxxxxxx>]

On 06/22/2015 07:40 AM, kernel@xxxxxxxxxxxxxxxx wrote:
> From: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> 
> This driver does NOT make use of native chip-selects but uses the
> generic cs-gpios facilities provided by the framework allowing for
> more than 3 chip-selects.

> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c

> +/*
> + * spi register defines
> + *
> + * note there is garbage in the "official" documentation,
> + * so somedata taken from the file:
> + *   brcm_usrlib/dag/vmcsx/vcinclude/bcm2708_chip/aux_io.h
> + * inside of:
> + *   http://www.broadcom.com/docs/support/videocore/Brcm_Android_ICS_Graphics_Stack.tar.gz
> + */

Hopefully the license of that tar file allows for that. I didn't look.

> +DEFINE_SPINLOCK(__bcm2835_aux_lock);
> +static void bcm2835_aux_modify_enable(struct bcm2835aux_spi *bs,
> +				      u32 mask, u32 val)
> +{
> +	unsigned long flags;
> +	u32 r;
> +
> +	spin_lock_irqsave(&__bcm2835_aux_lock, flags);
> +
> +	r = readl(bs->aux_regs + BCM2835_AUX_ENABLE);
> +	r &= mask;
> +	r |= val;
> +	writel(r, bs->aux_regs + BCM2835_AUX_ENABLE);
> +
> +	spin_unlock_irqrestore(&__bcm2835_aux_lock, flags);
> +}

As mentioned elsewhere, I'd hope all the shared aux register
manipulations can be pushed into a shared driver.

> +static void bcm2835aux_spi_enable(struct bcm2835aux_spi *bs)
> +{
> +	/* identify the spi device - needed to activate the right HW-block */
> +	u32 mask = (size_t)bs->regs & 0x00000040 ?
> +		   BCM2835_AUX_BIT_SPI2 : BCM2835_AUX_BIT_SPI1;
> +
> +	bcm2835_aux_modify_enable(bs, ~mask, mask);
> +}

I expect that function will go away when my previous comment is
resolved. If not, it'd be nice to encode the "ID" of the device into DT,
so that the driver has no hard-coded idea of what register addresses
exist; what happens when (a hypothetical) bcm2837 comes along with 3
instances of this HW block?

> +static int bcm2835aux_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
> +
> +	/* Clear FIFOs, and disable the HW block */
> +	clk_disable_unprepare(bs->clk);

You probably want remove() to do things in the reverse order of probe().
In particular, disable the clock after anything that could touch
registers in the HW.

> +	bcm2835aux_spi_reset_hw(bs);
> +
> +	/* disable HW block */
> +	bcm2835aux_spi_disable(bs);

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html