| 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
| ||
|
Message-ID: <CAPVz0n3T-SiTbDyQTTE4L==OGkpSQrfBAP5gRjEevt4ozZLB-g@mail.gmail.com>
Date: Wed, 22 Feb 2023 10:06:23 +0200
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Dan Carpenter <error27@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Maxim Schwalm <maxim.schwalm@...il.com>,
Dmitry Osipenko <digetx@...il.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
linux-tegra@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH v1 09/10] staging: dsp: add support for Fortemedia FM34NE DSP
вт, 21 лют. 2023 р. о 21:39 Dan Carpenter <error27@...il.com> пише:
>
> On Tue, Feb 21, 2023 at 08:32:10PM +0200, Svyatoslav Ryhel wrote:
> > +static int fm34ne_dsp_set_config(struct fm34ne_dsp_data *fm34, int state)
> > +{
> > + struct device *dev = &fm34->client->dev;
> > +
> > + const u8 *enable_ns_parameter = fm34->data->enable_noise_suppression;
> > + int enable_ns_length = fm34->data->enable_ns_length;
> > +
> > + const u8 *disable_ns_parameter = fm34->data->disable_noise_suppression;
> > + int disable_ns_length = fm34->data->disable_ns_length;
> > +
> > + int ret;
> > +
> > + gpiod_set_value_cansleep(fm34->bypass_gpio, 1);
> > + msleep(20);
> > +
> > + switch (state) {
> > + case FM34NE_NS_ENABLE:
> > + ret = fm34ne_dsp_write_config(fm34->client, enable_parameter,
> > + sizeof(enable_parameter));
> > + if (ret < 0) {
> > + dev_err(dev, "failed to set DSP enable with %d\n", ret);
> > + goto exit;
> > + }
> > +
> > + ret = fm34ne_dsp_write_config(fm34->client, enable_ns_parameter,
> > + enable_ns_length);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to enable DSP noise suppression with %d\n", ret);
> > + goto exit;
> > + }
> > +
> > + dev_info(dev, "noise suppression enable DSP parameter written\n");
>
> Delete this type of debug code.
>
> > + break;
> > +
> > + case FM34NE_NS_DISABLE:
> > + ret = fm34ne_dsp_write_config(fm34->client, enable_parameter,
> > + sizeof(enable_parameter));
> > + if (ret < 0) {
> > + dev_err(dev, "failed to set DSP enable with %d\n", ret);
> > + goto exit;
> > + }
> > +
> > + ret = fm34ne_dsp_write_config(fm34->client, disable_ns_parameter,
> > + disable_ns_length);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to disable DSP noise suppression with %d\n", ret);
> > + goto exit;
> > + }
> > +
> > + dev_info(dev, "noise suppression disable DSP parameter written\n");
>
> Same etc.
>
> > + break;
> > +
> > + case FM34NE_BYPASS:
> > + default:
> > + ret = fm34ne_dsp_write_config(fm34->client, bypass_parameter,
> > + sizeof(bypass_parameter));
> > + if (ret < 0) {
> > + dev_err(dev, "failed to set DSP bypass with %d\n", ret);
> > + goto exit;
> > + }
> > +
> > + dev_info(dev, "bypass DSP parameter written\n");
> > + break;
> > + }
> > +
> > +exit:
> > + gpiod_set_value_cansleep(fm34->bypass_gpio, 0);
> > +
> > + return ret;
> > +}
> > +
> > +static int fm34ne_dsp_set_hw(struct fm34ne_dsp_data *fm34)
> > +{
> > + struct device *dev = &fm34->client->dev;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(fm34->dap_mclk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable the DSP MCLK: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = regulator_enable(fm34->vdd_supply);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to enable vdd power supply\n");
>
> clk_disable_unprepare(fm34->dap_mclk);
No, dap_mclk has to be on, else there will be no sound on the device.
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void fm34ne_dsp_reset(struct fm34ne_dsp_data *fm34)
> > +{
> > + gpiod_set_value_cansleep(fm34->reset_gpio, 1);
> > + msleep(20);
> > +
> > + gpiod_set_value_cansleep(fm34->reset_gpio, 0);
> > + msleep(100);
> > +}
> > +
> > +static int fm34ne_dsp_init_chip(struct fm34ne_dsp_data *fm34)
> > +{
> > + const u8 *input_parameter = fm34->data->input_parameter;
> > + int input_parameter_length = fm34->data->input_parameter_length;
> > + int ret;
> > +
> > + ret = fm34ne_dsp_set_hw(fm34);
> > + if (ret)
> > + return ret;
> > +
> > + fm34ne_dsp_reset(fm34);
> > +
> > + gpiod_set_value_cansleep(fm34->bypass_gpio, 1);
> > + msleep(20);
> > +
> > + ret = i2c_smbus_write_byte(fm34->client, FM34NE_I2C_CHECK);
> > + if (ret < 0) {
> > + dev_info(&fm34->client->dev, "initial write failed\n");
>
> dev_warn()?
>
> > + msleep(50);
> > +
> > + fm34ne_dsp_reset(fm34);
> > + gpiod_set_value_cansleep(fm34->bypass_gpio, 0);
> > +
> > + return ret;
> > + }
> > +
> > + ret = fm34ne_dsp_write_config(fm34->client, input_parameter,
> > + input_parameter_length);
> > + if (ret < 0)
> > + return -EINVAL;
>
> Why not propagate the error code from fm34ne_dsp_write_config()?
>
> > +
> > + msleep(100);
> > + gpiod_set_value_cansleep(fm34->bypass_gpio, 0);
> > +
> > + dev_info(&fm34->client->dev, "%s detected\n", fm34->data->model);
> > +
> > + /* Constantly set DSP to bypass mode for now */
> > + ret = fm34ne_dsp_set_config(fm34, FM34NE_BYPASS);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
>
> regards,
> dan carpenter
All other notes are reasonable. Thanks
Best regards,
Svyatoslav R.
Powered by blists - more mailing lists