| Message ID | 20220620101402.2684366-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: Intel: haswell and broadwell boards update | expand |
On 6/20/22 05:13, Cezary Rojewski wrote: > A number of patches improving overall quality and readability of > haswell.c and broadwell.c source files found in sound/soc/intel/boards. > Both files are first renamed and only then actual changes are being > incrementally added. The respective names are: hsw_rt5640 and bdw_rt286 > to match the pattern found in more recent boards. > > Most patches bring no functional change - the more impactful patches at > are placed the end: > > Refactor of suspend/resume flow for the bdw_rt286 board by dropping > dev->remove() in favour of card->remove() and adjust jack handling to > reduce code size slightly by implementing card_set_jack(). > > The last patch is removing of FE DAI ops. Given the existence of > platform FE DAI capabilities (either static declaration or through > topology file), this code is redundant. Possibly a mistake in our tests, but this error seems to be introduced: [ 107.397637] kernel: rt286 i2c-INT343A:00: ASoC: DAPM unknown pin LDO1 I'll have to re-run the tests, sharing this information as is. > Changes in v4: > - just a rebase to fix missed conflicts with for-next > > Changes in v3: > - Patch 16/17 refactoring suspend/resume has been renamed to "Refactor > jack handling". Dropped the usage of card->remove() in favor of > link->exit() in that very patch > > Changes in v2: > - fixed wording error in patch 02/17 so it correctly mentions > 'haswell_rt5640', not 'broadwell_rt286' > - decided not to add kernel module names changes to this patchset so the > review is not complicated unnecessarily. Will send them separately > instead > > Cezary Rojewski (17): > ASoC: Intel: Rename haswell source file to hsw_rt5640 > ASoC: Intel: hsw_rt5640: Reword prefixes of all driver members > ASoC: Intel: hsw_rt5640: Reword driver name > ASoC: Intel: hsw_rt5640: Update code indentation > ASoC: Intel: hsw_rt5640: Update file comments > ASoC: Intel: hsw_rt5640: Improve probe() function quality > ASoC: Intel: hsw_rt5640: Improve hw_params() debug-ability > ASoC: Intel: Rename broadwell source file to bdw_rt286 > ASoC: Intel: bdw_rt286: Reword prefixes of all driver members > ASoC: Intel: bdw_rt286: Reword driver name > ASoC: Intel: bdw_rt286: Update code indentation > ASoC: Intel: bdw_rt286: Update file comments > ASoC: Intel: bdw_rt286: Improve probe() function quality > ASoC: Intel: bdw_rt286: Improve hw_params() debug-ability > ASoC: Intel: bdw_rt286: Improve codec_init() quality > ASoC: Intel: bdw_rt286: Refactor jack handling > ASoC: Intel: bdw_rt286: Remove FE DAI ops > > sound/soc/intel/boards/Kconfig | 4 +- > sound/soc/intel/boards/Makefile | 4 +- > sound/soc/intel/boards/bdw_rt286.c | 256 +++++++++++++ > sound/soc/intel/boards/broadwell.c | 338 ------------------ > sound/soc/intel/boards/haswell.c | 202 ----------- > sound/soc/intel/boards/hsw_rt5640.c | 176 +++++++++ > .../common/soc-acpi-intel-hsw-bdw-match.c | 6 +- > 7 files changed, 439 insertions(+), 547 deletions(-) > create mode 100644 sound/soc/intel/boards/bdw_rt286.c > delete mode 100644 sound/soc/intel/boards/broadwell.c > delete mode 100644 sound/soc/intel/boards/haswell.c > create mode 100644 sound/soc/intel/boards/hsw_rt5640.c >
On 2022-06-21 6:36 PM, Pierre-Louis Bossart wrote: > On 6/20/22 05:13, Cezary Rojewski wrote: >> A number of patches improving overall quality and readability of >> haswell.c and broadwell.c source files found in sound/soc/intel/boards. >> Both files are first renamed and only then actual changes are being >> incrementally added. The respective names are: hsw_rt5640 and bdw_rt286 >> to match the pattern found in more recent boards. >> >> Most patches bring no functional change - the more impactful patches at >> are placed the end: >> >> Refactor of suspend/resume flow for the bdw_rt286 board by dropping >> dev->remove() in favour of card->remove() and adjust jack handling to >> reduce code size slightly by implementing card_set_jack(). >> >> The last patch is removing of FE DAI ops. Given the existence of >> platform FE DAI capabilities (either static declaration or through >> topology file), this code is redundant. > > Possibly a mistake in our tests, but this error seems to be introduced: > > [ 107.397637] kernel: rt286 i2c-INT343A:00: ASoC: DAPM unknown pin LDO1 > > I'll have to re-run the tests, sharing this information as is. Hello, Thanks for the report! However, this has been reported earlier during the v2 review [1]. This is also why a fix have been provided [2] earlier today. Notice that shape of link->exit() found here is shared by other Intel boards e.g.: SOF ones. In general, the initial discussion regarding card->remove() revealed some 'probe vs remove' problems within the framework. [1]: https://lore.kernel.org/alsa-devel/69e4263a-e036-cb21-2360-55b06600911e@intel.com/ [2]: https://lore.kernel.org/alsa-devel/1cff4ac0-6d45-95e1-ed9f-6abaded3f8b7@intel.com/T/#t Regards, Czarek
On 6/21/22 12:47, Cezary Rojewski wrote: > On 2022-06-21 6:36 PM, Pierre-Louis Bossart wrote: >> On 6/20/22 05:13, Cezary Rojewski wrote: >>> A number of patches improving overall quality and readability of >>> haswell.c and broadwell.c source files found in sound/soc/intel/boards. >>> Both files are first renamed and only then actual changes are being >>> incrementally added. The respective names are: hsw_rt5640 and bdw_rt286 >>> to match the pattern found in more recent boards. >>> >>> Most patches bring no functional change - the more impactful patches at >>> are placed the end: >>> >>> Refactor of suspend/resume flow for the bdw_rt286 board by dropping >>> dev->remove() in favour of card->remove() and adjust jack handling to >>> reduce code size slightly by implementing card_set_jack(). >>> >>> The last patch is removing of FE DAI ops. Given the existence of >>> platform FE DAI capabilities (either static declaration or through >>> topology file), this code is redundant. >> >> Possibly a mistake in our tests, but this error seems to be introduced: >> >> [ 107.397637] kernel: rt286 i2c-INT343A:00: ASoC: DAPM unknown pin LDO1 >> >> I'll have to re-run the tests, sharing this information as is. > > > Hello, > > Thanks for the report! However, this has been reported earlier during > the v2 review [1]. This is also why a fix have been provided [2] earlier > today. Notice that shape of link->exit() found here is shared by other > Intel boards e.g.: SOF ones. In general, the initial discussion > regarding card->remove() revealed some 'probe vs remove' problems within > the framework. > > > [1]: > https://lore.kernel.org/alsa-devel/69e4263a-e036-cb21-2360-55b06600911e@intel.com/ > > [2]: > https://lore.kernel.org/alsa-devel/1cff4ac0-6d45-95e1-ed9f-6abaded3f8b7@intel.com/T/#t It's rather difficult to follow these changes and error reports buried in email report sent on a Sunday of a three-day week-end for me. I also had additional errors not reported, [ 36.125113] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin HV [ 36.125128] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin VREF [ 36.125130] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin LDO1 [ 36.125921] kernel: rt286 i2c-INT343A:00: ASoC: DAPM unknown pin LDO1 it's unclear to me why a dailink change in a machine driver would cause such codec-side issues. If the changes in this 17-patch series need to be tied to a framework fix, you have to make the dependencies explicit and better yet provide a self-contained patch series that does not introduce a temporary regression, or introduce the framework change first and clearly describe the dependency in a longer Broadwell-specific patchset. This is an 8-yr old device, it shouldn't be that hard.
On 2022-06-21 11:11 PM, Pierre-Louis Bossart wrote: > On 6/21/22 12:47, Cezary Rojewski wrote: >> Hello, >> >> Thanks for the report! However, this has been reported earlier during >> the v2 review [1]. This is also why a fix have been provided [2] earlier >> today. Notice that shape of link->exit() found here is shared by other >> Intel boards e.g.: SOF ones. In general, the initial discussion >> regarding card->remove() revealed some 'probe vs remove' problems within >> the framework. >> >> >> [1]: >> https://lore.kernel.org/alsa-devel/69e4263a-e036-cb21-2360-55b06600911e@intel.com/ >> >> [2]: >> https://lore.kernel.org/alsa-devel/1cff4ac0-6d45-95e1-ed9f-6abaded3f8b7@intel.com/T/#t > > It's rather difficult to follow these changes and error reports buried > in email report sent on a Sunday of a three-day week-end for me. > I also had additional errors not reported, > > [ 36.125113] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin HV > [ 36.125128] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin VREF > [ 36.125130] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin LDO1 > [ 36.125921] kernel: rt286 i2c-INT343A:00: ASoC: DAPM unknown pin LDO1 > > it's unclear to me why a dailink change in a machine driver would cause > such codec-side issues. > > If the changes in this 17-patch series need to be tied to a framework > fix, you have to make the dependencies explicit and better yet provide a > self-contained patch series that does not introduce a temporary > regression, or introduce the framework change first and clearly describe > the dependency in a longer Broadwell-specific patchset. This is an 8-yr > old device, it shouldn't be that hard. The last part is not helpful in solving the problem. This reply comments 00/17 whereas in fact you are speaking solely about 16/17. Because of that I'm suggesting: leave that patch (the 16/17 one) out when merging. It will be send later once link->exit() issue is dealt with. All other patches are independent of either of changes. Simultaneously the link->exit() fix, which is the fruit of this discussion, is still valid and can be send as standalone patch - what is already the case [1]. [1]: https://lore.kernel.org/alsa-devel/20220621115758.3154933-1-cezary.rojewski@intel.com/ Regards, Czarek
>>> Thanks for the report! However, this has been reported earlier during >>> the v2 review [1]. This is also why a fix have been provided [2] earlier >>> today. Notice that shape of link->exit() found here is shared by other >>> Intel boards e.g.: SOF ones. In general, the initial discussion >>> regarding card->remove() revealed some 'probe vs remove' problems within >>> the framework. >>> >>> >>> [1]: >>> https://lore.kernel.org/alsa-devel/69e4263a-e036-cb21-2360-55b06600911e@intel.com/ >>> >>> >>> [2]: >>> https://lore.kernel.org/alsa-devel/1cff4ac0-6d45-95e1-ed9f-6abaded3f8b7@intel.com/T/#t >>> >> >> It's rather difficult to follow these changes and error reports buried >> in email report sent on a Sunday of a three-day week-end for me. >> I also had additional errors not reported, >> >> [ 36.125113] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin HV >> [ 36.125128] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin VREF >> [ 36.125130] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin LDO1 >> [ 36.125921] kernel: rt286 i2c-INT343A:00: ASoC: DAPM unknown pin LDO1 >> >> it's unclear to me why a dailink change in a machine driver would cause >> such codec-side issues. >> >> If the changes in this 17-patch series need to be tied to a framework >> fix, you have to make the dependencies explicit and better yet provide a >> self-contained patch series that does not introduce a temporary >> regression, or introduce the framework change first and clearly describe >> the dependency in a longer Broadwell-specific patchset. This is an 8-yr >> old device, it shouldn't be that hard. > > > The last part is not helpful in solving the problem. > > This reply comments 00/17 whereas in fact you are speaking solely about > 16/17. Because of that I'm suggesting: leave that patch (the 16/17 one) > out when merging. It will be send later once link->exit() issue is dealt > with. All other patches are independent of either of changes. That's fine with me. It wasn't self-explanatory from this cover letter or your earlier answer that this patch 16 can be dropped for now. If that patch is omitted, feel free to add my Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Simultaneously the link->exit() fix, which is the fruit of this > discussion, is still valid and can be send as standalone patch - what is > already the case [1]. That's fine as well. What I was arguing on is the relationship between patchsets and dependencies, what you are suggesting is perfectly acceptable.
On 2022-06-22 8:55 PM, Pierre-Louis Bossart wrote: >>> I also had additional errors not reported, >>> >>> [ 36.125113] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin HV >>> [ 36.125128] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin VREF >>> [ 36.125130] kernel: rt286 i2c-INT343A:00: ASoC: unknown pin LDO1 >>> [ 36.125921] kernel: rt286 i2c-INT343A:00: ASoC: DAPM unknown pin LDO1 (save) > That's fine as well. What I was arguing on is the relationship between > patchsets and dependencies, what you are suggesting is perfectly acceptable. I agree. Frankly I was not aware about the dependencies myself - it wasn't done on purpose. Your report above (notice that there is more than just the LDO1 line) made me analyze our tree once more. Turned out that fixes related to Realtek codecs sent by Amadeo last week [1] address the problem and that's why I had not noticed anything. Will work on the codec series and see what's needed or not given Mark's review and then come back to the bdw_rt286 jack-handling subject. Thanks! [1]: https://lore.kernel.org/alsa-devel/20220609133541.3984886-1-amadeuszx.slawinski@linux.intel.com/ Regards, Czarek
On Mon, 20 Jun 2022 12:13:45 +0200, Cezary Rojewski wrote: > A number of patches improving overall quality and readability of > haswell.c and broadwell.c source files found in sound/soc/intel/boards. > Both files are first renamed and only then actual changes are being > incrementally added. The respective names are: hsw_rt5640 and bdw_rt286 > to match the pattern found in more recent boards. > > Most patches bring no functional change - the more impactful patches at > are placed the end: > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [01/17] ASoC: Intel: Rename haswell source file to hsw_rt5640 commit: 8b99e24de3fae72ff5ef38832b94b1e41059eeed [02/17] ASoC: Intel: hsw_rt5640: Reword prefixes of all driver members commit: 675002b6ca9132445e340bd106297d584e44fc9a [03/17] ASoC: Intel: hsw_rt5640: Reword driver name commit: a69615e81709da0ff1f035886e8b3faf6125cd22 [04/17] ASoC: Intel: hsw_rt5640: Update code indentation commit: 5b66dde4ada531c1a2417d8daf68004067932a19 [05/17] ASoC: Intel: hsw_rt5640: Update file comments commit: 2c53debbbf04eb40854fa33813514828fa455783 [06/17] ASoC: Intel: hsw_rt5640: Improve probe() function quality commit: 0439f262a9b39734c1440733850969f0342c50c3 [07/17] ASoC: Intel: hsw_rt5640: Improve hw_params() debug-ability commit: 6c65908251edc637b53bdeb9e79d918a8d081183 [08/17] ASoC: Intel: Rename broadwell source file to bdw_rt286 commit: 6d8758f6afd91cced9c6c5571337a5fbc6955bb2 [09/17] ASoC: Intel: bdw_rt286: Reword prefixes of all driver members commit: 40b5c9030a87e97c00c84403902481deadd2a57b [10/17] ASoC: Intel: bdw_rt286: Reword driver name commit: 86156bcbca08ee32d04ca56c57ff3fce6fc5fc4b [11/17] ASoC: Intel: bdw_rt286: Update code indentation commit: 9de833d2dcd43c953f7869f27bffd41896adb425 [12/17] ASoC: Intel: bdw_rt286: Update file comments commit: 128bb6fb530841348ee4d9b4234b30006c44c803 [13/17] ASoC: Intel: bdw_rt286: Improve probe() function quality commit: 9177203c209d9137dce52c7f0bc28e54960e5a41 [14/17] ASoC: Intel: bdw_rt286: Improve hw_params() debug-ability commit: 423cc2d0e8506a0ce6e3ef1806a561de1076e033 [15/17] ASoC: Intel: bdw_rt286: Improve codec_init() quality commit: 8fe4709962d74a19c0c1dfc877ba600101340c62 [17/17] ASoC: Intel: bdw_rt286: Remove FE DAI ops commit: e7f68863545163ec75b6bc3cc48fe888c28e0ec6 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