On 09/08/2023 21:10, James Ogletree wrote:

Introduce support for Cirrus Logic Device CS40L50: a
haptics driver with waveform memory DSP and closed-loop
algorithms.

Signed-off-by: James Ogletree <redacted>

...

+
+static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
+{
+	cs40l50->dsp.num = 1;
+	cs40l50->dsp.type = WMFW_HALO;
+	cs40l50->dsp.dev = cs40l50->dev;
+	cs40l50->dsp.regmap = cs40l50->regmap;
+	cs40l50->dsp.base = CS40L50_DSP1_CORE_BASE;
+	cs40l50->dsp.base_sysinfo = CS40L50_DSP1_SYS_INFO_ID;
+	cs40l50->dsp.mem = cs40l50_dsp_regions;
+	cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
+	cs40l50->dsp.lock_regions = 0xFFFFFFFF;
+	cs40l50->dsp.no_core_startstop = true;
+	cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
+
+	return cs_dsp_halo_init(&cs40l50->dsp);
+}
+
+int cs40l50_probe(struct cs40l50_private *cs40l50)
+{
+	int error, i, irq;
+	u32 val;
+
+	mutex_init(&cs40l50->lock);
+
+	error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
+			cs40l50_supplies);
+	if (error)
+		return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
+
+	error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
+	if (error)
+		return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
+
+	cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);

None of the lines above or below seem to be wrapped according to Linux
coding style (80).

+	if (IS_ERR(cs40l50->reset_gpio)) {
+		error = PTR_ERR(cs40l50->reset_gpio);
+		goto err;

Why do you disable IRQ now?

I asked to test this. Your entire cleanup path is:
1. not tested.
2. buggy.
3. done differently than Linux style. Use proper cleanup calls and
multiple goto labels.

+	}
+
+	usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
+
+	gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
+
+	usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
+
+	for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
+		error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
+		if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
+			break;
+
+		usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
+	}
+
+	if (i == CS40L50_DSP_TIMEOUT_COUNT) {
+		dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
+		goto err;
+	}
+
+	cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
+	if (!cs40l50->vibe_workqueue) {
+		error = -ENOMEM;
+		goto err;
+	}
+
+	INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
+	INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
+
+	error = cs40l50_cs_dsp_init(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_input_init(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_patch_firmware(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_pseq_init(cs40l50);
+	if (error)
+		goto err;
+
+	error = cs40l50_config_bst(cs40l50);
+	if (error)
+		goto err;
+
+	error = devm_regmap_add_irq_chip(cs40l50->dev, cs40l50->regmap, cs40l50->irq,
+			IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW, 0, &cs40l50_regmap_irq_chip,
+			&cs40l50->irq_data);
+	if (error) {
+		dev_err(cs40l50->dev, "Failed to register IRQ chip: %d\n", error);
+		goto err;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
+		irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
+		if (irq < 0) {
+			dev_err(cs40l50->dev, "Failed to get %s\n", cs40l50_irqs[i].name);
+			error = irq;
+			goto err;
+		}
+
+		error = devm_request_threaded_irq(cs40l50->dev, irq, NULL, cs40l50_irqs[i].handler,
+				IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW,
+						cs40l50_irqs[i].name, cs40l50);
+		if (error) {
+			dev_err(cs40l50->dev, "Failed to request IRQ %s: %d\n",
+					cs40l50_irqs[i].name, error);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	cs40l50_remove(cs40l50);
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(cs40l50_probe);
+
+int cs40l50_remove(struct cs40l50_private *cs40l50)
+{
+	disable_irq(cs40l50->irq);
+
+	if (cs40l50->dsp.booted)
+		cs_dsp_power_down(&cs40l50->dsp);
+	if (&cs40l50->dsp)
+		cs_dsp_remove(&cs40l50->dsp);
+
+	if (cs40l50->vibe_workqueue) {
+		flush_workqueue(cs40l50->vibe_workqueue);
+		destroy_workqueue(cs40l50->vibe_workqueue);
+	}
+
+	gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cs40l50_remove);
+
+MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
+MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. [off-list ref]");

Nothing improved here. Don't send new version immediately after
responding to my comment, but allow discussion.

Best regards,
Krzysztof