[PATCH 00/11] mfd and bluetooth: Add CG2900 support

[PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Par-Gunnar Hjalmdahl]

This is the 3rd patch set for the ST-Ericsson CG2900 connectivity
controller. The CG2900 is a combo controller supporting GPS, Bluetooth,
and FM radio. It uses HCI H:4 protocol to combine different functionalities
on a common transport, where first byte in the data indicates the current
channel. Channels 1-4 are standardized in the Bluetooth Core specification
while the other channels are vendor specific.

Compared to 2nd patch set this patch set has the following changes:
 * UART handling is moved from mfd to bluetooth folder. It now reuses the
   existing N_HCI line discipline.
 * mfd creation has been moved from cg2900_core into chip specific files.
 * All information for each channel, including API functions, exist in each
   MFD devices, making them independent of each other.
 * All chip specific information has been moved from cg2900_core into the
   chip specific files. cg2900_core now only handles registration and
   connection between transport and chip driver.
 * Fixes for several review comments including use of existing debug system.

Par-Gunnar Hjalmdahl (11):
  mfd: Add support for CG2900 controller framework
  mfd: Add CG2900 character devices
  mfd: Add support for CG2900 controller
  mfd: Add support for STLC2690 controller
  mfd: Add CG2900 audio
  mfd: Add CG2900 test character device
  Bluetooth: Add UART API functions to ldisc
  Bluetooth: Add support for CG2900 UART
  Bluetooth: Add support for CG2900 controller
  arch_mach-ux500: Add U8500 board support for CG2900
  Bluetooth and mach-ux500: Fix of minor issues

 arch/arm/mach-ux500/Makefile             |    1 +
 arch/arm/mach-ux500/board-mop500.c       |  152 ++
 arch/arm/mach-ux500/devices-cg2900.c     |  315 +++
 arch/arm/mach-ux500/devices-cg2900.h     |   19 +
 drivers/bluetooth/Kconfig                |    7 +
 drivers/bluetooth/Makefile               |    2 +
 drivers/bluetooth/btcg2900.c             | 1134 ++++++++++
 drivers/bluetooth/cg2900_uart.c          | 1849 ++++++++++++++++
 drivers/bluetooth/hci_ath.c              |    1 +
 drivers/bluetooth/hci_bcsp.c             |    3 +-
 drivers/bluetooth/hci_h4.c               |    1 +
 drivers/bluetooth/hci_ldisc.c            |  101 +-
 drivers/bluetooth/hci_ll.c               |    1 +
 drivers/bluetooth/hci_uart.h             |   18 +-
 drivers/mfd/Kconfig                      |   53 +
 drivers/mfd/Makefile                     |    2 +
 drivers/mfd/cg2900/Makefile              |   16 +
 drivers/mfd/cg2900/cg2900_audio.c        | 3415 ++++++++++++++++++++++++++++++
 drivers/mfd/cg2900/cg2900_char_devices.c |  701 ++++++
 drivers/mfd/cg2900/cg2900_chip.c         | 3250 ++++++++++++++++++++++++++++
 drivers/mfd/cg2900/cg2900_chip.h         |  602 ++++++
 drivers/mfd/cg2900/cg2900_core.c         |  711 +++++++
 drivers/mfd/cg2900/cg2900_core.h         |   51 +
 drivers/mfd/cg2900/cg2900_lib.c          |  391 ++++
 drivers/mfd/cg2900/cg2900_lib.h          |   61 +
 drivers/mfd/cg2900/cg2900_test.c         |  402 ++++
 drivers/mfd/cg2900/stlc2690_chip.c       | 1673 +++++++++++++++
 drivers/mfd/cg2900/stlc2690_chip.h       |   47 +
 include/linux/mfd/cg2900.h               |  287 +++
 include/linux/mfd/cg2900_audio.h         |  473 +++++
 include/net/bluetooth/hci.h              |    5 +
 31 files changed, 15734 insertions(+), 10 deletions(-)
 create mode 100644 arch/arm/mach-ux500/devices-cg2900.c
 create mode 100644 arch/arm/mach-ux500/devices-cg2900.h
 create mode 100644 drivers/bluetooth/btcg2900.c
 create mode 100644 drivers/bluetooth/cg2900_uart.c
 create mode 100644 drivers/mfd/cg2900/Makefile
 create mode 100644 drivers/mfd/cg2900/cg2900_audio.c
 create mode 100644 drivers/mfd/cg2900/cg2900_char_devices.c
 create mode 100644 drivers/mfd/cg2900/cg2900_chip.c
 create mode 100644 drivers/mfd/cg2900/cg2900_chip.h
 create mode 100644 drivers/mfd/cg2900/cg2900_core.c
 create mode 100644 drivers/mfd/cg2900/cg2900_core.h
 create mode 100644 drivers/mfd/cg2900/cg2900_lib.c
 create mode 100644 drivers/mfd/cg2900/cg2900_lib.h
 create mode 100644 drivers/mfd/cg2900/cg2900_test.c
 create mode 100644 drivers/mfd/cg2900/stlc2690_chip.c
 create mode 100644 drivers/mfd/cg2900/stlc2690_chip.h
 create mode 100644 include/linux/mfd/cg2900.h
 create mode 100644 include/linux/mfd/cg2900_audio.h

-- 
1.7.3.2

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Vitaly Wool]

Hi Par,

On Fri, Dec 17, 2010 at 12:20 PM, Par-Gunnar Hjalmdahl
<par-gunnar.p.hjalmdahl@stericsson.com> wrote:
> This is the 3rd patch set for the ST-Ericsson CG2900 connectivity
> controller. The CG2900 is a combo controller supporting GPS, Bluetooth,
> and FM radio. It uses HCI H:4 protocol to combine different functionalities
> on a common transport, where first byte in the data indicates the current
> channel. Channels 1-4 are standardized in the Bluetooth Core specification
> while the other channels are vendor specific.
>
> Compared to 2nd patch set this patch set has the following changes:
>  * UART handling is moved from mfd to bluetooth folder. It now reuses the
>   existing N_HCI line discipline.
>  * mfd creation has been moved from cg2900_core into chip specific files.
>  * All information for each channel, including API functions, exist in each
>   MFD devices, making them independent of each other.
>  * All chip specific information has been moved from cg2900_core into the
>   chip specific files. cg2900_core now only handles registration and
>   connection between transport and chip driver.
>  * Fixes for several review comments including use of existing debug system.

this patchset has only multiplied my confusion.

Not really diving into the details, I'm just trying to understand how
I'd rewrite the TI shared transport using your approach if I had to.
It looks like I'd have to, at least:
 - implement a heavyweight line discipline driver (like cg2900_uart.c)
half duplicating the functionality in yours;
 - implement specific MFD driver.

Actually the reuse of this implementation will be at a level of
statistics error. The only thing to reuse is actually the approach
which I'm not really happy about.

So...

If you are aiming for a generic solution, then why you get more and
more things handled in cg2900-specific code (e. g. packet routing
which is fairly generic)?

If you are solving your particular problems, isn't the change too
dramatic e. g. when it comes to line discipline drivers and baudrate
manipulation?

As of now, I really see no reason why anyone would promote this
solution instead of making something more generic out of ti-st and
reusing it then.

Thanks,
   Vitaly

RE: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Par-Gunnar HJALMDAHL]

Hi Vitaly,

> -----Original Message-----
> From: Vitaly Wool [mailto:vitalywool@gmail.com]
> Sent: den 17 december 2010 13:11
> To: Par-Gunnar HJALMDAHL
> Cc: Pavan Savoy; Alan Cox; Arnd Bergmann; Samuel Ortiz; Marcel
> Holtmann; linux-kernel@vger.kernel.org; linux-
> bluetooth@vger.kernel.org; Lukasz Rymanowski; Linus WALLEIJ; Par-Gunnar
> Hjalmdahl
> Subject: Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support
> 
> Hi Par,
> 
> On Fri, Dec 17, 2010 at 12:20 PM, Par-Gunnar Hjalmdahl
> <par-gunnar.p.hjalmdahl@stericsson.com> wrote:
> > This is the 3rd patch set for the ST-Ericsson CG2900 connectivity
> > controller. The CG2900 is a combo controller supporting GPS,
> Bluetooth,
> > and FM radio. It uses HCI H:4 protocol to combine different
> functionalities
> > on a common transport, where first byte in the data indicates the
> current
> > channel. Channels 1-4 are standardized in the Bluetooth Core
> specification
> > while the other channels are vendor specific.
> >
> > Compared to 2nd patch set this patch set has the following changes:
> >  * UART handling is moved from mfd to bluetooth folder. It now reuses
> the
> >   existing N_HCI line discipline.
> >  * mfd creation has been moved from cg2900_core into chip specific
> files.
> >  * All information for each channel, including API functions, exist
> in each
> >   MFD devices, making them independent of each other.
> >  * All chip specific information has been moved from cg2900_core into
> the
> >   chip specific files. cg2900_core now only handles registration and
> >   connection between transport and chip driver.
> >  * Fixes for several review comments including use of existing debug
> system.
> 
> this patchset has only multiplied my confusion.
> 
> Not really diving into the details, I'm just trying to understand how
> I'd rewrite the TI shared transport using your approach if I had to.
> It looks like I'd have to, at least:
>  - implement a heavyweight line discipline driver (like cg2900_uart.c)
> half duplicating the functionality in yours;
>  - implement specific MFD driver.
> 
> Actually the reuse of this implementation will be at a level of
> statistics error. The only thing to reuse is actually the approach
> which I'm not really happy about.
> 
> So...
> 
> If you are aiming for a generic solution, then why you get more and
> more things handled in cg2900-specific code (e. g. packet routing
> which is fairly generic)?
> 

You are absolutely correct that there is not much that is generic and the reason for this is that it is not much that is generic. If you for example look at packet routing the method can be considered pretty generic. Check first byte and choose a structure type... But first of all this must be done with minimum overhead since it is done on every packet received. And secondly if you look at number of lines of code, it is not really much you save.
If you look closer at the code for the CG2900 you will see that the majority of the code is things that you could probably never apply to the chip of another vendor. A lot of the code in e.g. cg2900_chip.c is regarding bt_audio and fm_audio, which is really chip specific.
Also, if you want to keep the structure modular and supporting e.g. multiple transports with the same chip driver, you must focus on what is chip specific, what is transport specific, and what is chip and transport specific. When you start to think about it like this it becomes very hard to make too much code generic. Usually what you get with that kind of generic code is a solution way more complex than you started out with.

> If you are solving your particular problems, isn't the change too
> dramatic e. g. when it comes to line discipline drivers and baudrate
> manipulation?
> 

The changes in hci_ldisc.c is not what I would call dramatic. It is quite simple functions. As I've stated earlier, if people object to this, there is no big issue to solve this within cg2900_uart.c instead. But we think that these API functions are a proper extension to the current hci_ldisc API.

> As of now, I really see no reason why anyone would promote this
> solution instead of making something more generic out of ti-st and
> reusing it then.
> 
> Thanks,
>    Vitaly

Just to be clear, I have no problem with having both solutions (our and ti-st) side by side. They are structured quite differently and has different focus. I'm in no way trying to replace ti-st. I'm not trying to force anyone into using our structure.

/P-G

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Linus Walleij]

2010/12/17 Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>:

> This is the 3rd patch set for the ST-Ericsson CG2900 connectivity
> controller. The CG2900 is a combo controller supporting GPS, Bluetooth,
> and FM radio. It uses HCI H:4 protocol to combine different functionalities
> on a common transport, where first byte in the data indicates the current
> channel. Channels 1-4 are standardized in the Bluetooth Core specification
> while the other channels are vendor specific.

I'm slightly confused by different comment threads on this patch set.

I would certainly appreciate if the subsystem maintainers and reviewers
who shaped this patch set could add their Acked-by to the parts
they're happy with.

Alan, Marcel, Sam & Arnd especially. (Your work is MUCH
appreciated.)

I'm trying to get an idea of what patches are OK and what patches
are being disputed here, so as to whether there is some consensus
or not.

Yours,
Linus Walleij

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Vitaly Wool]

Hi Linus,

On Sun, Dec 19, 2010 at 10:23 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> I'm slightly confused by different comment threads on this patch set.

let me first first repost the part from Arnd's email on the
architecture of the "shared transport" type of thing:

> I believe we already agree it should be something like
>
>   bt   ti-radio    st-e-radio    ti-gps   st-e-gps  broadcom-radio ...
>   |         |          |            |          |          |         |
>   +---------+----------+---------+--+----------+----------+---------+
>                                  |
>                         common-hci-module
>                                  |
>                      +-----------+-----------+
>                      |        |       |      |
>                    serial    spi     i2c    ...

I think an explanation on how the patchset from Par maps to this
architecture could be a good starting point for confusion minimization
:)

Thanks,
   Vitaly

RE: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Par-Gunnar HJALMDAHL]

Hi Vitaly,

> -----Original Message-----
> From: Vitaly Wool [mailto:vitalywool@gmail.com]
> Sent: den 19 december 2010 23:58
> To: Linus Walleij
> Cc: Par-Gunnar HJALMDAHL; Pavan Savoy; Alan Cox; Arnd Bergmann; Samuel
> Ortiz; Marcel Holtmann; linux-kernel@vger.kernel.org; linux-
> bluetooth@vger.kernel.org; Lukasz Rymanowski; Par-Gunnar Hjalmdahl
> Subject: Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support
> 
> Hi Linus,
> 
> On Sun, Dec 19, 2010 at 10:23 PM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
> > I'm slightly confused by different comment threads on this patch set.
> 
> let me first first repost the part from Arnd's email on the
> architecture of the "shared transport" type of thing:
> 
> > I believe we already agree it should be something like
> >
> >   bt   ti-radio    st-e-radio    ti-gps   st-e-gps  broadcom-radio
> ...
> >   |         |          |            |          |          |         |
> >   +---------+----------+---------+--+----------+----------+---------+
> >                                  |
> >                         common-hci-module
> >                                  |
> >                      +-----------+-----------+
> >                      |        |       |      |
> >                    serial    spi     i2c    ...
> 
> I think an explanation on how the patchset from Par maps to this
> architecture could be a good starting point for confusion minimization
> :)
> 
> Thanks,
>    Vitaly

I would say our design would map like this:
common-hci-module: cg2900_core
serial, spi, i2c,... : cg2900_uart together with hci_ldisc (for other transports it would be different files)
bt, ti-radio, st-e-radio,...: cg2900_chip together with btcg2900 and other users per channel (cg2900_char_devices for users in User space)
So it is not a 1-to-1 mapping for the upper parts. I would draw it like this:

                               bt   st-e-radio  st-e-gps
                                |         |          |
                                +---------+----------+
                                          |
                   ti-xx                st-e cg2900...
                     |                    |
                     +---------+----------+
                               |
                       common-hci-module
                               |
                   +-----------+-----------+
                   |        |       |      |
                 serial    spi     i2c    ...

The reason for this difference I've gone through before. Basically there are so many special behaviors and needed handling that is individual for each chip (like startup and shutdown and in the case of CG2900 flow control over FM and BT channels for audio commands). If you then look at the users I guess it would be possible to have one BT user, but it would have to be modified to handle vendor specific commands (as btcg2900 does with BT_ENABLE command). As Arnd has drawn for FM and GPS the users would be completely individual since they don't have a standardized  interface.

/P-G

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Pavan Savoy]

P-G, Vitaly,

>
> I would say our design would map like this:
> common-hci-module: cg2900_core
> serial, spi, i2c,... : cg2900_uart together with hci_ldisc (for other transports it would be different files)
> bt, ti-radio, st-e-radio,...: cg2900_chip together with btcg2900 and other users per channel (cg2900_char_devices for users in User space)
> So it is not a 1-to-1 mapping for the upper parts. I would draw it like this:
>
>                               bt   st-e-radio  st-e-gps
>                                |         |          |
>                                +---------+----------+
>                                          |
>                   ti-xx                st-e cg2900...
>                     |                    |
>                     +---------+----------+
>                               |
>                       common-hci-module
>                               |
>                   +-----------+-----------+
>                   |        |       |      |
>                 serial    spi     i2c    ...

