Re: bluetooth: Add hci_h4p driver

[Pavel Machek]

Hi!

> And you want to set the QUIRK_INVALID_BADDR. At least you want to do that in the final submission.
> 

Ok, I found out that I can do it and result works, provided that I do
hciconfig hci0 up/down first.

I have trouble understanding... h4p_hci_open() seems to be called as
soon as I insmod the driver. Who does that? Is it some kind of udev
magic?

...
> > +#include "hci_h4p.h"
> > +
> > +#define BT_DBG(a...) do {} while(0)
> 
> Why is this needed? BT_DBG is hooked up with dynamic debug.
...

I did all the changes as requested. Thanks. I did't see harm in
include guards, but I removed them; it is not important.

> > +	if (info->rx_count == 0) {
> > +		/* H4+ devices should always send word aligned packets */
> > +		if (!(info->rx_skb->len % 2))
> > +			info->garbage_bytes++;
> > +		h4p_recv_frame(info, info->rx_skb);
> 
> The h4p_recv_frame should maybe done inline here since it only handles 2 exception packets. Also to make it easy, just leave the function if (info->rx_count).
> 
> This packet processing feels like a bit of spaghetti code. I think that is better handled in a proper function that goes through it and not with many tiny sub functions.
>

Well, CodingStyle says something about functions that should be kept
short... And when manually inlined, the inner function would have to
be made less readable, as it uses return to shortcut processing. 


To use __hci_cmd_sync()

> > +
> > +	SET_HCIDEV_DEV(hdev, info->dev);
> > +
> > +	if (hci_register_dev(hdev) >= 0)
> > +		return 0;
> > +
> > +	dev_err(info->dev, "hci_register failed %s.\n", hdev->name);
> > +	hci_free_dev(info->hdev);
> > +	return -ENODEV;
> > +}
> 
> And normally this is folded into the probe handling and not in a
> separate function.

The probe function is too long, already...

> > +#include <linux/delay.h>
> > +#include <linux/clk.h>
> > +
> > +#include <linux/io.h>
> > +
> > +#include "hci_h4p.h"
> 
> Why is this a separate file? And if so, why not all UART specific details are in this file. Including bunch of the defines you have in the header.
> 

Well, you wanted less global functions, so I moved some as inlines to
headers. I guess I can merge nokia_core and nokia_uart if really wanted.

I did rest of requested changes.

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/