LinuxLists.cc - [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Friday 29 October 2010, Par-Gunnar Hjalmdahl wrote:
> I might have been a bit too quick there.
> The actual channel matching and packet creation is done in hci_h4.c
> while ldisc registration is done in hci_ldisc.c.
> So it might to be enough to create a new hci_h4-cg2900.c (or similar
> name) that can separate the right channels.

That sounds good, but would that still be h4?

There are currently six UART protocols that have drivers in Linux:
H4, bcsp, 3Weire, h4ds, ll and ath3k.

Can cg2900 be simply another one of those, or is it different
from the others?

> We must however do changes
> to hci_ldisc as well since it seems to always register to the
> Bluetooth stack here, which we definitely don't want since that is
> handled by btcg2900.c.

Can you elaborate? You said earlier that cg2900 is a standard HCI
with some extensions. If that's the case, why do you need your own
btcg2900 driver to handle bluetooth instead of the regular hci code?

> Also note that this ldisc issue is only valid when using UART as
> transport. We will also support SPI and then we will probably run into
> completely new, interesting problems. :-)

Is the link layer on SPI different from the UART variant? Maybe you cna
just add a SPI TTY driver if that doesn't exist yet and bind the same
ldisc to the SPI device.

Arnd

2010-10-30 00:01:01

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Friday 29 October 2010, Alan Cox wrote:

> > line discipline then even though that could cause other problems. I do
> > not know if it is possible to add a condition in Kconfig otherwise so
> > the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> > selected.
>
> Not sure I follow the concern here ?

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.

The solution would not be to make the two ldisc implementations mutually
exclusive, but to make cg2900 use a new ldisc number, e.g. N_HCI_CG2900.
However, I'm still not convinced that this is actually necessary, we might
be able to add hooks into the existing N_HCI implementation for the
extensions in cg2900.

Arnd

2010-10-29 16:24:43

by Alan

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

> The reason is that the work is generated so often that a work is not
> finished before next work of same type comes. This is especially true
> for transmit and receive. Then I get 0 back when queuing the work and
> there is no real way to solve it from what I can see than to allocate
> new work structures every time.

So if that is the case what bounds your memory usage - can a busy box end
up with thousands of work queue slos used ? It sounds like your model is
perhaps wrong - if there is a continual stream of work maybe you should
simply have a kernel thread to handle it if it cannot be deferred
- remember ldisc code is able to sleep in most paths.

2010-10-29 16:22:14

by Alan

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

> > Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
>
> We also need the ldisc code to handle events from FM and GPS and since
> that is chip specific we cannot add that to the generic hci_ldisc
> code.

Agreed - it's a different protocol.

> I agree that we might run into problems if two drivers try to register
> the same line discipline. It might then be better to introduce a new

If your ldisc is written properly it shouldn't matter. Each tty has a
private ldisc pointer to keep the per ldisc instance data.

> line discipline then even though that could cause other problems. I do
> not know if it is possible to add a condition in Kconfig otherwise so
> the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> selected.

Not sure I follow the concern here ?

2010-10-29 12:08:11

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

2010/10/29 Par-Gunnar Hjalmdahl <[email protected]>:
> 2010/10/28 Arnd Bergmann <[email protected]>:
>> On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:
>>
>>> >
>>> > So - NAK this for the moment, it needs to be split cleanly into ldisc
>>> > (thing which speaks the protocol and eg sees "speed change required" and
>>> > acts on it) and device (thing which knows about the hardware).
>>>
>>> OK. We will try to figure out a new design.
>>> I'm not too happy about putting the ldisc part in Bluetooth though
>>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>>> maybe be under char/?
>>
>> After getting a better idea of what the base mfd driver does, my impression
>> is now that you should not register a second N_HCI line discipline at all,
>> but instead extend the existing line discipline with this number.
>>
>> I'm not sure what happens if you need two modules that try to register
>> the same ldisc number, but I imagine it is not good.
>>
>> Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
>>
>> ? ? ? ?Arnd
>>
>
> We also need the ldisc code to handle events from FM and GPS and since
> that is chip specific we cannot add that to the generic hci_ldisc
> code.
> I agree that we might run into problems if two drivers try to register
> the same line discipline. It might then be better to introduce a new
> line discipline then even though that could cause other problems. I do
> not know if it is possible to add a condition in Kconfig otherwise so
> the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> selected.
>
> /P-G
>

Hi again,

I might have been a bit too quick there.
The actual channel matching and packet creation is done in hci_h4.c
while ldisc registration is done in hci_ldisc.c.
So it might to be enough to create a new hci_h4-cg2900.c (or similar
name) that can separate the right channels. We must however do changes
to hci_ldisc as well since it seems to always register to the
Bluetooth stack here, which we definitely don't want since that is
handled by btcg2900.c.
Also note that this ldisc issue is only valid when using UART as
transport. We will also support SPI and then we will probably run into
completely new, interesting problems. :-)

/P-G

2010-10-29 11:58:51

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

2010/10/28 Arnd Bergmann <[email protected]>:
> On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:
>
>> >
>> > So - NAK this for the moment, it needs to be split cleanly into ldisc
>> > (thing which speaks the protocol and eg sees "speed change required" and
>> > acts on it) and device (thing which knows about the hardware).
>>
>> OK. We will try to figure out a new design.
>> I'm not too happy about putting the ldisc part in Bluetooth though
>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>> maybe be under char/?
>
> After getting a better idea of what the base mfd driver does, my impression
> is now that you should not register a second N_HCI line discipline at all,
> but instead extend the existing line discipline with this number.
>
> I'm not sure what happens if you need two modules that try to register
> the same ldisc number, but I imagine it is not good.
>
> Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
>
> ? ? ? ?Arnd
>

We also need the ldisc code to handle events from FM and GPS and since
that is chip specific we cannot add that to the generic hci_ldisc
code.
I agree that we might run into problems if two drivers try to register
the same line discipline. It might then be better to introduce a new
line discipline then even though that could cause other problems. I do
not know if it is possible to add a condition in Kconfig otherwise so
the CG2900 ldisc cannot be active while the "normal" ldisc driver is
selected.

/P-G

2010-10-29 11:53:57

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

2010/10/22 Alan Cox <[email protected]>:
>> +
>> +/**
>> + * create_work_item() - Create work item and add it to the work queue.
>> + * @wq: ? ? ? ? ? ? ?work queue struct where the work will be added.
>> + * @work_func: ? ? ? Work function.
>> + * @data: ? ?Private data for the work.
>> + *
>> + * Returns:
>> + * ? 0 if there is no error.
>> + * ? -EBUSY if not possible to queue work.
>> + * ? -ENOMEM if allocation fails.
>> + */
>> +static int create_work_item(struct workqueue_struct *wq, work_func_t work_func,
>> + ? ? ? ? ? ? ? ? ? ? ? ? void *data)
>> +{
>> + ? ? struct uart_work_struct *new_work;
>> + ? ? int err;
>> +
>> + ? ? new_work = kmalloc(sizeof(*new_work), GFP_ATOMIC);
>
> So instead of a tiny object embedded in your device you've got a whole
> error path to worry about, a printk disguised in a macro and a text
> string longer than the struct size ? Surely this should be part of the
> device
>

I've tried now to use 3 separate work_structs in the device (one for
each work function used), but this doesn't work out.
The reason is that the work is generated so often that a work is not
finished before next work of same type comes. This is especially true
for transmit and receive. Then I get 0 back when queuing the work and
there is no real way to solve it from what I can see than to allocate
new work structures every time.

/P-G

2010-10-28 12:22:46

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Friday 22 October 2010, Par-Gunnar Hjalmdahl wrote:

> >
> > So - NAK this for the moment, it needs to be split cleanly into ldisc
> > (thing which speaks the protocol and eg sees "speed change required" and
> > acts on it) and device (thing which knows about the hardware).
>
> OK. We will try to figure out a new design.
> I'm not too happy about putting the ldisc part in Bluetooth though
> since it is only partly Bluetooth, it is also GPS and FM. Better could
> maybe be under char/?