regarding the architecture above suggested...
Is having the common-hci-module, only way ?
Why is this dependency on bluetooth at all?

for example: today I don't compile my kernel with BT support, but
still want to use
the chip for FM & GPS, It should be possible correct ?
Even in TI case, although the firmware download is HCI-VS way, we
don't use hci_core
to interpret the responses...

instead of common-hci-module, why not create a algo-driver layer 'ala
i2c ? where individual drivers can register their receive handlers for
data interpretation ?

>
> The reason for this difference I've gone through before. Basically there are so many special behaviors and needed handling that is individual for each chip (like startup and shutdown and in the case of CG2900 flow control over FM and BT channels for audio commands). If you then look at the users I guess it would be possible to have one BT user, but it would have to be modified to handle vendor specific commands (as btcg2900 does with BT_ENABLE command). As Arnd has drawn for FM and GPS the users would be completely individual since they don't have a standardized  interface.
>
> /P-G
>

RE: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Par-Gunnar HJALMDAHL]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4252 bytes --]

Hi Pavan,

Sorry for not answering sooner. I've been on Christmas and New Year vacation.

> -----Original Message-----
> From: Pavan Savoy [mailto:pavan_savoy@sify.com]
> Sent: den 23 december 2010 11:49
> To: Par-Gunnar HJALMDAHL
> Cc: Vitaly Wool; Linus Walleij; Alan Cox; Arnd Bergmann; Samuel Ortiz;
> Marcel Holtmann; linux-kernel@vger.kernel.org; linux-
> bluetooth@vger.kernel.org; Lukasz Rymanowski; Par-Gunnar Hjalmdahl
> Subject: Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support
> 
> P-G, Vitaly,
> 
> >
> > I would say our design would map like this:
> > common-hci-module: cg2900_core
> > serial, spi, i2c,... : cg2900_uart together with hci_ldisc (for other
> transports it would be different files)
> > bt, ti-radio, st-e-radio,...: cg2900_chip together with btcg2900 and
> other users per channel (cg2900_char_devices for users in User space)
> > So it is not a 1-to-1 mapping for the upper parts. I would draw it
> like this:
> >
> >                               bt   st-e-radio  st-e-gps
> >                                |         |          |
> >                                +---------+----------+
> >                                          |
> >                   ti-xx                st-e cg2900...
> >                     |                    |
> >                     +---------+----------+
> >                               |
> >                       common-hci-module
> >                               |
> >                   +-----------+-----------+
> >                   |        |       |      |
> >                 serial    spi     i2c    ...
> 
> regarding the architecture above suggested...
> Is having the common-hci-module, only way ?
> Why is this dependency on bluetooth at all?
> 

The reason I use Bluetooth is that it is the only standardized Host-Controller interface in these controllers (at least in the CG2900 which I'm primarily addressing).

> for example: today I don't compile my kernel with BT support, but
> still want to use
> the chip for FM & GPS, It should be possible correct ?

Yes, I use the header files from the Bluetooth files inside the driver (for packet structures and protocol IDs). I do not use the BT binaries at all in the common-hci-module or in the st-e cg2900 module. So it should be OK to configure this out. The header files will be there any way.

> Even in TI case, although the firmware download is HCI-VS way, we
> don't use hci_core
> to interpret the responses...
> 

We don't use that either. We just use the structs and defines in the BT headers. The dependency is only in btcg2900.c which is only included if BT is configuration enabled.

> instead of common-hci-module, why not create a algo-driver layer 'ala
> i2c ? where individual drivers can register their receive handlers for
> data interpretation ?
> 

In some way you then run into the same problem has I had in previous patch sets. The functionalities supported is really determined by each chip. You might or might not have for example GPS in a certain chip. So you do not want a central module to expose all possible channels to the stacks on top. You only want the actually supported features to be exposed to upper layers. Then the MFD system is pretty nice. It's easy and modularized to expose the different channels as MFD cells.

Also note that the common-hci-module is only really used until the connected chip has been detected. The chip handler will then set the callback functions so actual data transmissions never pass the common-hci-module. They go directly from transport to chip handler. That is not really shown in the picture above. Just imagine that common-hci-module is removed after a chip has been connected and a chip handler has been determined.

I hope I haven't misunderstood your question. I do not know much about the I2C system, but I tried to understand your question as best as I could.

/P-G

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Arnd Bergmann]

On Wednesday 05 January 2011, Par-Gunnar HJALMDAHL wrote:

> Sorry for not answering sooner. I've been on Christmas and New Year vacation.

I'm also still catching up with email that has accumulated over my
vacation, including your previous response.

> > -----Original Message-----

> > >
> > > I would say our design would map like this:
> > > common-hci-module: cg2900_core
> > > serial, spi, i2c,... : cg2900_uart together with hci_ldisc (for other
> > transports it would be different files)
> > > bt, ti-radio, st-e-radio,...: cg2900_chip together with btcg2900 and
> > other users per channel (cg2900_char_devices for users in User space)
> > > So it is not a 1-to-1 mapping for the upper parts. I would draw it
> > like this:
> > >
> > >                               bt   st-e-radio  st-e-gps
> > >                                |         |          |
> > >                                +---------+----------+
> > >                                          |
> > >                   ti-xx                st-e cg2900...
> > >                     |                    |
> > >                     +---------+----------+
> > >                               |
> > >                       common-hci-module
> > >                               |
> > >                   +-----------+-----------+
> > >                   |        |       |      |
> > >                 serial    spi     i2c    ...

Even if the cg2900 based drivers (bt, st-e-radio, ...) have things in common
that are not true in general, I'd still advocate a model where they all
register directly to the hardware-independent layer. Your base cg2900
driver can then become a library module that is used by some drivers that
use the cg2900 specific functionality, just like there can be other similar
libraries.

> > regarding the architecture above suggested...
> > Is having the common-hci-module, only way ?
> > Why is this dependency on bluetooth at all?
> > 
> 
> The reason I use Bluetooth is that it is the only standardized Host-Controller
> interface in these controllers (at least in the CG2900 which I'm primarily addressing).

That doesn't seem to make any sense if you don't present the standard interface
to Linux but basically still need to come up with your own abstraction layer
for it. It may have been the intention to use a standard HC interface, but either
the HW guys screwed up completely by making it impossible to use it that way,
or you haven't found the right way to integrate with the standard code.

> > for example: today I don't compile my kernel with BT support, but
> > still want to use
> > the chip for FM & GPS, It should be possible correct ?
> 
> Yes, I use the header files from the Bluetooth files inside the driver (for
> packet structures and protocol IDs). I do not use the BT binaries at all in
> the common-hci-module or in the st-e cg2900 module. So it should be OK to
> configure this out. The header files will be there any way.

This proves the point I just made: If it's a standard hardware interface, you
should be able to use the same driver module for a regular bluetooth HCI and
for a cf2900 without bluetooth. There is nothing wrong with requiring kernel
support for HCI if what you have is actually an HCI. Contrary to what Pavan
said, I would not require these to be independent implementations. Of course
in this case you would only require the base hci module to do this, not any of
the upper-level BT modules.

> > Even in TI case, although the firmware download is HCI-VS way, we
> > don't use hci_core
> > to interpret the responses...
> > 
> 
> We don't use that either. We just use the structs and defines in the BT
> headers. The dependency is only in btcg2900.c which is only included if
> BT is configuration enabled.

This sounds wrong for both TI and ST-E: AFAICT they are actually built
around an HCI interface. It's unfortunate that the TI code actually got
merged into the kernel like this.

> > instead of common-hci-module, why not create a algo-driver layer 'ala
> > i2c ? where individual drivers can register their receive handlers for
> > data interpretation ?

That would be what I suggested ;-)

> In some way you then run into the same problem has I had in previous patch
> sets. The functionalities supported is really determined by each chip.
> You might or might not have for example GPS in a certain chip. So you do not
> want a central module to expose all possible channels to the stacks on top.
>
> You only want the actually supported features to be exposed to upper layers.
> Then the MFD system is pretty nice. It's easy and modularized to expose the
> different channels as MFD cells.

But as soon as you have the concept of channels with a clearly defined
interface, you have almost abstracted it enough to go all the way.
 
