[PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Miguel Oliveira]

Creating this patch for the Eudyptula Challenge.
Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
Also inserted a blank line after adeclaration.

Signed-off-by: Miguel Oliveira <cmroliv@gmail.com>
---
 drivers/staging/nokia_h4p/nokia_core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/nokia_h4p/nokia_core.c b/drivers/staging/nokia_h4p/nokia_core.c
index 5e19cd6..ba0506d 100644
--- a/drivers/staging/nokia_h4p/nokia_core.c
+++ b/drivers/staging/nokia_h4p/nokia_core.c
@@ -724,7 +724,7 @@ static int hci_h4p_reset(struct hci_h4p_info *info)

	gpio_set_value(info->reset_gpio, 0);
	gpio_set_value(info->bt_wakeup_gpio, 1);
-	msleep(10);
+	usleep_range(10000, 15000);

	if (gpio_get_value(info->host_wakeup_gpio) == 1) {
		dev_err(info->dev, "host_wakeup_gpio not low\n");
@@ -756,6 +756,7 @@ static int hci_h4p_reset(struct hci_h4p_info *info)
static int hci_h4p_hci_flush(struct hci_dev *hdev)
{
	struct hci_h4p_info *info = hci_get_drvdata(hdev);
+
	skb_queue_purge(&info->txq);

	return 0;
--
1.7.10.4

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Pavel Machek]

On Fri 2014-05-30 11:27:13, Miguel Oliveira wrote:
> Creating this patch for the Eudyptula Challenge.
> Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
> Also inserted a blank line after adeclaration.

So you are changing timings without testing. Plus, burning CPU power
instead of sleeping.

Seems you'll need another patch for the challenge :-).
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Marcel Holtmann]

Hi Pavel,

>> Creating this patch for the Eudyptula Challenge.
>> Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
>> Also inserted a blank line after adeclaration.
> 
> So you are changing timings without testing. Plus, burning CPU power
> instead of sleeping.
> 
> Seems you'll need another patch for the challenge :-).

I actually wonder if anybody is seriously working on this driver. My concern with the staging drivers has always been that we are quick with merging them when the work on getting them into upstream shape is actually hard. However reality is once they are in staging nobody is doing the work to clean them up and fix the issues.

This one seems no exception. Merged in January and 5 month later none of the TODO items have been addressed. All the promises that we fix the issues in the staging tree went out the window.

Greg, instead of wasting our time with this, can we just remove this driver from staging.

Regards

Marcel

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Pavel Machek]

Hi!

> >> Creating this patch for the Eudyptula Challenge.
> >> Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
> >> Also inserted a blank line after adeclaration.
> > 
> > So you are changing timings without testing. Plus, burning CPU power
> > instead of sleeping.
> > 
> > Seems you'll need another patch for the challenge :-).
> 
> I actually wonder if anybody is seriously working on this driver. My concern with the staging drivers has always been that we are quick with merging them when the work on getting them into upstream shape is actually hard. However reality is once they are in staging nobody is doing the work to clean them up and fix the issues.
>

There is active work on merging n900 changes. And no, it does not
progress as quickly as I'd like, but we'll get there. It is also
requirement for n900 FM radio receiving...

> Greg, instead of wasting our time with this, can we just remove this driver from staging.

Please don't.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Greg KH]

On Fri, May 30, 2014 at 02:30:23PM +0200, Pavel Machek wrote:
> Hi!
> 
> > >> Creating this patch for the Eudyptula Challenge.
> > >> Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
> > >> Also inserted a blank line after adeclaration.
> > > 
> > > So you are changing timings without testing. Plus, burning CPU power
> > > instead of sleeping.
> > > 
> > > Seems you'll need another patch for the challenge :-).
> > 
> > I actually wonder if anybody is seriously working on this driver. My concern with the staging drivers has always been that we are quick with merging them when the work on getting them into upstream shape is actually hard. However reality is once they are in staging nobody is doing the work to clean them up and fix the issues.
> >
> 
> There is active work on merging n900 changes.

Really?  Where?

> And no, it does not progress as quickly as I'd like, but we'll get
> there. It is also requirement for n900 FM radio receiving...
> 
> > Greg, instead of wasting our time with this, can we just remove this driver from staging.
> 
> Please don't.

As there has not been any real work on it since it has been merged, I
don't see why I shouldn't remove it.  If you do get some work done on
it, you can always revert the removal and continue on.  But the
existance of code in staging that is not progressing forward at all is
something that I don't like at all, and will be doing a large sweep of
soon to remove.

thanks,

greg k-h

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2506 bytes --]

Hi Greg,

On Friday 30 May 2014 17:59:59 Greg KH wrote:
> On Fri, May 30, 2014 at 02:30:23PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > >> Creating this patch for the Eudyptula Challenge.
> > > >> Replaced msleep() for a delay < 20ms with a
> > > >> usleep_range() between 10000us and 15000us. Also
> > > >> inserted a blank line after adeclaration.
> > > > 
> > > > So you are changing timings without testing. Plus,
> > > > burning CPU power instead of sleeping.
> > > > 
> > > > Seems you'll need another patch for the challenge :-).
> > > 
> > > I actually wonder if anybody is seriously working on this
> > > driver. My concern with the staging drivers has always
> > > been that we are quick with merging them when the work on
> > > getting them into upstream shape is actually hard.
> > > However reality is once they are in staging nobody is
> > > doing the work to clean them up and fix the issues.
> > 
> > There is active work on merging n900 changes.
> 
> Really?  Where?
> 

You can look at elinux wiki where is table how process is going:
http://elinux.org/N900

Also look at planed future list and its progress:
http://elinux.org/N900/Changelog

You can see that drivers are including step by step.

> > And no, it does not progress as quickly as I'd like, but
> > we'll get there. It is also requirement for n900 FM radio
> > receiving...
> > 
> > > Greg, instead of wasting our time with this, can we just
> > > remove this driver from staging.
> > 
> > Please don't.
> 
> As there has not been any real work on it since it has been
> merged, I don't see why I shouldn't remove it.  If you do get
> some work done on it, you can always revert the removal and
> continue on.  But the existance of code in staging that is
> not progressing forward at all is something that I don't like
> at all, and will be doing a large sweep of soon to remove.
> 
> thanks,
> 
> greg k-h

Just look how much time took to include my patch for radio-
bcm2048 (fm radio part of this chip) which fixing wrong overflow 
check. I sent it at the end of December and... yes it is still 
not included in linus tree. Now it is somethere in media tree and 
probably will be pulled in next merge window.

This means that it takes about half of year to include patches 
for these drivers.

So why now you want to very quickly remove this driver from 
staging but you did not wanted to take my above patch?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Greg KH]

On Fri, May 30, 2014 at 06:15:09PM +0200, Pali Rohár wrote:
> Hi Greg,
> 
> On Friday 30 May 2014 17:59:59 Greg KH wrote:
> > On Fri, May 30, 2014 at 02:30:23PM +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > >> Creating this patch for the Eudyptula Challenge.
> > > > >> Replaced msleep() for a delay < 20ms with a
> > > > >> usleep_range() between 10000us and 15000us. Also
> > > > >> inserted a blank line after adeclaration.
> > > > > 
> > > > > So you are changing timings without testing. Plus,
> > > > > burning CPU power instead of sleeping.
> > > > > 
> > > > > Seems you'll need another patch for the challenge :-).
> > > > 
> > > > I actually wonder if anybody is seriously working on this
> > > > driver. My concern with the staging drivers has always
> > > > been that we are quick with merging them when the work on
> > > > getting them into upstream shape is actually hard.
> > > > However reality is once they are in staging nobody is
> > > > doing the work to clean them up and fix the issues.
> > > 
> > > There is active work on merging n900 changes.
> > 
> > Really?  Where?
> > 
> 
> You can look at elinux wiki where is table how process is going:
> http://elinux.org/N900
> 
> Also look at planed future list and its progress:
> http://elinux.org/N900/Changelog
> 
> You can see that drivers are including step by step.

I'm not going to dig through random web pages, sorry.  If patches aren't
sent to me for a driver, I consider it dead, that's only fair for me
given my workload, don't you think?

> > > And no, it does not progress as quickly as I'd like, but
> > > we'll get there. It is also requirement for n900 FM radio
> > > receiving...
> > > 
> > > > Greg, instead of wasting our time with this, can we just
> > > > remove this driver from staging.
> > > 
> > > Please don't.
> > 
> > As there has not been any real work on it since it has been
> > merged, I don't see why I shouldn't remove it.  If you do get
> > some work done on it, you can always revert the removal and
> > continue on.  But the existance of code in staging that is
> > not progressing forward at all is something that I don't like
> > at all, and will be doing a large sweep of soon to remove.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Just look how much time took to include my patch for radio-
> bcm2048 (fm radio part of this chip) which fixing wrong overflow 
> check. I sent it at the end of December and... yes it is still 
> not included in linus tree. Now it is somethere in media tree and 
> probably will be pulled in next merge window.
> 
> This means that it takes about half of year to include patches 
> for these drivers.

Just because the media drivers take a long time to get fixes merged,
don't use that as an excuse to not fix up the staging drivers.

In fact, it sounds like you have lots of time while that patch is
getting merged, why not work on fixing up the staging driver?  :)

> So why now you want to very quickly remove this driver from 
> staging but you did not wanted to take my above patch?

I'm not the maintainer of drivers/staging/media/ and have no insight
into what goes on there at all, nor do I care.  I'm not the one
responsible for taking your patch, so I did not "want" to take your
patch at all.

thanks,

greg k-h

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Marcel Holtmann]

Hi Greg,

>>>>>>> Creating this patch for the Eudyptula Challenge.
>>>>>>> Replaced msleep() for a delay < 20ms with a
>>>>>>> usleep_range() between 10000us and 15000us. Also
>>>>>>> inserted a blank line after adeclaration.
>>>>>> 
>>>>>> So you are changing timings without testing. Plus,
>>>>>> burning CPU power instead of sleeping.
>>>>>> 
>>>>>> Seems you'll need another patch for the challenge :-).
>>>>> 
>>>>> I actually wonder if anybody is seriously working on this
>>>>> driver. My concern with the staging drivers has always
>>>>> been that we are quick with merging them when the work on
>>>>> getting them into upstream shape is actually hard.
>>>>> However reality is once they are in staging nobody is
>>>>> doing the work to clean them up and fix the issues.
>>>> 
>>>> There is active work on merging n900 changes.
>>> 
>>> Really?  Where?
>>> 
>> 
>> You can look at elinux wiki where is table how process is going:
>> http://elinux.org/N900
>> 
>> Also look at planed future list and its progress:
>> http://elinux.org/N900/Changelog
>> 
>> You can see that drivers are including step by step.
> 
> I'm not going to dig through random web pages, sorry.  If patches aren't
> sent to me for a driver, I consider it dead, that's only fair for me
> given my workload, don't you think?
> 
>>>> And no, it does not progress as quickly as I'd like, but
>>>> we'll get there. It is also requirement for n900 FM radio
>>>> receiving...
>>>> 
>>>>> Greg, instead of wasting our time with this, can we just
>>>>> remove this driver from staging.
>>>> 
>>>> Please don't.
>>> 
>>> As there has not been any real work on it since it has been
>>> merged, I don't see why I shouldn't remove it.  If you do get
>>> some work done on it, you can always revert the removal and
>>> continue on.  But the existance of code in staging that is
>>> not progressing forward at all is something that I don't like
>>> at all, and will be doing a large sweep of soon to remove.
>>> 
>>> thanks,
>>> 
>>> greg k-h
>> 
>> Just look how much time took to include my patch for radio-
>> bcm2048 (fm radio part of this chip) which fixing wrong overflow 
>> check. I sent it at the end of December and... yes it is still 
>> not included in linus tree. Now it is somethere in media tree and 
>> probably will be pulled in next merge window.
>> 
>> This means that it takes about half of year to include patches 
>> for these drivers.
> 
> Just because the media drivers take a long time to get fixes merged,
> don't use that as an excuse to not fix up the staging drivers.
> 
> In fact, it sounds like you have lots of time while that patch is
> getting merged, why not work on fixing up the staging driver?  :)

and 2 month later, we still have nothing accomplished with this driver.

So this driver is 6 month in staging and not a single commit to address anything in the TODO file. I only see pointless coding style change.

There is a reason why I dislike staging drivers. And this is exactly it. It gets treated as dumping ground. So can we please remove this driver now. Maybe someone finally wakes up and does their promised job.

Regards

Marcel

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Pavel Machek]

On Wed 2014-08-06 13:56:33, Marcel Holtmann wrote:
> Hi Greg,
> 
> >>>>>>> Creating this patch for the Eudyptula Challenge.
> >>>>>>> Replaced msleep() for a delay < 20ms with a
> >>>>>>> usleep_range() between 10000us and 15000us. Also
> >>>>>>> inserted a blank line after adeclaration.
> >>>>>> 
> >>>>>> So you are changing timings without testing. Plus,
> >>>>>> burning CPU power instead of sleeping.
> >>>>>> 
> >>>>>> Seems you'll need another patch for the challenge :-).
> >>>>> 
> >>>>> I actually wonder if anybody is seriously working on this
> >>>>> driver. My concern with the staging drivers has always
> >>>>> been that we are quick with merging them when the work on
> >>>>> getting them into upstream shape is actually hard.
> >>>>> However reality is once they are in staging nobody is
> >>>>> doing the work to clean them up and fix the issues.
> >>>> 
> >>>> There is active work on merging n900 changes.
> >>> 
> >>> Really?  Where?
> >>> 
> >> 
> >> You can look at elinux wiki where is table how process is going:
> >> http://elinux.org/N900
> >> 
> >> Also look at planed future list and its progress:
> >> http://elinux.org/N900/Changelog
> >> 
> >> You can see that drivers are including step by step.
> > 
> > I'm not going to dig through random web pages, sorry.  If patches aren't
> > sent to me for a driver, I consider it dead, that's only fair for me
> > given my workload, don't you think?
> > 
> >>>> And no, it does not progress as quickly as I'd like, but
> >>>> we'll get there. It is also requirement for n900 FM radio
> >>>> receiving...
> >>>> 
> >>>>> Greg, instead of wasting our time with this, can we just
> >>>>> remove this driver from staging.
> >>>> 
> >>>> Please don't.
> >>> 
> >>> As there has not been any real work on it since it has been
> >>> merged, I don't see why I shouldn't remove it.  If you do get
> >>> some work done on it, you can always revert the removal and
> >>> continue on.  But the existance of code in staging that is
> >>> not progressing forward at all is something that I don't like
> >>> at all, and will be doing a large sweep of soon to remove.
> >>> 
> >>> thanks,
> >>> 
> >>> greg k-h
> >> 
> >> Just look how much time took to include my patch for radio-
> >> bcm2048 (fm radio part of this chip) which fixing wrong overflow 
> >> check. I sent it at the end of December and... yes it is still 
> >> not included in linus tree. Now it is somethere in media tree and 
> >> probably will be pulled in next merge window.
> >> 
> >> This means that it takes about half of year to include patches 
> >> for these drivers.
> > 
> > Just because the media drivers take a long time to get fixes merged,
> > don't use that as an excuse to not fix up the staging drivers.
> > 
> > In fact, it sounds like you have lots of time while that patch is
> > getting merged, why not work on fixing up the staging driver?  :)
> 
> and 2 month later, we still have nothing accomplished with this driver.
> 
> So this driver is 6 month in staging and not a single commit to address anything in the TODO file. I only see pointless coding style change.
> 
> There is a reason why I dislike staging drivers. And this is exactly it. It gets treated as dumping ground. So can we please remove this driver now. Maybe someone finally wakes up and does their promised job.
>

Yeah, so I clearly make a mistake of cleaning the driver up _before_
merging it into staging.

And yes, I'm currently trying to get 3.16 working in the qemu, which
is prerequisite for any useful n900 work.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Greg KH]

On Wed, Aug 06, 2014 at 11:30:55PM +0200, Pavel Machek wrote:
> On Wed 2014-08-06 13:56:33, Marcel Holtmann wrote:
> > Hi Greg,
> > 
> > >>>>>>> Creating this patch for the Eudyptula Challenge.
> > >>>>>>> Replaced msleep() for a delay < 20ms with a
> > >>>>>>> usleep_range() between 10000us and 15000us. Also
> > >>>>>>> inserted a blank line after adeclaration.
> > >>>>>> 
> > >>>>>> So you are changing timings without testing. Plus,
> > >>>>>> burning CPU power instead of sleeping.
> > >>>>>> 
> > >>>>>> Seems you'll need another patch for the challenge :-).
> > >>>>> 
> > >>>>> I actually wonder if anybody is seriously working on this
> > >>>>> driver. My concern with the staging drivers has always
> > >>>>> been that we are quick with merging them when the work on
> > >>>>> getting them into upstream shape is actually hard.
> > >>>>> However reality is once they are in staging nobody is
> > >>>>> doing the work to clean them up and fix the issues.
> > >>>> 
> > >>>> There is active work on merging n900 changes.
> > >>> 
> > >>> Really?  Where?
> > >>> 
> > >> 
> > >> You can look at elinux wiki where is table how process is going:
> > >> http://elinux.org/N900
> > >> 
> > >> Also look at planed future list and its progress:
> > >> http://elinux.org/N900/Changelog
> > >> 
> > >> You can see that drivers are including step by step.
> > > 
> > > I'm not going to dig through random web pages, sorry.  If patches aren't
> > > sent to me for a driver, I consider it dead, that's only fair for me
> > > given my workload, don't you think?
> > > 
> > >>>> And no, it does not progress as quickly as I'd like, but
> > >>>> we'll get there. It is also requirement for n900 FM radio
> > >>>> receiving...
> > >>>> 
> > >>>>> Greg, instead of wasting our time with this, can we just
> > >>>>> remove this driver from staging.
> > >>>> 
> > >>>> Please don't.
> > >>> 
> > >>> As there has not been any real work on it since it has been
> > >>> merged, I don't see why I shouldn't remove it.  If you do get
> > >>> some work done on it, you can always revert the removal and
> > >>> continue on.  But the existance of code in staging that is
> > >>> not progressing forward at all is something that I don't like
> > >>> at all, and will be doing a large sweep of soon to remove.
> > >>> 
> > >>> thanks,
> > >>> 
> > >>> greg k-h
> > >> 
> > >> Just look how much time took to include my patch for radio-
> > >> bcm2048 (fm radio part of this chip) which fixing wrong overflow 
> > >> check. I sent it at the end of December and... yes it is still 
> > >> not included in linus tree. Now it is somethere in media tree and 
> > >> probably will be pulled in next merge window.
> > >> 
> > >> This means that it takes about half of year to include patches 
> > >> for these drivers.
> > > 
> > > Just because the media drivers take a long time to get fixes merged,
> > > don't use that as an excuse to not fix up the staging drivers.
> > > 
> > > In fact, it sounds like you have lots of time while that patch is
> > > getting merged, why not work on fixing up the staging driver?  :)
> > 
> > and 2 month later, we still have nothing accomplished with this driver.
> > 
> > So this driver is 6 month in staging and not a single commit to address anything in the TODO file. I only see pointless coding style change.
> > 
> > There is a reason why I dislike staging drivers. And this is exactly it. It gets treated as dumping ground. So can we please remove this driver now. Maybe someone finally wakes up and does their promised job.
> >
> 
> Yeah, so I clearly make a mistake of cleaning the driver up _before_
> merging it into staging.

