Re: [PATCH 1/1] [Add support Mediatek mt7921U]

[Marcel Holtmann <marcel@holtmann.org>]

Hi Mark,

> Thanks for your suggestions, I will remove the duplicate definitions and functions.
> 
> Firstly, we will add the support of enabling MT7921U in btusb.c
> Secondary, we will discuss the driver architecture with you.
> Finally, we update the common part and hif part for MT7921.
> 
> I have a couple questions for driver architecture.
> 1. Global dev.
> 2. Unify common part.
> 3. HIF part (usb/sdio/pcie/uart)
>> 
>>> This patch adds the support of enabling MT7921U, it's USB-based
>>> Bluetooth function.
>>> 
>>> There are some component in the Mediatek driver.
>>> 1. Btmtk_main: it's common code for Mediatek devices,
>>>  such as firmware download, chip initialization,
>>>  state machine handling and etc.
>>> 2. Btmtkusb: it's for usb interface,
>>>  such as usb endpoint enumeration, urb handling and etc.
>>> 
>>> Firstly, we update the common part and usb part for MT7921U.
>>> Secondly, we will add the support MT7921S, it's SDIO-based device.
>>> Finally, we will add the procedure to support uart/pcie interfaces.
>> 
>> create a btmtk.[ch] module like the other vendors did if it makes sense.
>> Otherwise just skip that part for now and get btmtkusb.c driver working. You
>> can later unify between all 3 transports.
>> 
>> I would do the latter since it would first make sense to really see where the
>> common parts are. And I have to be frank, this driver needs massive cleanup. I
>> am not going to accept this tons of copy-and-paste left and right.
>> 
>> Please provide the content of /sys/kernel/debug/usb/devices in the commit
>> message.
>> 
>>> +/* To support dynamic mount of interface can be probed */
>>> +static int btmtk_intf_num = BT_MCU_MINIMUM_INTERFACE_NUM;
>>> +/* To allow g_bdev being sized from btmtk_intf_num setting */
>>> +static struct btmtk_dev **g_bdev;
>> 
>> NO. Period. No global dev instances.
> 
> [Global dev.]
> The global dev is for our state machine that design for error recovery, such as chip reset, memory dump and etc.
> We must to make sure state machine transition that is the same flow for each interfaces (usb/sdio/pcie/uart).
> [Mediatek driver]
> -> Create a dev before interface probe.
> [Linux kernel Bluetooth driver]
> -> Create a dev in interface probe (btusb_probe).
> 
> May we create a global dev before interface probe?

No. Please design things properly. Non of the drivers have global devices.

>>> +
>>> +/**
>>> + * Kernel Module init/exit Functions
>>> + */
>>> +static int __init main_driver_init(void)
>>> +{
>>> +	int ret = 0;
>>> +	int i;
>>> +
>>> +	/* Mediatek Driver Version */
>>> +	BTMTK_INFO("%s: MTK BT Driver Version : %s", __func__, VERSION);
>>> +
>>> +	ret = main_init();
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	for (i = 0; i < btmtk_intf_num; i++)
>>> +		btmtk_set_chip_state(g_bdev[i], BTMTK_STATE_DISCONNECT);
>>> +
>>> +	ret = btmtk_cif_register();
>>> +	if (ret < 0) {
>>> +		BTMTK_ERR("*** USB registration failed(%d)! ***", ret);
>>> +		main_exit();
>>> +		return ret;
>>> +	}
>>> +
>>> +	BTMTK_INFO("%s: Done", __func__);
>>> +	return ret;
>>> +}
>> 
>> NO. Period. Use module_usb_driver() and if you need anything more, you are
>> doing something wrong.
> 
> We would like to unify state machine, dev allocate, hif_hook and hif_register.
> [Unify Common Part]: btmtk_main
> State machine: Mediatek chip error recovery
> Dev allocate: Bluetooth dev.
> Mediatek chip-related behavior: Firmware download.
> HCI device-related: hci register, open, close and send_frame.
> 
> [HIF Part] : btmtkusb/btmtksdio/btmtkuart
> hif_hook (cif interface): read/write register, open/close, chip reset and etc.
> hif_register (cif register): hif registration-related, such as usb_register/sdio_register_driver.
> 
> May we use the driver architecture?

You can do that, but then first start with the existing btmtksdio and btmtkuart drivers to show me how you want to design it. You still have to design it cleanly.

Regards

Marcel