> Also note that the common-hci-module is only really used until the connected
> chip has been detected. The chip handler will then set the callback functions
> so actual data transmissions never pass the common-hci-module. They go directly
> from transport to chip handler. That is not really shown in the picture above.
> Just imagine that common-hci-module is removed after a chip has been connected
> and a chip handler has been determined.
> 
> I hope I haven't misunderstood your question. I do not know much about the I2C
> system, but I tried to understand your question as best as I could.

I think there is a disconnect when talking about hierarchies, as it can be applied
to different areas:

* module dependencies
* device detection
* sysfs object hierarchy
* data flow

These are often the same or at least related, but we may be talking about different
aspects here. One issue that is often confusing is that from a module layering,
you typically have a common module at the bottom and have both providers (e.g. hosts
controller) and consumers of data (e.g. protocol drivers) on the top, where in a
data flow chart, you typically have the provider below the common code and the
consumer above if (or the other way round, if you prefer).

Since the HCI code in this case is the common component, it really needs to be the
module that everything else registers with in some way. You can have multiple
modules providing HCIs to that, and I suppose a module that provides an HCI should
also be the one that identifies the channels that are present.

The consumers of those channels however should not interface with the module that
provides the channels, but use the HCI code or something on top of it as an
abstraction.

	Arnd

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Pavan Savoy]

On Fri, Jan 7, 2011 at 12:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 05 January 2011, Par-Gunnar HJALMDAHL wrote:
>
>> Sorry for not answering sooner. I've been on Christmas and New Year vacation.
>
> I'm also still catching up with email that has accumulated over my
> vacation, including your previous response.
>
> This sounds wrong for both TI and ST-E: AFAICT they are actually built
> around an HCI interface. It's unfortunate that the TI code actually got
> merged into the kernel like this.

I am not sure what does built around HCI Interface mean? Also yes, in TI- code
we do refer to Bluetooth headers.
However the fact that I wanted to point out to Par-Gunnar was, that we
don't want to use
hciattach and enable HCI-UART + HCI-H4 for enabling our driver or our
driver should not
depend on those modules as such...

The references to bluetooth headers in a certain way is inevitable
because as he pointed
out, firmware is downloaded as HCI-VS commands, too bad the firmware
doesn't have any other
means :(, But it sorts of allows violations, as in we can afford to
have HCI-VS commands sent after
disabling events, which would mean they need not be interpreted at all..

>> > instead of common-hci-module, why not create a algo-driver layer 'ala
>> > i2c ? where individual drivers can register their receive handlers for
>> > data interpretation ?
>
> That would be what I suggested ;-)

But even here too, the algos layer if you imagine which can be the
sort of the first
receiver of data from the transport would refer to BT headers to
interpret the data (not just BT, but FM/GPS)
and pass it onto other protocol/client drivers,
The only abstraction being that different adapter drivers can register
their own receive function
which the algo-driver can sort of call, (again all imagination....)


>> In some way you then run into the same problem has I had in previous patch
>> sets. The functionalities supported is really determined by each chip.
>> You might or might not have for example GPS in a certain chip. So you do not
>> want a central module to expose all possible channels to the stacks on top.
>>
>> You only want the actually supported features to be exposed to upper layers.
>> Then the MFD system is pretty nice. It's easy and modularized to expose the
>> different channels as MFD cells.
>
> But as soon as you have the concept of channels with a clearly defined
> interface, you have almost abstracted it enough to go all the way.

Something like this is what the recent RFC posted to
lkml/linux-bluetooth
http://www.spinics.net/lists/linux-bluetooth/msg09990.html,
it kinda looks clumsy
but what I feel is we shouldn't shy away from not referencing
Bluetooth, (or may be tomorrow GPS
with NMEA headers)....


>> Also note that the common-hci-module is only really used until the connected
>> chip has been detected. The chip handler will then set the callback functions
>> so actual data transmissions never pass the common-hci-module. They go directly
>> from transport to chip handler. That is not really shown in the picture above.
>> Just imagine that common-hci-module is removed after a chip has been connected
>> and a chip handler has been determined.
>>
>> I hope I haven't misunderstood your question. I do not know much about the I2C
>> system, but I tried to understand your question as best as I could.
>
> I think there is a disconnect when talking about hierarchies, as it can be applied
> to different areas:
>
> * module dependencies
> * device detection
> * sysfs object hierarchy
> * data flow

module dependecy-wise I agree,
I would want FM-V4L2 without BT or I might want GPS simplistic char
driver without BT or FM V4L2.

device detection wise, It is a problem, there is not "_probe"
mechanism for UART based transport as it is
understandable, and pretty much the driver would end up being platform
device driver ...

data flow is where I guess the abstraction has to lie in, for
different vendors...

> These are often the same or at least related, but we may be talking about different
> aspects here. One issue that is often confusing is that from a module layering,
> you typically have a common module at the bottom and have both providers (e.g. hosts
> controller) and consumers of data (e.g. protocol drivers) on the top, where in a
> data flow chart, you typically have the provider below the common code and the
> consumer above if (or the other way round, if you prefer).
>
> Since the HCI code in this case is the common component, it really needs to be the
> module that everything else registers with in some way. You can have multiple
> modules providing HCIs to that, and I suppose a module that provides an HCI should
> also be the one that identifies the channels that are present.
>
> The consumers of those channels however should not interface with the module that
> provides the channels, but use the HCI code or something on top of it as an
> abstraction.
>
>        Arnd

Thanks for the comments...
>

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 support

[Vitaly Wool]

On Sun, Jan 9, 2011 at 7:12 PM, Pavan Savoy <pavan_savoy@sify.com> wrote:
>> This sounds wrong for both TI and ST-E: AFAICT they are actually built
>> around an HCI interface. It's unfortunate that the TI code actually got
>> merged into the kernel like this.
>
> I am not sure what does built around HCI Interface mean? Also yes, in TI- code
> we do refer to Bluetooth headers.

I think that the first and the major problem with TI code is that it
is _very_ dependent on the HCI-LL protocol.
I urge you again to sort that out first and then the whole thing will
get quite a bit clearer to all the parties.

> However the fact that I wanted to point out to Par-Gunnar was, that we
> don't want to use
> hciattach and enable HCI-UART + HCI-H4 for enabling our driver or our
> driver should not
> depend on those modules as such...

I really don't care _that_ much if we have to enable HCI interface to
bring say the FM radio up. What I do care about is the startup time
for the same FM radio, or GPS, which turns to be far longer if we go
the awkward way of launching hciattach and friends which are not in
fact needed at all. So I tend to agree with you here.

>
> The references to bluetooth headers in a certain way is inevitable
> because as he pointed
> out, firmware is downloaded as HCI-VS commands, too bad the firmware
> doesn't have any other
> means :(, But it sorts of allows violations, as in we can afford to
> have HCI-VS commands sent after
> disabling events, which would mean they need not be interpreted at all..

Well if we've got this common-hci-module Arnd was talking about, I
believe that firmware download can be sort of abstracted.

>>> > instead of common-hci-module, why not create a algo-driver layer 'ala
>>> > i2c ? where individual drivers can register their receive handlers for
>>> > data interpretation ?
>>
>> That would be what I suggested ;-)
>
> But even here too, the algos layer if you imagine which can be the
> sort of the first
> receiver of data from the transport would refer to BT headers to
> interpret the data (not just BT, but FM/GPS)
> and pass it onto other protocol/client drivers,
> The only abstraction being that different adapter drivers can register
> their own receive function
> which the algo-driver can sort of call, (again all imagination....)
>

Let's keep things simple. Could you please come up with a step-by-step
on how you would transform the existing TI solution for shared
transport into something really generic?
Or, the other option would IMHO be to start with sorting out some
obvious shared transport flaws.

Thanks,
   Vitaly

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Arnd Bergmann]

On Sunday 09 January 2011, Pavan Savoy wrote:
> On Fri, Jan 7, 2011 at 12:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 05 January 2011, Par-Gunnar HJALMDAHL wrote:
> >
> >> Sorry for not answering sooner. I've been on Christmas and New Year vacation.
> >
> > I'm also still catching up with email that has accumulated over my
> > vacation, including your previous response.
> >
> > This sounds wrong for both TI and ST-E: AFAICT they are actually built
> > around an HCI interface. It's unfortunate that the TI code actually got
> > merged into the kernel like this.
> 
> I am not sure what does built around HCI Interface mean? Also yes, in TI- code
> we do refer to Bluetooth headers.
> However the fact that I wanted to point out to Par-Gunnar was, that we
> don't want to use
> hciattach and enable HCI-UART + HCI-H4 for enabling our driver or our
> driver should not
> depend on those modules as such...

Good point about hciattach, you certainly shouldn't need to use that if
the kernel already knows that a tty is connected to an HCI and what the
parameters are. Even more so if the HCI is not actually on a rs232 line
but something like i2c or spi. Automatically binding to the right line
discipline should be easily doable in the drivers though.

I don't understand the problem with relying on the hci-uart or hci-h4
modules. If the hardware uses the H4 protocol, we certainly should use
the kernel module that knows how to deal with H4, and we don't want
to have two modules implementing the same protocol.

> >> > instead of common-hci-module, why not create a algo-driver layer 'ala
> >> > i2c ? where individual drivers can register their receive handlers for
> >> > data interpretation ?
> >
> > That would be what I suggested ;-)
> 
> But even here too, the algos layer if you imagine which can be the
> sort of the first
> receiver of data from the transport would refer to BT headers to
> interpret the data (not just BT, but FM/GPS)
> and pass it onto other protocol/client drivers,

Right, that is the entire idea, and I don't see a problem here.
If you do this, you use the headers of the two subsystems you
interface with. What you should /not/ instead is use header
files of a subsystem you don't interface with and reinterprete
the definitions in creative ways, which is what I understood
was being discussed earlier.

> >> In some way you then run into the same problem has I had in previous patch
> >> sets. The functionalities supported is really determined by each chip.
> >> You might or might not have for example GPS in a certain chip. So you do not
> >> want a central module to expose all possible channels to the stacks on top.
> >>
> >> You only want the actually supported features to be exposed to upper layers.
> >> Then the MFD system is pretty nice. It's easy and modularized to expose the
> >> different channels as MFD cells.
> >
> > But as soon as you have the concept of channels with a clearly defined
> > interface, you have almost abstracted it enough to go all the way.
> 
> Something like this is what the recent RFC posted to
> lkml/linux-bluetooth
> http://www.spinics.net/lists/linux-bluetooth/msg09990.html,
> it kinda looks clumsy
> but what I feel is we shouldn't shy away from not referencing
> Bluetooth, (or may be tomorrow GPS
> with NMEA headers)....

The one important goal here should be to avoid code duplication.
Using the header to get the data structures from separate code
means you need to have similar parsing functions in each of the modules
using it. Not sharing the header wouldn't help, because then you end
up duplicating even more. The real solution, speaking very broadly,
must be to have a general module that deals with whatever the more
specific modules have in common, and have a header file that defines
the interface to it.

> >> Also note that the common-hci-module is only really used until the connected
> >> chip has been detected. The chip handler will then set the callback functions
> >> so actual data transmissions never pass the common-hci-module. They go directly
> >> from transport to chip handler. That is not really shown in the picture above.
> >> Just imagine that common-hci-module is removed after a chip has been connected
> >> and a chip handler has been determined.
> >>
> >> I hope I haven't misunderstood your question. I do not know much about the I2C
> >> system, but I tried to understand your question as best as I could.
> >
> > I think there is a disconnect when talking about hierarchies, as it can be applied
> > to different areas:
> >
> > * module dependencies
> > * device detection
> > * sysfs object hierarchy
> > * data flow
> 
> module dependecy-wise I agree,
> I would want FM-V4L2 without BT or I might want GPS simplistic char
> driver without BT or FM V4L2.

But you'd still need to have an HCI module. I believe right now, the
hci module depends on the bluetooth module, and you are right that this
is an undesirable dependency to have for a GPS module. However, the solution
to this should not be to make GPS independent of HCI, but to make parts
of HCI independent of bluetooth.

> device detection wise, It is a problem, there is not "_probe"
> mechanism for UART based transport as it is
> understandable, and pretty much the driver would end up being platform
> device driver ...

The platform device is just the lowest level in the device hierarchy.

If each HCI channel is a device, you can stack bt, gps, audio, ...
devices all on top of the HCI device, which is either stacked on top
of a serial port or in some other way connected to the underying transport.

In this case, the channels themselves are not platform devices, but
would get a new bus for them. That bus is populated by a driver that
owns the HCI and that knows (e.g. from its platform data, hardware
registers, user config or device tree) what channels are present.

> data flow is where I guess the abstraction has to lie in, for
> different vendors...

I don't know what you mean with that. My point was that we need to
consider all the hierarchies, and that they might be different.

	Arnd

RE: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Par-Gunnar HJALMDAHL]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 10074 bytes --]

Hi,

Sorry for not answering earlier. I've been overloaded with things to do now after New Year. See below.

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: den 9 januari 2011 19:55
> To: Pavan Savoy
> Cc: Par-Gunnar HJALMDAHL; Vitaly Wool; Linus Walleij; Alan Cox; Samuel
> Ortiz; Marcel Holtmann; linux-kernel@vger.kernel.org; linux-
> bluetooth@vger.kernel.org; Lukasz Rymanowski; Par-Gunnar Hjalmdahl
> Subject: Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor
> 
> On Sunday 09 January 2011, Pavan Savoy wrote:
> > On Fri, Jan 7, 2011 at 12:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Wednesday 05 January 2011, Par-Gunnar HJALMDAHL wrote:
> > >
> > >> Sorry for not answering sooner. I've been on Christmas and New
> Year vacation.
> > >
> > > I'm also still catching up with email that has accumulated over my
> > > vacation, including your previous response.
> > >
> > > This sounds wrong for both TI and ST-E: AFAICT they are actually
> built
> > > around an HCI interface. It's unfortunate that the TI code actually
> got
> > > merged into the kernel like this.
> >
> > I am not sure what does built around HCI Interface mean? Also yes, in
> TI- code
> > we do refer to Bluetooth headers.
> > However the fact that I wanted to point out to Par-Gunnar was, that
> we
> > don't want to use
> > hciattach and enable HCI-UART + HCI-H4 for enabling our driver or our
> > driver should not
> > depend on those modules as such...
> 
> Good point about hciattach, you certainly shouldn't need to use that if
> the kernel already knows that a tty is connected to an HCI and what the
> parameters are. Even more so if the HCI is not actually on a rs232 line
> but something like i2c or spi. Automatically binding to the right line
> discipline should be easily doable in the drivers though.
> 

When having UART as transport you need something to open the UART and set the line discipline. If this is hciattach or something else is up to each developer to suit what they are doing. I don't see a problem with using hciattach even if you don't use the Bluetooth part (if the exe is part of the system anyway), but if a company/developer want to use something else they can do that. It's a usage of standard interfaces using open() and ioctl().
I still don't think that you should have a line discipline for other transports than UART. If I would look at how I would implement an SPI driver for CG2900, there would almost be no code that could be used in common between cg2900_uart and cg2900_spi. SPI doesn't change baud rate, SPI uses commands for sleep/wake instead of Break, SPI packets doesn't need extra packetizing, etc.

> I don't understand the problem with relying on the hci-uart or hci-h4
> modules. If the hardware uses the H4 protocol, we certainly should use
> the kernel module that knows how to deal with H4, and we don't want
> to have two modules implementing the same protocol.
> 

I must say I don't understand this problem either. Unless the protocol driver is activated through ioctl SET_PROTOCOL, the code will not be executed, and the amount of ROM needed is negligible.
If you look at our submission, the hci-h4 could possibly be reused to some extent. Basically the packetizing to the Bluetooth H4 channels 1-4 could be re-used. In order to allow for vendor specific channels to be handled some new registration system on top of hci-h4 plus a callback system for data reception would have to be added (in order to facilitate systems that do not want all data to be sent directly to the Bluetooth stack such as the CG2900 driver). I'm afraid that we would have a significantly more complex system and larger amount of code if we would try to generalize the hci-h4 module. I definitely prefer the current model where a vendor specific driver replaces the hci-h4 protocol driver.