No, the mistake was that you didn't ever follow it up with any real
work, not that it was "cleaned up" at all.

> And yes, I'm currently trying to get 3.16 working in the qemu, which
> is prerequisite for any useful n900 work.

So I guess I'll queue up a patch to delete this for 3.17-rc2.  Pavel, if
you get the device working properly, we can easily revert the deletion
and you can send follow-on patches to fix things up properly.

thanks,

greg k-h

[PATCH] staging: nokia_h4: use more consistent name for intermediate object

[Pavel Machek]

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/staging/nokia_h4p/Makefile b/drivers/staging/nokia_h4p/Makefile
index 9625db4..daffe3c 100644
--- a/drivers/staging/nokia_h4p/Makefile
+++ b/drivers/staging/nokia_h4p/Makefile
@@ -1,6 +1,6 @@
 
-obj-$(CONFIG_BT_NOKIA_H4P)		+= btnokia_h4p.o
-btnokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
+obj-$(CONFIG_BT_NOKIA_H4P)		+= hci_h4p.o
+hci_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
 		nokia_fw-bcm.o nokia_fw-ti1273.o
 
 ccflags-y += -D__CHECK_ENDIAN__
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: use more consistent name for intermediate object

[Greg KH]

On Thu, Aug 07, 2014 at 10:13:29AM +0200, Pavel Machek wrote:
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/staging/nokia_h4p/Makefile b/drivers/staging/nokia_h4p/Makefile
> index 9625db4..daffe3c 100644
> --- a/drivers/staging/nokia_h4p/Makefile
> +++ b/drivers/staging/nokia_h4p/Makefile
> @@ -1,6 +1,6 @@
>  
> -obj-$(CONFIG_BT_NOKIA_H4P)		+= btnokia_h4p.o
> -btnokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
> +obj-$(CONFIG_BT_NOKIA_H4P)		+= hci_h4p.o
> +hci_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
>  		nokia_fw-bcm.o nokia_fw-ti1273.o
>  
>  ccflags-y += -D__CHECK_ENDIAN__

You just changed the kernel module name, did you really mean to do that?

Why?

It's not an "intermediate" object here, it's a .ko file, right?

greg k-h

Re: [PATCH] staging: nokia_h4: use more consistent name for intermediate object

[Pavel Machek]

On Thu 2014-08-07 13:24:07, Greg KH wrote:
> On Thu, Aug 07, 2014 at 10:13:29AM +0200, Pavel Machek wrote:
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > diff --git a/drivers/staging/nokia_h4p/Makefile b/drivers/staging/nokia_h4p/Makefile
> > index 9625db4..daffe3c 100644
> > --- a/drivers/staging/nokia_h4p/Makefile
> > +++ b/drivers/staging/nokia_h4p/Makefile
> > @@ -1,6 +1,6 @@
> >  
> > -obj-$(CONFIG_BT_NOKIA_H4P)		+= btnokia_h4p.o
> > -btnokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
> > +obj-$(CONFIG_BT_NOKIA_H4P)		+= hci_h4p.o
> > +hci_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
> >  		nokia_fw-bcm.o nokia_fw-ti1273.o
> >  
> >  ccflags-y += -D__CHECK_ENDIAN__
> 
> You just changed the kernel module name, did you really mean to do that?
> 
> Why?
> 
> It's not an "intermediate" object here, it's a .ko file, right?

Ok, lets not do that now. btnokia_h4p is really strange name, and it
will probably need to be fixed in future, but now may not be a good
time.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 1/3] staging: nokia_h4: switch to right types and use bdaddr_t

[Pavel Machek]