After getting a better idea of what the base mfd driver does, my impression
is now that you should not register a second N_HCI line discipline at all,
but instead extend the existing line discipline with this number.

I'm not sure what happens if you need two modules that try to register
the same ldisc number, but I imagine it is not good.

Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?

Arnd

2010-10-28 10:37:20

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Thanks again for your comments Alan.

Next patch set will contain resolution to all your comments. See below.

/P-G

2010/10/22 Alan Cox <[email protected]>:
>> The existence of the callback is checked in the function
>> uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
>> sleep and this code will never run.
>
> OK yes see this now.
>
>> >> + ? ? ? ? ? ? CG2900_ERR("Failed to alloc memory for uart_work_struct!");
>> >
>> > Please use the standard dev_dbg etc functionality - or pr_err etc when
>> > you have no device pointer. The newest kernel tty object has a device
>> > pointer added so you could use that.
>> >
>>
>> OK. I like the debug system we have now (using module parameter to set
>> debug level in runtime), but I've received comments regarding this
>> before so I assume we will have to switch to standard printouts.
>> Is it OK to use defines or inline functions to add "CG2900" before and
>> '\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?
>
> The pr_fmt bit will do what you want for a non dev_dbg type thing. See
> include/linux/kernel.h
>

OK. Added. I'm however using dev_err, dev_dbg, etc where possible.

>> >> + * unset_cts_irq() - Disable interrupt on CTS.
>> >> + */
>> >> +static void unset_cts_irq(void)
>> >
>> > And no ability to support multiple devices
>> >
>>
>> OK. We will try to fix this.
>
> That may go away when you clean up the device side. The line discipline
> can be attached to any device so must be multi-device aware, the hardware
> driver can certainly be single device only if you can only ever have one
>
> [Although its a good idea to design it so it can be fixed because you
> ?never know when you'll find your sales team just sold someone a two
> ?device solution ;) ]
>

OK. Design still ongoing, but will be fixed in some way.

>> >> + ? ? ? ? ? ? set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> >> + ? ? ? ? ? ? len = tty->ops->write(tty, skb->data, skb->len);
>> >
>> > A tty isn't required to have a write function
>>
>> I don't quite understand your comment here. This particular code is
>> inspired of the Bluetooth ldisc code and there it is used like. It
>> seems to work fine so we do it the same way.
>
> You copied a security hole from the bluetooth driver which we found a
> couple of weeks ago
>
>> How would you prefer it to be?
>
> Check it is valid, in your case probably on open just refuse to attach to
> a read only port.
>

OK. Done.

>> OK. We will try to figure out a new design.
>> I'm not too happy about putting the ldisc part in Bluetooth though
>> since it is only partly Bluetooth, it is also GPS and FM. Better could
>> maybe be under char/?
>
> Works for me - there is an ongoing "we must move tty ldiscs and core tty
> code somewhere more sensible of their own" discussion, so if it is
> dropped into char, it'll move with them just fine.
>
> Alan
>

Again, thanks for the comments.

/P-G

2010-10-22 15:33:59

by Alan

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

> The existence of the callback is checked in the function
> uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
> sleep and this code will never run.

OK yes see this now.

> >> + ? ? ? ? ? ? CG2900_ERR("Failed to alloc memory for uart_work_struct!");
> >
> > Please use the standard dev_dbg etc functionality - or pr_err etc when
> > you have no device pointer. The newest kernel tty object has a device
> > pointer added so you could use that.
> >
>
> OK. I like the debug system we have now (using module parameter to set
> debug level in runtime), but I've received comments regarding this
> before so I assume we will have to switch to standard printouts.
> Is it OK to use defines or inline functions to add "CG2900" before and
> '\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?

The pr_fmt bit will do what you want for a non dev_dbg type thing. See
include/linux/kernel.h

> >> + * unset_cts_irq() - Disable interrupt on CTS.
> >> + */
> >> +static void unset_cts_irq(void)
> >
> > And no ability to support multiple devices
> >
>
> OK. We will try to fix this.

That may go away when you clean up the device side. The line discipline
can be attached to any device so must be multi-device aware, the hardware
driver can certainly be single device only if you can only ever have one

[Although its a good idea to design it so it can be fixed because you
never know when you'll find your sales team just sold someone a two
device solution ;) ]

> >> + ? ? ? ? ? ? set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> >> + ? ? ? ? ? ? len = tty->ops->write(tty, skb->data, skb->len);
> >
> > A tty isn't required to have a write function
>
> I don't quite understand your comment here. This particular code is
> inspired of the Bluetooth ldisc code and there it is used like. It
> seems to work fine so we do it the same way.

You copied a security hole from the bluetooth driver which we found a
couple of weeks ago

> How would you prefer it to be?

Check it is valid, in your case probably on open just refuse to attach to
a read only port.

> OK. We will try to figure out a new design.
> I'm not too happy about putting the ldisc part in Bluetooth though
> since it is only partly Bluetooth, it is also GPS and FM. Better could
> maybe be under char/?

Works for me - there is an ongoing "we must move tty ldiscs and core tty
code somewhere more sensible of their own" discussion, so if it is
dropped into char, it'll move with them just fine.

Alan

2010-10-22 14:54:44

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi and thanks for your comments Alan.
See below for answers.

/P-G

2010/10/22 Alan Cox <[email protected]>:
>
>> + * is_chip_flow_off() - Check if chip has set flow off.
>> + * @tty: ? ? Pointer to tty.
>> + *
>> + * Returns:
>> + * ? true - chip flows off.
>> + * ? false - chip flows on.
>> + */
>> +static bool is_chip_flow_off(struct tty_struct *tty)
>> +{
>> + ? ? int lines;
>> +
>> + ? ? lines = tty->ops->tiocmget(tty, uart_info->fd);
>> +
>> + ? ? if (lines & TIOCM_CTS)
>> + ? ? ? ? ? ? return false;
>> + ? ? else
>> + ? ? ? ? ? ? return true;
>> +}
>
> What if the device doesn't have a tiocmget ? You must check this. If you
> want to call into the ttys own tiocmget froma sleeping context fine, but
> see tty_tiocmget and you'll notice you need to check the op is non NULL
> somewhere. You could do this when the ldisc is opened and refuse to open
> - some ldiscs do that
>

The existence of the callback is checked in the function
uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow
sleep and this code will never run.

