On Aug 10, 2023, at 1:17 AM, Krzysztof Kozlowski [off-list ref] wrote:

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

quoted

+
+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).

This patch abides by the 100-column line length limit which checkpatch.pl enforces.
However, I can see how some of the lines might be less jarring to the eyes if wrapped.
That will be addressed in V4.

quoted

+ 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.

Disabling IRQ there is a mistake. The probe error path will be tightened up and made
to utilize multiple goto labels instead in V4.

Regards,
James Ogletree