All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rob Herring <robh@kernel.org>
To: Bui Duc Phuc <phucduc.bui@gmail.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>, Lee Jones <lee@kernel.org>,
	Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Joseph Chen <chenjh@rock-chips.com>,
	Chris Zhong <zyw@rock-chips.com>,
	Zhang Qing <zhangqing@rock-chips.com>,
	David Rau <David.Rau.opensource@dm.renesas.com>,
	Animesh Agarwal <animeshagarwal28@gmail.com>,
	devicetree@vger.kernel.org, linux-sound@vger.kernel.org,
	linux-rockchip@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] ASoC: dt-bindings: drop redundant wakeup-source definitions
Date: Tue, 5 May 2026 16:10:06 -0500	[thread overview]
Message-ID: <20260505211006.GA3875657-robh@kernel.org> (raw)
In-Reply-To: <CAABR9nHaGBU9KAWJZrnBy8HO0Waccp8CCGmBdKJ3QOEb8WZsXA@mail.gmail.com>

On Thu, Apr 30, 2026 at 12:12:55AM +0700, Bui Duc Phuc wrote:
> Hi,
> 
> > Yes. And the ABI. You cannot have ABI which has an incompatible
> > implementation. IOW, when implementation contradicts the ABI, something
> > is wrong.
> >
> > The question of course if read_bool() is here incompatible. From the
> > actual code point of view, it is compatible, but how it is documented
> > and how it is intended to use: it is not compatible.
> >
> > Also if future schema-kernel-ABI checker gets implemented, the tool
> > might report here a mistake for that reason. read_bool() means property
> > is bool.

It is doubtful such a tool would do per binding type checking given 
types are global and we've avoided making types per binding so far.

> >
> > > If the hardware supports wakeup functionality,
> > > referencing the core schema is sufficient. Hardware description should
> > > not be constrained by the current driver implementation
> > > ( e.g. the use of device_property_read_bool() ).
> > > Bindings should remain stable and generic, while drivers can evolve over time.
> > So you claim that bindings can define property as integer, but drivers
> > can evolve and for example read it as string?
> 
> I see your point regarding the ABI semantics and the intended use of
> read_bool().
> My understanding is that the I2C core currently uses of_property_read_bool()
> for wakeup-source as a presence check, even though it has no way to
> determine in advance whether a specific device will define the property as
> a boolean or a phandle-array in its DTS.

If this was a problem, then it probably would have been fixed already 
because it generates a warning. An I2C device probably isn't all that 
coupled to SoC power states, so boolean is probably always going to be 
used here. But maybe not.

If we do any cleanup here, I think that should have exactly 1 location 
that reads this property instead of 77. Perhaps the driver core can just 
handle everything and drivers don't have to deal with it.

> >From a behavioral point of view, switching to of_property_present() would
> not change anything, but it would better reflect that the driver only checks
> for the existence of the property without assuming its type.
> 
> If the expectation is to strictly follow the binding types, then
> of_property_present()
> seems more appropriate here.
> I can prepare a patch accordingly and send it to the I2C maintainers for
> review and feedback.
> 
> > >
> > > Re-defining the type locally duplicates the core definition. If the
> > > core schema evolves,
> >
> > There is no re-definition here. This is choice of subset of types.
> 
> The flow is: core schema → YAML binding (ABI) → DTS (actual usage).
> >From a high-level perspective, the binding may appear to redefine the
> property type by narrowing its scope, while the DTS selects one valid
> representation from the permitted set.
> 
> > Where is Rob's suggestion to do such cleanups for EXISTING code? I only
> > see that new code should come like that.
> >
> > Anyway, your commit msg is for me incorrect because it misses all this
> > points I made. Whether the schema code is correct, I'll defer to Rob,
> > although I still claim the same I claimed before at v2 or v3 of your
> > previous work - this should have defined type.
> 
> You are right that Rob did not explicitly request cleanups to existing code.
> I included them to improve consistency while working on the new parts,
> but for now, I will stop modifying the existing code and wait for
> Rob's feedback.
> Since there are differing perspectives, I would appreciate a consensus
> on the preferred
> approach before I send out the next version to avoid redundant rework.

It's really outside the scope of a device binding what is used for 
wakeup-source as that depends on the platform. So I think just 
'wakeup-source: true' in device bindings is fine. I don't think we have 
to change all the existing cases either, but the patches are already 
written so I don't have an issue applying them.

Rob


WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <robh@kernel.org>
To: Bui Duc Phuc <phucduc.bui@gmail.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>, Lee Jones <lee@kernel.org>,
	Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Joseph Chen <chenjh@rock-chips.com>,
	Chris Zhong <zyw@rock-chips.com>,
	Zhang Qing <zhangqing@rock-chips.com>,
	David Rau <David.Rau.opensource@dm.renesas.com>,
	Animesh Agarwal <animeshagarwal28@gmail.com>,
	devicetree@vger.kernel.org, linux-sound@vger.kernel.org,
	linux-rockchip@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] ASoC: dt-bindings: drop redundant wakeup-source definitions
Date: Tue, 5 May 2026 16:10:06 -0500	[thread overview]
Message-ID: <20260505211006.GA3875657-robh@kernel.org> (raw)
In-Reply-To: <CAABR9nHaGBU9KAWJZrnBy8HO0Waccp8CCGmBdKJ3QOEb8WZsXA@mail.gmail.com>

