Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASoC: tas2781: Add tas2783 driver #4700

Closed

Conversation

Shenghao-Ding
Copy link

Support both TX and RX.

Signed-off-by: Shenghao Ding <[email protected]>
@sofci
Copy link
Collaborator

sofci commented Nov 13, 2023

Can one of the admins verify this patch?

reply test this please to run this test once

@bardliao
Copy link
Collaborator

@Shenghao-Ding Please split the commit into 1. codec driver 2. The machine driver change (sof_sdw.c) 3. The acpi_mach change (soc-acpi-intel-mtl/tgl-match.c)

@@ -587,6 +611,12 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_mtl_sdw_machines[] = {
.drv_name = "sof_sdw",
.sof_tplg_filename = "sof-mtl-rt713-l0-rt1316-l12.tplg",
},
{
.link_mask = 0xf, //HACK for all sdw links are enabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be BIT(x) in the upstream version.

}
};

static const struct snd_soc_acpi_link_adr tas2783_link1[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename tas2783_link1[] to tas2783_link0[] as the amp is on link 0, and so does tas2783_1_adr[]

@plbossart
Copy link
Member

test this please

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAK. None of my comments on alsa-devel were taken into account, not sure what the expectations are if nothing has changed since the last review. Please be mindful to reviewer fatigue....

* https://www.ti.com
*
* The TAS2783 driver implements a flexible and configurable
* algo coff setting for single TAS2783 chips.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please take review comments into account, it must be the 4th time I write down that 'coff' is a typo for 'coeff' or 'coefficient'


stream = kzalloc(sizeof(*stream), GFP_KERNEL);
if (!stream)
return -ENOMEM;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's also the 3rd time I write down that this is not necessary. see the simplifications done for all other codecs.

I stop the review here since this is the same code that's submitted for review WITHOUT ANY CHANGES

@plbossart
Copy link
Member

Erm, please don't add merge commits in your branch. You need to rebase on top of topic/sof-dev, then use git push --force for any update.

Shenghao-Ding and others added 4 commits November 21, 2023 15:28
Signed-off-by: Shenghao Ding <[email protected]>

---
Change in v2:
 - simplify tasdevice_set_sdw_stream
 - fixed some Linux coding style
 - fixed the spelling mistakes
Signed-off-by: Shenghao Ding <[email protected]>

---
Change in v2:
 - Add machine driver for tas2783
Signed-off-by: Shenghao Ding <[email protected]>

---
Change in v2:
 - modify the 0xf to GENMASK(3, 0)
 - modify tas2783_link1 to tas2783_link0
 - modify tas2783_1_adr to tas2783_0_adr
@plbossart
Copy link
Member

I don't follow how you are using GitHub...

Please remove all merge commits and squash all changes. when you have a new version, please use "git push --force" to make sure we ONLY see the latest contribution - similar to what you would do when sending a patchset upstream on alsa-devel.

mfg_id, part_id, class_id);
else
codec->name = devm_kasprintf(dev, GFP_KERNEL,
"sdw:0:%01x:%04x:%04x:%02x:%01x", link_id,
"sdw:%01x:%04x:%04x:%02x:%01x", link_id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need this change?

@plbossart
Copy link
Member

test this please

@plbossart
Copy link
Member

lots of checkpatch issues @Shenghao-Ding

Fixing them will also deal with quite a few review comments already

@plbossart
Copy link
Member

closing, no updates from TI

@plbossart plbossart closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants