| Message ID | 20230221183211.21964-1-clamor95@gmail.com |
|---|---|
| Headers | show |
| Series | Fix sound on ASUS Transformers | expand |
On Tue, Feb 21, 2023 at 08:32:05PM +0200, Svyatoslav Ryhel wrote: > diff --git a/sound/soc/tegra/tegra_asoc_machine.c b/sound/soc/tegra/tegra_asoc_machine.c > index 78faa8bcae27..607800ec07a6 100644 > --- a/sound/soc/tegra/tegra_asoc_machine.c > +++ b/sound/soc/tegra/tegra_asoc_machine.c > @@ -51,6 +51,17 @@ static struct snd_soc_jack_gpio tegra_machine_headset_jack_gpio = { > }; > > /* Mic Jack */ This comment doesn't make sense now. It was never super useful, though. Just delete it. > +static int headset_check(void *data) > +{ > + struct tegra_machine *machine = (struct tegra_machine *)data; > + > + /* Detect mic insertion only if 3.5 jack is in */ > + if (gpiod_get_value_cansleep(machine->gpiod_hp_det) && > + gpiod_get_value_cansleep(machine->gpiod_mic_det)) > + return SND_JACK_MICROPHONE; > + > + return 0; > +} > > static struct snd_soc_jack tegra_machine_mic_jack; > > @@ -183,8 +194,15 @@ int tegra_asoc_machine_init(struct snd_soc_pcm_runtime *rtd) regards, dan carpenter
On Tue, Feb 21, 2023 at 08:32:05PM +0200, Svyatoslav Ryhel wrote: > Add Realtek ALC5631/RT5631 codec support to the Tegra ASoC machine driver. > The RT5631 codec is found on devices like ASUS Transformer TF201, TF700T > and other Tegra-based Android tablets. > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> > Signed-off-by: Ion Agorria <ion@agorria.com> Your signoff should be last if you're the one sending this. > +static unsigned int tegra_machine_mclk_rate_6mhz(unsigned int srate) > +{ > + unsigned int mclk; > + > + switch (srate) { > + case 64000: > + case 88200: > + case 96000: > + mclk = 128 * srate; > + break; > + default: > + mclk = 256 * srate; > + break; > + } > + /* FIXME: Codec only requires >= 3MHz if OSR==0 */ > + while (mclk < 6000000) > + mclk *= 2; It feels like this is complicated enough and looks like the clocking is flexible enough that it might be easier to just have a table of values or otherwise enumerate standard rates, seeing the code I feel like I need to worry about what happens if we pick a clock rate over 6MHz (the loop could give a value over that), and it's not clear why we have the switch statement rather than just starting at a multiple of 128 and looping an extra time. I suspect there's going to be no meaningful downside for having the clock held at over 3MHz on a tablet form factor, the usual issue would be power consumption but between the larger battery size you tend to have on a tablet and the power draw of the screen if that's on it's likely to be into the noise practially speaking.
вт, 21 лют. 2023 р. о 21:32 Dan Carpenter <error27@gmail.com> пише: > > On Tue, Feb 21, 2023 at 08:32:05PM +0200, Svyatoslav Ryhel wrote: > > diff --git a/sound/soc/tegra/tegra_asoc_machine.c b/sound/soc/tegra/tegra_asoc_machine.c > > index 78faa8bcae27..607800ec07a6 100644 > > --- a/sound/soc/tegra/tegra_asoc_machine.c > > +++ b/sound/soc/tegra/tegra_asoc_machine.c > > @@ -51,6 +51,17 @@ static struct snd_soc_jack_gpio tegra_machine_headset_jack_gpio = { > > }; > > > > /* Mic Jack */ > > This comment doesn't make sense now. It was never super useful, though. > Just delete it. It does. Headset is Mic Jack + Headphones combined. headset_check function performs check for a Mic Jack component in plugged Jack 3.5 > > +static int headset_check(void *data) > > +{ > > + struct tegra_machine *machine = (struct tegra_machine *)data; > > + > > + /* Detect mic insertion only if 3.5 jack is in */ > > + if (gpiod_get_value_cansleep(machine->gpiod_hp_det) && > > + gpiod_get_value_cansleep(machine->gpiod_mic_det)) > > + return SND_JACK_MICROPHONE; > > + > > + return 0; > > +} > > > > static struct snd_soc_jack tegra_machine_mic_jack; > > > > @@ -183,8 +194,15 @@ int tegra_asoc_machine_init(struct snd_soc_pcm_runtime *rtd) > > regards, > dan carpenter
ср, 22 лют. 2023 р. о 00:23 Mark Brown <broonie@kernel.org> пише: > > On Tue, Feb 21, 2023 at 08:32:05PM +0200, Svyatoslav Ryhel wrote: > > > Add Realtek ALC5631/RT5631 codec support to the Tegra ASoC machine driver. > > The RT5631 codec is found on devices like ASUS Transformer TF201, TF700T > > and other Tegra-based Android tablets. > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> > > Signed-off-by: Ion Agorria <ion@agorria.com> > > Your signoff should be last if you're the one sending this. Thanks > > +static unsigned int tegra_machine_mclk_rate_6mhz(unsigned int srate) > > +{ > > + unsigned int mclk; > > + > > + switch (srate) { > > + case 64000: > > + case 88200: > > + case 96000: > > + mclk = 128 * srate; > > + break; > > + default: > > + mclk = 256 * srate; > > + break; > > + } > > + /* FIXME: Codec only requires >= 3MHz if OSR==0 */ > > + while (mclk < 6000000) > > + mclk *= 2; > > It feels like this is complicated enough and looks like the > clocking is flexible enough that it might be easier to just have > a table of values or otherwise enumerate standard rates, seeing > the code I feel like I need to worry about what happens if we > pick a clock rate over 6MHz (the loop could give a value over > that), and it's not clear why we have the switch statement rather > than just starting at a multiple of 128 and looping an extra time. > > I suspect there's going to be no meaningful downside for having > the clock held at over 3MHz on a tablet form factor, the usual > issue would be power consumption but between the larger battery > size you tend to have on a tablet and the power draw of the > screen if that's on it's likely to be into the noise practially > speaking. This is how downstream handled mclk rate for RT5631.
On Wed, Feb 22, 2023 at 10:00:58AM +0200, Svyatoslav Ryhel wrote: > ср, 22 лют. 2023 р. о 00:23 Mark Brown <broonie@kernel.org> пише: > > It feels like this is complicated enough and looks like the > > clocking is flexible enough that it might be easier to just have > > a table of values or otherwise enumerate standard rates, seeing > > the code I feel like I need to worry about what happens if we > > pick a clock rate over 6MHz (the loop could give a value over > > that), and it's not clear why we have the switch statement rather > > than just starting at a multiple of 128 and looping an extra time. > > I suspect there's going to be no meaningful downside for having > > the clock held at over 3MHz on a tablet form factor, the usual > > issue would be power consumption but between the larger battery > > size you tend to have on a tablet and the power draw of the > > screen if that's on it's likely to be into the noise practially > > speaking. > This is how downstream handled mclk rate for RT5631. That doesn't mean it shouldn't be fixed or improved.
On Wed, Feb 22, 2023 at 09:55:52AM +0200, Svyatoslav Ryhel wrote: > вт, 21 лют. 2023 р. о 21:32 Dan Carpenter <error27@gmail.com> пише: > > > > On Tue, Feb 21, 2023 at 08:32:05PM +0200, Svyatoslav Ryhel wrote: > > > diff --git a/sound/soc/tegra/tegra_asoc_machine.c b/sound/soc/tegra/tegra_asoc_machine.c > > > index 78faa8bcae27..607800ec07a6 100644 > > > --- a/sound/soc/tegra/tegra_asoc_machine.c > > > +++ b/sound/soc/tegra/tegra_asoc_machine.c > > > @@ -51,6 +51,17 @@ static struct snd_soc_jack_gpio tegra_machine_headset_jack_gpio = { > > > }; > > > > > > /* Mic Jack */ > > > > This comment doesn't make sense now. It was never super useful, though. > > Just delete it. > > It does. Headset is Mic Jack + Headphones combined. headset_check function > performs check for a Mic Jack component in plugged Jack 3.5 > I feel if we need to discuess what a comment means or if it even means anything then that's a useless comment by definition. regards, dan carpenter
On Wed, Feb 22, 2023 at 04:28:09PM +0300, Dan Carpenter wrote: > On Wed, Feb 22, 2023 at 09:55:52AM +0200, Svyatoslav Ryhel wrote: > > вт, 21 лют. 2023 р. о 21:32 Dan Carpenter <error27@gmail.com> пише: > > > > /* Mic Jack */ > > > This comment doesn't make sense now. It was never super useful, though. > > > Just delete it. > > It does. Headset is Mic Jack + Headphones combined. headset_check function > > performs check for a Mic Jack component in plugged Jack 3.5 > I feel if we need to discuess what a comment means or if it even means > anything then that's a useless comment by definition. If the device doesn't have a distinct mic jack then it's not ideal to talk about there being one (as opposed to the microphone on the headset jack).
On Tue, 21 Feb 2023 20:32:01 +0200, Svyatoslav Ryhel wrote: > - add quirk for headset detection used by some T30 devices > (ASUS Transformers, LG Optimus 4X HD and Vu); > - add RT5631 and MAX9808x machine drivers > - add Fortemedia FM34NE DSP driver used by ASUS Transformers > and mandatory for correct sound work > - bind everything into working configuration > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [02/10] sound: soc: jack: allow multiple interrupt per gpio commit: a2d4051b0bd6dffcd736888ae89a550d6f60b060 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark