linux-kernel - Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Aug 2023 15:56:14 +0000
From:   James Ogletree <James.Ogletree@...rus.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
CC:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Fred Treven <Fred.Treven@...rus.com>,
        Ben Bright <Ben.Bright@...rus.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, Lee Jones <lee@...nel.org>,
        Jeff LaBundy <jeff@...undy.com>, Joel Stanley <joel@....id.au>,
        Arnd Bergmann <arnd@...db.de>, Jacky Bai <ping.bai@....com>,
        Jean Delvare <jdelvare@...e.de>,
        Eddie James <eajames@...ux.ibm.com>,
        Markus Schneider-Pargmann <msp@...libre.com>,
        ChiYuan Huang <cy_huang@...htek.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        "patches@...rus.com" <patches@...rus.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic
 CS40L50



> On Aug 10, 2023, at 1:17 AM, Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> wrote:
> 
> On 09/08/2023 21:10, James Ogletree wrote:
> 
>> +
>> +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.

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