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

[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);

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