2022-06-27 10:11:36

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix typo in DAIFMT handling

On 27/06/2022 11:43, Charles Keepax wrote:
> The conversion of the set_fmt callback to direct clock specification
> included a small typo, correct the affected code.
>
> Fixes: 91c49199e6d6 ("ASoC: samsung: Update to use set_fmt_new callback")

Where is this commit from? It's not in next.

> Reported-by: kernel test robot <[email protected]>

You should put such big patchsets in your own repo (e.g. on
Github/Gitlab) and feed it to linux-next or at least to LKP.

This way you would get build coverage... because it seems the build was
missing in your case.

If you prefer not to include it in LKP or linux-next, no problem, but
then you need to build it.

> Signed-off-by: Charles Keepax <[email protected]>
> ---


Reviewed-by: Krzysztof Kozlowski <[email protected]>


Best regards,
Krzysztof


2022-06-27 12:25:41

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix typo in DAIFMT handling

On Mon, Jun 27, 2022 at 11:49:46AM +0200, Krzysztof Kozlowski wrote:
> On 27/06/2022 11:43, Charles Keepax wrote:
> > The conversion of the set_fmt callback to direct clock specification
> > included a small typo, correct the affected code.

> > Fixes: 91c49199e6d6 ("ASoC: samsung: Update to use set_fmt_new callback")

> Where is this commit from? It's not in next.

0b491c7c1b2555ef08285fd49a8567f2f9f34ff8 - if you can't find something
search for the subject, people often get things wrong.

> You should put such big patchsets in your own repo (e.g. on
> Github/Gitlab) and feed it to linux-next or at least to LKP.

The size of the patch set isn't really relevant here, the same issue can
apply to anything that can be built in more than one configuration.
People should of course try to do things that work but equally we
shouldn't be putting procedural blockers in place, we have integration
trees for a reason.

> This way you would get build coverage... because it seems the build was
> missing in your case.

That coverage has apparently also been missing in -next for several
weeks.


Attachments:
(No filename) (1.09 kB)
signature.asc (499.00 B)
Download all attachments

2022-06-27 12:35:54

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix typo in DAIFMT handling

On 27/06/2022 13:45, Mark Brown wrote:
> On Mon, Jun 27, 2022 at 11:49:46AM +0200, Krzysztof Kozlowski wrote:
>> On 27/06/2022 11:43, Charles Keepax wrote:
>>> The conversion of the set_fmt callback to direct clock specification
>>> included a small typo, correct the affected code.
>
>>> Fixes: 91c49199e6d6 ("ASoC: samsung: Update to use set_fmt_new callback")
>
>> Where is this commit from? It's not in next.
>
> 0b491c7c1b2555ef08285fd49a8567f2f9f34ff8 - if you can't find something
> search for the subject, people often get things wrong.

Finding it by subject does not solve problem with Fixes tag, that it
might be pointing to incorrect commit (e.g. rebased).

>
>> You should put such big patchsets in your own repo (e.g. on
>> Github/Gitlab) and feed it to linux-next or at least to LKP.
>
> The size of the patch set isn't really relevant here, the same issue can
> apply to anything that can be built in more than one configuration.
> People should of course try to do things that work but equally we
> shouldn't be putting procedural blockers in place, we have integration
> trees for a reason.

I would say that size of the patchset is a proof someone is doing bigger
work and we want the bigger work to be tested even before hitting
maintainer's tree.

My comment was not a requirement (procedural blocker) but a suggestion,
because maybe Charles was not aware that developer trees can be tested
for free.

>
>> This way you would get build coverage... because it seems the build was
>> missing in your case.
>
> That coverage has apparently also been missing in -next for several
> weeks.

Eh, it seems defconfigs for this old platform do not select sound, so we
rely on randconfig. :(

Best regards,
Krzysztof

2022-06-27 13:32:09

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix typo in DAIFMT handling

On Mon, Jun 27, 2022 at 02:11:13PM +0200, Krzysztof Kozlowski wrote:
> On 27/06/2022 13:45, Mark Brown wrote:
> > On Mon, Jun 27, 2022 at 11:49:46AM +0200, Krzysztof Kozlowski wrote:

> > 0b491c7c1b2555ef08285fd49a8567f2f9f34ff8 - if you can't find something
> > search for the subject, people often get things wrong.

> Finding it by subject does not solve problem with Fixes tag, that it
> might be pointing to incorrect commit (e.g. rebased).

Sure, but that's an incorrect SHA rather than not being able to find the
commit which is what you said.

> >> This way you would get build coverage... because it seems the build was
> >> missing in your case.

> > That coverage has apparently also been missing in -next for several
> > weeks.

> Eh, it seems defconfigs for this old platform do not select sound, so we
> rely on randconfig. :(

It's not even turning up in an allmodconfig?


Attachments:
(No filename) (910.00 B)
signature.asc (499.00 B)
Download all attachments

2022-06-27 13:36:53

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix typo in DAIFMT handling

On 27/06/2022 15:23, Mark Brown wrote:
>>> That coverage has apparently also been missing in -next for several
>>> weeks.
>
>> Eh, it seems defconfigs for this old platform do not select sound, so we
>> rely on randconfig. :(
>
> It's not even turning up in an allmodconfig?

No, because it is old driver for S3C24xx platform which:
1. Does not have compile test (I can try to fix that),
2. Depends on/Is selected by S3C24xx code which is not multiplatform
thus is not enabled on ARM allyes/allmod.

Best regards,
Krzysztof

2022-06-27 13:56:42

by Charles Keepax

[permalink] [raw]
Subject: Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix typo in DAIFMT handling

On Mon, Jun 27, 2022 at 02:11:13PM +0200, Krzysztof Kozlowski wrote:
> On 27/06/2022 13:45, Mark Brown wrote:
> > On Mon, Jun 27, 2022 at 11:49:46AM +0200, Krzysztof Kozlowski wrote:
> >> On 27/06/2022 11:43, Charles Keepax wrote:
> My comment was not a requirement (procedural blocker) but a suggestion,
> because maybe Charles was not aware that developer trees can be tested
> for free.
>

Would be awesome if I could run things through the build bot
before sending them up. Are there any docs anywhere on how to get
a tree added to that?

Thanks,
Charles

2022-06-27 13:57:05

by Charles Keepax

[permalink] [raw]
Subject: Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix typo in DAIFMT handling

On Mon, Jun 27, 2022 at 02:11:13PM +0200, Krzysztof Kozlowski wrote:
> On 27/06/2022 13:45, Mark Brown wrote:
> > On Mon, Jun 27, 2022 at 11:49:46AM +0200, Krzysztof Kozlowski wrote:
> >> On 27/06/2022 11:43, Charles Keepax wrote:
> >>> The conversion of the set_fmt callback to direct clock specification
> >>> included a small typo, correct the affected code.
> >
> >>> Fixes: 91c49199e6d6 ("ASoC: samsung: Update to use set_fmt_new callback")
> >
> >> Where is this commit from? It's not in next.
> >
> > 0b491c7c1b2555ef08285fd49a8567f2f9f34ff8 - if you can't find something
> > search for the subject, people often get things wrong.
>
> Finding it by subject does not solve problem with Fixes tag, that it
> might be pointing to incorrect commit (e.g. rebased).
>

Apologies I will resend with the SHA corrected there and sorry
about missing the issue in the first place. I was pretty careful
trying to make sure I built everything but somehow missed this
one.

Thanks,
Charles