2010/10/31, Alan Cox <[email protected]>:
>> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>
> Ah I see - I had assumed any actual final merge would be assigning it a
> new LDISC code as is required.
>
Hi,
Sorry for not answering quicker. We in my department have been
discussing new design as well as trying out some solutions.
As an answer to previous comments, both from Arnd and Alan, we would
like to do the following:
- Introduce a new ldisc called N_H4_COMBO with a ldisc driver
accordingly that will be placed under drivers/char
- Keep CG2900 driver as an MFD. We will however register the MFD
cells in each driver and remove the function API. The functions
(write, reset, etc) will instead be placed in each MFD cell using
function pointers. This way we can use different functions for
different channels as needed. By placing the MFD cell registration in
each chip driver we will also only expose the channels that are
actually supported by each chip. This way we can also remove the
pretty ugly matching between the devices and respective H4 channel as
well as the add_h4_user function and the similar other functions.
We were thinking about if we could re-use the existing hci_ldisc
driver. As stated before the big problem here is however that
hci_ldisc directly registers to the Bluetooth stack. We could separate
the data for FM and GPS in the protocol driver, but it is pretty ugly
to have two separate data paths within the same driver.
As I've state earlier this would also not work with other transports
such as SPI.I appreciate Arnd's suggestion to create a TTY for SPI,
but I think that would overcomplicate the situation and create a
solution more complex than actually needed. We think that creating a
new ldisc creates a cleaner solution. It will to some extent remind of
hci_ldisc, but it will not register to any framework except tty.
We've thought about if we should switch cg2900 to a bus, but after
discussing this we feel that CG2900 has such individual
characteristics that it could be hard to create a bus that would suite
it. We therefore will keep the MFD type. We might in the future add
new chips to the driver, but they will most likely not require so much
rework of the driver that we will have to create something completely
new. We will rather create a lib file that contains common code to
reduce total code size of the driver.
Do you see any problems with our suggestions?
/P-G
> - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
> accordingly that will be placed under drivers/char
That seems sensible. That code should also be entirely hardware
independent.
> solution more complex than actually needed. We think that creating a
> new ldisc creates a cleaner solution. It will to some extent remind of
> hci_ldisc, but it will not register to any framework except tty.
Ok. So presumably both the SPI and the tty interfaces would then use the
same library code to do the demux - or are the protocols different ?
> We've thought about if we should switch cg2900 to a bus, but after
> discussing this we feel that CG2900 has such individual
> characteristics that it could be hard to create a bus that would suite
> it. We therefore will keep the MFD type. We might in the future add
> new chips to the driver, but they will most likely not require so much
> rework of the driver that we will have to create something completely
> new. We will rather create a lib file that contains common code to
> reduce total code size of the driver.
No particular opinion either way. Whatever works best and keeps it
modular.
On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
> 2010/10/31, Alan Cox <[email protected]>:
> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
> >
> > Ah I see - I had assumed any actual final merge would be assigning it a
> > new LDISC code as is required.
>
> Sorry for not answering quicker. We in my department have been
> discussing new design as well as trying out some solutions.
>
> As an answer to previous comments, both from Arnd and Alan, we would
> like to do the following:
> - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
> accordingly that will be placed under drivers/char
I'm not convinced, although that's what Alan was suggesting. I'd like
to hear from the bluetooth people what they think about this.
Could you summarize why you think that cg2900 is different enough from
an HCI to require a different line discipline? From your previous
explanation it sounded like it was mostly added functionality,
not something entirely different.
> - Keep CG2900 driver as an MFD. We will however register the MFD
> cells in each driver and remove the function API. The functions
> (write, reset, etc) will instead be placed in each MFD cell using
> function pointers. This way we can use different functions for
> different channels as needed. By placing the MFD cell registration in
> each chip driver we will also only expose the channels that are
> actually supported by each chip. This way we can also remove the
> pretty ugly matching between the devices and respective H4 channel as
> well as the add_h4_user function and the similar other functions.
Ok, excellent.
> We were thinking about if we could re-use the existing hci_ldisc
> driver. As stated before the big problem here is however that
> hci_ldisc directly registers to the Bluetooth stack. We could separate
> the data for FM and GPS in the protocol driver, but it is pretty ugly
> to have two separate data paths within the same driver.
One of us must be misreading the code. As far as I can tell, hci_ldisc
does not register to the bluetooth stack at all, it just provides
the basic multiplex for multiple HCI protocols, while hci_h4.c
communicates to the bluetooth stack.
If I read it correctly, that means that you can still use hci_ldisc,
but need to add another protocol next to hci_h4 and hci_bcsp for
your cg2900.
> As I've state earlier this would also not work with other transports
> such as SPI.I appreciate Arnd's suggestion to create a TTY for SPI,
> but I think that would overcomplicate the situation and create a
> solution more complex than actually needed. We think that creating a
> new ldisc creates a cleaner solution. It will to some extent remind of
> hci_ldisc, but it will not register to any framework except tty.
How would registering ony to tty solve the problem of SPI?
If you just add another hci_cg2900 module, it can register to both
the hci_ldisc and the spi code.
> We've thought about if we should switch cg2900 to a bus, but after
> discussing this we feel that CG2900 has such individual
> characteristics that it could be hard to create a bus that would suite
> it. We therefore will keep the MFD type. We might in the future add
> new chips to the driver, but they will most likely not require so much
> rework of the driver that we will have to create something completely
> new. We will rather create a lib file that contains common code to
> reduce total code size of the driver.
Yes, makes sense.
Arnd
2010/11/8 Arnd Bergmann <[email protected]>:
> On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
>> 2010/10/31, Alan Cox <[email protected]>:
>> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>> >
>> > Ah I see - I had assumed any actual final merge would be assigning it a
>> > new LDISC code as is required.
>>
>> Sorry for not answering quicker. We in my department have been
>> discussing new design as well as trying out some solutions.
>>
>> As an answer to previous comments, both from Arnd and Alan, we would
>> like to do the following:
>> ?- Introduce a new ldisc called N_H4_COMBO with a ldisc driver
>> accordingly that will be placed under drivers/char
>
> I'm not convinced, although that's what Alan was suggesting. I'd like
> to hear from the bluetooth people what they think about this.
>
> Could you summarize why you think that cg2900 is different enough from
> an HCI to require a different line discipline? From your previous
> explanation it sounded like it was mostly added functionality,
> not something entirely different.
>
>> We were thinking about if we could re-use the existing hci_ldisc
>> driver. As stated before the big problem here is however that
>> hci_ldisc directly registers to the Bluetooth stack. We could separate
>> the data for FM and GPS in the protocol driver, but it is pretty ugly
>> to have two separate data paths within the same driver.
>
> One of us must be misreading the code. As far as I can tell, hci_ldisc
> does not register to the bluetooth stack at all, it just provides
> the basic multiplex for multiple HCI protocols, while hci_h4.c
> communicates to the bluetooth stack.
>
> If I read it correctly, that means that you can still use hci_ldisc,
> but need to add another protocol next to hci_h4 and hci_bcsp for
> your cg2900.
>
If you look in function hci_ldisc.c/hci_uart_register_dev(), it here
registers as a driver to the Bluetooth stack. This means that received
Bluetooth packets would go ldisc->protocol->ldisc->bluetooth, while FM
and GPS would go ldisc->protocol->(FM/GPS)stack. I think it's quite
bad to have two different data paths like this. The new ldisc we're
creating looks a lot like hci_ldisc, but it does not register itself
to an overlaying stack directly.
One option would of course be to modify the existing hci_ldisc.c but I
feel that be rather dangerous and which could create a lot of
problems.
/P-G
2010/11/11 Par-Gunnar Hjalmdahl <[email protected]>:
> 2010/11/8 Arnd Bergmann <[email protected]>:
>> On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
>>> 2010/10/31, Alan Cox <[email protected]>:
>>> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>>> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>>> >
>>> > Ah I see - I had assumed any actual final merge would be assigning it a
>>> > new LDISC code as is required.
>>>
>>> Sorry for not answering quicker. We in my department have been
>>> discussing new design as well as trying out some solutions.
>>>
>>> As an answer to previous comments, both from Arnd and Alan, we would
>>> like to do the following:
>>> ?- Introduce a new ldisc called N_H4_COMBO with a ldisc driver
>>> accordingly that will be placed under drivers/char
>>
>> I'm not convinced, although that's what Alan was suggesting. I'd like
>> to hear from the bluetooth people what they think about this.
>>
>> Could you summarize why you think that cg2900 is different enough from
>> an HCI to require a different line discipline? From your previous
>> explanation it sounded like it was mostly added functionality,
>> not something entirely different.
>>
>>> We were thinking about if we could re-use the existing hci_ldisc
>>> driver. As stated before the big problem here is however that
>>> hci_ldisc directly registers to the Bluetooth stack. We could separate
>>> the data for FM and GPS in the protocol driver, but it is pretty ugly
>>> to have two separate data paths within the same driver.
>>
>> One of us must be misreading the code. As far as I can tell, hci_ldisc
>> does not register to the bluetooth stack at all, it just provides
>> the basic multiplex for multiple HCI protocols, while hci_h4.c
>> communicates to the bluetooth stack.
>>
>> If I read it correctly, that means that you can still use hci_ldisc,
>> but need to add another protocol next to hci_h4 and hci_bcsp for
>> your cg2900.
>>
>
> If you look in function hci_ldisc.c/hci_uart_register_dev(), it here
> registers as a driver to the Bluetooth stack. This means that received
> Bluetooth packets would go ldisc->protocol->ldisc->bluetooth, while FM
> and GPS would go ldisc->protocol->(FM/GPS)stack. I think it's quite
> bad to have two different data paths like this. The new ldisc we're
> creating looks a lot like hci_ldisc, but it does not register itself
> to an overlaying stack directly.
> One option would of course be to modify the existing hci_ldisc.c but I
> feel that be rather dangerous and which could create a lot of
> problems.
>
> /P-G
>
After looking again at the code I see that I was wrong.
For the receiving path the data will go ldics->protocol->stack. It's
actually the TX path (to the chip) that is a bit strange where
Bluetooth data is going to stack->ldisc->protocol->ldisc->uart. Here
we would have a separate path for FM and GPS.
I still don't like the idea of making a tty/ldisc for the SPI
transport. I definitely would prefer instead a new ldisc which doesn't
register to any stack on top. My preference would be to keep the
solutions independent, where we use tty/ldisc for UART and a direct
transport protocol driver for SPI (i.e. registering using
spi_register_driver).
/P-G
On Thursday 11 November 2010, Par-Gunnar Hjalmdahl wrote:
> > One option would of course be to modify the existing hci_ldisc.c but I
> > feel that be rather dangerous and which could create a lot of
> > problems.
>
> After looking again at the code I see that I was wrong.
> For the receiving path the data will go ldics->protocol->stack. It's
> actually the TX path (to the chip) that is a bit strange where
> Bluetooth data is going to stack->ldisc->protocol->ldisc->uart. Here
> we would have a separate path for FM and GPS.
Ok, but is that a problem? You really should not be afraid of
touching existing code in order to make it more generic or removing
strange code paths like the one you just described. Cleaning up
code is usually better than duplicating it when you notice something
wrong with the existing implementation.
Simply calling the underlying ldisc module from the FM and GPS modules
should not even require changes, right?
> I still don't like the idea of making a tty/ldisc for the SPI
> transport. I definitely would prefer instead a new ldisc which doesn't
> register to any stack on top.
Yes, agreed. You had already convinced me in the previous reply, sorry
if that wasn't clear.
> My preference would be to keep the
> solutions independent, where we use tty/ldisc for UART and a direct
> transport protocol driver for SPI (i.e. registering using
> spi_register_driver).
Yes. However, you can probably share substantial parts of the code
between the spi_driver and the hci_uart_proto implementations.
You can do this either by making them a single module that registers
to both hci_ldisc and spi_bus, or having a common cg2900 base module
that does the common parts and add separate modules for spi and
hci_uart that register a small wrapper to the respective subsystems.
Arnd