On Sep 04 2024, Li Zetao wrote:
> Currently, the hid-picolcd module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> that hid resources are consistent with the device life cycle, and release
> hid resources before device is released. At the same time, it can avoid
> the goto-release encoding, drop the err_cleanup_hid_ll and
> err_cleanup_hid_hw lables.
>
> Signed-off-by: Li Zetao <lizetao1@xxxxxxxxxx>
> ---
> drivers/hid/hid-picolcd_core.c | 22 ++++------------------
> 1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 297103be3381..973822d1b2db 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -551,23 +551,15 @@ static int picolcd_probe(struct hid_device *hdev,
> goto err_cleanup_data;
> }
>
> - error = hid_hw_start(hdev, 0);
> + error = devm_hid_hw_start_and_open(hdev, 0);
> if (error) {
> - hid_err(hdev, "hardware start failed\n");
> + hid_err(hdev, "hardware start and open failed\n");
> goto err_cleanup_data;
> }
>
> - error = hid_hw_open(hdev);
> - if (error) {
> - hid_err(hdev, "failed to open input interrupt pipe for key and IR events\n");
> - goto err_cleanup_hid_hw;
> - }
> -
> error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
> - if (error) {
> - hid_err(hdev, "failed to create sysfs attributes\n");
> - goto err_cleanup_hid_ll;
> - }
> + if (error)
> + goto err_cleanup_data;
>
> error = device_create_file(&hdev->dev, &dev_attr_operation_mode);
> if (error) {
> @@ -589,10 +581,6 @@ static int picolcd_probe(struct hid_device *hdev,
> device_remove_file(&hdev->dev, &dev_attr_operation_mode);
> err_cleanup_sysfs1:
> device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
> -err_cleanup_hid_ll:
> - hid_hw_close(hdev);
> -err_cleanup_hid_hw:
> - hid_hw_stop(hdev);
> err_cleanup_data:
> kfree(data);
> return error;
> @@ -611,8 +599,6 @@ static void picolcd_remove(struct hid_device *hdev)
> picolcd_exit_devfs(data);
> device_remove_file(&hdev->dev, &dev_attr_operation_mode);
> device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
> - hid_hw_close(hdev);
> - hid_hw_stop(hdev);
Again, this doesn't seem correct. the spin_lock from below expects the
hw to be stopped, and this patch changes that by postponing the stop
after the remove.
CHeers,
Benjamin
>
> /* Shortcut potential pending reply that will never arrive */
> spin_lock_irqsave(&data->lock, flags);
> --
> 2.34.1
>
