On Fri, May 18, 2018 at 08:04:46PM +0530, Balakrishna Godavarthi wrote:
> as i was on vacation i couldn't able to reply to your comments.
> find my comments inline.
Also sorry for my late reply
> On 2018-05-11 23:10, Matthias Kaehlcke wrote:
> > Hi Bala,
> >
> > On Fri, May 11, 2018 at 05:51:02PM +0530, Balakrishna Godavarthi wrote:
> > > This patch enables the RAM and NV patch download for wcn3990.
> > >
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/bluetooth/btqca.c | 46
> > > +++++++++++++++++++++++++++++++++++++++++++++-
> > > drivers/bluetooth/btqca.h | 13 +++++++++++++
> > > 2 files changed, 58 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > index 8219816..40c6b4f 100644
> > > --- a/drivers/bluetooth/btqca.c
> > > +++ b/drivers/bluetooth/btqca.c
> > > @@ -27,7 +27,7 @@
> > >
> > > #define VERSION "0.1"
> > >
> > > -static int rome_patch_ver_req(struct hci_dev *hdev, u32
> > > *rome_version)
> > > +int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
> >
> > If this and other functions aren't really Rome specific they should
> > probably be renamed to qca_...
> >
> [Bala]:- will update the functions name in separate patch.
>
> > > +int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
> > > + u32 *soc_ver)
> > > +{
> > > + struct rome_config config;
> > > + int err;
> > > +
> > > + /* we are using the existing funciton of ROME,
> > > + * instead of duplicating the function for wcn3990.
> > > + */
> >
> > The comment isn't quite accurate, the qca_uart_setup_cherokee() calls
> > existing functions, but is essentially a copy of qca_uart_setup_rome().
> >
> > The main difference is that the patch version is passed as a parameter
> > instead of determining it in the uart setup function. There seems to
> > be no need to pass the version number in, or if you prefer to do it
> > this way, you could change the Rome code to do this.
> >
> > The other delta is the filename extension of the rampatch file, which
> > is .bin for Rome and .tlv for Cherokee. Is there a good reason to use
> > a different extension? If not just stick to the existing naming
> > scheme, otherwise you could pass the chip type as a parameter and
> > chose the extensions based on that.
> >
> [Bala]:- as we see the process to setup of BTchip ROME and wcn3990 setup.
> For Rome:
> 1. change the operating speed to 3.0mbps.
> 2. request the version
> 3. download RAM patch (.bin extestion)
> 4. downlaod NV patch (.bin extersnion)
> 5. Reset ROME
>
> steps 2 to 5 are done in the function qca_uart_setup_rome()
>
> For WCN3990:
> 1. Set baudrate to 115200.
> 2. request the version
> 3. set baudrate to operating speed.
> 4. download RAM patch (.tlv extestion)
> 5. downlaod NV patch (.bin extersnion)
> 6. Reset wcn3990
>
> steps 2 is done in function rome_patch_ver_req(), where as steps 4 to
> 6 qca_uart_setup_cherokee()
>
> the big difference is .bin extension for RAM patch is deprecated and
> BTCCHIP vendor will not support .bin for ram patch any more for latest
> chips.
> so that could be reason we using a separate function
> qca_uart_setup_cherokee(). if i use the same function by passing the chip
> name to decide the file extension.
> but again unnecessary i have to request the version command to
> BTCHIP.
You could just pass the version as parameter in both cases, Rome and
Cherokee could go through their respective hoops to obtain it before
calling qca_uart_setup().
The different extension and format strings could easily be fixed. I
admit though that the length of the function is on the edge of where
the redundance is an issue, so if Marcel is fine with it I won't inisit.
(slightly off-topic: is qca_uart_setup_xyz() actually the appropriate
name for the function? Admittedly I know little about the platform,
but it seems the most important thing the function does is to load the
two firmware files, which sounds more like qca_setup_xyz() ...)
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
