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 <james.ogletree@xxxxxxxxxx>
...
> +
> +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. <james.ogletree@xxxxxxxxxx>");
Nothing improved here. Don't send new version immediately after
responding to my comment, but allow discussion.
Best regards,
Krzysztof