> > >> > instead of common-hci-module, why not create a algo-driver layer
> 'ala
> > >> > i2c ? where individual drivers can register their receive
> handlers for
> > >> > data interpretation ?
> > >
> > > That would be what I suggested ;-)
> >
> > But even here too, the algos layer if you imagine which can be the
> > sort of the first
> > receiver of data from the transport would refer to BT headers to
> > interpret the data (not just BT, but FM/GPS)
> > and pass it onto other protocol/client drivers,
> 
> Right, that is the entire idea, and I don't see a problem here.
> If you do this, you use the headers of the two subsystems you
> interface with. What you should /not/ instead is use header
> files of a subsystem you don't interface with and reinterprete
> the definitions in creative ways, which is what I understood
> was being discussed earlier.
> 
> > >> In some way you then run into the same problem has I had in
> previous patch
> > >> sets. The functionalities supported is really determined by each
> chip.
> > >> You might or might not have for example GPS in a certain chip. So
> you do not
> > >> want a central module to expose all possible channels to the
> stacks on top.
> > >>
> > >> You only want the actually supported features to be exposed to
> upper layers.
> > >> Then the MFD system is pretty nice. It's easy and modularized to
> expose the
> > >> different channels as MFD cells.
> > >
> > > But as soon as you have the concept of channels with a clearly
> defined
> > > interface, you have almost abstracted it enough to go all the way.
> >
> > Something like this is what the recent RFC posted to
> > lkml/linux-bluetooth
> > http://www.spinics.net/lists/linux-bluetooth/msg09990.html,
> > it kinda looks clumsy
> > but what I feel is we shouldn't shy away from not referencing
> > Bluetooth, (or may be tomorrow GPS
> > with NMEA headers)....
> 
> The one important goal here should be to avoid code duplication.
> Using the header to get the data structures from separate code
> means you need to have similar parsing functions in each of the modules
> using it. Not sharing the header wouldn't help, because then you end
> up duplicating even more. The real solution, speaking very broadly,
> must be to have a general module that deals with whatever the more
> specific modules have in common, and have a header file that defines
> the interface to it.
> 

In general I agree with you, but there are some problems here.
The most used BT HCI events are Command Status and Command Complete.
Command Status could be parsed completely in a good way (retrieving op code, nbr of commands allowed, and status of command sent).
Command Complete is however quite complex since the returned data will differ depending on command sent. Op code and nbr of commands allowed can be retrieved but everything else have to be extracted differently depending on command. This means that there is not much that will be saved here. But maybe we could extract some parsing into common functions, but I don't think you would gain that much.
Moreover, this would lead to a major reimplementation of the hci_core.c and related files, since they do not use any exported common functions as it is today. I do not know if they (BlueZ community) would want this and I do not think that that should be part of the CG2900 driver to do. It should in that case be done separately.

> > >> Also note that the common-hci-module is only really used until the
> connected
> > >> chip has been detected. The chip handler will then set the
> callback functions
> > >> so actual data transmissions never pass the common-hci-module.
> They go directly
> > >> from transport to chip handler. That is not really shown in the
> picture above.
> > >> Just imagine that common-hci-module is removed after a chip has
> been connected
> > >> and a chip handler has been determined.
> > >>
> > >> I hope I haven't misunderstood your question. I do not know much
> about the I2C
> > >> system, but I tried to understand your question as best as I
> could.
> > >
> > > I think there is a disconnect when talking about hierarchies, as it
> can be applied
> > > to different areas:
> > >
> > > * module dependencies
> > > * device detection
> > > * sysfs object hierarchy
> > > * data flow
> >
> > module dependecy-wise I agree,
> > I would want FM-V4L2 without BT or I might want GPS simplistic char
> > driver without BT or FM V4L2.
> 
> But you'd still need to have an HCI module. I believe right now, the
> hci module depends on the bluetooth module, and you are right that this
> is an undesirable dependency to have for a GPS module. However, the
> solution
> to this should not be to make GPS independent of HCI, but to make parts
> of HCI independent of bluetooth.
> 

For the CG2900 that is not possible. Even if you are running just GPS you still must download patches and settings and that is done through HCI commands over the Bluetooth command interface. We also use Bluetooth commands to identify the controller.

> > device detection wise, It is a problem, there is not "_probe"
> > mechanism for UART based transport as it is
> > understandable, and pretty much the driver would end up being
> platform
> > device driver ...
> 
> The platform device is just the lowest level in the device hierarchy.
> 
> If each HCI channel is a device, you can stack bt, gps, audio, ...
> devices all on top of the HCI device, which is either stacked on top
> of a serial port or in some other way connected to the underying
> transport.
> 
> In this case, the channels themselves are not platform devices, but
> would get a new bus for them. That bus is populated by a driver that
> owns the HCI and that knows (e.g. from its platform data, hardware
> registers, user config or device tree) what channels are present.
> 
> > data flow is where I guess the abstraction has to lie in, for
> > different vendors...
> 
> I don't know what you mean with that. My point was that we need to
> consider all the hierarchies, and that they might be different.
> 
> 	Arnd

/P-G

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Par-Gunnar HJALMDAHL]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11389 bytes --]

Hi,

I sent the mail below one month ago, but have still not received any answers or further comments.
Has anyone of you who made comments on the CG2900 patches earlier, any more comments or questions?

It is unclear to me where we stand with the CG2900 patches. I am myself happy with the current architecture, but I understand there are differing opinions.
I would like to know what we need to do in order to get this driver into the Linux Kernel tree.

/P-G

