Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support

[Simon Horman <simon.horman@corigine.com>]

On Fri, May 12, 2023 at 01:14:18PM +0200, Luca Weiss wrote:
> Hi Simon,
> 
> On Mon May 1, 2023 at 3:11 PM CEST, Simon Horman wrote:
> > On Fri, Apr 21, 2023 at 04:11:39PM +0200, Luca Weiss wrote:
> > > Add support for the Bluetooth chip codenamed APACHE which is part of
> > > WCN3988.
> > > 
> > > The firmware for this chip has a slightly different naming scheme
> > > compared to most others. For ROM Version 0x0200 we need to use
> > > apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv +
> > > apnv11.bin
> > > 
> > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > ---
> > >  drivers/bluetooth/btqca.c   | 13 +++++++++++--
> > >  drivers/bluetooth/btqca.h   | 12 ++++++++++--
> > >  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
> > >  3 files changed, 33 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > index fd0941fe8608..3ee1ef88a640 100644
> > > --- a/drivers/bluetooth/btqca.c
> > > +++ b/drivers/bluetooth/btqca.c
> > > @@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> > >  	/* Firmware files to download are based on ROM version.
> > >  	 * ROM version is derived from last two bytes of soc_ver.
> > >  	 */
> > > -	rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> > > +	if (soc_type == QCA_WCN3988)
> > > +		rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f);
> > > +	else
> > > +		rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> >
> > Hi Luca,
> >
> > perhaps it's just me. But I was wondering if this can be improved on a little.
> >
> > * Move the common portion outside of the conditional
> > * And also, I think it's normal to use decimal for shift values.
> >
> > e.g.
> > 	unsigned shift;
> > 	...
> >
> > 	shift = soc_type == QCA_WCN3988 ? 5 : 4;
> > 	rom_ver = ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f);
> >
> > Using some helpers such as GENMASK and FIELD_PREP might also be nice.
> 
> While I'm not opposed to the idea, I'm not sure it's worth making
> beautiful macros for this since - to my eyes - how the mapping of
> soc_ver to firmware name works is rather obscure since the sources from
> Qualcomm just have a static lookup table of soc_ver to firmware name so
> doing this dynamically like here is different.
> 
> And I haven't looked at other chips that are covered there to see if
> there's a pattern to this, for the most part it seems the original
> formula works for most chips and the one I added works for WCN3988 (and
> the other "APACHE" chips, whatever they are).
> 
> If a third way is added then I would say for sure this line should be
> made nicer but for now I think it's easier to keep this as I sent it
> because we don't know what the future will hold.

Thanks. My feeling is that my suggestion mainly makes sense
if it lease to improved readability and maintainability.
It sounds like that might not be the case here.

> > >  	if (soc_type == QCA_WCN6750)
> > >  		qca_send_patch_config_cmd(hdev);
> > >  
> > >  	/* Download rampatch file */
> > >  	config.type = TLV_TYPE_PATCH;
> > > -	if (qca_is_wcn399x(soc_type)) {
> > > +	if (soc_type == QCA_WCN3988) {
> > > +		snprintf(config.fwname, sizeof(config.fwname),
> > > +			 "qca/apbtfw%02x.tlv", rom_ver);
> > > +	} else if (qca_is_wcn399x(soc_type)) {
> > >  		snprintf(config.fwname, sizeof(config.fwname),
> > >  			 "qca/crbtfw%02x.tlv", rom_ver);
> > >  	} else if (soc_type == QCA_QCA6390) {
> > > @@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> > >  	if (firmware_name)
> > >  		snprintf(config.fwname, sizeof(config.fwname),
> > >  			 "qca/%s", firmware_name);
> > > +	else if (soc_type == QCA_WCN3988)
> > > +		snprintf(config.fwname, sizeof(config.fwname),
> > > +			 "qca/apnv%02x.bin", rom_ver);
> > >  	else if (qca_is_wcn399x(soc_type)) {
> > >  		if (ver.soc_id == QCA_WCN3991_SOC_ID) {
> >
> > Not strictly related to this patch, but while reviewing this I noticed that
> > ver.soc_id is __le32 but QCA_WCN3991_SOC_ID is in host byteorder.
> >
> > Perhaps a cpu_to_le32() or le32_to_cpu() call is in order here?
> 
> Good catch, as you've seen I sent a patch separately to fix that. :)

Thanks!