Hi Marcel,
Thanks for your review
>> +
>> + ret = kstrtol(buf, 10, &result);
>> + if (ret)
>> + return ret;
>
>I wonder why not use strtobool and check for the result being true.
>
Sure. We will change this in updated version.
>> +
>> + if (result)
>> + btmrvl_firmware_dump(priv);
>> +
>> + return count;
>> +}
>> +
>> +static const struct file_operations btmrvl_fwdump_fops = {
>> + .write = btmrvl_fwdump_write,
>> + .open = simple_open,
>> + .llseek = default_llseek,
>> +};
>> +
>> void btmrvl_debugfs_init(struct hci_dev *hdev) {
>> struct btmrvl_private *priv = hci_get_drvdata(hdev); @@ -197,6
>+225,8
>> @@ void btmrvl_debugfs_init(struct hci_dev *hdev)
>> priv, &btmrvl_hscmd_fops);
>> debugfs_create_file("hscfgcmd", 0644, dbg->config_dir,
>> priv, &btmrvl_hscfgcmd_fops);
>> + debugfs_create_file("fw_dump", 0444, dbg->config_dir,
>> + priv, &btmrvl_fwdump_fops);
>
>Shouldn't this be 0200 actually? Since you can only write to it, right?
>
Right.
>> int (*hw_process_int_status) (struct btmrvl_private *priv);
>> + void (*firmware_dump)(struct btmrvl_private *priv);
>
>While we can do this as a first step to get this merged, but I wonder if
>we might want to make this generic and just add a hdev->firmware_dump
>that drivers then can implement.
>
>We could actually add an option to Controller Configuration of the
>management interface and add a command to trigger the firmware dump.
>Something to think about. However just starting with a debugfs trigger
>via Bluetooth core might be a good idea no matter what. That why more
>drivers could start looking implementing this without having to
>duplicate the trigger.
>
Sounds good to me.
Regards,
Amitkumar
--
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
