Re: [PATCH 10/12] bluetooth: hci_wilc: add wilc hci driver

[Krzysztof Kozlowski]

On Wed, Feb 12, 2025 at 04:46:29PM +0100, Alexis Lothoré wrote:
> +#include "linux/bitops.h"
> +#include "linux/byteorder/generic.h"
> +#include "linux/err.h"
> +#include "linux/gfp_types.h"
> +#include "net/bluetooth/bluetooth.h"
> +#include "net/bluetooth/hci.h"

Keep some order here. Why some are <> some "", why net is mixed with
linux...

> +#include <linux/module.h>
> +#include <linux/firmware.h>
> +#include <linux/of.h>
> +#include <linux/serdev.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +#include <net/wilc.h>
> +
> +#include "hci_uart.h"
> +

...

> +static const struct hci_uart_proto wilc_bt_proto = {
> +	.id = HCI_UART_WILC,
> +	.name = "Microchip",
> +	.manufacturer = WILC_BT_UART_MANUFACTURER,
> +	.init_speed = WILC_UART_DEFAULT_BAUDRATE,
> +	.open = wilc_open,
> +	.close = wilc_close,
> +	.flush = wilc_flush,
> +	.recv = wilc_recv,
> +	.enqueue = wilc_enqueue,
> +	.dequeue = wilc_dequeue,
> +	.setup = wilc_setup,
> +	.set_baudrate = wilc_set_baudrate,
> +};
> +
> +static int wilc_bt_serdev_probe(struct serdev_device *serdev)
> +{
> +	struct wilc_adapter *wilc_adapter;
> +	struct device_node *wlan_node;
> +	void *wlan = NULL;
> +	int ret;
> +
> +	wilc_adapter = kzalloc(sizeof(*wilc_adapter), GFP_KERNEL);

Why not devm?

> +	if (!wilc_adapter)
> +		return -ENOMEM;
> +
> +	wlan_node = of_parse_phandle(serdev->dev.of_node, "wlan", 0);
> +	if (!wlan_node) {
> +		BT_ERR("Can not run wilc bluetooth without wlan node");
> +		ret = -EINVAL;
> +		goto exit_free_adapter;
> +	}
> +
> +#if IS_ENABLED(CONFIG_WILC1000_SDIO)
> +	wlan = wilc_sdio_get_byphandle(wlan_node);
> +#endif
> +#if IS_ENABLED(CONFIG_WILC1000_SPI)
> +	if (!wlan || wlan == ERR_PTR(-EPROBE_DEFER))
> +		wlan = wilc_spi_get_byphandle(wlan_node);
> +#endif
> +	if (IS_ERR(wlan)) {
> +		pr_warn("Can not initialize bluetooth: %pe\n", wlan);

dev_warn or even dev_err_probe to handle deferral.


> +		ret = PTR_ERR(wlan);
> +		goto exit_put_wlan_node;
> +	}
> +
> +	of_node_put(wlan_node);
> +	wilc_adapter->wlan_priv = wlan;
> +	ret = wilc_bt_init(wlan);
> +	if (ret) {
> +		pr_err("Failed to initialize bluetooth firmware (%d)\n", ret);

dev_err_probe

> +		goto exit_put_wlan;
> +	}
> +
> +	wilc_adapter->dev = &serdev->dev;
> +	wilc_adapter->hu.serdev = serdev;
> +	wilc_adapter->flow_control = false;
> +	serdev_device_set_drvdata(serdev, wilc_adapter);
> +	ret = hci_uart_register_device(&wilc_adapter->hu, &wilc_bt_proto);
> +	if (ret) {
> +		dev_err(&serdev->dev, "Failed to register hci device");
> +		goto exit_deinit_bt;
> +	}
> +
> +	dev_info(&serdev->dev, "WILC hci interface registered");

Drop simple probe statuses. sysfs already provides this.


> +	return 0;
> +
> +exit_deinit_bt:
> +	wilc_bt_shutdown(wlan);
> +exit_put_wlan:
> +	wilc_put(wlan);
> +exit_put_wlan_node:
> +	of_node_put(wlan_node);
> +exit_free_adapter:
> +	kfree(wilc_adapter);
> +	return ret;
> +}
> +
> +static void wilc_bt_serdev_remove(struct serdev_device *serdev)
> +{
> +	struct wilc_adapter *wilc_adapter = serdev_device_get_drvdata(serdev);
> +
> +	hci_uart_unregister_device(&wilc_adapter->hu);
> +	wilc_bt_shutdown(wilc_adapter->wlan_priv);
> +	wilc_put(wilc_adapter->wlan_priv);
> +	kfree(wilc_adapter);
> +}
> +
> +static const struct of_device_id wilc_bt_of_match[] = {
> +	{ .compatible = "microchip,wilc3000-bt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, wilc_bt_of_match);
> +
> +static struct serdev_device_driver wilc_bt_serdev_driver = {
> +	.probe = wilc_bt_serdev_probe,
> +	.remove = wilc_bt_serdev_remove,
> +	.driver = {
> +		.name = "hci_uart_wilc",
> +		.of_match_table = of_match_ptr(wilc_bt_of_match),

Drop of_match_tr, you have warnings here.

Best regards,
Krzysztof