> -----Original Message-----
> From: Par-Gunnar HJALMDAHL
> Sent: den 17 januari 2011 16:33
> To: 'Arnd Bergmann'; Pavan Savoy
> Cc: Vitaly Wool; Linus Walleij; Alan Cox; Samuel Ortiz; Marcel
> Holtmann; linux-kernel@vger.kernel.org; linux-
> bluetooth@vger.kernel.org; Lukasz Rymanowski; Par-Gunnar Hjalmdahl
> Subject: RE: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor
> 
> Hi,
> 
> Sorry for not answering earlier. I've been overloaded with things to do
> now after New Year. See below.
> 
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > Sent: den 9 januari 2011 19:55
> > To: Pavan Savoy
> > Cc: Par-Gunnar HJALMDAHL; Vitaly Wool; Linus Walleij; Alan Cox;
> Samuel
> > Ortiz; Marcel Holtmann; linux-kernel@vger.kernel.org; linux-
> > bluetooth@vger.kernel.org; Lukasz Rymanowski; Par-Gunnar Hjalmdahl
> > Subject: Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor
> >
> > On Sunday 09 January 2011, Pavan Savoy wrote:
> > > On Fri, Jan 7, 2011 at 12:16 AM, Arnd Bergmann <arnd@arndb.de>
> wrote:
> > > > On Wednesday 05 January 2011, Par-Gunnar HJALMDAHL wrote:
> > > >
> > > >> Sorry for not answering sooner. I've been on Christmas and New
> > Year vacation.
> > > >
> > > > I'm also still catching up with email that has accumulated over
> my
> > > > vacation, including your previous response.
> > > >
> > > > This sounds wrong for both TI and ST-E: AFAICT they are actually
> > built
> > > > around an HCI interface. It's unfortunate that the TI code
> actually
> > got
> > > > merged into the kernel like this.
> > >
> > > I am not sure what does built around HCI Interface mean? Also yes,
> in
> > TI- code
> > > we do refer to Bluetooth headers.
> > > However the fact that I wanted to point out to Par-Gunnar was, that
> > we
> > > don't want to use
> > > hciattach and enable HCI-UART + HCI-H4 for enabling our driver or
> our
> > > driver should not
> > > depend on those modules as such...
> >
> > Good point about hciattach, you certainly shouldn't need to use that
> if
> > the kernel already knows that a tty is connected to an HCI and what
> the
> > parameters are. Even more so if the HCI is not actually on a rs232
> line
> > but something like i2c or spi. Automatically binding to the right
> line
> > discipline should be easily doable in the drivers though.
> >
> 
> When having UART as transport you need something to open the UART and
> set the line discipline. If this is hciattach or something else is up
> to each developer to suit what they are doing. I don't see a problem
> with using hciattach even if you don't use the Bluetooth part (if the
> exe is part of the system anyway), but if a company/developer want to
> use something else they can do that. It's a usage of standard
> interfaces using open() and ioctl().
> I still don't think that you should have a line discipline for other
> transports than UART. If I would look at how I would implement an SPI
> driver for CG2900, there would almost be no code that could be used in
> common between cg2900_uart and cg2900_spi. SPI doesn't change baud
> rate, SPI uses commands for sleep/wake instead of Break, SPI packets
> doesn't need extra packetizing, etc.
> 
> > I don't understand the problem with relying on the hci-uart or hci-h4
> > modules. If the hardware uses the H4 protocol, we certainly should
> use
> > the kernel module that knows how to deal with H4, and we don't want
> > to have two modules implementing the same protocol.
> >
> 
> I must say I don't understand this problem either. Unless the protocol
> driver is activated through ioctl SET_PROTOCOL, the code will not be
> executed, and the amount of ROM needed is negligible.
> If you look at our submission, the hci-h4 could possibly be reused to
> some extent. Basically the packetizing to the Bluetooth H4 channels 1-4
> could be re-used. In order to allow for vendor specific channels to be
> handled some new registration system on top of hci-h4 plus a callback
> system for data reception would have to be added (in order to
> facilitate systems that do not want all data to be sent directly to the
> Bluetooth stack such as the CG2900 driver). I'm afraid that we would
> have a significantly more complex system and larger amount of code if
> we would try to generalize the hci-h4 module. I definitely prefer the
> current model where a vendor specific driver replaces the hci-h4
> protocol driver.
> 
> > > >> > instead of common-hci-module, why not create a algo-driver
> layer
> > 'ala
> > > >> > i2c ? where individual drivers can register their receive
> > handlers for
> > > >> > data interpretation ?
> > > >
> > > > That would be what I suggested ;-)
> > >
> > > But even here too, the algos layer if you imagine which can be the
> > > sort of the first
> > > receiver of data from the transport would refer to BT headers to
> > > interpret the data (not just BT, but FM/GPS)
> > > and pass it onto other protocol/client drivers,
> >
> > Right, that is the entire idea, and I don't see a problem here.
> > If you do this, you use the headers of the two subsystems you
> > interface with. What you should /not/ instead is use header
> > files of a subsystem you don't interface with and reinterprete
> > the definitions in creative ways, which is what I understood
> > was being discussed earlier.
> >
> > > >> In some way you then run into the same problem has I had in
> > previous patch
> > > >> sets. The functionalities supported is really determined by each
> > chip.
> > > >> You might or might not have for example GPS in a certain chip.
> So
> > you do not
> > > >> want a central module to expose all possible channels to the
> > stacks on top.
> > > >>
> > > >> You only want the actually supported features to be exposed to
> > upper layers.
> > > >> Then the MFD system is pretty nice. It's easy and modularized to
> > expose the
> > > >> different channels as MFD cells.
> > > >
> > > > But as soon as you have the concept of channels with a clearly
> > defined
> > > > interface, you have almost abstracted it enough to go all the
> way.
> > >
> > > Something like this is what the recent RFC posted to
> > > lkml/linux-bluetooth
> > > http://www.spinics.net/lists/linux-bluetooth/msg09990.html,
> > > it kinda looks clumsy
> > > but what I feel is we shouldn't shy away from not referencing
> > > Bluetooth, (or may be tomorrow GPS
> > > with NMEA headers)....
> >
> > The one important goal here should be to avoid code duplication.
> > Using the header to get the data structures from separate code
> > means you need to have similar parsing functions in each of the
> modules
> > using it. Not sharing the header wouldn't help, because then you end
> > up duplicating even more. The real solution, speaking very broadly,
> > must be to have a general module that deals with whatever the more
> > specific modules have in common, and have a header file that defines
> > the interface to it.
> >
> 
> In general I agree with you, but there are some problems here.
> The most used BT HCI events are Command Status and Command Complete.
> Command Status could be parsed completely in a good way (retrieving op
> code, nbr of commands allowed, and status of command sent).
> Command Complete is however quite complex since the returned data will
> differ depending on command sent. Op code and nbr of commands allowed
> can be retrieved but everything else have to be extracted differently
> depending on command. This means that there is not much that will be
> saved here. But maybe we could extract some parsing into common
> functions, but I don't think you would gain that much.
> Moreover, this would lead to a major reimplementation of the hci_core.c
> and related files, since they do not use any exported common functions
> as it is today. I do not know if they (BlueZ community) would want this
> and I do not think that that should be part of the CG2900 driver to do.
> It should in that case be done separately.
> 
> > > >> Also note that the common-hci-module is only really used until
> the
> > connected
> > > >> chip has been detected. The chip handler will then set the
> > callback functions
> > > >> so actual data transmissions never pass the common-hci-module.
> > They go directly
> > > >> from transport to chip handler. That is not really shown in the
> > picture above.
> > > >> Just imagine that common-hci-module is removed after a chip has
> > been connected
> > > >> and a chip handler has been determined.
> > > >>
> > > >> I hope I haven't misunderstood your question. I do not know much
> > about the I2C
> > > >> system, but I tried to understand your question as best as I
> > could.
> > > >
> > > > I think there is a disconnect when talking about hierarchies, as
> it
> > can be applied
> > > > to different areas:
> > > >
> > > > * module dependencies
> > > > * device detection
> > > > * sysfs object hierarchy
> > > > * data flow
> > >
> > > module dependecy-wise I agree,
> > > I would want FM-V4L2 without BT or I might want GPS simplistic char
> > > driver without BT or FM V4L2.
> >
> > But you'd still need to have an HCI module. I believe right now, the
> > hci module depends on the bluetooth module, and you are right that
> this
> > is an undesirable dependency to have for a GPS module. However, the
> > solution
> > to this should not be to make GPS independent of HCI, but to make
> parts
> > of HCI independent of bluetooth.
> >
> 
> For the CG2900 that is not possible. Even if you are running just GPS
> you still must download patches and settings and that is done through
> HCI commands over the Bluetooth command interface. We also use
> Bluetooth commands to identify the controller.
> 
> > > device detection wise, It is a problem, there is not "_probe"
> > > mechanism for UART based transport as it is
> > > understandable, and pretty much the driver would end up being
> > platform
> > > device driver ...
> >
> > The platform device is just the lowest level in the device hierarchy.
> >
> > If each HCI channel is a device, you can stack bt, gps, audio, ...
> > devices all on top of the HCI device, which is either stacked on top
> > of a serial port or in some other way connected to the underying
> > transport.
> >
> > In this case, the channels themselves are not platform devices, but
> > would get a new bus for them. That bus is populated by a driver that
> > owns the HCI and that knows (e.g. from its platform data, hardware
> > registers, user config or device tree) what channels are present.
> >
> > > data flow is where I guess the abstraction has to lie in, for
> > > different vendors...
> >
> > I don't know what you mean with that. My point was that we need to
> > consider all the hierarchies, and that they might be different.
> >
> > 	Arnd
> 
> /P-G

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Linus Walleij]

2011/2/24 Par-Gunnar HJALMDAHL <par-gunnar.p.hjalmdahl@stericsson.com>:

> I sent the mail below one month ago, but have still not received any answers or further comments.
> Has anyone of you who made comments on the CG2900 patches earlier, any more comments or questions?

I think I have provided
Acked-by: Linus Walleij <linus.walleij@linaro.org>
(or the earlier ST-Ericsson address, which is equal) for these patches.
Else I do so now.

FWIW: I think those who want another architectural solution
can propose a refactoring patch any day they like, and if it's
recieved like "ah, that's better" ACK from Pär-Gunnar et al, then
it's no big deal.

This makes the hardware work with 2.6.39 which is really most
important IMO, Sam can you merge this as it stands?

Yours,
Linus Walleij

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Arnd Bergmann]

Hi Pär-Gunnar,

Sorry for the long delay, this time on my side.

On Thursday 24 February 2011, Par-Gunnar HJALMDAHL wrote:
> I sent the mail below one month ago, but have still not received any answers or further comments.
> Has anyone of you who made comments on the CG2900 patches earlier, any more comments or questions?

