Re: [PATCH v3 2/3] ALSA: hda/tas2781: Main code of tas2781 driver for SPI
From: Andy Shevchenko
Date: Thu Apr 18 2024 - 10:03:37 EST
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