Switch bluetooth address to right type and use bacmp to replace
explicit checks for zero addresses.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/staging/nokia_h4p/hci_h4p.h b/drivers/staging/nokia_h4p/hci_h4p.h
index 99c4da6..76eaaaa 100644
--- a/drivers/staging/nokia_h4p/hci_h4p.h
+++ b/drivers/staging/nokia_h4p/hci_h4p.h
@@ -77,7 +77,7 @@ struct hci_h4p_info {
 	unsigned long rx_state;
 	unsigned long garbage_bytes;
 
-	u8 bd_addr[6];
+	bdaddr_t bd_addr;
 	struct sk_buff_head *fw_q;
 
 	int pm_enabled;
diff --git a/drivers/staging/nokia_h4p/nokia_core.c b/drivers/staging/nokia_h4p/nokia_core.c
index 775e1d0..79d8b01 100644
--- a/drivers/staging/nokia_h4p/nokia_core.c
+++ b/drivers/staging/nokia_h4p/nokia_core.c
@@ -993,7 +993,7 @@ static ssize_t hci_h4p_store_bdaddr(struct device *dev,
 	for (i = 0; i < 6; i++) {
 		if (bdaddr[i] > 0xff)
 			return -EINVAL;
-		info->bd_addr[i] = bdaddr[i] & 0xff;
+		info->bd_addr.b[i] = bdaddr[i] & 0xff;
 	}
 
 	return count;
@@ -1004,7 +1004,7 @@ static ssize_t hci_h4p_show_bdaddr(struct device *dev,
 {
 	struct hci_h4p_info *info = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%pMR\n", info->bd_addr);
+	return sprintf(buf, "%pMR\n", info->bd_addr.b);
 }
 
 static DEVICE_ATTR(bdaddr, S_IRUGO | S_IWUSR, hci_h4p_show_bdaddr,
diff --git a/drivers/staging/nokia_h4p/nokia_fw-bcm.c b/drivers/staging/nokia_h4p/nokia_fw-bcm.c
index 14fe3ae..054a78b 100644
--- a/drivers/staging/nokia_h4p/nokia_fw-bcm.c
+++ b/drivers/staging/nokia_h4p/nokia_fw-bcm.c
@@ -30,17 +30,17 @@ static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info,
 {
 	int i;
 	static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
-	int not_valid = !bacmp(info->bd_addr, BDADDR_ANY);
+	int not_valid = !bacmp(&info->bd_addr, BDADDR_ANY);
 
 	if (not_valid) {
 		dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
 		/* When address is not valid, use some random but Nokia MAC */
-		memcpy(info->bd_addr, nokia_oui, 3);
-		get_random_bytes(info->bd_addr + 3, 3);
+		memcpy(info->bd_addr.b, nokia_oui, 3);
+		get_random_bytes(info->bd_addr.b + 3, 3);
 	}
 
 	for (i = 0; i < 6; i++)
-		skb->data[9 - i] = info->bd_addr[i];
+		skb->data[9 - i] = info->bd_addr.b[i];
 
 	return 0;
 }
diff --git a/drivers/staging/nokia_h4p/nokia_fw-csr.c b/drivers/staging/nokia_h4p/nokia_fw-csr.c
index 925ed86d0..a2c58c4 100644
--- a/drivers/staging/nokia_h4p/nokia_fw-csr.c
+++ b/drivers/staging/nokia_h4p/nokia_fw-csr.c
@@ -48,7 +48,7 @@ int hci_h4p_bc4_send_fw(struct hci_h4p_info *info,
 	static const u8 nokia_oui[3] = {0x00, 0x19, 0x4F};
 	struct sk_buff *skb;
 	unsigned int offset;
-	int retries, count, i, not_valid;
+	int retries, count, not_valid;
 	unsigned long flags;
 
 	info->fw_error = 0;
@@ -65,21 +65,21 @@ int hci_h4p_bc4_send_fw(struct hci_h4p_info *info,
 		skb->data[offset + 1] = 0x00;
 		skb->data[offset + 5] = 0x00;
 
-		not_valid = !bacmp(info->bd_addr, BDADDR_ANY);
+		not_valid = !bacmp(&info->bd_addr, BDADDR_ANY);
 
 		if (not_valid) {
 			dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
 			/* When address is not valid, use some random */
-			memcpy(info->bd_addr, nokia_oui, 3);
-			get_random_bytes(info->bd_addr + 3, 3);
+			memcpy(info->bd_addr.b, nokia_oui, 3);
+			get_random_bytes(info->bd_addr.b + 3, 3);
 		}
 
-		skb->data[offset + 7] = info->bd_addr[0];
-		skb->data[offset + 6] = info->bd_addr[1];
-		skb->data[offset + 4] = info->bd_addr[2];
-		skb->data[offset + 0] = info->bd_addr[3];
-		skb->data[offset + 3] = info->bd_addr[4];
-		skb->data[offset + 2] = info->bd_addr[5];
+		skb->data[offset + 7] = info->bd_addr.b[0];
+		skb->data[offset + 6] = info->bd_addr.b[1];
+		skb->data[offset + 4] = info->bd_addr.b[2];
+		skb->data[offset + 0] = info->bd_addr.b[3];
+		skb->data[offset + 3] = info->bd_addr.b[4];
+		skb->data[offset + 2] = info->bd_addr.b[5];
 	}
 
 	for (count = 1; ; count++) {

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 2/3] staging: nokia_h4: avoid __uX types

[Pavel Machek]

__u8/16 types should not be neccessary inside kernel, u8/16 should
be enough.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/staging/nokia_h4p/hci_h4p.h b/drivers/staging/nokia_h4p/hci_h4p.h
index 76eaaaa..b91e8b3 100644
--- a/drivers/staging/nokia_h4p/hci_h4p.h
+++ b/drivers/staging/nokia_h4p/hci_h4p.h
@@ -100,12 +100,12 @@ struct hci_h4p_info {
 };
 
 struct hci_h4p_radio_hdr {
-	__u8 evt;
-	__u8 dlen;
+	u8 evt;
+	u8 dlen;
 } __packed;
 
 struct hci_h4p_neg_hdr {
-	__u8 dlen;
+	u8 dlen;
 } __packed;
 #define H4P_NEG_HDR_SIZE 1
 
@@ -121,36 +121,36 @@ struct hci_h4p_neg_hdr {
 #define H4P_ID_TI1271	0x31
 
 struct hci_h4p_neg_cmd {
-	__u8	ack;
-	__u16	baud;
-	__u16	unused1;
-	__u8	proto;
-	__u16	sys_clk;
-	__u16	unused2;
+	u8	ack;
+	u16	baud;
+	u16	unused1;
+	u8	proto;
+	u16	sys_clk;
+	u16	unused2;
 } __packed;
 
 struct hci_h4p_neg_evt {
-	__u8	ack;
-	__u16	baud;
-	__u16	unused1;
-	__u8	proto;
-	__u16	sys_clk;
-	__u16	unused2;
-	__u8	man_id;
-	__u8	ver_id;
+	u8	ack;
+	u16	baud;
+	u16	unused1;
+	u8	proto;
+	u16	sys_clk;
+	u16	unused2;
+	u8	man_id;
+	u8	ver_id;
 } __packed;
 
 #define H4P_ALIVE_REQ	0x55
 #define H4P_ALIVE_RESP	0xcc
 
 struct hci_h4p_alive_hdr {
-	__u8	dlen;
+	u8	dlen;
 } __packed;
 #define H4P_ALIVE_HDR_SIZE 1
 
 struct hci_h4p_alive_pkt {
-	__u8	mid;
-	__u8	unused;
+	u8	mid;
+	u8	unused;
 } __packed;
 
 #define MAX_BAUD_RATE		921600

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 3/3] staging: use inlines where it makes sense

[Pavel Machek]

Make some uart function static inline to reduce ammount of exported code.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/staging/nokia_h4p/hci_h4p.h b/drivers/staging/nokia_h4p/hci_h4p.h
index b91e8b3..6c72ca5 100644
--- a/drivers/staging/nokia_h4p/hci_h4p.h
+++ b/drivers/staging/nokia_h4p/hci_h4p.h
@@ -26,6 +26,8 @@
 #include <net/bluetooth/hci_core.h>
 #include <net/bluetooth/hci.h>
 
+#include <linux/serial_reg.h>
+
 #define UART_SYSC_OMAP_RESET	0x03
 #define UART_SYSS_RESETDONE	0x01
 #define UART_OMAP_SCR_EMPTY_THR	0x08
@@ -205,9 +207,28 @@ int hci_h4p_read_fw(struct hci_h4p_info *info, struct sk_buff_head *fw_queue);
 int hci_h4p_send_fw(struct hci_h4p_info *info, struct sk_buff_head *fw_queue);
 void hci_h4p_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb);
 
-void hci_h4p_outb(struct hci_h4p_info *info, unsigned int offset, u8 val);
-u8 hci_h4p_inb(struct hci_h4p_info *info, unsigned int offset);
-void hci_h4p_set_rts(struct hci_h4p_info *info, int active);
+static inline void hci_h4p_outb(struct hci_h4p_info *info, unsigned int offset, u8 val)
+{
+	__raw_writeb(val, info->uart_base + (offset << 2));
+}
+
+static inline u8 hci_h4p_inb(struct hci_h4p_info *info, unsigned int offset)
+{
+	return __raw_readb(info->uart_base + (offset << 2));
+}
+
+static inline void hci_h4p_set_rts(struct hci_h4p_info *info, int active)
+{
+	u8 b;
+
+	b = hci_h4p_inb(info, UART_MCR);
+	if (active)
+		b |= UART_MCR_RTS;
+	else
+		b &= ~UART_MCR_RTS;
+	hci_h4p_outb(info, UART_MCR, b);
+}
+
 int hci_h4p_wait_for_cts(struct hci_h4p_info *info, int active, int timeout_ms);
 void __hci_h4p_set_auto_ctsrts(struct hci_h4p_info *info, int on, u8 which);
 void hci_h4p_set_auto_ctsrts(struct hci_h4p_info *info, int on, u8 which);
diff --git a/drivers/staging/nokia_h4p/nokia_uart.c b/drivers/staging/nokia_h4p/nokia_uart.c
index 0fb57de..9aab7bb 100644
--- a/drivers/staging/nokia_h4p/nokia_uart.c
+++ b/drivers/staging/nokia_h4p/nokia_uart.c
@@ -19,7 +19,6 @@
  *
  */
 
-#include <linux/serial_reg.h>
 #include <linux/delay.h>
 #include <linux/clk.h>
 
@@ -27,27 +26,7 @@
 
 #include "hci_h4p.h"
 
-inline void hci_h4p_outb(struct hci_h4p_info *info, unsigned int offset, u8 val)
-{
-	__raw_writeb(val, info->uart_base + (offset << 2));
-}
-
-inline u8 hci_h4p_inb(struct hci_h4p_info *info, unsigned int offset)
-{
-	return __raw_readb(info->uart_base + (offset << 2));
-}
 
-void hci_h4p_set_rts(struct hci_h4p_info *info, int active)
-{
-	u8 b;
-
-	b = hci_h4p_inb(info, UART_MCR);
-	if (active)
-		b |= UART_MCR_RTS;
-	else
-		b &= ~UART_MCR_RTS;
-	hci_h4p_outb(info, UART_MCR, b);
-}
 
 int hci_h4p_wait_for_cts(struct hci_h4p_info *info, int active,
 			 int timeout_ms)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] bluetooth: avoid using __uX types

[Pavel Machek]

__u8/16 types should not be neccessary inside kernel, u8/16 should
be enough.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 373000d..1176e67 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -58,8 +58,8 @@
 
 #define BT_SECURITY	4
 struct bt_security {
-	__u8 level;
-	__u8 key_size;
+	u8 level;
+	u8 key_size;
 };
 #define BT_SECURITY_SDP		0
 #define BT_SECURITY_LOW		1
@@ -76,7 +76,7 @@ struct bt_security {
 
 #define BT_POWER	9
 struct bt_power {
-	__u8 force_active;
+	u8 force_active;
 };
 #define BT_POWER_FORCE_ACTIVE_OFF 0
 #define BT_POWER_FORCE_ACTIVE_ON  1
@@ -110,7 +110,7 @@ struct bt_power {
 
 #define BT_VOICE		11
 struct bt_voice {
-	__u16 setting;
+	u16 setting;
 };
 
 #define BT_VOICE_TRANSPARENT			0x0003
@@ -170,7 +170,7 @@ static inline const char *state_to_string(int state)
 
 /* BD Address */
 typedef struct {
-	__u8 b[6];
+	u8 b[6];
 } __packed bdaddr_t;
 
 /* BD Address type */
@@ -178,7 +178,7 @@ typedef struct {
 #define BDADDR_LE_PUBLIC	0x01
 #define BDADDR_LE_RANDOM	0x02
 
-static inline bool bdaddr_type_is_valid(__u8 type)
+static inline bool bdaddr_type_is_valid(u8 type)
 {
 	switch (type) {
 	case BDADDR_BREDR:
@@ -190,7 +190,7 @@ static inline bool bdaddr_type_is_valid(__u8 type)
 	return false;
 }
 
-static inline bool bdaddr_type_is_le(__u8 type)
+static inline bool bdaddr_type_is_le(u8 type)
 {
 	switch (type) {
 	case BDADDR_LE_PUBLIC:
@@ -260,15 +260,15 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
 
 /* Skb helpers */
 struct l2cap_ctrl {
-	__u8	sframe:1,
+	u8	sframe:1,
 		poll:1,
 		final:1,
 		fcs:1,
 		sar:2,
 		super:2;
-	__u16	reqseq;
-	__u16	txseq;
-	__u8	retries;
+	u16	reqseq;
+	u16	txseq;
+	u8	retries;
 };
 
 struct hci_dev;
@@ -282,10 +282,10 @@ struct hci_req_ctrl {
 };
 
 struct bt_skb_cb {
-	__u8 pkt_type;
-	__u8 incoming;
-	__u16 expect;
-	__u8 force_active;
+	u8 pkt_type;
+	u8 incoming;
+	u16 expect;
+	u8 force_active;
 	struct l2cap_chan *chan;
 	struct l2cap_ctrl control;
 	struct hci_req_ctrl req;
@@ -336,7 +336,7 @@ out:
 	return NULL;
 }
 
-int bt_to_errno(__u16 code);
+int bt_to_errno(u16 code);
 
 int hci_sock_init(void);
 void hci_sock_cleanup(void);
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] staging: nokia_h4: merge firmware together so that we can reduce ammount of exports

[Pavel Machek]

Merge firmware files to nokia_core, so that we can reduce ammount of
exported functions. Also replace hand-coded check for invalid
bluetooth address with bacmp.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/staging/nokia_h4p/Makefile b/drivers/staging/nokia_h4p/Makefile
index 310b0f2..3398a1c 100644
--- a/drivers/staging/nokia_h4p/Makefile
+++ b/drivers/staging/nokia_h4p/Makefile
@@ -1,6 +1,5 @@
 
 obj-$(CONFIG_BT_NOKIA_H4P)		+= nokia_h4p.o
-nokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
-		nokia_fw-bcm.o nokia_fw-ti1273.o
+nokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o
 
 ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/staging/nokia_h4p/nokia_core.c b/drivers/staging/nokia_h4p/nokia_core.c
index 775e1d0..501be06 100644
--- a/drivers/staging/nokia_h4p/nokia_core.c
+++ b/drivers/staging/nokia_h4p/nokia_core.c
@@ -1191,6 +1191,330 @@ static int hci_h4p_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/* Code specific for BCM firmware -------------------------------- */
+
+static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info,
+				struct sk_buff *skb)
+{
+	int i;
+	static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
+	int not_valid = !bacmp(info->bd_addr, BDADDR_ANY);
+
+	if (not_valid) {
+		dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
+		/* When address is not valid, use some random but Nokia MAC */
+		memcpy(info->bd_addr, nokia_oui, 3);
+		get_random_bytes(info->bd_addr + 3, 3);
+	}
+
+	for (i = 0; i < 6; i++)
+		skb->data[9 - i] = info->bd_addr[i];
+
+	return 0;
+}
+
+void hci_h4p_bcm_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
+{
+	struct sk_buff *fw_skb;
+	int err;
+	unsigned long flags;
+
+	if (skb->data[5] != 0x00) {
+		dev_err(info->dev, "Firmware sending command failed 0x%.2x\n",
+			skb->data[5]);
+		info->fw_error = -EPROTO;
+	}
+
+	kfree_skb(skb);
+
+	fw_skb = skb_dequeue(info->fw_q);
+	if (fw_skb == NULL || info->fw_error) {
+		complete(&info->fw_completion);
+		return;
+	}
+
+	if (fw_skb->data[1] == 0x01 && fw_skb->data[2] == 0xfc &&
+			fw_skb->len >= 10) {
+		BT_DBG("Setting bluetooth address");
+		err = hci_h4p_bcm_set_bdaddr(info, fw_skb);
+		if (err < 0) {
+			kfree_skb(fw_skb);
+			info->fw_error = err;
+			complete(&info->fw_completion);
+			return;
+		}
+	}
+
+	skb_queue_tail(&info->txq, fw_skb);
+	spin_lock_irqsave(&info->lock, flags);
+	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
+			UART_IER_THRI);
+	spin_unlock_irqrestore(&info->lock, flags);
+}
+
+
+int hci_h4p_bcm_send_fw(struct hci_h4p_info *info,
+			struct sk_buff_head *fw_queue)
+{
+	struct sk_buff *skb;
+	unsigned long flags, time;
+
+	info->fw_error = 0;
+
+	BT_DBG("Sending firmware");
+
+	time = jiffies;
+
+	info->fw_q = fw_queue;
+	skb = skb_dequeue(fw_queue);
+	if (!skb)
+		return -ENODATA;
+
+	BT_DBG("Sending commands");
+
+	/*
+	 * Disable smart-idle as UART TX interrupts
+	 * are not wake-up capable
+	 */
+	hci_h4p_smart_idle(info, 0);
+
+	/* Check if this is bd_address packet */
+	init_completion(&info->fw_completion);
+	skb_queue_tail(&info->txq, skb);
+	spin_lock_irqsave(&info->lock, flags);
+	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
+			UART_IER_THRI);
+	spin_unlock_irqrestore(&info->lock, flags);
+
+	if (!wait_for_completion_timeout(&info->fw_completion,
+				msecs_to_jiffies(2000))) {
+		dev_err(info->dev, "No reply to fw command\n");
+		return -ETIMEDOUT;
+	}
+
+	if (info->fw_error) {
+		dev_err(info->dev, "FW error\n");
+		return -EPROTO;
+	}
+
+	BT_DBG("Firmware sent in %d msecs",
+		   jiffies_to_msecs(jiffies-time));
+
+	hci_h4p_set_auto_ctsrts(info, 0, UART_EFR_RTS);
+	hci_h4p_set_rts(info, 0);
+	hci_h4p_change_speed(info, BC4_MAX_BAUD_RATE);
+	hci_h4p_set_auto_ctsrts(info, 1, UART_EFR_RTS);
+
+	return 0;
+}
+
+/* Code specific for CSR firmware -------------------------------- */
+
+void hci_h4p_bc4_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
+{
+	/* Check if this is fw packet */
+	if (skb->data[0] != 0xff) {
+		hci_recv_frame(info->hdev, skb);
+		return;
+	}
+
+	if (skb->data[11] || skb->data[12]) {
+		dev_err(info->dev, "Firmware sending command failed\n");
+		info->fw_error = -EPROTO;
+	}
+
+	kfree_skb(skb);
+	complete(&info->fw_completion);
+}
+
+int hci_h4p_bc4_send_fw(struct hci_h4p_info *info,
+			struct sk_buff_head *fw_queue)
+{
+	static const u8 nokia_oui[3] = {0x00, 0x19, 0x4F};
+	struct sk_buff *skb;
+	unsigned int offset;
+	int retries, count, i, not_valid;
+	unsigned long flags;
+
+	info->fw_error = 0;
+
+	BT_DBG("Sending firmware");
+	skb = skb_dequeue(fw_queue);
+
+	if (!skb)
+		return -ENOMSG;
+
+	/* Check if this is bd_address packet */
+	if (skb->data[15] == 0x01 && skb->data[16] == 0x00) {
+		offset = 21;
+		skb->data[offset + 1] = 0x00;
+		skb->data[offset + 5] = 0x00;
+
+		not_valid = !bacmp(info->bd_addr, BDADDR_ANY);
+
+		if (not_valid) {
+			dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
+			/* When address is not valid, use some random */
+			memcpy(info->bd_addr, nokia_oui, 3);
+			get_random_bytes(info->bd_addr + 3, 3);
+		}
+
+		skb->data[offset + 7] = info->bd_addr[0];
+		skb->data[offset + 6] = info->bd_addr[1];
+		skb->data[offset + 4] = info->bd_addr[2];
+		skb->data[offset + 0] = info->bd_addr[3];
+		skb->data[offset + 3] = info->bd_addr[4];
+		skb->data[offset + 2] = info->bd_addr[5];
+	}
+
+	for (count = 1; ; count++) {
+		BT_DBG("Sending firmware command %d", count);
+		init_completion(&info->fw_completion);
+		skb_queue_tail(&info->txq, skb);
+		spin_lock_irqsave(&info->lock, flags);
+		hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
+							 UART_IER_THRI);
+		spin_unlock_irqrestore(&info->lock, flags);
+
+		skb = skb_dequeue(fw_queue);
+		if (!skb)
+			break;
+
+		if (!wait_for_completion_timeout(&info->fw_completion,
+						 msecs_to_jiffies(1000))) {
+			dev_err(info->dev, "No reply to fw command\n");
+			return -ETIMEDOUT;
+		}
+
+		if (info->fw_error) {
+			dev_err(info->dev, "FW error\n");
+			return -EPROTO;
+		}
+	};
+
+	/* Wait for chip warm reset */
+	retries = 100;
+	while ((!skb_queue_empty(&info->txq) ||
+	       !(hci_h4p_inb(info, UART_LSR) & UART_LSR_TEMT)) &&
+	       retries--) {
+		msleep(10);
+	}
+	if (!retries) {
+		dev_err(info->dev, "Transmitter not empty\n");
+		return -ETIMEDOUT;
+	}
+
+	hci_h4p_change_speed(info, BC4_MAX_BAUD_RATE);
+
+	if (hci_h4p_wait_for_cts(info, 1, 100)) {
+		dev_err(info->dev, "cts didn't deassert after final speed\n");
+		return -ETIMEDOUT;
+	}
+
+	retries = 100;
+	do {
+		init_completion(&info->init_completion);
+		hci_h4p_send_alive_packet(info);
+		retries--;
+	} while (!wait_for_completion_timeout(&info->init_completion, 100) &&
+		 retries > 0);
+
+	if (!retries) {
+		dev_err(info->dev, "No alive reply after speed change\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+/* Code specific for ti1273 firmware ----------------------------- */
+
+static struct sk_buff_head *fw_q;
+
+void hci_h4p_ti1273_parse_fw_event(struct hci_h4p_info *info,
+			struct sk_buff *skb)
+{
+	struct sk_buff *fw_skb;
+	unsigned long flags;
+
+	if (skb->data[5] != 0x00) {
+		dev_err(info->dev, "Firmware sending command failed 0x%.2x\n",
+			skb->data[5]);
+		info->fw_error = -EPROTO;
+	}
+
+	kfree_skb(skb);
+
+	fw_skb = skb_dequeue(fw_q);
+	if (fw_skb == NULL || info->fw_error) {
+		complete(&info->fw_completion);
+		return;
+	}
+
+	skb_queue_tail(&info->txq, fw_skb);
+	spin_lock_irqsave(&info->lock, flags);
+	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
+			UART_IER_THRI);
+	spin_unlock_irqrestore(&info->lock, flags);
+}
+
+
+int hci_h4p_ti1273_send_fw(struct hci_h4p_info *info,
+			struct sk_buff_head *fw_queue)
+{
+	struct sk_buff *skb;
+	unsigned long flags, time;
+
+	info->fw_error = 0;
+
+	BT_DBG("Sending firmware");
+
+	time = jiffies;
+
+	fw_q = fw_queue;
+	skb = skb_dequeue(fw_queue);
+	if (!skb)
+		return -ENODATA;
+
+	BT_DBG("Sending commands");
+	/* Check if this is bd_address packet */
+	init_completion(&info->fw_completion);
+	hci_h4p_smart_idle(info, 0);
+	skb_queue_tail(&info->txq, skb);
+	spin_lock_irqsave(&info->lock, flags);
+	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
+			UART_IER_THRI);
+	spin_unlock_irqrestore(&info->lock, flags);
+
+	if (!wait_for_completion_timeout(&info->fw_completion,
+				msecs_to_jiffies(2000))) {
+		dev_err(info->dev, "No reply to fw command\n");
+		return -ETIMEDOUT;
+	}
+
+	if (info->fw_error) {
+		dev_err(info->dev, "FW error\n");
+		return -EPROTO;
+	}
+
+	BT_DBG("Firmware sent in %d msecs",
+		   jiffies_to_msecs(jiffies-time));
+
+	hci_h4p_set_auto_ctsrts(info, 0, UART_EFR_RTS);
+	hci_h4p_set_rts(info, 0);
+	hci_h4p_change_speed(info, BC4_MAX_BAUD_RATE);
+	if (hci_h4p_wait_for_cts(info, 1, 100)) {
+		dev_err(info->dev,
+			"cts didn't go down after final speed change\n");
+		return -ETIMEDOUT;
+	}
+	hci_h4p_set_auto_ctsrts(info, 1, UART_EFR_RTS);
+
+	return 0;
+}
+
+/* Generic code -------------------------------------------------- */
+
 static struct platform_driver hci_h4p_driver = {
 	.probe		= hci_h4p_probe,
 	.remove		= hci_h4p_remove,
diff --git a/drivers/staging/nokia_h4p/nokia_fw-bcm.c b/drivers/staging/nokia_h4p/nokia_fw-bcm.c
deleted file mode 100644
index b55f5ba..0000000
--- a/drivers/staging/nokia_h4p/nokia_fw-bcm.c
+++ /dev/null
@@ -1,149 +0,0 @@
-/*
- * This file is part of Nokia H4P bluetooth driver
- *
- * Copyright (C) 2005-2008 Nokia Corporation.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
- * 02110-1301 USA
- *
- */
-
-#include <linux/skbuff.h>
-#include <linux/delay.h>
-#include <linux/serial_reg.h>
-
-#include "hci_h4p.h"
-
-static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info,
-				struct sk_buff *skb)
-{
-	int i;
-	static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
-	int not_valid;
-
-	not_valid = 1;
-	for (i = 0; i < 6; i++) {
-		if (info->bd_addr[i] != 0x00) {
-			not_valid = 0;
-			break;
-		}
-	}
-
-	if (not_valid) {
-		dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
-		/* When address is not valid, use some random but Nokia MAC */
-		memcpy(info->bd_addr, nokia_oui, 3);
-		get_random_bytes(info->bd_addr + 3, 3);
-	}
-
-	for (i = 0; i < 6; i++)
-		skb->data[9 - i] = info->bd_addr[i];
-
-	return 0;
-}
-
-void hci_h4p_bcm_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
-{
-	struct sk_buff *fw_skb;
-	int err;
-	unsigned long flags;
-
-	if (skb->data[5] != 0x00) {
-		dev_err(info->dev, "Firmware sending command failed 0x%.2x\n",
-			skb->data[5]);
-		info->fw_error = -EPROTO;
-	}
-
-	kfree_skb(skb);
-
-	fw_skb = skb_dequeue(info->fw_q);
-	if (fw_skb == NULL || info->fw_error) {
-		complete(&info->fw_completion);
-		return;
-	}
-
-	if (fw_skb->data[1] == 0x01 && fw_skb->data[2] == 0xfc &&
-			fw_skb->len >= 10) {
-		BT_DBG("Setting bluetooth address");
-		err = hci_h4p_bcm_set_bdaddr(info, fw_skb);
-		if (err < 0) {
-			kfree_skb(fw_skb);
-			info->fw_error = err;
-			complete(&info->fw_completion);
-			return;
-		}
-	}
-
-	skb_queue_tail(&info->txq, fw_skb);
-	spin_lock_irqsave(&info->lock, flags);
-	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
-			UART_IER_THRI);
-	spin_unlock_irqrestore(&info->lock, flags);
-}
-
-
-int hci_h4p_bcm_send_fw(struct hci_h4p_info *info,
-			struct sk_buff_head *fw_queue)
-{
-	struct sk_buff *skb;
-	unsigned long flags, time;
-
-	info->fw_error = 0;
-
-	BT_DBG("Sending firmware");
-
-	time = jiffies;
-
-	info->fw_q = fw_queue;
-	skb = skb_dequeue(fw_queue);
-	if (!skb)
-		return -ENODATA;
-
-	BT_DBG("Sending commands");
-
-	/*
-	 * Disable smart-idle as UART TX interrupts
-	 * are not wake-up capable
-	 */
-	hci_h4p_smart_idle(info, 0);
-
-	/* Check if this is bd_address packet */
-	init_completion(&info->fw_completion);
-	skb_queue_tail(&info->txq, skb);
-	spin_lock_irqsave(&info->lock, flags);
-	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
-			UART_IER_THRI);
-	spin_unlock_irqrestore(&info->lock, flags);
-
-	if (!wait_for_completion_timeout(&info->fw_completion,
-				msecs_to_jiffies(2000))) {
-		dev_err(info->dev, "No reply to fw command\n");
-		return -ETIMEDOUT;
-	}
-
-	if (info->fw_error) {
-		dev_err(info->dev, "FW error\n");
-		return -EPROTO;
-	}
-
-	BT_DBG("Firmware sent in %d msecs",
-		   jiffies_to_msecs(jiffies-time));
-
-	hci_h4p_set_auto_ctsrts(info, 0, UART_EFR_RTS);
-	hci_h4p_set_rts(info, 0);
-	hci_h4p_change_speed(info, BC4_MAX_BAUD_RATE);
-	hci_h4p_set_auto_ctsrts(info, 1, UART_EFR_RTS);
-
-	return 0;
-}
diff --git a/drivers/staging/nokia_h4p/nokia_fw-csr.c b/drivers/staging/nokia_h4p/nokia_fw-csr.c
deleted file mode 100644
index fe6b704..0000000
--- a/drivers/staging/nokia_h4p/nokia_fw-csr.c
+++ /dev/null
@@ -1,149 +0,0 @@
-/*
- * This file is part of Nokia H4P bluetooth driver
- *
- * Copyright (C) 2005-2008 Nokia Corporation.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
- * 02110-1301 USA
- *
- */
-
-#include <linux/skbuff.h>
-#include <linux/delay.h>
-#include <linux/serial_reg.h>
-
-#include "hci_h4p.h"
-
-void hci_h4p_bc4_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
-{
-	/* Check if this is fw packet */
-	if (skb->data[0] != 0xff) {
-		hci_recv_frame(info->hdev, skb);
-		return;
-	}
-
-	if (skb->data[11] || skb->data[12]) {
-		dev_err(info->dev, "Firmware sending command failed\n");
-		info->fw_error = -EPROTO;
-	}
-
-	kfree_skb(skb);
-	complete(&info->fw_completion);
-}
-
-int hci_h4p_bc4_send_fw(struct hci_h4p_info *info,
-			struct sk_buff_head *fw_queue)
-{
-	static const u8 nokia_oui[3] = {0x00, 0x19, 0x4F};
-	struct sk_buff *skb;
-	unsigned int offset;
-	int retries, count, i, not_valid;
-	unsigned long flags;
-
-	info->fw_error = 0;
-
-	BT_DBG("Sending firmware");
-	skb = skb_dequeue(fw_queue);
-
-	if (!skb)
-		return -ENOMSG;
-
-	/* Check if this is bd_address packet */
-	if (skb->data[15] == 0x01 && skb->data[16] == 0x00) {
-		offset = 21;
-		skb->data[offset + 1] = 0x00;
-		skb->data[offset + 5] = 0x00;
-
-		not_valid = 1;
-		for (i = 0; i < 6; i++) {
-			if (info->bd_addr[i] != 0x00) {
-				not_valid = 0;
-				break;
-			}
-		}
-
-		if (not_valid) {
-			dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
-			/* When address is not valid, use some random */
-			memcpy(info->bd_addr, nokia_oui, 3);
-			get_random_bytes(info->bd_addr + 3, 3);
-		}
-
-		skb->data[offset + 7] = info->bd_addr[0];
-		skb->data[offset + 6] = info->bd_addr[1];
-		skb->data[offset + 4] = info->bd_addr[2];
-		skb->data[offset + 0] = info->bd_addr[3];
-		skb->data[offset + 3] = info->bd_addr[4];
-		skb->data[offset + 2] = info->bd_addr[5];
-	}
-
-	for (count = 1; ; count++) {
-		BT_DBG("Sending firmware command %d", count);
-		init_completion(&info->fw_completion);
-		skb_queue_tail(&info->txq, skb);
-		spin_lock_irqsave(&info->lock, flags);
-		hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
-							 UART_IER_THRI);
-		spin_unlock_irqrestore(&info->lock, flags);
-
-		skb = skb_dequeue(fw_queue);
-		if (!skb)
-			break;
-
-		if (!wait_for_completion_timeout(&info->fw_completion,
-						 msecs_to_jiffies(1000))) {
-			dev_err(info->dev, "No reply to fw command\n");
-			return -ETIMEDOUT;
-		}
-
-		if (info->fw_error) {
-			dev_err(info->dev, "FW error\n");
-			return -EPROTO;
-		}
-	};
-
-	/* Wait for chip warm reset */
-	retries = 100;
-	while ((!skb_queue_empty(&info->txq) ||
-	       !(hci_h4p_inb(info, UART_LSR) & UART_LSR_TEMT)) &&
-	       retries--) {
-		msleep(10);
-	}
-	if (!retries) {
-		dev_err(info->dev, "Transmitter not empty\n");
-		return -ETIMEDOUT;
-	}
-
-	hci_h4p_change_speed(info, BC4_MAX_BAUD_RATE);
-
-	if (hci_h4p_wait_for_cts(info, 1, 100)) {
-		dev_err(info->dev, "cts didn't deassert after final speed\n");
-		return -ETIMEDOUT;
-	}
-
-	retries = 100;
-	do {
-		init_completion(&info->init_completion);
-		hci_h4p_send_alive_packet(info);
-		retries--;
-	} while (!wait_for_completion_timeout(&info->init_completion, 100) &&
-		 retries > 0);
-
-	if (!retries) {
-		dev_err(info->dev, "No alive reply after speed change\n");
-		return -ETIMEDOUT;
-	}
-
-	return 0;
-}
diff --git a/drivers/staging/nokia_h4p/nokia_fw-ti1273.c b/drivers/staging/nokia_h4p/nokia_fw-ti1273.c
deleted file mode 100644
index f5500f7..0000000
--- a/drivers/staging/nokia_h4p/nokia_fw-ti1273.c
+++ /dev/null
@@ -1,110 +0,0 @@
-/*
- * This file is part of Nokia H4P bluetooth driver
- *
- * Copyright (C) 2009 Nokia Corporation.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
- * 02110-1301 USA
- *
- */
-
-#include <linux/skbuff.h>
-#include <linux/delay.h>
-#include <linux/serial_reg.h>
-
-#include "hci_h4p.h"
-
-static struct sk_buff_head *fw_q;
-
-void hci_h4p_ti1273_parse_fw_event(struct hci_h4p_info *info,
-			struct sk_buff *skb)
-{
-	struct sk_buff *fw_skb;
-	unsigned long flags;
-
-	if (skb->data[5] != 0x00) {
-		dev_err(info->dev, "Firmware sending command failed 0x%.2x\n",
-			skb->data[5]);
-		info->fw_error = -EPROTO;
-	}
-
-	kfree_skb(skb);
-
-	fw_skb = skb_dequeue(fw_q);
-	if (fw_skb == NULL || info->fw_error) {
-		complete(&info->fw_completion);
-		return;
-	}
-
-	skb_queue_tail(&info->txq, fw_skb);
-	spin_lock_irqsave(&info->lock, flags);
-	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
-			UART_IER_THRI);
-	spin_unlock_irqrestore(&info->lock, flags);
-}
-
-
-int hci_h4p_ti1273_send_fw(struct hci_h4p_info *info,
-			struct sk_buff_head *fw_queue)
-{
-	struct sk_buff *skb;
-	unsigned long flags, time;
-
-	info->fw_error = 0;
-
-	BT_DBG("Sending firmware");
-
-	time = jiffies;
-
-	fw_q = fw_queue;
-	skb = skb_dequeue(fw_queue);
-	if (!skb)
-		return -ENODATA;
-
-	BT_DBG("Sending commands");
-	/* Check if this is bd_address packet */
-	init_completion(&info->fw_completion);
-	hci_h4p_smart_idle(info, 0);
-	skb_queue_tail(&info->txq, skb);
-	spin_lock_irqsave(&info->lock, flags);
-	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
-			UART_IER_THRI);
-	spin_unlock_irqrestore(&info->lock, flags);
-
-	if (!wait_for_completion_timeout(&info->fw_completion,
-				msecs_to_jiffies(2000))) {
-		dev_err(info->dev, "No reply to fw command\n");
-		return -ETIMEDOUT;
-	}
-
-	if (info->fw_error) {
-		dev_err(info->dev, "FW error\n");
-		return -EPROTO;
-	}
-
-	BT_DBG("Firmware sent in %d msecs",
-		   jiffies_to_msecs(jiffies-time));
-
-	hci_h4p_set_auto_ctsrts(info, 0, UART_EFR_RTS);
-	hci_h4p_set_rts(info, 0);
-	hci_h4p_change_speed(info, BC4_MAX_BAUD_RATE);
-	if (hci_h4p_wait_for_cts(info, 1, 100)) {
-		dev_err(info->dev,
-			"cts didn't go down after final speed change\n");
-		return -ETIMEDOUT;
-	}
-	hci_h4p_set_auto_ctsrts(info, 1, UART_EFR_RTS);
-
-	return 0;
-}