I tend to procrastinate replying to emails when it's a lot of work, and
yours dropped off the radar. I have not even read it until now, because
it's too long, has broken line-wraps and does not crop the citations
to a minimum. If you want people to listen to what you have to say, try
following email netiquette and put a little more effort into clear
communication like the rest of us do.

Please excuse my ignorance about topics we have already discussed here,
I don't know much about bluetooth and had to reread all the discussions
and patches.

> It is unclear to me where we stand with the CG2900 patches. I am myself happy with the
> current architecture, but I understand there are differing opinions.
> I would like to know what we need to do in order to get this driver into the Linux Kernel tree.

I have no idea what the current architecture is. In doubt, keep re-posting
the patches until all differences are resolved. If a thread does not
get a new message within a week, you can normally assume that poeple
no longer bother reading, and you should try harder to get feedback,
e.g. by sending a reminder that you are waiting or by resending
the modified patch set.

I still haven't looked at the patches from December, but I assume that
the code has changed in the meantime. Please go ahead and post the
current version again, then we can discuss the new code.

On Monday 17 January 2011, Par-Gunnar HJALMDAHL wrote:
> Arnd Bergmann wrote:
> > 
> > Good point about hciattach, you certainly shouldn't need to use that if
> > the kernel already knows that a tty is connected to an HCI and what the
> > parameters are. Even more so if the HCI is not actually on a rs232 line
> > but something like i2c or spi. Automatically binding to the right line
> > discipline should be easily doable in the drivers though.
> > 
> 
> When having UART as transport you need something to open the UART and set the
> line discipline. If this is hciattach or something else is up to each developer
> to suit what they are doing. I don't see a problem with using hciattach even 
> if you don't use the Bluetooth part (if the exe is part of the system anyway),
> but if a company/developer want to use something else they can do that. It's a
> usage of standard interfaces using open() and ioctl().

That's not what I meant. You don't really need to use the ioctl if the
kernel already knows what the device does. Just call tty_set_ldisc from
an appropriate location in the code.

> I still don't think that you should have a line discipline for other transports
> than UART. If I would look at how I would implement an SPI driver for CG2900,
> there would almost be no code that could be used in common between cg2900_uart 
> and cg2900_spi. SPI doesn't change baud rate, SPI uses commands for sleep/wake
> instead of Break, SPI packets doesn't need extra packetizing, etc.

If you don't want the others to be handled in a line discipline, you should
start out with a patch that splits the hci uart protocol registration from the
line discipline code in drivers/bluetooth/hci_ldisc.c. Today, you can have
multiple HCI drivers in the system, but only the serial one can have
subprotocols.

> > I don't understand the problem with relying on the hci-uart or hci-h4
> > modules. If the hardware uses the H4 protocol, we certainly should use
> > the kernel module that knows how to deal with H4, and we don't want
> > to have two modules implementing the same protocol.
> > 
> 
> I must say I don't understand this problem either. Unless the protocol driver is
> activated through ioctl SET_PROTOCOL, the code will not be executed, and the amount
> of ROM needed is negligible.
> If you look at our submission, the hci-h4 could possibly be reused to some extent.
> Basically the packetizing to the Bluetooth H4 channels 1-4 could be re-used. In order 
> to allow for vendor specific channels to be handled some new registration system on
> top of hci-h4 plus a callback system for data reception would have to be added (in 
> order to facilitate systems that do not want all data to be sent directly to the
> Bluetooth stack such as the CG2900 driver). I'm afraid that we would have a 
> significantly more complex system and larger amount of code if we would try to generalize
> the hci-h4 module. I definitely prefer the current model where a vendor specific driver
> replaces the hci-h4 protocol driver.

Then remove the hci-h4 driver from the kernel and make sure that your driver can
handle all the hardware that h4 can today, using identical user interfaces.

Two infrastructure drivers doing almost the same thing are not acceptable.
We sometimes have device drivers that are meant for the same hardware, but
then one of them is going away over time, when it is shown that the new one
does everything we need. For infrastructure, this is not as easy, because
then you get other people writing new backend for both of them and they
won't be interoperable.
 
> > The one important goal here should be to avoid code duplication.
> > Using the header to get the data structures from separate code
> > means you need to have similar parsing functions in each of the modules
> > using it. Not sharing the header wouldn't help, because then you end
> > up duplicating even more. The real solution, speaking very broadly,
> > must be to have a general module that deals with whatever the more
> > specific modules have in common, and have a header file that defines
> > the interface to it.
> > 
> 
> In general I agree with you, but there are some problems here.
> The most used BT HCI events are Command Status and Command Complete.
> Command Status could be parsed completely in a good way (retrieving op
> code, nbr of commands allowed, and status of command sent).
> Command Complete is however quite complex since the returned data will
> differ depending on command sent. Op code and nbr of commands allowed can
> be retrieved but everything else have to be extracted differently depending
> on command. This means that there is not much that will be saved here. But
> maybe we could extract some parsing into common functions, but I don't
> think you would gain that much.
>
> Moreover, this would lead to a major reimplementation of the hci_core.c and
> related files, since they do not use any exported common functions as it is
> today. I do not know if they (BlueZ community) would want this and I do not
> think that that should be part of the CG2900 driver to do. It should in that
> case be done separately.

No. If hci_core is not sufficient to work with cg2900, the answer is certainly
not to ignore hci_core and roll your own copy. Can you give an example
of how the command complete logic needs to react to different commands?
Can this be done using callbacks into the code that initially sent the
commands?

> For the CG2900 that is not possible. Even if you are running just GPS you
> still must download patches and settings and that is done through HCI
> commands over the Bluetooth command interface. We also use Bluetooth
> commands to identify the controller.

I don't understand. Do you mean the settings are sent over the wireless
interface in order to control devices that are connected directly to
the HCI? Why would you do that instead of just attaching the
GPS to the HCI on the non-bluetooth side?

	Arnd

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Linus Walleij]

On Fri, Feb 25, 2011 at 6:55 PM, Vitaly Wool <vitalywool@gmail.com> wrote:

> Making hardware work is, if course, a valid point, but porting a windows
> driver would do that as well, and it's not the reason to pull windows
> drivers' ports into mainline.

Yeah hm, that sounds like it has "drivers/staging" printed all over it then,
so either we solve these issues or we need to put it into staging so we
can get help from others in fixing it up (we have a similar situation with
the U8500 graphics stuff).

P-G do you think it would be proper to take this into staging for the
moment, or do you prefer to drive it outside the mainline kernel tree until
it lands in the proper place(s)?

Yours,
Linus Walleij

RE: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Par-Gunnar HJALMDAHL]

As you say, I think it could be good to put it into staging so we have a driver in place and then we can continue discussing the architecture from there.

/P-G

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: den 28 februari 2011 23:44
> To: Vitaly Wool; Par-Gunnar Hjalmdahl
> Cc: Par-Gunnar HJALMDAHL; Samuel Ortiz; Marcel Holtmann; Lee Jones;
> Alan Cox; Arnd Bergmann; linux-bluetooth@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lukasz Rymanowski; Pavan Savoy
> Subject: Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor
> 
> On Fri, Feb 25, 2011 at 6:55 PM, Vitaly Wool <vitalywool@gmail.com>
> wrote:
> 
> > Making hardware work is, if course, a valid point, but porting a
> windows
> > driver would do that as well, and it's not the reason to pull windows
> > drivers' ports into mainline.
> 
> Yeah hm, that sounds like it has "drivers/staging" printed all over it
> then,
> so either we solve these issues or we need to put it into staging so we
> can get help from others in fixing it up (we have a similar situation
> with
> the U8500 graphics stuff).
> 
> P-G do you think it would be proper to take this into staging for the
> moment, or do you prefer to drive it outside the mainline kernel tree
> until
> it lands in the proper place(s)?
> 
> Yours,
> Linus Walleij

Re: [PATCH 00/11] mfd and bluetooth: Add CG2900 suppor

[Arnd Bergmann]

On Tuesday 01 March 2011, Par-Gunnar HJALMDAHL wrote:
> As you say, I think it could be good to put it into staging so we have
> a driver in place and then we can continue discussing the architecture from there.

Yes, makes sense.

	Arnd