RE: [PATCH 1/2] staging: Add ST-Ericsson CG2900 driver

Hi Greg,

Thanks for your comments.

quoted

+
+ - Decide upon architecture. Some people consider architecture in

the cg2900

quoted

+   driver to be too complex. We consider it to be not more complex

than needed.
=20
What do you mean by this?  It sounds as if you do not consider this a
valid thing.  If so, why list it?
=20

I've been trying to get this driver into the "normal" driver tree
(Bluetooth and mfd) for the past half year. We have at this point
come to such a standstill that we wanted to get the driver into staging
where we could then continue the architecture discussion.

quoted

+ - Currently the cg2900_uart registers as protocol driver against

hci_ldisc.c.

quoted

+   There is however some common functionality with hci_h4.c and the

cg2900 could

quoted

+   therefore register it's vendor specific channels to hci_h4.c, but

this would

quoted

+   require adding a registration functionality in the hci_h4 file.

=20
Putting a "but" makes it sound like this is something you will not do.
If so, why list it?
=20

Again, this is due to discussions and putting the driver in staging
will make it easier to get a view of the driver. Making such a change
would also quite heavily affect the Bluetooth driver.

quoted

+ - Some people demand that the cg2900 driver re-use the Bluetooth

driver to send

quoted

+   and receive BT commands and events. That is however not possible

with current

quoted

+   BT API and might not be feasible, for example when using FM only

in

quoted

+   the cg2900 chip.

=20
Again, a review comment that you are saying is not valid.  Why list it?
=20

And, again, it was blocking us from getting the driver into the Kernel.
So we have to solve the issue in some way, but it is not clear at this
point exactly how it will be solved (and therefore also not where it
will be solved).

quoted

+ - TI has already delivered a driver for a multi-function chip

called ti-st.

quoted

+   This driver is currently located in drivers/misc/ti-st/. There

has however

quoted

+   been criticism raised against design/architecture of the driver.

There

quoted

+   currently also doesn't seem to be a way to add support for cg2900

in that

quoted

+   driver even though some people has raised this as an alternative.

=20
And again, the same thing.
=20
What criticism of that driver?  It's now accepted and is working and in
the tree.
=20

I will remove this text. There was criticism against the driver in the
mail discussions, but I agree that it should not be stated in this TODO
file.

My main point here is that this looks like a rant against people who
have reviewed your code in the past and why you feel you can not
address
those complaints.  That's not a valid thing for a TODO file at all.
Please list things that need to be fixed in the driver to get it merged
into the main tree.  As it is, you have a list of things that you say
you will not do, which is not encouraging at all.
=20

I'm sorry if it sounds like a rant against people who've reviewed the code.
That was never my intention.
I will see if I can rephrase, but the problem is that nothing has still bee=
n
decided so at this point it is hard to say exactly what shall be fixed.
But as I said, I will try to rewrite it in a better way.

quoted

diff --git a/drivers/staging/cg2900/bluetooth/Makefile

b/drivers/staging/cg2900/bluetooth/Makefile

quoted

new file mode 100644
index 0000000..6f4255b

--- /dev/null
+++ b/drivers/staging/cg2900/bluetooth/Makefile

@@ -0,0 +1,9 @@
+#
+# Makefile for ST-Ericsson CG2900 connectivity combo controller
+#
+
+ccflags-y :=3D					\
+	-Idrivers/staging/cg2900/include
+

=20
Trailing whitespace, did you run this through checkpatch.pl before
sending it to me?
=20
thanks,
=20
greg k-h

I will fix and make certain that I haven't missed anything more.
I have run checkpatch but in the hurry I must have made a last
minute change and forgot to run checkpatch on the last patch version.

/P-G