Re: [PATCH] staging: nokia_h4: merge firmware together so that we can reduce ammount of exports

[Pavel Machek]

On Thu 2014-08-07 11:07:34, Pavel Machek wrote:
> 
> Merge firmware files to nokia_core, so that we can reduce ammount of
> exported functions. Also replace hand-coded check for invalid
> bluetooth address with bacmp.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

Actually, it might be better if you forgot about this
patch. The code is more tangled up than I expected.


					Pavel

Re: [PATCH] staging: nokia_h4: merge firmware together so that we can reduce ammount of exports

[Marcel Holtmann]

Hi Pavel,

> Merge firmware files to nokia_core, so that we can reduce ammount of
> exported functions. Also replace hand-coded check for invalid
> bluetooth address with bacmp.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/staging/nokia_h4p/Makefile b/drivers/staging/nokia_h4p/Makefile
> index 310b0f2..3398a1c 100644
> --- a/drivers/staging/nokia_h4p/Makefile
> +++ b/drivers/staging/nokia_h4p/Makefile
> @@ -1,6 +1,5 @@
> 
> obj-$(CONFIG_BT_NOKIA_H4P)		+= nokia_h4p.o
> -nokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \
> -		nokia_fw-bcm.o nokia_fw-ti1273.o
> +nokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o
> 
> ccflags-y += -D__CHECK_ENDIAN__
> diff --git a/drivers/staging/nokia_h4p/nokia_core.c b/drivers/staging/nokia_h4p/nokia_core.c
> index 775e1d0..501be06 100644
> --- a/drivers/staging/nokia_h4p/nokia_core.c
> +++ b/drivers/staging/nokia_h4p/nokia_core.c
> @@ -1191,6 +1191,330 @@ static int hci_h4p_remove(struct platform_device *pdev)
> 	return 0;
> }
> 
> +/* Code specific for BCM firmware -------------------------------- */
> +
> +static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info,
> +				struct sk_buff *skb)
> +{
> +	int i;
> +	static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
> +	int not_valid = !bacmp(info->bd_addr, BDADDR_ANY);
> +
> +	if (not_valid) {
> +		dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
> +		/* When address is not valid, use some random but Nokia MAC */
> +		memcpy(info->bd_addr, nokia_oui, 3);
> +		get_random_bytes(info->bd_addr + 3, 3);
> +	}
> +
> +	for (i = 0; i < 6; i++)
> +		skb->data[9 - i] = info->bd_addr[i];
> +
> +	return 0;
> +}
> +
> +void hci_h4p_bcm_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
> +{
> +	struct sk_buff *fw_skb;
> +	int err;
> +	unsigned long flags;
> +
> +	if (skb->data[5] != 0x00) {
> +		dev_err(info->dev, "Firmware sending command failed 0x%.2x\n",
> +			skb->data[5]);
> +		info->fw_error = -EPROTO;
> +	}
> +
> +	kfree_skb(skb);
> +
> +	fw_skb = skb_dequeue(info->fw_q);
> +	if (fw_skb == NULL || info->fw_error) {
> +		complete(&info->fw_completion);
> +		return;
> +	}
> +
> +	if (fw_skb->data[1] == 0x01 && fw_skb->data[2] == 0xfc &&
> +			fw_skb->len >= 10) {
> +		BT_DBG("Setting bluetooth address");
> +		err = hci_h4p_bcm_set_bdaddr(info, fw_skb);
> +		if (err < 0) {
> +			kfree_skb(fw_skb);
> +			info->fw_error = err;
> +			complete(&info->fw_completion);
> +			return;
> +		}
> +	}
> +
> +	skb_queue_tail(&info->txq, fw_skb);
> +	spin_lock_irqsave(&info->lock, flags);
> +	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
> +			UART_IER_THRI);
> +	spin_unlock_irqrestore(&info->lock, flags);
> +}

