Message-ID: <AANLkTimpESWXt9i1R54fm7=Fmpd95bw6HAP1s1x1q5=n@mail.gmail.com>
Date:	Thu, 28 Oct 2010 12:37:20 +0200
From:	Par-Gunnar Hjalmdahl <pghatwork@...il.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	linus.walleij@...ricsson.com, linux-bluetooth@...r.kernel.org,
	linux-kernel@...r.kernel.org, Lukasz.Rymanowski@...to.com
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 <alan@...rguk.ukuu.org.uk>:
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists