On Tue, Feb 21, 2023 at 09:55:41PM +0530, Neeraj Sanjay Kale wrote:
> + bt_dev_info(hdev, "Set UART break: %s, status=%d",
> + ps_state == PS_STATE_AWAKE ? "off" : "on", status);
You have a lot of "noise" in this driver, remove all "info" messages, as
if a driver is working properly, it is quiet.
> + break;
> + }
> + psdata->ps_state = ps_state;
> + if (ps_state == PS_STATE_AWAKE)
> + btnxpuart_tx_wakeup(nxpdev);
> +}
> +
> +static void ps_work_func(struct work_struct *work)
> +{
> + struct ps_data *data = container_of(work, struct ps_data, work);
> +
> + if (!data)
> + return;
You did not test this, that check can never happen, please do not do
pointless checks.
> +
> + if (data->ps_cmd == PS_CMD_ENTER_PS && data->cur_psmode == PS_MODE_ENABLE)
> + ps_control(data->hdev, PS_STATE_SLEEP);
> + else if (data->ps_cmd == PS_CMD_EXIT_PS)
> + ps_control(data->hdev, PS_STATE_AWAKE);
> +}
> +
> +static void ps_timeout_func(struct timer_list *t)
> +{
> + struct ps_data *data = from_timer(data, t, ps_timer);
> + struct hci_dev *hdev = data->hdev;
> + struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> + data->timer_on = false;
> + if (test_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state)) {
> + ps_start_timer(nxpdev);
> + } else {
> + data->ps_cmd = PS_CMD_ENTER_PS;
> + schedule_work(&data->work);
> + }
> +}
> +
> +static int ps_init_work(struct hci_dev *hdev)
> +{
> + struct ps_data *psdata = kzalloc(sizeof(*psdata), GFP_KERNEL);
Don't do allocations in variable declarations :(
> + } else if (req_len == sizeof(uart_config)) {
> + uart_config.clkdiv.address = __cpu_to_le32(CLKDIVADDR);
> + uart_config.clkdiv.value = __cpu_to_le32(0x00c00000);
> + uart_config.uartdiv.address = __cpu_to_le32(UARTDIVADDR);
> + uart_config.uartdiv.value = __cpu_to_le32(1);
> + uart_config.mcr.address = __cpu_to_le32(UARTMCRADDR);
> + uart_config.mcr.value = __cpu_to_le32(MCR);
> + uart_config.re_init.address = __cpu_to_le32(UARTREINITADDR);
> + uart_config.re_init.value = __cpu_to_le32(INIT);
> + uart_config.icr.address = __cpu_to_le32(UARTICRADDR);
> + uart_config.icr.value = __cpu_to_le32(ICR);
> + uart_config.fcr.address = __cpu_to_le32(UARTFCRADDR);
> + uart_config.fcr.value = __cpu_to_le32(FCR);
> + uart_config.crc = swab32(nxp_fw_dnld_update_crc(0UL,
> + (char *)&uart_config,
> + sizeof(uart_config) - 4));
> + serdev_device_write_buf(nxpdev->serdev, (u8 *)&uart_config, req_len);
> + serdev_device_wait_until_sent(nxpdev->serdev, 0);
You are sending magic commands over the serial connection, are you sure
that is ok?
> + if (requested_len & 0x01) {
> + /* The CRC did not match at the other end.
> + * Simply send the same bytes again.
> + */
> + requested_len = nxpdev->fw_v1_sent_bytes;
> + bt_dev_err(hdev, "CRC error. Resend %d bytes of FW.", requested_len);
Why is this an error sent to the kernel log?
Again, be quiet if there is nothing that a user can do.
thanks,
greg k-h