>
>> +
>> +/**
>> + * create_work_item() - Create work item and add it to the work queue.
>> + * @wq: ? ? ? ? ? ? ?work queue struct where the work will be added.
>> + * @work_func: ? ? ? Work function.
>> + * @data: ? ?Private data for the work.
>> + *
>> + * Returns:
>> + * ? 0 if there is no error.
>> + * ? -EBUSY if not possible to queue work.
>> + * ? -ENOMEM if allocation fails.
>> + */
>> +static int create_work_item(struct workqueue_struct *wq, work_func_t work_func,
>> + ? ? ? ? ? ? ? ? ? ? ? ? void *data)
>> +{
>> + ? ? struct uart_work_struct *new_work;
>> + ? ? int err;
>> +
>> + ? ? new_work = kmalloc(sizeof(*new_work), GFP_ATOMIC);
>
> So instead of a tiny object embedded in your device you've got a whole
> error path to worry about, a printk disguised in a macro and a text
> string longer than the struct size ? Surely this should be part of the
> device
>

OK. We can embed the work struct in the device structure.

>> + ? ? if (!new_work) {
>> + ? ? ? ? ? ? CG2900_ERR("Failed to alloc memory for uart_work_struct!");
>
> Please use the standard dev_dbg etc functionality - or pr_err etc when
> you have no device pointer. The newest kernel tty object has a device
> pointer added so you could use that.
>

OK. I like the debug system we have now (using module parameter to set
debug level in runtime), but I've received comments regarding this
before so I assume we will have to switch to standard printouts.
Is it OK to use defines or inline functions to add "CG2900" before and
'\n' after (as BT_INFO in include/net/bluetooth/bluetooth.h)?
>
>> + * set_tty_baud() - Called to set specific baud in TTY.
>> + * @tty: ? ? Tty device.
>> + * @baud: ? ?Baud to set.
>> + *
>> + * Returns:
>> + * ? true - baudrate set with success.
>> + * ? false - baundrate set failure.
>> + */
>> +static bool set_tty_baud(struct tty_struct *tty, int baud)
>> +{
>> + ? ? struct ktermios *old_termios;
>> + ? ? bool retval = true;
>> +
>> + ? ? old_termios = kmalloc(sizeof(*old_termios), GFP_ATOMIC);
>
> termios struct is easily small enough to be fine on the stack
>

OK.

>> + ? ? /* Let's mark that CG2900 driver uses c_ispeed and c_ospeed fields. */
>> + ? ? tty->termios->c_cflag |= BOTHER;
>
> This shouldn't be needed - the tty_encode_baud_rate logic works out of
> BOTHER must be set
>

I will have to check with the guy who wrote this code. I think he put
it there for a reason.

>> + ? ? tty->termios->c_cflag &= ~BOTHER;
>
> And your termios is now potentially invalid
>

As above, I will check.

>
>> + ? ? /* Set IRQ on CTS. */
>> + ? ? err = request_irq(pf_data->uart.cts_irq,
>> + ? ? ? ? ? ? ? ? ? ? ? cts_interrupt,
>> + ? ? ? ? ? ? ? ? ? ? ? IRQF_TRIGGER_FALLING,
>> + ? ? ? ? ? ? ? ? ? ? ? UART_NAME,
>> + ? ? ? ? ? ? ? ? ? ? ? NULL);
>
> So we now ave terminal tty/ldisc confusion
>

The guy responsible for this is thinking through a new design, where
we can move the hardware specific handling to another place.

>> + ? ? if (err) {
>> + ? ? ? ? ? ? CG2900_ERR("Could not request CTS IRQ (%d)", err);
>> + ? ? ? ? ? ? goto error;
>> + ? ? }
>> +
>> +#ifdef CONFIG_PM
>> + ? ? enable_irq_wake(pf_data->uart.cts_irq);
>> +#endif
>> + ? ? return 0;
>> +
>> +error:
>> + ? ? if (pf_data->uart.enable_uart)
>> + ? ? ? ? ? ? (void)pf_data->uart.enable_uart();
>> + ? ? return err;
>
>> +}
>> +
>> +/**
>> + * unset_cts_irq() - Disable interrupt on CTS.
>> + */
>> +static void unset_cts_irq(void)
>
> And no ability to support multiple devices
>

OK. We will try to fix this.

>> + ? ? CG2900_DBG("Clear break");
>> + ? ? tty->ops->break_ctl(tty, TTY_BREAK_OFF);
>
> What if the tty doesn't have one ?
>

See answer above. It is checked in another place.

>
>> + ? ? if (CHIP_AWAKE == uart_info->sleep_state) {
>> + ? ? ? ? ? ? uart_info->tty->ops->break_ctl(uart_info->tty, TTY_BREAK_ON);
>
> Ditto
>

And ditto as well :-)

>
>> + ? ? ? ? ? ? set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> + ? ? ? ? ? ? len = tty->ops->write(tty, skb->data, skb->len);
>
> A tty isn't required to have a write function

I don't quite understand your comment here. This particular code is
inspired of the Bluetooth ldisc code and there it is used like. It
seems to work fine so we do it the same way.
How would you prefer it to be?

>
>> + ? ? ? ? ? ? if ((BAUD_START == uart_info->baud_rate_state) &&
>> + ? ? ? ? ? ? ? ? (is_set_baud_rate_cmd(skb->data))) {
>> + ? ? ? ? ? ? ? ? ? ? CG2900_INFO("UART set baud rate cmd found.");
>> + ? ? ? ? ? ? ? ? ? ? SET_BAUD_STATE(BAUD_SENDING);
>
> Do we really need this all in capitals ?
>

We use a define to get a debug printout when setting state, but I
understand it's not so popular so I guess we will have to skip it.
I guess it won't help if we would use an inline function instead of a
define (which would mean we could skip the capitals)?

>
>
>> + * uart_tty_open() - Called when UART line discipline changed to N_HCI.
>> + * @tty: ? ? Pointer to associated TTY instance data.
>> + *
>> + * Returns:
>> + * ? 0 if there is no error.
>> + * ? Errors from cg2900_register_trans_driver.
>> + */
>> +static int uart_tty_open(struct tty_struct *tty)
>> +{
>> + ? ? int err;
>> +
>> + ? ? CG2900_INFO("uart_tty_open");
>> +
>> + ? ? /* Set the structure pointers and set the UART receive room */
>> + ? ? uart_info->tty = tty;
>
> You must respect the kref handling in the kernel tty code. Take
> references by all means but do it properly.
>

OK. We will add tty_kref_get/put handling.

>
>> +static void uart_tty_wakeup(struct tty_struct *tty)
>> +{
>> + ? ? int err;
>> +
>> + ? ? CG2900_INFO("uart_tty_wakeup");
>> +
>> + ? ? /*
>> + ? ? ?* Clear the TTY_DO_WRITE_WAKEUP bit that is set in
>> + ? ? ?* work_do_transmit().
>> + ? ? ?*/
>> + ? ? clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> +
>> + ? ? if (tty != uart_info->tty)
>> + ? ? ? ? ? ? return;
>
> How can this occur - and why check it after you've changed tty->flags ???
>

OK. Will be removed.

>
>> +/**
>> + * uart_tty_receive() - Called by TTY low level driver when receive
>> data is available.
>> + * @tty: ? ? Pointer to TTY instance data
>> + * @data: ? ?Pointer to received data
>> + * @flags: ? Pointer to flags for data
>> + * @count: ? Count of received data in bytes
>> + */
>> +static void uart_tty_receive(struct tty_struct *tty, const u8 *data,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?char *flags, int count)
>> +{
>> + ? ? CG2900_INFO("uart_tty_receive");
>> +
>> + ? ? if (tty != uart_info->tty)
>> + ? ? ? ? ? ? return;
>
> Again this is nonsense
>

OK.Will be removed.

>
>> +/*
>> + * We don't provide read/write/poll interface for user space.
>> + */
>> +static ssize_t uart_tty_read(struct tty_struct *tty, struct file *file,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned char __user *buf, size_t nr)
>> +{
>> + ? ? CG2900_INFO("uart_tty_read");
>> + ? ? return 0;
>> +}
>
> -EINVAL then
>

OK. We can probably skip registering the callback function completely otherwise.

>> +
>> +static ssize_t uart_tty_write(struct tty_struct *tty, struct file *file,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? const unsigned char *data, size_t count)
>> +{
>> + ? ? CG2900_INFO("uart_tty_write");
>> + ? ? return count;
>
> This is wrong - you can't jusr vanish the data

OK. To be changed.

>> +}
>
>
>
> This needs some restructuring I think
>
> A line discipline should contain no hardware awareness, that goes in the
> actual uart hardware driver. So we shouldn't have magic irq code in this
> part of things.
>
> You also need to sort out the tty kref handling in open/close and the
> fact you've got magic hardcoded single instance stuff buried i nit.
>
> Finally tty ldiscs don't go buried in the mfd directory, or they'll get
> missed during chanegs/updates. The ldisc probably belongs in bluetooth a
> and the hardware support in the mfd directory.
>
>
> So - NAK this for the moment, it needs to be split cleanly into ldisc
> (thing which speaks the protocol and eg sees "speed change required" and
> acts on it) and device (thing which knows about the hardware).
>
>

OK. We will try to figure out a new design.
I'm not too happy about putting the ldisc part in Bluetooth though
since it is only partly Bluetooth, it is also GPS and FM. Better could
maybe be under char/?

2010-10-22 12:51:30

by Alan

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

> + * is_chip_flow_off() - Check if chip has set flow off.
> + * @tty: Pointer to tty.
> + *
> + * Returns:
> + * true - chip flows off.
> + * false - chip flows on.
> + */
> +static bool is_chip_flow_off(struct tty_struct *tty)
> +{
> + int lines;
> +
> + lines = tty->ops->tiocmget(tty, uart_info->fd);
> +
> + if (lines & TIOCM_CTS)
> + return false;
> + else
> + return true;
> +}

What if the device doesn't have a tiocmget ? You must check this. If you
want to call into the ttys own tiocmget froma sleeping context fine, but
see tty_tiocmget and you'll notice you need to check the op is non NULL
somewhere. You could do this when the ldisc is opened and refuse to open
- some ldiscs do that

> +
> +/**
> + * create_work_item() - Create work item and add it to the work queue.
> + * @wq: work queue struct where the work will be added.
> + * @work_func: Work function.
> + * @data: Private data for the work.
> + *
> + * Returns:
> + * 0 if there is no error.
> + * -EBUSY if not possible to queue work.
> + * -ENOMEM if allocation fails.
> + */
> +static int create_work_item(struct workqueue_struct *wq, work_func_t work_func,
> + void *data)
> +{
> + struct uart_work_struct *new_work;
> + int err;
> +
> + new_work = kmalloc(sizeof(*new_work), GFP_ATOMIC);

So instead of a tiny object embedded in your device you've got a whole
error path to worry about, a printk disguised in a macro and a text
string longer than the struct size ? Surely this should be part of the
device

> + if (!new_work) {
> + CG2900_ERR("Failed to alloc memory for uart_work_struct!");

Please use the standard dev_dbg etc functionality - or pr_err etc when
you have no device pointer. The newest kernel tty object has a device
pointer added so you could use that.

> + * set_tty_baud() - Called to set specific baud in TTY.
> + * @tty: Tty device.
> + * @baud: Baud to set.
> + *
> + * Returns:
> + * true - baudrate set with success.
> + * false - baundrate set failure.
> + */
> +static bool set_tty_baud(struct tty_struct *tty, int baud)
> +{
> + struct ktermios *old_termios;
> + bool retval = true;
> +
> + old_termios = kmalloc(sizeof(*old_termios), GFP_ATOMIC);

termios struct is easily small enough to be fine on the stack

> + /* Let's mark that CG2900 driver uses c_ispeed and c_ospeed fields. */
> + tty->termios->c_cflag |= BOTHER;

This shouldn't be needed - the tty_encode_baud_rate logic works out of
BOTHER must be set

> + tty->termios->c_cflag &= ~BOTHER;

And your termios is now potentially invalid

> + /* Set IRQ on CTS. */
> + err = request_irq(pf_data->uart.cts_irq,
> + cts_interrupt,
> + IRQF_TRIGGER_FALLING,
> + UART_NAME,
> + NULL);

So we now ave terminal tty/ldisc confusion

> + if (err) {
> + CG2900_ERR("Could not request CTS IRQ (%d)", err);
> + goto error;
> + }
> +
> +#ifdef CONFIG_PM
> + enable_irq_wake(pf_data->uart.cts_irq);
> +#endif
> + return 0;
> +
> +error:
> + if (pf_data->uart.enable_uart)
> + (void)pf_data->uart.enable_uart();
> + return err;

> +}
> +
> +/**
> + * unset_cts_irq() - Disable interrupt on CTS.
> + */
> +static void unset_cts_irq(void)

And no ability to support multiple devices

> + CG2900_DBG("Clear break");
> + tty->ops->break_ctl(tty, TTY_BREAK_OFF);

What if the tty doesn't have one ?

> + if (CHIP_AWAKE == uart_info->sleep_state) {
> + uart_info->tty->ops->break_ctl(uart_info->tty, TTY_BREAK_ON);

Ditto

> + set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> + len = tty->ops->write(tty, skb->data, skb->len);

A tty isn't required to have a write function

> + if ((BAUD_START == uart_info->baud_rate_state) &&
> + (is_set_baud_rate_cmd(skb->data))) {
> + CG2900_INFO("UART set baud rate cmd found.");
> + SET_BAUD_STATE(BAUD_SENDING);

Do we really need this all in capitals ?

> + * uart_tty_open() - Called when UART line discipline changed to N_HCI.
> + * @tty: Pointer to associated TTY instance data.
> + *
> + * Returns:
> + * 0 if there is no error.
> + * Errors from cg2900_register_trans_driver.
> + */
> +static int uart_tty_open(struct tty_struct *tty)
> +{
> + int err;
> +
> + CG2900_INFO("uart_tty_open");
> +
> + /* Set the structure pointers and set the UART receive room */
> + uart_info->tty = tty;

You must respect the kref handling in the kernel tty code. Take
references by all means but do it properly.

> +static void uart_tty_wakeup(struct tty_struct *tty)
> +{
> + int err;
> +
> + CG2900_INFO("uart_tty_wakeup");
> +
> + /*
> + * Clear the TTY_DO_WRITE_WAKEUP bit that is set in
> + * work_do_transmit().
> + */
> + clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +
> + if (tty != uart_info->tty)
> + return;

How can this occur - and why check it after you've changed tty->flags ???

> +/**
> + * uart_tty_receive() - Called by TTY low level driver when receive
> data is available.
> + * @tty: Pointer to TTY instance data
> + * @data: Pointer to received data
> + * @flags: Pointer to flags for data
> + * @count: Count of received data in bytes
> + */
> +static void uart_tty_receive(struct tty_struct *tty, const u8 *data,
> + char *flags, int count)
> +{
> + CG2900_INFO("uart_tty_receive");
> +
> + if (tty != uart_info->tty)
> + return;

Again this is nonsense

> +/*
> + * We don't provide read/write/poll interface for user space.
> + */
> +static ssize_t uart_tty_read(struct tty_struct *tty, struct file *file,
> + unsigned char __user *buf, size_t nr)
> +{
> + CG2900_INFO("uart_tty_read");
> + return 0;
> +}

-EINVAL then

> +
> +static ssize_t uart_tty_write(struct tty_struct *tty, struct file *file,
> + const unsigned char *data, size_t count)
> +{
> + CG2900_INFO("uart_tty_write");
> + return count;

This is wrong - you can't jusr vanish the data
> +}

This needs some restructuring I think

A line discipline should contain no hardware awareness, that goes in the
actual uart hardware driver. So we shouldn't have magic irq code in this
part of things.

You also need to sort out the tty kref handling in open/close and the
fact you've got magic hardcoded single instance stuff buried i nit.

Finally tty ldiscs don't go buried in the mfd directory, or they'll get
missed during chanegs/updates. The ldisc probably belongs in bluetooth a
and the hardware support in the mfd directory.

So - NAK this for the moment, it needs to be split cleanly into ldisc
(thing which speaks the protocol and eg sees "speed change required" and
acts on it) and device (thing which knows about the hardware).

2010-11-11 15:12:30

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

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

2010-11-11 14:40:58

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

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

2010-11-11 14:28:51

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

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-08 05:24:49

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

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-05 17:19:52

by Alan

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

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

2010-11-05 17:02:46

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

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

2010-12-08 12:51:43

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Wednesday 08 December 2010, Par-Gunnar Hjalmdahl wrote:
> > Marcel did previously suggest a bus-driver sort of a structure, where
> > the transport are sort of adapter drivers, the data interpretation
> > logic like splitting HCI-events, FM CH-8 packets all fall into the
> > algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
> > drivers falling into the client/protocol drivers....
>
> What the cg2900 driver has now turned into is almost like the
> cg2900_core acting as a bus, identifying which chip is connected and
> choosing correct chip driver from this. This cg2900_core module should
> not contain any vendor specific code. When identified, the chip driver
> then uses MFD to setup channels according to the functionality it
> supports (one MFD device per H4 channel). It's hard to explain
> everything of the new architecture here. I would like you to check the
> new patch set I'm trying hard to create at the moment. I will try to
> get it out on Friday, but it's hard to promise anything (there is a
> lot of work to do with it). There you could then see if it is
> something you could consider useful.

It sounds really good, I'm looking forward to your new patch set!
If it's generic enough to replace the ti-st code, that would be a
clear sign that you are on the right track ;-)

I'm not asking to fix their code yourself, but it might be good to
look at it and see if it would work.

Arnd

2010-12-08 12:21:16

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Pavan et al,

2010/12/8 Pavan Savoy <[email protected]>:
> On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <[email protected]> wrote:
>> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:
>>>
>>> >> But I was trying to make a different point here. On a basic level,
>>> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>>> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>>> >> chip that does BT+FM. They all use HCI to access the other functions
>>> >> of the combo chip and they do it in a really simiar way, with the
>>> >> differences mostly in power management techniques. So I think it's
>>> >> quite sensible to have some kind of framework that is suitable for
>>> >> such devices.
>>> >
>>> > Yes, I agree 100% in principle. I could not find the code that
>>> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
>>>
>>> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
>>>
>>> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
>>> sure about the mainline support for those.
>>
>> Ah, it had not occured to me to look in drivers/misc. Looking at the
>> code over there, it becomes much clearer what your point is. Evidently
>> the ti-st driver implements a line discipline similar to, but conflictin=
g
>> with hci_ldisc, and it has its own copy of the hci_ll code.
>>
>> I guess this is exactly what we're trying to avoid having with the
>> cg2900 code ;-).
>>
>>> > The cg2900 solution for this was to use MFD (plus another layer
>>> > in the posted version, but that will go away I assume). Using
>>> > MFD is not the only possibility here, but I could not see anything
>>> > wrong with it either. Do you think we can move them all over to
>>> > use MFD infrastructure?
>>>
>>> I guess so but it's probably more of a detail than what I'm up to now :=
)
>>
>> Agreed.
>>
>>> >> But generally speaking, isn't a line discipline/driver attached to a
>>> >> tty? We can use dumb tty for e. g. SPI and still be able to use
>>> >> hci_ll, right?
>>> >
>>> > I suggested that as well, but the point was made that this would
>>> > add an unnecessary indirection for the SPI case, which is not
>>> > really much like a serial port. It's certainly possible to do it
>>> > like you say, but if we add a way to register the high-level
>>> > protocols with an HCI-like multi-function device, we could
>>> > also do it in a way that does not rely on tty-ldisc but keeps it
>>> > as one of the options.
>>>
>>> I actually don't think it's such a big indirection, I prefer to think
>>> of it more as of the abstraction layer. If not use this, are we going
>>> to have direct SPI device drivers? I'm afraid we might end up with a
>>> huge duplication of code in that case.
>>
>> The question is how it should be modeled in a better way than today.
>>
>> I believe we already agree it should be something like
>>
>> =A0 bt =A0 ti-radio =A0 =A0st-e-radio =A0 =A0ti-gps =A0 st-e-gps =A0broa=
dcom-radio ...
>> =A0 | =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0 =A0| =
=A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0 |
>> =A0 +---------+----------+---------+--+----------+----------+---------+
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0common-hci-module
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0+-----------+-----------+
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A0 =A0 =A0| =A0 =A0 =
=A0 | =A0 =A0 =A0|
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0serial =A0 =A0spi =A0 =A0 i2c =A0=
=A0...
>
> Yes, this sort of architecture would certainly help...
> However, I suppose the most common question as one of you stated above
> was how can the channel -ID and other factors such as offset-header
> packet format be generalized?
>
> As in over here, will the common-hci-module work fine, If say ti-radio
> says he is expecting packets starting from id-8 which is of length 10?
> and also work for st-e-radio which would say expecting data from id-6
> with 15 max bytes?
> Answering this I suppose would solve a lot of our problems....
>
> Marcel did previously suggest a bus-driver sort of a structure, where
> the transport are sort of adapter drivers, the data interpretation
> logic like splitting HCI-events, FM CH-8 packets all fall into the
> algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
> drivers falling into the client/protocol drivers....
>

What the cg2900 driver has now turned into is almost like the
cg2900_core acting as a bus, identifying which chip is connected and
choosing correct chip driver from this. This cg2900_core module should
not contain any vendor specific code. When identified, the chip driver
then uses MFD to setup channels according to the functionality it
supports (one MFD device per H4 channel). It's hard to explain
everything of the new architecture here. I would like you to check the
new patch set I'm trying hard to create at the moment. I will try to
get it out on Friday, but it's hard to promise anything (there is a
lot of work to do with it). There you could then see if it is
something you could consider useful.

/P-G

>
>
>> Any objections to this?
>>
>> If you agree, I think we should discuss the alternatives for the common
>> module. I think you are saying we should make the common module a single
>> ldisc doing the multiplexing and have all transports register as ttys in=
to
>> it, right?
>>
>> What we discussed before was to split it into multiple modules, one for
>> the tty ldisc and one for the common code, and then have the spi/i2c/...
>> drivers feed into the common multiplexer without going through tty.
>>
>> I don't currently see a significant advantage or disadvantage with eithe=
r
>> approach.
>>
>> =A0 =A0 =A0 =A0Arnd
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoot=
h" in
>> the body of a message to [email protected]
>> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>>
>

2010-12-08 07:41:35

by Pavan Savoy

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <[email protected]> wrote:
> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:
>>
>> >> But I was trying to make a different point here. On a basic level,
>> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>> >> chip that does BT+FM. They all use HCI to access the other functions
>> >> of the combo chip and they do it in a really simiar way, with the
>> >> differences mostly in power management techniques. So I think it's
>> >> quite sensible to have some kind of framework that is suitable for
>> >> such devices.
>> >
>> > Yes, I agree 100% in principle. I could not find the code that
>> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
>>
>> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
>>
>> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
>> sure about the mainline support for those.
>
> Ah, it had not occured to me to look in drivers/misc. Looking at the
> code over there, it becomes much clearer what your point is. Evidently
> the ti-st driver implements a line discipline similar to, but conflicting
> with hci_ldisc, and it has its own copy of the hci_ll code.
>
> I guess this is exactly what we're trying to avoid having with the
> cg2900 code ;-).
>
>> > The cg2900 solution for this was to use MFD (plus another layer
>> > in the posted version, but that will go away I assume). Using
>> > MFD is not the only possibility here, but I could not see anything
>> > wrong with it either. Do you think we can move them all over to
>> > use MFD infrastructure?
>>
>> I guess so but it's probably more of a detail than what I'm up to now :)
>
> Agreed.
>
>> >> But generally speaking, isn't a line discipline/driver attached to a
>> >> tty? We can use dumb tty for e. g. SPI and still be able to use
>> >> hci_ll, right?
>> >
>> > I suggested that as well, but the point was made that this would
>> > add an unnecessary indirection for the SPI case, which is not
>> > really much like a serial port. It's certainly possible to do it
>> > like you say, but if we add a way to register the high-level
>> > protocols with an HCI-like multi-function device, we could
>> > also do it in a way that does not rely on tty-ldisc but keeps it
>> > as one of the options.
>>
>> I actually don't think it's such a big indirection, I prefer to think
>> of it more as of the abstraction layer. If not use this, are we going
>> to have direct SPI device drivers? I'm afraid we might end up with a
>> huge duplication of code in that case.
>
> The question is how it should be modeled in a better way than today.
>
> I believe we already agree it should be something like
>
> =C2=A0 bt =C2=A0 ti-radio =C2=A0 =C2=A0st-e-radio =C2=A0 =C2=A0ti-gps =C2=
=A0 st-e-gps =C2=A0broadcom-radio ...
> =C2=A0 | =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
| =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0 |
> =C2=A0 +---------+----------+---------+--+----------+----------+---------=
+
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0common-hci-module
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0+-----------+-----------+
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0| =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A0 =C2=
=A0|
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0seri=
al =C2=A0 =C2=A0spi =C2=A0 =C2=A0 i2c =C2=A0 =C2=A0...

Yes, this sort of architecture would certainly help...
However, I suppose the most common question as one of you stated above
was how can the channel -ID and other factors such as offset-header
packet format be generalized?

As in over here, will the common-hci-module work fine, If say ti-radio
says he is expecting packets starting from id-8 which is of length 10?
and also work for st-e-radio which would say expecting data from id-6
with 15 max bytes?
Answering this I suppose would solve a lot of our problems....

Marcel did previously suggest a bus-driver sort of a structure, where
the transport are sort of adapter drivers, the data interpretation
logic like splitting HCI-events, FM CH-8 packets all fall into the
algos drivers and the actual Bluetooth driver or GPS or FM-V4L2
drivers falling into the client/protocol drivers....

> Any objections to this?
>
> If you agree, I think we should discuss the alternatives for the common
> module. I think you are saying we should make the common module a single
> ldisc doing the multiplexing and have all transports register as ttys int=
o
> it, right?
>
> What we discussed before was to split it into multiple modules, one for
> the tty ldisc and one for the common code, and then have the spi/i2c/...
> drivers feed into the common multiplexer without going through tty.
>
> I don't currently see a significant advantage or disadvantage with either
> approach.
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html
>

2010-12-06 23:07:19

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:
>
> >> But I was trying to make a different point here. On a basic level,
> >> there's this cg2000 chip from STE that does BT, FM and GPS. There's
> >> the chip from TI that does BT, FM and GPS, and there's the Broadcom
> >> chip that does BT+FM. They all use HCI to access the other functions
> >> of the combo chip and they do it in a really simiar way, with the
> >> differences mostly in power management techniques. So I think it's
> >> quite sensible to have some kind of framework that is suitable for
> >> such devices.
> >
> > Yes, I agree 100% in principle. I could not find the code that
> > Broadcom/TI FM and GPS stuff so far, can you point us to that?
>
> Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.
>
> Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
> sure about the mainline support for those.

Ah, it had not occured to me to look in drivers/misc. Looking at the
code over there, it becomes much clearer what your point is. Evidently
the ti-st driver implements a line discipline similar to, but conflicting
with hci_ldisc, and it has its own copy of the hci_ll code.

I guess this is exactly what we're trying to avoid having with the
cg2900 code ;-).

> > The cg2900 solution for this was to use MFD (plus another layer
> > in the posted version, but that will go away I assume). Using
> > MFD is not the only possibility here, but I could not see anything
> > wrong with it either. Do you think we can move them all over to
> > use MFD infrastructure?
>
> I guess so but it's probably more of a detail than what I'm up to now :)

Agreed.

> >> But generally speaking, isn't a line discipline/driver attached to a
> >> tty? We can use dumb tty for e. g. SPI and still be able to use
> >> hci_ll, right?
> >
> > I suggested that as well, but the point was made that this would
> > add an unnecessary indirection for the SPI case, which is not
> > really much like a serial port. It's certainly possible to do it
> > like you say, but if we add a way to register the high-level
> > protocols with an HCI-like multi-function device, we could
> > also do it in a way that does not rely on tty-ldisc but keeps it
> > as one of the options.
>
> I actually don't think it's such a big indirection, I prefer to think
> of it more as of the abstraction layer. If not use this, are we going
> to have direct SPI device drivers? I'm afraid we might end up with a
> huge duplication of code in that case.

The question is how it should be modeled in a better way than today.

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

Any objections to this?

If you agree, I think we should discuss the alternatives for the common
module. I think you are saying we should make the common module a single
ldisc doing the multiplexing and have all transports register as ttys into
it, right?

What we discussed before was to split it into multiple modules, one for
the tty ldisc and one for the common code, and then have the spi/i2c/...
drivers feed into the common multiplexer without going through tty.

I don't currently see a significant advantage or disadvantage with either
approach.

Arnd

2010-12-06 21:24:34

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:

>> But I was trying to make a different point here. On a basic level,
>> there's this cg2000 chip from STE that does BT, FM and GPS. There's
>> the chip from TI that does BT, FM and GPS, and there's the Broadcom
>> chip that does BT+FM. They all use HCI to access the other functions
>> of the combo chip and they do it in a really simiar way, with the
>> differences mostly in power management techniques. So I think it's
>> quite sensible to have some kind of framework that is suitable for
>> such devices.
>
> Yes, I agree 100% in principle. I could not find the code that
> Broadcom/TI FM and GPS stuff so far, can you point us to that?

Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st.

Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not
sure about the mainline support for those.

> The cg2900 solution for this was to use MFD (plus another layer
> in the posted version, but that will go away I assume). Using
> MFD is not the only possibility here, but I could not see anything
> wrong with it either. Do you think we can move them all over to
> use MFD infrastructure?

I guess so but it's probably more of a detail than what I'm up to now :)

>> But generally speaking, isn't a line discipline/driver attached to a
>> tty? We can use dumb tty for e. g. SPI and still be able to use
>> hci_ll, right?
>
> I suggested that as well, but the point was made that this would
> add an unnecessary indirection for the SPI case, which is not
> really much like a serial port. It's certainly possible to do it
> like you say, but if we add a way to register the high-level
> protocols with an HCI-like multi-function device, we could
> also do it in a way that does not rely on tty-ldisc but keeps it
> as one of the options.

I actually don't think it's such a big indirection, I prefer to think
of it more as of the abstraction layer. If not use this, are we going
to have direct SPI device drivers? I'm afraid we might end up with a
huge duplication of code in that case.

Thanks,
Vitaly

2010-12-06 16:54:44

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 4:15 PM, Arnd Bergmann <[email protected]> wrote:
> > Yes, that makes sense. I'm probably missing something here, but
> > it seems to me that hci_ll is only about the power management stuff
> > on TI (and broadcom, as you say) chips, and the multi-protocol
> > support is currently handled in hci_ldisc by allowing multiple
> > protocols like h4 and ll to be registered. It that correct?
>
> Yeah, it's basic for TI and it's supported by Broadcom although they
> have their own protocol for power saving.
>
> But I was trying to make a different point here. On a basic level,
> there's this cg2000 chip from STE that does BT, FM and GPS. There's
> the chip from TI that does BT, FM and GPS, and there's the Broadcom
> chip that does BT+FM. They all use HCI to access the other functions
> of the combo chip and they do it in a really simiar way, with the
> differences mostly in power management techniques. So I think it's
> quite sensible to have some kind of framework that is suitable for
> such devices.

Yes, I agree 100% in principle. I could not find the code that
Broadcom/TI FM and GPS stuff so far, can you point us to that?

The cg2900 solution for this was to use MFD (plus another layer
in the posted version, but that will go away I assume). Using
MFD is not the only possibility here, but I could not see anything
wrong with it either. Do you think we can move them all over to
use MFD infrastructure?

> > One aspect that Par-Gunnar mentioned was that the multi-protocol
> > stuff for cg2900 and I suspect other similar devices would also
> > need to work with non-UART-based HCIs, which don't use hci_uart_proto
> > but would need something similar. Also, hci_uart is currently not
> > modular, e.g. you cannot build hci_ll as a loadable module
> > as you'd need for dynamic registration.
>
> But generally speaking, isn't a line discipline/driver attached to a
> tty? We can use dumb tty for e. g. SPI and still be able to use
> hci_ll, right?

I suggested that as well, but the point was made that this would
add an unnecessary indirection for the SPI case, which is not
really much like a serial port. It's certainly possible to do it
like you say, but if we add a way to register the high-level
protocols with an HCI-like multi-function device, we could
also do it in a way that does not rely on tty-ldisc but keeps it
as one of the options.

Arnd

2010-12-06 15:28:57

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Mon, Dec 6, 2010 at 4:15 PM, Arnd Bergmann <[email protected]> wrote:
> Yes, that makes sense. I'm probably missing something here, but
> it seems to me that hci_ll is only about the power management stuff
> on TI (and broadcom, as you say) chips, and the multi-protocol
> support is currently handled in hci_ldisc by allowing multiple
> protocols like h4 and ll to be registered. It that correct?

Yeah, it's basic for TI and it's supported by Broadcom although they
have their own protocol for power saving.

But I was trying to make a different point here. On a basic level,
there's this cg2000 chip from STE that does BT, FM and GPS. There's
the chip from TI that does BT, FM and GPS, and there's the Broadcom
chip that does BT+FM. They all use HCI to access the other functions
of the combo chip and they do it in a really simiar way, with the
differences mostly in power management techniques. So I think it's
quite sensible to have some kind of framework that is suitable for
such devices.

> One aspect that Par-Gunnar mentioned was that the multi-protocol
> stuff for cg2900 and I suspect other similar devices would also
> need to work with non-UART-based HCIs, which don't use hci_uart_proto
> but would need something similar. Also, hci_uart is currently not
> modular, e.g. you cannot build hci_ll as a loadable module
> as you'd need for dynamic registration.

But generally speaking, isn't a line discipline/driver attached to a
tty? We can use dumb tty for e. g. SPI and still be able to use
hci_ll, right?

Thanks,
Vitaly

2010-12-06 15:15:27

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 3:06 PM, Arnd Bergmann <[email protected]> wrote:
>
> > hci_ll is not the line discipline but the a TI specific uart protocol.
> > The line discipline driver (hci_ldisc.c) is shared across all protocols
> > (h4, ll, ...) and gets extended slightly so it can deal with cg2900
> > in addition to the existing HCIs. The rest of the cg2900 support is
> > about adding more hci_uart_protos.
>
> I was referring to hci_ll as to the line discipline driver, not the
> line discipline itself.
>
> The thing is, there are different Bluetooth combo chips which use HCI
> to encapsulate commands to the sub-chips behind, and there's also the
> implementation for TI chip doing that which is in the mainline. So
> instead of keeping reinventing the wheel, I think it's fairly
> beneficial for everything if we finally agree on something that works
> for all the combo chips of the type.

Yes, that makes sense. I'm probably missing something here, but
it seems to me that hci_ll is only about the power management stuff
on TI (and broadcom, as you say) chips, and the multi-protocol
support is currently handled in hci_ldisc by allowing multiple
protocols like h4 and ll to be registered. It that correct?

One aspect that Par-Gunnar mentioned was that the multi-protocol
stuff for cg2900 and I suspect other similar devices would also
need to work with non-UART-based HCIs, which don't use hci_uart_proto
but would need something similar. Also, hci_uart is currently not
modular, e.g. you cannot build hci_ll as a loadable module
as you'd need for dynamic registration.

Arnd

2010-12-06 14:57:43

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Mon, Dec 6, 2010 at 3:49 PM, Arnd Bergmann <[email protected]> wrote:

> As far as I understand, the point is that it's no longer a 'solution'
> at the core, i.e. there is no replacement for hci_ldisc or any
> of these, just modules for the additional h4 protocols that don't
> have a linux implementation yet.
>
> The patch set that was originally posted here had a new framework,
> but after the comments from Alan and me, Par-Gunnar agreed to use
> the existing framework instead and extend it in a useful way.
> Please read all of the discussion we already had. You made a good
> point here, but I fear you had both Par-Gunnar and me confused
> because it was a point that already got resolved.

Yeah I've read through that thread but the only conclusion I'd been
able to make was that you agreed on not introducing yet another line
discipline. I'm not sure if I could make a conclusion that Par gave up
on his attempt to make his patchset generic/extensible/reusable for
other chips of similar type. But if that's really the case, I see no
point in this patchset at all, as opposed to generalizing the TI
shared transport thingie and using that one.

Thanks,
Vitaly

2010-12-06 14:54:39

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Arnd,

On Mon, Dec 6, 2010 at 3:06 PM, Arnd Bergmann <[email protected]> wrote:

> hci_ll is not the line discipline but the a TI specific uart protocol.
> The line discipline driver (hci_ldisc.c) is shared across all protocols
> (h4, ll, ...) and gets extended slightly so it can deal with cg2900
> in addition to the existing HCIs. The rest of the cg2900 support is
> about adding more hci_uart_protos.

I was referring to hci_ll as to the line discipline driver, not the
line discipline itself.

The thing is, there are different Bluetooth combo chips which use HCI
to encapsulate commands to the sub-chips behind, and there's also the
implementation for TI chip doing that which is in the mainline. So
instead of keeping reinventing the wheel, I think it's fairly
beneficial for everything if we finally agree on something that works
for all the combo chips of the type.

Thanks,
Vitaly

2010-12-06 14:49:01

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:
> > As a quick answer to your question: that would depend on the
> > difference between the different controllers, I guess. But CG2900
> > doesn't support the LL protocol so it is not an issue for that.
>
> Right, but if you are only aiming cg2000, why would you create a
> framework for that? I initially thought your solution was generic
> enough to handle other "many-in-one" Bluetooth chips but I'm
> completely unsure about that now.

As far as I understand, the point is that it's no longer a 'solution'
at the core, i.e. there is no replacement for hci_ldisc or any
of these, just modules for the additional h4 protocols that don't
have a linux implementation yet.

The patch set that was originally posted here had a new framework,
but after the comments from Alan and me, Par-Gunnar agreed to use
the existing framework instead and extend it in a useful way.
Please read all of the discussion we already had. You made a good
point here, but I fear you had both Par-Gunnar and me confused
because it was a point that already got resolved.

Arnd

2010-12-06 14:06:04

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:

> On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
> <[email protected]> wrote:

> >> Isn't it better to have a new line discipline that the standard
> >> drivers (H4, LL, ...) will be applicable to?
>
> > I'm not certain what you mean. We are modifying the line discipline a
> > bit, but the only thing we have needed to do with existing protocol
> > drivers has been to add a boolean parameter for the protocol settings
> > (so they will register to the Bluetooth stack). I don't see why we
> > should create a new line discipline driver and then move the existing
> > protocol drivers to this.
>
> Okay, let me try to be more specific. There's for instance
> drivers/bluetooth/hci_ll.c which is the line discipline driver that is
> meant to work with any BT chip supporting LL protocol. Your solution
> seems to imply that one will have to create a variation of this
> implementation for each BT chip supporting LL that will use your
> shared transport implementation. Is it really the case?

hci_ll is not the line discipline but the a TI specific uart protocol.
The line discipline driver (hci_ldisc.c) is shared across all protocols
(h4, ll, ...) and gets extended slightly so it can deal with cg2900
in addition to the existing HCIs. The rest of the cg2900 support is
about adding more hci_uart_protos.

Arnd

2010-12-06 12:25:42

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Par,

> Well, from what I can see LL is an extension of the H4, basically it
> adds sleep mode handling in a vendor specific way to the normal H4
> protocol. So it is also based on the hci_h4 just as our file is. But
> our file has basically nothing in common with what has been for the LL
> file. We don't support any of the LL sleep commands for example.
> So if I would make a driver for a combo chip supporting LL, I would
> either modify the existing hci_ll.c or I would make a new file based
> on hci_ll.c. There is not much you could really reuse from our new
> file. Basically it would be the handling of any common channels, so if
> you would for example have the same specification of FM and GPS you
> could maybe save around 20 lines of common code, but you would
> probably have to add a lot of more code just to keep the solution
> generic.

Right, but this gives me the hard time seeing how your implementation
is applicable to other multi-functional chips with similar
functionality.

> One major difference is also that hci_ll never changes baud rate or
> other settings. I assume that is done from hciattach during startup
> instead. But we cannot run with that since we have to shut down the
> chip when no user is connected in order to save power. This means that
> we have to add vendor specific commands in order to for example set
> baud rate. And then you run into these vendor specific problems. If
> there would be a standardized specification on how to set baud rate
> and how to put chip in sleep I assume things could be solved
> differently, but that is not the case.

Again, there are at least TI and Broadcom chips that support HCI_LL
and if they were to use your implementation of the core, they would
have had to add 2 more implementations of the corresponding line
discipline driver.

> As a quick answer to your question: that would depend on the
> difference between the different controllers, I guess. But CG2900
> doesn't support the LL protocol so it is not an issue for that.

Right, but if you are only aiming cg2000, why would you create a
framework for that? I initially thought your solution was generic
enough to handle other "many-in-one" Bluetooth chips but I'm
completely unsure about that now.

Thanks,
Vitaly

2010-12-06 12:01:46

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Vitaly,

2010/12/6 Vitaly Wool <[email protected]>:
> Hi Par,
>
> On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
> <[email protected]> wrote:
>
>>> So is it true that if there's the other chip with similar
>>> functionality I have to implement support for, I'll have to pretty
>>> much duplicate your line discipline driver?
>>>
>>
>> What will be in the protocol file, i.e. the same level as hci_h4.c,
>> hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
>> that are in a standardized specification are the Bluetooth channels
>> 1-4. Other channels such as FM and GPS are vendor specific. I've seen
>> that for example TI has the same channels for FM and GPS, but these
>> channels, 8 and 9, are in no way standardized. The CG2900 also carries
>> a number of other channels, such as debug and device channels, which
>> I'm pretty sure is individual for this chip.
>> This identification of the channels is also only one function, even
>> though it is an important function. There are also a lot of other code
>> regarding baud rate settings, low power handling, etc that I'm
>> positive will not apply to chips of other vendors. It is of course
>> possible that other chips from ST-Ericsson, current and future, will
>> be able to reuse this file, but as I said I doubt that other vendors
>> might be able to use it.
>
> Wait. What level of abstraction is this file at if even some future
> STE chips are not guaranteed to work with it? Is it at a level of
> hardcoding the GPIO numbers?
>

There are no GPIOs or similar in the driver code; those are defined
and used in the board specific files under the /arch/ files. What
could be a problem with a future chip would be if for example the HCI
command to change baud rate would change. We have no indication that
this will change, but I cannot give a lifetime guarantee that nothing
will ever change with how the chip startup is handled.

>>> Isn't it better to have a new line discipline that the standard
>>> drivers (H4, LL, ...) will be applicable to?
>
>> I'm not certain what you mean. We are modifying the line discipline a
>> bit, but the only thing we have needed to do with existing protocol
>> drivers has been to add a boolean parameter for the protocol settings
>> (so they will register to the Bluetooth stack). I don't see why we
>> should create a new line discipline driver and then move the existing
>> protocol drivers to this.
>
> Okay, let me try to be more specific. There's for instance
> drivers/bluetooth/hci_ll.c which is the line discipline driver that is
> meant to work with any BT chip supporting LL protocol. Your solution
> seems to imply that one will have to create a variation of this
> implementation for each BT chip supporting LL that will use your
> shared transport implementation. Is it really the case?
>
> Thanks,
> =A0 Vitaly
>

Well, from what I can see LL is an extension of the H4, basically it
adds sleep mode handling in a vendor specific way to the normal H4
protocol. So it is also based on the hci_h4 just as our file is. But
our file has basically nothing in common with what has been for the LL
file. We don't support any of the LL sleep commands for example.
So if I would make a driver for a combo chip supporting LL, I would
either modify the existing hci_ll.c or I would make a new file based
on hci_ll.c. There is not much you could really reuse from our new
file. Basically it would be the handling of any common channels, so if
you would for example have the same specification of FM and GPS you
could maybe save around 20 lines of common code, but you would
probably have to add a lot of more code just to keep the solution
generic.
One major difference is also that hci_ll never changes baud rate or
other settings. I assume that is done from hciattach during startup
instead. But we cannot run with that since we have to shut down the
chip when no user is connected in order to save power. This means that
we have to add vendor specific commands in order to for example set
baud rate. And then you run into these vendor specific problems. If
there would be a standardized specification on how to set baud rate
and how to put chip in sleep I assume things could be solved
differently, but that is not the case.

As a quick answer to your question: that would depend on the
difference between the different controllers, I guess. But CG2900
doesn't support the LL protocol so it is not an issue for that.

/P-G

2010-12-06 09:46:43

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Par,

On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
<[email protected]> wrote:

>> So is it true that if there's the other chip with similar
>> functionality I have to implement support for, I'll have to pretty
>> much duplicate your line discipline driver?
>>
>
> What will be in the protocol file, i.e. the same level as hci_h4.c,
> hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
> that are in a standardized specification are the Bluetooth channels
> 1-4. Other channels such as FM and GPS are vendor specific. I've seen
> that for example TI has the same channels for FM and GPS, but these
> channels, 8 and 9, are in no way standardized. The CG2900 also carries
> a number of other channels, such as debug and device channels, which
> I'm pretty sure is individual for this chip.
> This identification of the channels is also only one function, even
> though it is an important function. There are also a lot of other code
> regarding baud rate settings, low power handling, etc that I'm
> positive will not apply to chips of other vendors. It is of course
> possible that other chips from ST-Ericsson, current and future, will
> be able to reuse this file, but as I said I doubt that other vendors
> might be able to use it.

Wait. What level of abstraction is this file at if even some future
STE chips are not guaranteed to work with it? Is it at a level of
hardcoding the GPIO numbers?

>> Isn't it better to have a new line discipline that the standard
>> drivers (H4, LL, ...) will be applicable to?

> I'm not certain what you mean. We are modifying the line discipline a
> bit, but the only thing we have needed to do with existing protocol
> drivers has been to add a boolean parameter for the protocol settings
> (so they will register to the Bluetooth stack). I don't see why we
> should create a new line discipline driver and then move the existing
> protocol drivers to this.

Okay, let me try to be more specific. There's for instance
drivers/bluetooth/hci_ll.c which is the line discipline driver that is
meant to work with any BT chip supporting LL protocol. Your solution
seems to imply that one will have to create a variation of this
implementation for each BT chip supporting LL that will use your
shared transport implementation. Is it really the case?

Thanks,
Vitaly

2010-12-06 09:06:02

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Vitaly,

2010/12/3 Vitaly Wool <[email protected]>:
> Hi Par,
>
> On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
> <[email protected]> wrote:
>> 2010/10/29 Alan Cox <[email protected]>:
>
>> By the way, I will soon release a new patch set for CG2900 (hopefully
>> next week), which contains major rework. It's hard to explain
>> everything here and now but changes include:
>> =A0- Reuse of existing HCI line discipline under /drivers/bluetooth/.
>> Line discipline has been modified so it is selectable from protocol if
>> line discipline should register to Bluetooth stack or not.
>
> So is it true that if there's the other chip with similar
> functionality I have to implement support for, I'll have to pretty
> much duplicate your line discipline driver?
>

What will be in the protocol file, i.e. the same level as hci_h4.c,
hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels
that are in a standardized specification are the Bluetooth channels
1-4. Other channels such as FM and GPS are vendor specific. I've seen
that for example TI has the same channels for FM and GPS, but these
channels, 8 and 9, are in no way standardized. The CG2900 also carries
a number of other channels, such as debug and device channels, which
I'm pretty sure is individual for this chip.
This identification of the channels is also only one function, even
though it is an important function. There are also a lot of other code
regarding baud rate settings, low power handling, etc that I'm
positive will not apply to chips of other vendors. It is of course
possible that other chips from ST-Ericsson, current and future, will
be able to reuse this file, but as I said I doubt that other vendors
might be able to use it.

> Isn't it better to have a new line discipline that the standard
> drivers (H4, LL, ...) will be applicable to?
>
> ~Vitaly
>

I'm not certain what you mean. We are modifying the line discipline a
bit, but the only thing we have needed to do with existing protocol
drivers has been to add a boolean parameter for the protocol settings
(so they will register to the Bluetooth stack). I don't see why we
should create a new line discipline driver and then move the existing
protocol drivers to this.

/P-G

2010-12-03 11:42:49

[permalink] [raw]

Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Par,

On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
<[email protected]> wrote:
> 2010/10/29 Alan Cox <[email protected]>:

> By the way, I will soon release a new patch set for CG2900 (hopefully
> next week), which contains major rework. It's hard to explain
> everything here and now but changes include:
> =A0- Reuse of existing HCI line discipline under /drivers/bluetooth/.
> Line discipline has been modified so it is selectable from protocol if
> line discipline should register to Bluetooth stack or not.

So is it true that if there's the other chip with similar
functionality I have to implement support for, I'll have to pretty
much duplicate your line discipline driver?

Isn't it better to have a new line discipline that the standard
drivers (H4, LL, ...) will be applicable to?

~Vitaly

2010-12-03 09:16:25