On Thu, Apr 30, 2026 at 12:12:55AM +0700, Bui Duc Phuc wrote:
> Hi,
> 
> > Yes. And the ABI. You cannot have ABI which has an incompatible
> > implementation. IOW, when implementation contradicts the ABI, something
> > is wrong.
> >
> > The question of course if read_bool() is here incompatible. From the
> > actual code point of view, it is compatible, but how it is documented
> > and how it is intended to use: it is not compatible.
> >
> > Also if future schema-kernel-ABI checker gets implemented, the tool
> > might report here a mistake for that reason. read_bool() means property
> > is bool.

It is doubtful such a tool would do per binding type checking given 
types are global and we've avoided making types per binding so far.

> >
> > > If the hardware supports wakeup functionality,
> > > referencing the core schema is sufficient. Hardware description should
> > > not be constrained by the current driver implementation
> > > ( e.g. the use of device_property_read_bool() ).
> > > Bindings should remain stable and generic, while drivers can evolve over time.
> > So you claim that bindings can define property as integer, but drivers
> > can evolve and for example read it as string?
> 
> I see your point regarding the ABI semantics and the intended use of
> read_bool().
> My understanding is that the I2C core currently uses of_property_read_bool()
> for wakeup-source as a presence check, even though it has no way to
> determine in advance whether a specific device will define the property as
> a boolean or a phandle-array in its DTS.

If this was a problem, then it probably would have been fixed already 
because it generates a warning. An I2C device probably isn't all that 
coupled to SoC power states, so boolean is probably always going to be 
used here. But maybe not.

If we do any cleanup here, I think that should have exactly 1 location 
that reads this property instead of 77. Perhaps the driver core can just 
handle everything and drivers don't have to deal with it.

> >From a behavioral point of view, switching to of_property_present() would
> not change anything, but it would better reflect that the driver only checks
> for the existence of the property without assuming its type.
> 
> If the expectation is to strictly follow the binding types, then
> of_property_present()
> seems more appropriate here.
> I can prepare a patch accordingly and send it to the I2C maintainers for
> review and feedback.
> 
> > >
> > > Re-defining the type locally duplicates the core definition. If the
> > > core schema evolves,
> >
> > There is no re-definition here. This is choice of subset of types.
> 
> The flow is: core schema → YAML binding (ABI) → DTS (actual usage).
> >From a high-level perspective, the binding may appear to redefine the
> property type by narrowing its scope, while the DTS selects one valid
> representation from the permitted set.
> 
> > Where is Rob's suggestion to do such cleanups for EXISTING code? I only
> > see that new code should come like that.
> >
> > Anyway, your commit msg is for me incorrect because it misses all this
> > points I made. Whether the schema code is correct, I'll defer to Rob,
> > although I still claim the same I claimed before at v2 or v3 of your
> > previous work - this should have defined type.
> 
> You are right that Rob did not explicitly request cleanups to existing code.
> I included them to improve consistency while working on the new parts,
> but for now, I will stop modifying the existing code and wait for
> Rob's feedback.
> Since there are differing perspectives, I would appreciate a consensus
> on the preferred
> approach before I send out the next version to avoid redundant rework.

It's really outside the scope of a device binding what is used for 
wakeup-source as that depends on the platform. So I think just 
'wakeup-source: true' in device bindings is fine. I don't think we have 
to change all the existing cases either, but the patches are already 
written so I don't have an issue applying them.

Rob

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

  reply	other threads:[~2026-05-05 21:10 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-23  4:28 [PATCH 1/2] dt-bindings: mfd: rockchip: drop redundant wakeup-source definitions phucduc.bui
2026-04-23  4:28 ` phucduc.bui
2026-04-23  4:28 ` [PATCH 2/2] ASoC: dt-bindings: " phucduc.bui
2026-04-23  4:28   ` phucduc.bui
2026-04-23  9:19   ` Krzysztof Kozlowski
2026-04-23  9:19     ` Krzysztof Kozlowski
2026-04-26 23:40     ` Bui Duc Phuc
2026-04-26 23:40       ` Bui Duc Phuc
2026-04-28  8:33       ` Krzysztof Kozlowski
2026-04-28  8:33         ` Krzysztof Kozlowski
2026-04-29 17:12         ` Bui Duc Phuc
2026-04-29 17:12           ` Bui Duc Phuc
2026-05-05 21:10           ` Rob Herring [this message]
2026-05-05 21:10             ` Rob Herring
2026-05-11  4:22             ` Bui Duc Phuc
2026-05-11  4:22               ` Bui Duc Phuc
2026-04-23  9:18 ` [PATCH 1/2] dt-bindings: mfd: rockchip: " Krzysztof Kozlowski
2026-04-23  9:18   ` Krzysztof Kozlowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260505211006.GA3875657-robh@kernel.org \
    --to=robh@kernel.org \
    --cc=David.Rau.opensource@dm.renesas.com \
    --cc=animeshagarwal28@gmail.com \
    --cc=broonie@kernel.org \
    --cc=chenjh@rock-chips.com \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=heiko@sntech.de \
    --cc=krzk+dt@kernel.org \
    --cc=krzk@kernel.org \
    --cc=lee@kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-sound@vger.kernel.org \
    --cc=phucduc.bui@gmail.com \
    --cc=zhangqing@rock-chips.com \
    --cc=zyw@rock-chips.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.