Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

From: RafaÅ MiÅecki
Date: Mon Apr 06 2015 - 06:36:37 EST

On 3 April 2015 at 19:52, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
> On 03/04/15 06:38, Andy Shevchenko wrote:
>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>> <jonathar@xxxxxxxxxxxx> wrote:
>>> The Broadcom MSPI controller is used on various chips. The driver only
>>> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
>>> refactored to make BCMA optional and provides a new config for non BCMA
>>> systems.
>>> struct bcm_mspi {
>>> struct bcma_device *core;
>>> - struct spi_master *master;
>>> + #endif
>>> + void __iomem *base;
>>> + struct spi_master *master;
>>> size_t read_offset;
>>> + void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
>>> + u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
>>> +};
>> To avoid ugly ifdefs I think better to split driver to core part and
>> the actual driver part, at the end you will have something like
>> mspi-core.c mspi-53xx.c mspi-whatever.c. Check for example spi-dw*.c
> Actually, I am really curious whether we need the special BCMA I/O
> accessors in the first place, cannot we just access the MSPI core on
> BCM53xx chips using regular MMIO? That would probably solve the
> "problem" entirely. Rafal, did you try this before?

It's a matter of choice between:
1) Using one design for all bcma users
2) Using one design for all bcm-mspi users
I believe no matter which one you choose, you'll break another one.

If you take a look at drivers/bcma/host_soc.c, you'll see we've there
core->io_addr. I guess you could use it as the base in bcm-mspi. That
of course will make you a bit less compatible with other bcma drivers
(skipping bcma R/W layer).

> As for splitting the driver into a "library" driver which is mostly
> independent from the bus and a bus-specific wrapper, I think BCMA is
> really the only special case here, which is why I suggested earlier to
> Jonathan that we might just prefer ifdefing things out instead of
> creating a separate layer just for BCMA.

I think you may be right, this #if for bcma shouldn't be that bad and
it shouldn't grow in the future.
Still, I'd like to get this patch split nicely to review independent changes.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at