and as I explained before, this crazy can not continue. Bluetooth drivers can provide a hdev->setup callback for loading firmware and doing other setup details. You can just bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the firmware. Especially if the firmware just consists of HCI commands. Which is clearly the case with the Nokia firmware files.

In addition with 3.17 you can also provide hdev->set_bdaddr to allow changing the public BD_ADDR. You can also set HCI_QUIRK_INVALID_BDADDR and bring up the controller in an unconfigured state. That way you can have userspace provide the address and avoid the hacking around the missing address issue.

Maybe at some point someone realizes that focusing on the N900 Bluetooth hardware would help in making this code simpler. Adding support for older Maemo devices can be done later.

Regards

Marcel

Re: [PATCH] staging: nokia_h4: merge firmware together so that we can reduce ammount of exports

[Pavel Machek]

Hi!


> > +
> > +	skb_queue_tail(&info->txq, fw_skb);
> > +	spin_lock_irqsave(&info->lock, flags);
> > +	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
> > +			UART_IER_THRI);
> > +	spin_unlock_irqrestore(&info->lock, flags);
> > +}
> 
> and as I explained before, this crazy can not continue. Bluetooth drivers can provide a hdev->setup callback for loading firmware and doing other setup details. You can just bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the firmware. Especially if the firmware just consists of HCI commands. Which is clearly the case with the Nokia firmware files.
> 

