Hi, Krzysztof, Thank you very much for your advice, but I have a few questions I'd like to discuss with you
On 18/07/2023 16:58, krzysztof.kozlowski@linaro.org wrote:
On 17/07/2023 13:58, wangweidong.a@awinic.com wrote:
From: Weidong Wang wangweidong.a@awinic.com
The AW88261 is an I2S/TDM input, high efficiency digital Smart K audio amplifier with an integrated 10.25V smart boost convert
It's the same as in patch before. This does not help and does not justify having one driver split into two.
I will modify the commit information and differentiate the commit information for each file
...
+static void aw_dev_i2s_tx_enable(struct aw_device *aw_dev, bool flag) +{
- int ret;
- if (flag) {
ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCFG1_REG,~AW88261_I2STXEN_MASK, AW88261_I2STXEN_ENABLE_VALUE);- } else {
ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCFG1_REG,~AW88261_I2STXEN_MASK, AW88261_I2STXEN_DISABLE_VALUE);- }
You should not need {} here and in multiple other places.
I will delete {} as suggested
- if (ret)
dev_dbg(aw_dev->dev, "%s failed", __func__);
Why you are not handling the errors properly?
Do you need to use dev_err instead?
+}
+static void aw_dev_pwd(struct aw_device *aw_dev, bool pwd) +{
- int ret;
- if (pwd) {
ret = regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,~AW88261_PWDN_MASK, AW88261_PWDN_POWER_DOWN_VALUE);- } else {
ret = regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,~AW88261_PWDN_MASK, AW88261_PWDN_WORKING_VALUE);- }
- if (ret)
dev_dbg(aw_dev->dev, "%s failed", __func__);+}
...
+int aw88261_dev_fw_update(struct aw_device *aw_dev) +{
- struct aw_prof_desc *prof_index_desc;
- struct aw_sec_data_desc *sec_desc;
- char *prof_name;
- int ret;
- prof_name = aw88261_dev_get_prof_name(aw_dev, aw_dev->prof_index);
- if (!prof_name) {
dev_err(aw_dev->dev, "get prof name failed");return -EINVAL;- }
- dev_dbg(aw_dev->dev, "start update %s", prof_name);
- ret = aw88261_dev_get_prof_data(aw_dev, aw_dev->prof_index, &prof_index_desc);
- if (ret)
return ret;- /* update reg */
- sec_desc = prof_index_desc->sec_desc;
- ret = aw_dev_reg_update(aw_dev, sec_desc[AW88261_DATA_TYPE_REG].data,
sec_desc[AW88261_DATA_TYPE_REG].len);- if (ret) {
dev_err(aw_dev->dev, "update reg failed");return ret;- }
- aw_dev->prof_cur = aw_dev->prof_index;
- return ret;
+} +EXPORT_SYMBOL_GPL(aw88261_dev_fw_update);
Why do you need to export this? Where is the user?
I will delete this export
+int aw88261_dev_reg_update(struct aw_device *aw_dev, bool force) +{
- int ret;
- if (force) {
aw88261_dev_soft_reset(aw_dev);ret = aw88261_dev_fw_update(aw_dev);if (ret < 0)return ret;- } else {
if (aw_dev->prof_cur != aw_dev->prof_index) {ret = aw88261_dev_fw_update(aw_dev);if (ret < 0)return ret;}- }
- aw_dev->prof_cur = aw_dev->prof_index;
- return ret;
+} +EXPORT_SYMBOL_GPL(aw88261_dev_reg_update);
Same question. And in all other places as well.
This function will be called in aw88261.c, can I keep it?
Best regards, Weidong Wang