On 26.09.2023 21:06, Stephan Gerhold wrote:
> On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote:
>> On 26.09.2023 18:51, Stephan Gerhold wrote:
>>> Most MSM8916/MSM8939 devices use very similar setups for the modem,
>>> because most of the device-specific details are abstracted by the modem
>>> firmware. There are several definitions (status switches, DAI links
>>> etc) that will be exactly the same for every board.
>>>
>>> Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to
>>> simplify enabling the modem for such devices. By default the
>>> digital/analog codec in the SoC/PMIC is used, but boards can define
>>> additional codecs using the templates for Secondary and Quaternary
>>> MI2S.
>>>
>>> Signed-off-by: Stephan Gerhold <[email protected]>
>>> ---
>> I'd rather see at least one usage so that you aren't introducing
>> effectively non-compiled code..
>>
>
> There are 10 usages in the rest of the patch series.
> Is that enough? :D
>
> IMHO it doesn't make sense to squash this with one of the device
> patches, especially considering several of them are primarily authored
> by others.
I see..
Well, I guess I don't have better counter-arguments, but please
consider this the next time around.
[...]
>>> +&lpass_codec {
>>> + status = "okay";
>>> +};
>> Any reason for it to stay disabled?
>>
>
> You mean in msm8916.dtsi?
Yes
> For the SoC dtsi we don't make assumptions
> what devices use or not. There could be devices that ignore the internal
> codec entirely. If there is nothing connected to the codec lpass_codec
> should not be enabled (e.g. the msm8916-ufi.dtsi devices).
See my reply to patch 5
[...]
>>> + sound_dai_secondary: mi2s-secondary-dai-link {
>>> + link-name = "Secondary MI2S";
>>> + status = "disabled"; /* Needs extra codec configuration */
>> Hmm.. Potential good user of /omit-if-no-ref/?
>>
>
> AFAICT /omit-if-no-ref/ is for phandle references only. Basically it
> would only work if you would somewhere reference the phandle:
>
> list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>;
>
> But this doesn't exist so /omit-if-no-ref/ cannot be used here.
Ahh right, this is the one we don't reference.. Too bad,
would be a nice fit :/
I only see one usage of it though (patch 7), perhaps it could
be kept local to that one?
Konrad
On Tue, Sep 26, 2023 at 10:01:21PM +0200, Konrad Dybcio wrote:
> On 26.09.2023 21:06, Stephan Gerhold wrote:
> > On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote:
> >> On 26.09.2023 18:51, Stephan Gerhold wrote:
> >>> Most MSM8916/MSM8939 devices use very similar setups for the modem,
> >>> because most of the device-specific details are abstracted by the modem
> >>> firmware. There are several definitions (status switches, DAI links
> >>> etc) that will be exactly the same for every board.
> >>>
> >>> Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to
> >>> simplify enabling the modem for such devices. By default the
> >>> digital/analog codec in the SoC/PMIC is used, but boards can define
> >>> additional codecs using the templates for Secondary and Quaternary
> >>> MI2S.
> >>>
> >>> Signed-off-by: Stephan Gerhold <[email protected]>
> >>> ---
> >> I'd rather see at least one usage so that you aren't introducing
> >> effectively non-compiled code..
> >>
> >
> > There are 10 usages in the rest of the patch series.
> > Is that enough? :D
> >
> > IMHO it doesn't make sense to squash this with one of the device
> > patches, especially considering several of them are primarily authored
> > by others.
> I see..
>
> Well, I guess I don't have better counter-arguments, but please
> consider this the next time around.
>
Will do!
> [...]
>
> >>> +&lpass_codec {
> >>> + status = "okay";
> >>> +};
> >> Any reason for it to stay disabled?
> >>
> >
> > You mean in msm8916.dtsi?
> Yes
>
> > For the SoC dtsi we don't make assumptions
> > what devices use or not. There could be devices that ignore the internal
> > codec entirely. If there is nothing connected to the codec lpass_codec
> > should not be enabled (e.g. the msm8916-ufi.dtsi devices).
> See my reply to patch 5
>
> [...]
>
Let's continue discussing that there I guess. :D
> >>> + sound_dai_secondary: mi2s-secondary-dai-link {
> >>> + link-name = "Secondary MI2S";
> >>> + status = "disabled"; /* Needs extra codec configuration */
> >> Hmm.. Potential good user of /omit-if-no-ref/?
> >>
> >
> > AFAICT /omit-if-no-ref/ is for phandle references only. Basically it
> > would only work if you would somewhere reference the phandle:
> >
> > list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>;
> >
> > But this doesn't exist so /omit-if-no-ref/ cannot be used here.
> Ahh right, this is the one we don't reference.. Too bad,
> would be a nice fit :/
>
> I only see one usage of it though (patch 7), perhaps it could
> be kept local to that one?
>
This patch series just contains the initial set of
msm8916-modem-qdsp6.dtsi users (for devices which are already upstream).
We probably have like 20 more that still need to be upstreamed. :D
sound_dai_secondary is fairly rare, but there is at least one more user
that will probably end up upstream soon.
I think the overhead of these template notes is absolutely negligible
compared to all the (potentially) unused SoC nodes we have. :D
Thanks,
Stephan
On 9/26/23 22:17, Stephan Gerhold wrote:
> On Tue, Sep 26, 2023 at 10:01:21PM +0200, Konrad Dybcio wrote:
>> On 26.09.2023 21:06, Stephan Gerhold wrote:
>>> On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote:
>>>> On 26.09.2023 18:51, Stephan Gerhold wrote:
>>>>> Most MSM8916/MSM8939 devices use very similar setups for the modem,
>>>>> because most of the device-specific details are abstracted by the modem
>>>>> firmware. There are several definitions (status switches, DAI links
>>>>> etc) that will be exactly the same for every board.
>>>>>
>>>>> Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to
>>>>> simplify enabling the modem for such devices. By default the
>>>>> digital/analog codec in the SoC/PMIC is used, but boards can define
>>>>> additional codecs using the templates for Secondary and Quaternary
>>>>> MI2S.
>>>>>
>>>>> Signed-off-by: Stephan Gerhold <[email protected]>
>>>>> ---
>>>> I'd rather see at least one usage so that you aren't introducing
>>>> effectively non-compiled code..
>>>>
>>>
>>> There are 10 usages in the rest of the patch series.
>>> Is that enough? :D
>>>
>>> IMHO it doesn't make sense to squash this with one of the device
>>> patches, especially considering several of them are primarily authored
>>> by others.
>> I see..
>>
>> Well, I guess I don't have better counter-arguments, but please
>> consider this the next time around.
>>
>
> Will do!
>
>> [...]
>>
>>>>> +&lpass_codec {
>>>>> + status = "okay";
>>>>> +};
>>>> Any reason for it to stay disabled?
>>>>
>>>
>>> You mean in msm8916.dtsi?
>> Yes
>>
>>> For the SoC dtsi we don't make assumptions
>>> what devices use or not. There could be devices that ignore the internal
>>> codec entirely. If there is nothing connected to the codec lpass_codec
>>> should not be enabled (e.g. the msm8916-ufi.dtsi devices).
>> See my reply to patch 5
>>
>> [...]
>>
>
> Let's continue discussing that there I guess. :D
>
>>>>> + sound_dai_secondary: mi2s-secondary-dai-link {
>>>>> + link-name = "Secondary MI2S";
>>>>> + status = "disabled"; /* Needs extra codec configuration */
>>>> Hmm.. Potential good user of /omit-if-no-ref/?
>>>>
>>>
>>> AFAICT /omit-if-no-ref/ is for phandle references only. Basically it
>>> would only work if you would somewhere reference the phandle:
>>>
>>> list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>;
>>>
>>> But this doesn't exist so /omit-if-no-ref/ cannot be used here.
>> Ahh right, this is the one we don't reference.. Too bad,
>> would be a nice fit :/
>>
>> I only see one usage of it though (patch 7), perhaps it could
>> be kept local to that one?
>>
>
> This patch series just contains the initial set of
> msm8916-modem-qdsp6.dtsi users (for devices which are already upstream).
> We probably have like 20 more that still need to be upstreamed. :D
>
> sound_dai_secondary is fairly rare, but there is at least one more user
> that will probably end up upstream soon.
2 users don't sound particularly great in a devicetree included by 20
other non-users
> I think the overhead of these template notes is absolutely negligible
> compared to all the (potentially) unused SoC nodes we have. :D
Yes, however the unused SoC nodes are mostly standardized and could be
used as-they-are on a vast majority of devices
Konrad
On Mon, Oct 02, 2023 at 11:59:21AM +0200, Konrad Dybcio wrote:
>
>
> On 9/26/23 22:17, Stephan Gerhold wrote:
> > On Tue, Sep 26, 2023 at 10:01:21PM +0200, Konrad Dybcio wrote:
> > > On 26.09.2023 21:06, Stephan Gerhold wrote:
> > > > On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote:
> > > > > On 26.09.2023 18:51, Stephan Gerhold wrote:
> > > > > > Most MSM8916/MSM8939 devices use very similar setups for the modem,
> > > > > > because most of the device-specific details are abstracted by the modem
> > > > > > firmware. There are several definitions (status switches, DAI links
> > > > > > etc) that will be exactly the same for every board.
> > > > > >
> > > > > > Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to
> > > > > > simplify enabling the modem for such devices. By default the
> > > > > > digital/analog codec in the SoC/PMIC is used, but boards can define
> > > > > > additional codecs using the templates for Secondary and Quaternary
> > > > > > MI2S.
> > > > > >
> > > > > > Signed-off-by: Stephan Gerhold <[email protected]>
> > > > > > ---
> > > > > I'd rather see at least one usage so that you aren't introducing
> > > > > effectively non-compiled code..
> > > > >
> > > >
> > > > There are 10 usages in the rest of the patch series.
> > > > Is that enough? :D
> > > >
> > > > IMHO it doesn't make sense to squash this with one of the device
> > > > patches, especially considering several of them are primarily authored
> > > > by others.
> > > I see..
> > >
> > > Well, I guess I don't have better counter-arguments, but please
> > > consider this the next time around.
> > >
> >
> > Will do!
> >
> > > [...]
> > >
> > > > > > +&lpass_codec {
> > > > > > + status = "okay";
> > > > > > +};
> > > > > Any reason for it to stay disabled?
> > > > >
> > > >
> > > > You mean in msm8916.dtsi?
> > > Yes
> > >
> > > > For the SoC dtsi we don't make assumptions
> > > > what devices use or not. There could be devices that ignore the internal
> > > > codec entirely. If there is nothing connected to the codec lpass_codec
> > > > should not be enabled (e.g. the msm8916-ufi.dtsi devices).
> > > See my reply to patch 5
> > >
> > > [...]
> > >
> >
> > Let's continue discussing that there I guess. :D
> >
> > > > > > + sound_dai_secondary: mi2s-secondary-dai-link {
> > > > > > + link-name = "Secondary MI2S";
> > > > > > + status = "disabled"; /* Needs extra codec configuration */
> > > > > Hmm.. Potential good user of /omit-if-no-ref/?
> > > > >
> > > >
> > > > AFAICT /omit-if-no-ref/ is for phandle references only. Basically it
> > > > would only work if you would somewhere reference the phandle:
> > > >
> > > > list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>;
> > > >
> > > > But this doesn't exist so /omit-if-no-ref/ cannot be used here.
> > > Ahh right, this is the one we don't reference.. Too bad,
> > > would be a nice fit :/
> > >
> > > I only see one usage of it though (patch 7), perhaps it could
> > > be kept local to that one?
> > >
> >
> > This patch series just contains the initial set of
> > msm8916-modem-qdsp6.dtsi users (for devices which are already upstream).
> > We probably have like 20 more that still need to be upstreamed. :D
> >
> > sound_dai_secondary is fairly rare, but there is at least one more user
> > that will probably end up upstream soon.
> 2 users don't sound particularly great in a devicetree included by 20 other
> non-users
>
> > I think the overhead of these template notes is absolutely negligible
> > compared to all the (potentially) unused SoC nodes we have. :D
> Yes, however the unused SoC nodes are mostly standardized and could be used
> as-they-are on a vast majority of devices
>
To be fair we're talking about 152 bytes difference here, in a DTB that
is like 60,000 bytes total. But I can't think of enough compelling
arguments for my "template node" approach, so I will try to rework this
in v2. Let's see if I can get rid of the unused nodes without too much
mess. :)
Thanks,
Stephan