I know that this crazyness can not continue; but I need to be able to
move "bad" code around to get the cleanups, ok? Because you also asked
me to reduce ammount of exports.

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Miguel Oliveira]

So the /Documentation/timers/timers-howto.txt stats:

SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
* Use usleep_range

- Why not msleep for (1ms - 20ms)?
Explained originally here:
http://lkml.org/lkml/2007/8/3/250
msleep(1~20) may not do what the caller intends, and
will often sleep longer (~20 ms actual sleep for any
value given in the 1~20ms range). In many cases this
is not the desired behavior.

- Why is there no "usleep" / What is a good range?
Since usleep_range is built on top of hrtimers, the
wakeup will be very precise (ish), thus a simple
usleep function would likely introduce a large number
of undesired interrupts.

With the introduction of a range, the scheduler is
free to coalesce your wakeup with any other wakeup
that may have happened for other reasons, or at the
worst case, fire an interrupt for your upper bound.

The larger a range you supply, the greater a chance
that you will not trigger an interrupt; this should
be balanced with what is an acceptable upper bound on
delay / performance for your specific code path. Exact
tolerances here are very situation specific, thus it
is left to the caller to determine a reasonable range.

Should the documentation should be ignored?

Regards

2014-05-30 12:34 GMT+01:00 Pavel Machek <pavel@ucw.cz>:
> On Fri 2014-05-30 11:27:13, Miguel Oliveira wrote:
>> Creating this patch for the Eudyptula Challenge.
>> Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
>> Also inserted a blank line after adeclaration.
>
> So you are changing timings without testing. Plus, burning CPU power
> instead of sleeping.
>
> Seems you'll need another patch for the challenge :-).
>                                                                         Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Pavel Machek]

