Re: [PATCH v1 09/10] staging: dsp: add support for Fortemedia FM34NE DSP

вт, 21 лют. 2023 р. о 21:39 Dan Carpenter error27@gmail.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.