On Thu, Apr 18, 2024 at 12:02:38PM +0800, Baojun Xu wrote: > Main source code for tas2781 driver for SPI. ... > +#ifndef __TAS2781_SPI_H__ > +#define __TAS2781_SPI_H__ > +#include <sound/tas2781-dsp.h> You have the missing inclusions like + types.h // for bool and so on You missed a lot of forward declarations this header better to have. I think I pointed this out already. ... > +/*PAGE Control Register (available in page0 of each book) */ Missing space. ... > +#define TASDEVICE_BOOKCTL_REG 127 > +#define TASDEVICE_BOOK_ID(reg) (reg / (256 * 128)) > +#define TASDEVICE_PAGE_ID(reg) ((reg % (256 * 128)) / 128) > +#define TASDEVICE_PAGE_REG(reg) ((reg % (256 * 128)) % 128) > +#define TASDEVICE_PGRG(reg) (reg % (256 * 128)) > +#define TASDEVICE_REG(book, page, reg) (((book * 256 * 128) + \ > + (page * 128)) + reg) > + > +#define TASDEVICE_MAX_BOOK_NUM 256 > +#define TASDEVICE_MAX_PAGE 128 So, shouldn't the above use these definitions as well? ... > +/*Software Reset */ Missing spaces. Please, fix all problems like this in the entire series. ... > +#define TAS2781_SPI_MAX_FREQ 4000000 (4 * HZ_PER_MHZ) ? Will need units.h IIRC for this. > +#include <linux/acpi.h> + array_size.h > +#include <linux/bits.h> > +#include <linux/crc8.h> > +#include <linux/crc32.h> > +#include <linux/efi.h> > +#include <linux/firmware.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/pm_runtime.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/spi/spi.h> > +#include <linux/time.h> > +#include <linux/types.h> ... > + .range_max = TASDEVICE_MAX_BOOK_NUM * TASDEVICE_MAX_PAGE, > + .selector_reg = TASDEVICE_PAGE_SELECT, > + .selector_mask = 0xff, GENMASK() ? > + .selector_shift = 0, > + .window_start = 0, > + .window_len = 128, Shouldn't these 0xff and 128 be applied thry previously defined constants? I mean are they have a relation to _BOOK_NUM and _MAX_PAGE? ... > +static int tasdevice_spi_switch_book(struct tasdevice_priv *tas_priv, int book) > +{ > + struct tasdevice *tasdev = &tas_priv->tasdevice; > + struct regmap *map = tas_priv->regmap; > + int ret = 0; Redundant assignment, see below > + if (tasdev->cur_book != book) { if (tasdev->cur_book == book) return 0; > + /* Change to page 0 before book change. */ > + ret = regmap_write(map, TASDEVICE_PAGE_SELECT, 0); > + if (ret < 0) { > + dev_err(tas_priv->dev, "%s, E=%d\n", __func__, ret); > + return ret; > + } > + ret = regmap_write(map, TASDEVICE_BOOKCTL_REG, book); > + if (ret < 0) { > + dev_err(tas_priv->dev, "%s, E=%d\n", __func__, ret); > + return ret; > + } > + tasdev->cur_book = book; > + } > + > + return ret; return 0; > +} ... > + ret = regmap_bulk_read(map, TASDEVICE_PGRG(reg), > + buf, len + 1); THis can be one line. ... > + if (tas_dev->reset) { > + gpiod_set_value_cansleep(tas_dev->reset, 0); > + usleep_range(500, 1000); fsleep() > + gpiod_set_value_cansleep(tas_dev->reset, 1); > + } else { > + ret = tasdevice_spi_dev_write(tas_dev, > + TAS2781_REG_SWRESET, TAS2781_REG_SWRESET_RESET); Indentation is broken and seems you have a room for one parameter above. > + if (ret < 0) > + dev_err(tas_dev->dev, "dev sw-reset fail, %d\n", ret); > + } > + usleep_range(1000, 1050); fsleep() ,,, > +{ > + int ret = 0; Redundant assignment. > + /* > + * Codec Lock Hold to ensure that codec_probe and firmware parsing and > + * loading do not simultaneously execute. > + */ > + mutex_lock(&tas_priv->codec_lock); > + > + scnprintf(tas_priv->rca_binaryname, 64, "%sRCA%d.bin", Why 'c' variant as you don't check the return value anyway. And shouldn't it be sizeof() in the second parameter? > + tas_priv->dev_name, tas_priv->index); > + crc8_populate_msb(tas_priv->crc8_lkp_tbl, TASDEVICE_CRC8_POLYNOMIAL); > + tas_priv->codec = codec; > + ret = request_firmware_nowait(module, FW_ACTION_UEVENT, > + tas_priv->rca_binaryname, tas_priv->dev, GFP_KERNEL, tas_priv, > + cont); > + if (ret) > + dev_err(tas_priv->dev, "request_firmware_nowait err:0x%08x\n", > + ret); > + > + /* Codec Lock Release*/ > + mutex_unlock(&tas_priv->codec_lock); Why not using cleanup.h, i.e. guard() and scoped_guard(), from day 1? > + return ret; > +} ... > +static int tasdevice_clamp(int val, int max, unsigned int invert) > +{ > + if (val > max) > + val = max; > + if (invert) > + val = max - val; > + if (val < 0) > + val = 0; > + > + return val; Wouldn't below be the same? return clamp(invert ? max - val : val, 0, max); (if max can be negative the above won't work, but an original code would also give a surprising values AFAICS.) > +} ... > + mask = (1 << fls(max)) - 1; Use respective roundup/down_pow_of_two(). Same in other similar cases. ... > + ret = device_property_count_u32(physdev, property); > + if (ret <= 0) So, when 0 you return 0. Is it expected? > + goto err; ... > + p->index = 0xFF; Why capital? Shouldn't it be rather U8_MAX or something like this? > + for (i = 0; i < nval; i++) { > + if (values[i] == id) { > + p->index = i; > + break; > + } > + } > + if (p->index == 0xFF) { Ditto. > + dev_dbg(p->dev, "No index found in %s\n", property); > + ret = -ENODEV; > + goto err; > + } ... > + p->reset = devm_gpiod_get_index_optional(physdev, > + "reset", p->index, GPIOD_OUT_LOW); Broken indentation. You have plenty of lines like this, please fix them all. > + if (IS_ERR(p->reset)) { > + ret = PTR_ERR(p->reset); > + > + /* > + * If RESET is shared the first amp to probe will grab the > + * reset line and reset all the amps > + */ > + if (ret != -EBUSY) { > + dev_err_probe(p->dev, ret, > + "Failed to get reset GPIO\n"); > + goto err; > + } > + dev_info(p->dev, "Reset GPIO busy, assume shared reset\n"); > + p->reset = NULL; > + } Why can't this use reset framework which supports shared / exclusive resets? ... > + * Manually set the Chip Select for the second amp <cs_gpio_index> > + * in the node. > + * This is only supported for systems with 2 amps, since we cannot > + * expand the default number of chip selects without using cs-gpios Missing period at the end. Btw, SPI has multi-CS support, would it help here? > + * The CS GPIO must be set high prior to communicating with the > + * first amp (which uses a native chip select), to ensure the second > + * amp does not clash with the first. > + if (IS_ENABLED(CONFIG_SPI)) { I would rather prefer to see in spi.h #if IS_ENABLED(CONFIG_SPI) #define dev_is_spi(d) ((d)->bus == &spi_bus_type) #else #define dev_is_spi(d) false #endif and here if (dev_is_spi(...)) { > + spi = to_spi_device(p->dev); > + > + /* > + * This is obtained using driver_gpios, since only one GPIO > + * for CS exists, this can be obtained using index 0. > + */ > + cs_gpiod = devm_gpiod_get_index_optional(physdev, "cs", > + p->index, GPIOD_OUT_LOW); > + if (IS_ERR(cs_gpiod)) { > + dev_err(p->dev, "Unable to get CS GPIO descriptor\n"); > + ret = PTR_ERR(cs_gpiod); > + goto err; > + } > + if (id == 1) { > + spi_set_csgpiod(spi, 0, cs_gpiod); > + p->chipselect = cs_gpiod; > + } else { > + gpiod_set_value_cansleep(cs_gpiod, true); > + gpiod_put(cs_gpiod); How can this be correct in conjunction with devm? > + } > + spi_setup(spi); > + } ... > +static int tasdevice_set_profile_id(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol); > + int max = tas_priv->rcabin.ncfgs - 1; > + int val, ret = 0; Redundant assignment and even entire ret is not needed. > + val = clamp(ucontrol->value.integer.value[0], 0, max); > + Redundant blank line. > + if (tas_priv->rcabin.profile_cfg_id != val) { if (tas_priv->rcabin.profile_cfg_id == val) return 0; > + tas_priv->rcabin.profile_cfg_id = val; > + ret = 1; > + } > + > + return ret; return 1; > +} ... > +static int tasdevice_program_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) As per above function comments. ... > +static int tasdevice_config_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) Ditto. ... > +/* Why not marking it as kernel doc? > + * tas2781_digital_getvol - get the volum control > + * @kcontrol: control pointer > + * @ucontrol: User data + blank line. > + * Customer Kcontrol for tas2781 is primarily for regmap booking, paging > + * depends on internal regmap mechanism. > + * tas2781 contains book and page two-level register map, especially > + * book switching will set the register BXXP00R7F, after switching to the > + * correct book, then leverage the mechanism for paging to access the > + * register. + blank line. + Return: section. > + */ ... > + dev_dbg(tas_priv->dev, "%s : Force FWload %s\n", __func__, > + tas_priv->force_fwload_status ? "ON" : "OFF"); str_on_off() from string_choices.h ... > + dev_dbg(tas_priv->dev, "%s : Force FWload %s\n", __func__, > + tas_priv->force_fwload_status ? "ON" : "OFF"); Ditto. ... > + for (j = 0; j < CALIB_MAX; j++) { > + rc = tasdevice_spi_dev_bulk_write(tas_priv, > + TASDEVICE_REG(0, page_array[j], rgno_array[j]), > + (unsigned char *)&tas_priv->calibration_data[j], 4); > + if (rc < 0) > + dev_err(tas_priv->dev, > + "chn %d calib %d bulk_wr err = %d\n", > + tas_priv->index, j, rc); Non-fatal? How it will work then? > + } > +} ... > + efi_guid_t efi_guid = EFI_GUID(0x02f9af02, 0x7734, 0x4233, 0xb4, 0x3d, > + 0x93, 0xfe, 0x5a, 0xa3, 0x5d, 0xb3); Make it look more standard as three line assignment: efi_guid_t efi_guid = EFI_GUID(0x02f9af02, 0x7734, 0x4233, 0xb4, 0x3d, 0x93, 0xfe, 0x5a, 0xa3, 0x5d, 0xb3); Also add a comment that will have this GUID in human-readable format suitable for googling. ... > + if (status == EFI_BUFFER_TOO_SMALL) { > + /* Allocate data buffer of data_size bytes */ > + data = devm_kzalloc(tas_priv->dev, total_sz, GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + /* Get variable contents into buffer */ > + status = efi.get_variable(efi_name, &efi_guid, &attr, > + &total_sz, data); > + } > + if (status != EFI_SUCCESS) { > + kfree(data); Really you should read and understand what devm means and what is the scope when you can use it. The code is full of double-free bugs. > + return -EINVAL; > + } ... > + if (crc == tmp_val[3+tmp_val[0]*6]) { > + time64_to_tm(tmp_val[2], 0, tm); > + dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n", > + tm->tm_year, tm->tm_mon, tm->tm_mday, > + tm->tm_hour, tm->tm_min, tm->tm_sec); No, use respective %pt. > + for (int i = 0; i < CALIB_MAX; i++) > + tas_priv->calibration_data[i] = > + tmp_val[3 + tas_priv->index * 6 + i]; > + tas_priv->apply_calibration(tas_priv); > + } else { > + total_sz = 0; > + } ... > + if (crc == tmp_val[21]) { > + time64_to_tm(tmp_val[20], 0, tm); > + dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n", > + tm->tm_year, tm->tm_mon, tm->tm_mday, > + tm->tm_hour, tm->tm_min, tm->tm_sec); Ditto. > + for (int i = 0; i < CALIB_MAX; i++) > + tas_priv->calibration_data[i] = > + tmp_val[tas_priv->index * 5 + i]; > + tas_priv->apply_calibration(tas_priv); > + } > + kfree(data); KABOOM! > + > + return 0; > +} ... > + for (int i = ARRAY_SIZE(tas_hda->snd_ctls) - 1; i >= 0; i--) > + if (tas_hda->snd_ctls[i]) > + snd_ctl_remove(codec->card, tas_hda->snd_ctls[i]); Cleanup loops are better in a while (i--) form. unsigned int i = ARRAY_SIZE(tas_hda->snd_ctls); while (i--) { if (tas_hda->snd_ctls[i]) snd_ctl_remove(codec->card, tas_hda->snd_ctls[i]); } BUT, if not yet, just add a support of NULL into snd_ctl_remove(), so it becomes a NULL-aware one. With that you will have while (i--) snd_ctl_remove(codec->card, tas_hda->snd_ctls[i]); as a traditional pattern for many things in Linux kernel. .. > +static const struct dev_pm_ops tas2781_hda_pm_ops = { > + RUNTIME_PM_OPS(tas2781_runtime_suspend, tas2781_runtime_resume, NULL) > + SYSTEM_SLEEP_PM_OPS(tas2781_system_suspend, tas2781_system_resume) > +}; > + > +static const struct spi_device_id tas2781_hda_spi_id[] = { > + { "tas2781-hda", 0 }, ', 0' part is redundant. > + {} > +}; > + > +static const struct acpi_device_id tas2781_acpi_hda_match[] = { > + {"TXNW2781", 0 }, Ditto. > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match); > + > +static struct spi_driver tas2781_hda_spi_driver = { > + .driver = { > + .name = "tas2781-hda", > + .acpi_match_table = tas2781_acpi_hda_match, > + .pm = &tas2781_hda_pm_ops, Use proper pm_*() macro here. > + }, > + .id_table = tas2781_hda_spi_id, > + .probe = tas2781_hda_spi_probe, > + .remove = tas2781_hda_spi_remove, > +}; ... I stop here, you need much more cleaning work on these patches. Also, with cleanup.h and other things in mind you can easily shrink the code by 10% or so, I will expect -300 LoCs in the next version. -- With Best Regards, Andy Shevchenko