Messages in this thread | | | Date | Thu, 18 Apr 2024 17:03:14 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 2/3] ALSA: hda/tas2781: Main code of tas2781 driver for SPI |
| |
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
| |