Re: [PATCH 2/2] ALSA: hda/tas2563: Add tas2563 HDA driver

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 9677c09cf7a9..1d3e9f77c9d4 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -6770,7 +6770,7 @@ static int comp_match_cs35l41_dev_name(struct device *dev, void *data) return !strcmp(d + n, tmp); }

-static int comp_match_tas2781_dev_name(struct device *dev, +static int comp_match_tas2xxx_dev_name(struct device *dev, void *data) { struct scodec_dev_name *p = data; @@ -6823,7 +6823,7 @@ static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char } }

-static void tas2781_generic_fixup(struct hda_codec *cdc, int action, +static void tas2xxx_generic_fixup(struct hda_codec *cdc, int action, const char *bus, const char *hid) { struct device *dev = hda_codec_dev(cdc); @@ -6841,7 +6841,7 @@ static void tas2781_generic_fixup(struct hda_codec *cdc, int action, rec->index = 0; spec->comps[0].codec = cdc; component_match_add(dev, &spec->match,

• 	comp_match_tas2781_dev_name, rec);
  

• 	comp_match_tas2xxx_dev_name, rec);
  

  ret = component_master_add_with_match(dev, &comp_master_ops, spec->match); if (ret)

@@ -6888,7 +6888,13 @@ static void alc287_fixup_legion_16ithg6_speakers(struct hda_codec *cdc, const st static void tas2781_fixup_i2c(struct hda_codec *cdc, const struct hda_fixup *fix, int action) {

• tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
  

• tas2xxx_generic_fixup(cdc, action, "i2c", "TIAS2781");
  

+}

this sort of rename should be part of a separate patch IMHO, it'd be easier to review.

+static void tas2563_fixup_i2c(struct hda_codec *cdc,

• const struct hda_fixup *fix, int action)

+{

• tas2xxx_generic_fixup(cdc, action, "i2c", "INT8866");
  

Any specific reason to use an Intel ACPI identifier? Why not use "TIAS2563" ?

+#define TAS2563_REG_INIT_N 12

newline

+static const struct reg_default tas2563_reg_init[TAS2563_MAX_CHANNELS]

• [TAS2563_REG_INIT_N] = {
• {

• { TAS2562_TDM_CFG2, 0x5a },
  

• { TAS2562_TDM_CFG4, 0xf3 },
  

• { TAS2562_TDM_CFG5, 0x42 },
  

• { TAS2562_TDM_CFG6, 0x40 },
  

• { TAS2562_BOOST_CFG1, 0xd4 },
  

• { TAS2562_BOOST_CFG3, 0xa4 },
  

• { TAS2562_REG(0x00, 0x36), 0x0b },
  

• { TAS2562_REG(0x00, 0x38), 0x21 },
  

• { TAS2562_REG(0x00, 0x3c), 0x58 },
  

• { TAS2562_BOOST_CFG4, 0xb6 },
  

• { TAS2562_ASI_CONFIG3, 0x04},
  

• { TAS2562_REG(0x00, 0x47), 0xb1 },
  

+/* Update the calibrate data, including speaker impedance, f0, etc, into algo.

update the calibration data,

• • Calibrate data is done by manufacturer in the factory. These data are used

The manufacturer calibrates the data in the factory.

• • by Algo for calucating the speaker temperature, speaker membrance excursion

calculating

membrane

+static int tas2563_hda_i2c_probe(struct i2c_client *client) +{

• struct tas2563_data *tas2563;
• int ret;
• tas2563 = devm_kzalloc(&client->dev, sizeof(struct tas2563_data),

• GFP_KERNEL);
  

• if (!tas2563)

• return -ENOMEM;
  

• tas2563->dev = &client->dev;
• tas2563->client = client;
• dev_set_drvdata(tas2563->dev, tas2563);
• ret = tas2563_read_acpi(tas2563);
• if (ret)

• return dev_err_probe(tas2563->dev, ret,
  

• 	"Platform not supported\n");
  

• for (int i = 0; i < tas2563->ndev; ++i) {

• struct tas2563_dev *tasdev = &tas2563->tasdevs[i];
  

• ret = tas2563_tasdev_read_efi(tas2563, tasdev);
  

• if (ret)
  

• 	return dev_err_probe(tas2563->dev, ret,
  

• 		"Calibration data cannot be read from EFI\n");
  

• ret = tas2563_tasdev_init_client(tas2563, tasdev);
  

• if (ret)
  

• 	return dev_err_probe(tas2563->dev, ret,
  

• 		"Failed to init i2c client\n");
  

• ret = tas2563_tasdev_init_regmap(tas2563, tasdev);
  

• if (ret)
  

• 	return dev_err_probe(tas2563->dev, ret,
  

• 		"Failed to allocate register map\n");
  

• }
• ret = component_add(tas2563->dev, &tas2563_hda_comp_ops);
• if (ret) {

• return dev_err_probe(tas2563->dev, ret,
  

• 	"Register component failed\n");
  

• }

I wonder how many of those tests actually depend on deferred probe, and if this isn't a case of copy-paste "just in case"?

• pm_runtime_set_autosuspend_delay(tas2563->dev, 3000);
• pm_runtime_use_autosuspend(tas2563->dev);
• pm_runtime_mark_last_busy(tas2563->dev);
• pm_runtime_set_active(tas2563->dev);
• pm_runtime_get_noresume(tas2563->dev);
• pm_runtime_enable(tas2563->dev);
• pm_runtime_put_autosuspend(tas2563->dev);

the sequence get_noresume/enable/put_autosuspend makes no sense to me. doing a get_noresume *before* enable should do exactly nothing, and releasing the resource would already be handled with autosuspend based on the last_busy mark.

• return 0;

+}

+static void tas2563_hda_i2c_remove(struct i2c_client *client) +{

• struct tas2563_data *tas2563 = dev_get_drvdata(&client->dev);
• pm_runtime_get_sync(tas2563->dev);
• pm_runtime_disable(tas2563->dev);
• component_del(tas2563->dev, &tas2563_hda_comp_ops);
• pm_runtime_put_noidle(tas2563->dev);

that pm_runtime sequence also makes no sense to me, if you disable pm_runtime the last command is useless/no-op.

+}

+static int tas2563_system_suspend(struct device *dev) +{

• struct tas2563_data *tas2563 = dev_get_drvdata(dev);
• int ret;
• dev_dbg(tas2563->dev, "System Suspend\n");
• ret = pm_runtime_force_suspend(dev);
• if (ret)

• return ret;
  

• return 0;

+}

+static int tas2563_system_resume(struct device *dev) +{

• int ret;
• struct tas2563_data *tas2563 = dev_get_drvdata(dev);
• dev_dbg(tas2563->dev, "System Resume\n");
• ret = pm_runtime_force_resume(dev);
• if (ret)

• return ret;
  

• for (int i = 0; i < tas2563->ndev; ++i)

• tas2563_tasdev_setup(tas2563, &tas2563->tasdevs[i]);
  

• return 0;

+}

+static const struct dev_pm_ops tas2563_hda_pm_ops = {

• SYSTEM_SLEEP_PM_OPS(tas2563_system_suspend, tas2563_system_resume)

where's the pm_runtime stuff?

+};