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

2015-04-06 Thread Jonathan Richardson
On 15-04-06 03:36 AM, Rafał Miłecki wrote:
> On 3 April 2015 at 19:52, Florian Fainelli  wrote:
>> On 03/04/15 06:38, Andy Shevchenko wrote:
>>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>>>  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 {
 +   #ifdef CONFIG_SPI_BCMA_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).

That would require compiling in BCMA for a driver/chip that doesn't use
BCMA but then I could do DT parsing in init only anyway. I don't think
that's really an option so I'm going to leave as is unless there is
further opinion on it.

> 
> 
>> 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.
> 

Making BCMA optional was made possible by using DT. I'm not sure I could
split it into two commits. I would have to add a hard coded SPI device
for non-BMCA as well. I thought the driver was a bit odd in that this
was hard coded. Normally this should be in a separate driver. How would
you use it if you wanted to use m25p80 for example?


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


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

2015-04-06 Thread Jonathan Richardson
On 15-04-03 10:52 AM, Florian Fainelli wrote:
> On 03/04/15 06:38, Andy Shevchenko wrote:
>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>>  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 {
>>> +   #ifdef CONFIG_SPI_BCMA_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?
> 
> 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 cringed adding the ifdefs to the driver but didn't think the small
amount of code that wouldn't be used again warranted 3 files. I could
also handle the two different probe routines by doing some DT parsing in
init, but then BCMA would have to be compiled for the non-BCMA MSPI
driver and I didn't want to do that either.



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


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

2015-04-06 Thread Rafał Miłecki
On 3 April 2015 at 19:52, Florian Fainelli  wrote:
> On 03/04/15 06:38, Andy Shevchenko wrote:
>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>>  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 {
>>> +   #ifdef CONFIG_SPI_BCMA_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.

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


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

2015-04-06 Thread Rafał Miłecki
On 3 April 2015 at 15:38, Andy Shevchenko  wrote:
> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>  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 {
>> +   #ifdef CONFIG_SPI_BCMA_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

I also believe we usually (always?) don't align any #if-s (no indent/tabs).

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


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

2015-04-06 Thread Rafał Miłecki
On 2 April 2015 at 21:23, Jonathan Richardson  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.

I think this patch provides 3 changes instead of just one described
above. You refactored R/W ops (pointers), made bcma optional and added
DT support. What about patch-per-change?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2015-04-06 Thread Jonathan Richardson
On 15-04-06 03:36 AM, Rafał Miłecki wrote:
 On 3 April 2015 at 19:52, Florian Fainelli f.faine...@gmail.com wrote:
 On 03/04/15 06:38, Andy Shevchenko wrote:
 On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
 jonat...@broadcom.com 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 {
 +   #ifdef CONFIG_SPI_BCMA_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).

That would require compiling in BCMA for a driver/chip that doesn't use
BCMA but then I could do DT parsing in init only anyway. I don't think
that's really an option so I'm going to leave as is unless there is
further opinion on it.

 
 
 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.
 

Making BCMA optional was made possible by using DT. I'm not sure I could
split it into two commits. I would have to add a hard coded SPI device
for non-BMCA as well. I thought the driver was a bit odd in that this
was hard coded. Normally this should be in a separate driver. How would
you use it if you wanted to use m25p80 for example?


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


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

2015-04-06 Thread Jonathan Richardson
On 15-04-03 10:52 AM, Florian Fainelli wrote:
 On 03/04/15 06:38, Andy Shevchenko wrote:
 On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
 jonat...@broadcom.com 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 {
 +   #ifdef CONFIG_SPI_BCMA_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?
 
 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 cringed adding the ifdefs to the driver but didn't think the small
amount of code that wouldn't be used again warranted 3 files. I could
also handle the two different probe routines by doing some DT parsing in
init, but then BCMA would have to be compiled for the non-BCMA MSPI
driver and I didn't want to do that either.



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


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

2015-04-06 Thread Rafał Miłecki
On 3 April 2015 at 19:52, Florian Fainelli f.faine...@gmail.com wrote:
 On 03/04/15 06:38, Andy Shevchenko wrote:
 On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
 jonat...@broadcom.com 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 {
 +   #ifdef CONFIG_SPI_BCMA_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.

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


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

2015-04-06 Thread Rafał Miłecki
On 2 April 2015 at 21:23, Jonathan Richardson jonat...@broadcom.com 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.

I think this patch provides 3 changes instead of just one described
above. You refactored R/W ops (pointers), made bcma optional and added
DT support. What about patch-per-change?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2015-04-06 Thread Rafał Miłecki
On 3 April 2015 at 15:38, Andy Shevchenko andy.shevche...@gmail.com wrote:
 On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
 jonat...@broadcom.com 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 {
 +   #ifdef CONFIG_SPI_BCMA_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

I also believe we usually (always?) don't align any #if-s (no indent/tabs).

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


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

2015-04-03 Thread Florian Fainelli
On 03/04/15 06:38, Andy Shevchenko wrote:
> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>  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 {
>> +   #ifdef CONFIG_SPI_BCMA_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?

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


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

2015-04-03 Thread Andy Shevchenko
On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
 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 {
> +   #ifdef CONFIG_SPI_BCMA_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

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


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

2015-04-03 Thread Florian Fainelli
On 03/04/15 06:38, Andy Shevchenko wrote:
 On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
 jonat...@broadcom.com 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 {
 +   #ifdef CONFIG_SPI_BCMA_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?

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.
-- 
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2015-04-03 Thread Andy Shevchenko
On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
jonat...@broadcom.com 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 {
 +   #ifdef CONFIG_SPI_BCMA_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

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