Hi!

> So the /Documentation/timers/timers-howto.txt stats:
> 
> SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
> * Use usleep_range
> 
> - Why not msleep for (1ms - 20ms)?
> Explained originally here:
> http://lkml.org/lkml/2007/8/3/250
> msleep(1~20) may not do what the caller intends, and
> will often sleep longer (~20 ms actual sleep for any
> value given in the 1~20ms range). In many cases this
> is not the desired behavior.
...
> Should the documentation should be ignored?

Caller wants to sleep for 10ms; docs said it may sleep for 10-20msec;
that sounds reasonable. You replaced it with busy-loop for 10-15msec.

That does not sound like improvement.

Thanks,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Miguel Oliveira]

Not on my prespective. Its says "~20 ms actual sleep for any value
given in the 1~20ms range", so from my point of view it will sleep for
20ms, so I will save between 5 to 10ms. But maybe I'm seeing it the
wrong way.


Regards

2014-05-30 13:35 GMT+01:00 Pavel Machek <pavel@ucw.cz>:
> Hi!
>
>> So the /Documentation/timers/timers-howto.txt stats:
>>
>> SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>> * Use usleep_range
>>
>> - Why not msleep for (1ms - 20ms)?
>> Explained originally here:
>> http://lkml.org/lkml/2007/8/3/250
>> msleep(1~20) may not do what the caller intends, and
>> will often sleep longer (~20 ms actual sleep for any
>> value given in the 1~20ms range). In many cases this
>> is not the desired behavior.
> ...
>> Should the documentation should be ignored?
>
> Caller wants to sleep for 10ms; docs said it may sleep for 10-20msec;
> that sounds reasonable. You replaced it with busy-loop for 10-15msec.
>
> That does not sound like improvement.
>
> Thanks,
>                                                                 Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Greg KH]

On Fri, May 30, 2014 at 11:27:13AM +0100, Miguel Oliveira wrote:
> Creating this patch for the Eudyptula Challenge.
> Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
> Also inserted a blank line after adeclaration.

As you are doing 2 different things in one patch, please break this up
into two different patches.  Remember, each patch can only do 1 thing.

That also lets us argue about the msleep() change, while I take your
whitespace fix :)

thanks,

greg k-h

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Miguel Oliveira]

Greg should I make a patch for the whitespace while you are
considering in remove the driver?

Regards

2014-05-30 17:01 GMT+01:00 Greg KH <gregkh@linuxfoundation.org>:
> On Fri, May 30, 2014 at 11:27:13AM +0100, Miguel Oliveira wrote:
>> Creating this patch for the Eudyptula Challenge.
>> Replaced msleep() for a delay < 20ms with a usleep_range() between 10000us and 15000us.
>> Also inserted a blank line after adeclaration.
>
> As you are doing 2 different things in one patch, please break this up
> into two different patches.  Remember, each patch can only do 1 thing.
>
> That also lets us argue about the msleep() change, while I take your
> whitespace fix :)
>
> thanks,
>
> greg k-h

Re: [PATCH] staging: nokia_h4: nokia_core.c: use usleep_range() instead of msleep()

[Greg KH]

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Fri, May 30, 2014 at 05:05:54PM +0100, Miguel Oliveira wrote:
> Greg should I make a patch for the whitespace while you are
> considering in remove the driver?

Yes, that would be fine, it will be another kernel release cycle before
I remove any more drivers.

thanks,

greg k-h