All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chris Morgan <macroalpha82@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: alsa-devel@alsa-project.org, lgirdwood@gmail.com,
	pierre-louis.bossart@linux.intel.com, tiwai@suse.com,
	heiko@sntech.de, lee.jones@linaro.org, robh+dt@kernel.org,
	perex@perex.cz, jbx6244@gmail.com, devicetree@vger.kernel.org,
	linux-rockchip@lists.infradead.org, maccraft123mc@gmail.com,
	Chris Morgan <macromorgan@hotmail.com>
Subject: Re: [PATCH v10 2/4] ASoC: Add Rockchip rk817 audio CODEC support
Date: Fri, 14 May 2021 13:33:24 -0500	[thread overview]
Message-ID: <20210514183324.GA20917@wintermute.localdomain> (raw)
In-Reply-To: <20210514174958.GC6516@sirena.org.uk>

On Fri, May 14, 2021 at 06:49:58PM +0100, Mark Brown wrote:
> On Fri, May 14, 2021 at 12:19:38PM -0500, Chris Morgan wrote:
> 
> > +static int rk817_codec_parse_dt_property(struct device *dev,
> > +					 struct rk817_codec_priv *rk817)
> > +{
> > +	struct device_node *node = dev->parent->of_node;
> > +
> > +	if (!node) {
> > +		dev_err(dev, "%s() dev->parent->of_node is NULL\n",
> > +			__func__);
> > +		return -ENODEV;
> > +	}
> 
> There's no need to fail the probe here, you won't be able to read any DT
> properties but that shouldn't stop the driver binding.

If I'm not mistaken this is actually telling us to fail if the parent
device (the PMIC itself) isn't present. I think I'll remove this as not
necessary since if the parent node isn't present the mfd driver will
never load, meaning this driver will never load either.

Below this line however we're failing if the codec node isn't present.
Are you telling me we want to bind the driver if the node isn't present
(such as in the edge case where the driver is present and the PMIC is a
rk817, but the CODEC is not in use)? I will remove the return code if
you think that is what needs to be done. My concern there though is if
we do that we'll either be in a position to again report a bunch of
errors for the edge case of users who want to use the PMIC but not the
codec (in this case missing clocks and whatnot) if we try to bind the
driver and the user doesn't want it. I can also set those errors to
debug level, but I think that they might be important enough for users
who DO want to use the codec to keep them as dev_err.

Let me know what you think.

Thank you.

WARNING: multiple messages have this Message-ID (diff)
From: Chris Morgan <macroalpha82@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org, heiko@sntech.de,
	devicetree@vger.kernel.org, tiwai@suse.com, lgirdwood@gmail.com,
	linux-rockchip@lists.infradead.org, robh+dt@kernel.org,
	Chris Morgan <macromorgan@hotmail.com>,
	jbx6244@gmail.com, lee.jones@linaro.org, maccraft123mc@gmail.com
Subject: Re: [PATCH v10 2/4] ASoC: Add Rockchip rk817 audio CODEC support
Date: Fri, 14 May 2021 13:33:24 -0500	[thread overview]
Message-ID: <20210514183324.GA20917@wintermute.localdomain> (raw)
In-Reply-To: <20210514174958.GC6516@sirena.org.uk>

On Fri, May 14, 2021 at 06:49:58PM +0100, Mark Brown wrote:
> On Fri, May 14, 2021 at 12:19:38PM -0500, Chris Morgan wrote:
> 
> > +static int rk817_codec_parse_dt_property(struct device *dev,
> > +					 struct rk817_codec_priv *rk817)
> > +{
> > +	struct device_node *node = dev->parent->of_node;
> > +
> > +	if (!node) {
> > +		dev_err(dev, "%s() dev->parent->of_node is NULL\n",
> > +			__func__);
> > +		return -ENODEV;
> > +	}
> 
> There's no need to fail the probe here, you won't be able to read any DT
> properties but that shouldn't stop the driver binding.

If I'm not mistaken this is actually telling us to fail if the parent
device (the PMIC itself) isn't present. I think I'll remove this as not
necessary since if the parent node isn't present the mfd driver will
never load, meaning this driver will never load either.

Below this line however we're failing if the codec node isn't present.
Are you telling me we want to bind the driver if the node isn't present
(such as in the edge case where the driver is present and the PMIC is a
rk817, but the CODEC is not in use)? I will remove the return code if
you think that is what needs to be done. My concern there though is if
we do that we'll either be in a position to again report a bunch of
errors for the edge case of users who want to use the PMIC but not the
codec (in this case missing clocks and whatnot) if we try to bind the
driver and the user doesn't want it. I can also set those errors to
debug level, but I think that they might be important enough for users
who DO want to use the codec to keep them as dev_err.

Let me know what you think.

Thank you.

WARNING: multiple messages have this Message-ID (diff)
From: Chris Morgan <macroalpha82@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: alsa-devel@alsa-project.org, lgirdwood@gmail.com,
	pierre-louis.bossart@linux.intel.com, tiwai@suse.com,
	heiko@sntech.de, lee.jones@linaro.org, robh+dt@kernel.org,
	perex@perex.cz, jbx6244@gmail.com, devicetree@vger.kernel.org,
	linux-rockchip@lists.infradead.org, maccraft123mc@gmail.com,
	Chris Morgan <macromorgan@hotmail.com>
Subject: Re: [PATCH v10 2/4] ASoC: Add Rockchip rk817 audio CODEC support
Date: Fri, 14 May 2021 13:33:24 -0500	[thread overview]
Message-ID: <20210514183324.GA20917@wintermute.localdomain> (raw)
In-Reply-To: <20210514174958.GC6516@sirena.org.uk>

On Fri, May 14, 2021 at 06:49:58PM +0100, Mark Brown wrote:
> On Fri, May 14, 2021 at 12:19:38PM -0500, Chris Morgan wrote:
> 
> > +static int rk817_codec_parse_dt_property(struct device *dev,
> > +					 struct rk817_codec_priv *rk817)
> > +{
> > +	struct device_node *node = dev->parent->of_node;
> > +
> > +	if (!node) {
> > +		dev_err(dev, "%s() dev->parent->of_node is NULL\n",
> > +			__func__);
> > +		return -ENODEV;
> > +	}
> 
> There's no need to fail the probe here, you won't be able to read any DT
> properties but that shouldn't stop the driver binding.

If I'm not mistaken this is actually telling us to fail if the parent
device (the PMIC itself) isn't present. I think I'll remove this as not
necessary since if the parent node isn't present the mfd driver will
never load, meaning this driver will never load either.

Below this line however we're failing if the codec node isn't present.
Are you telling me we want to bind the driver if the node isn't present
(such as in the edge case where the driver is present and the PMIC is a
rk817, but the CODEC is not in use)? I will remove the return code if
you think that is what needs to be done. My concern there though is if
we do that we'll either be in a position to again report a bunch of
errors for the edge case of users who want to use the PMIC but not the
codec (in this case missing clocks and whatnot) if we try to bind the
driver and the user doesn't want it. I can also set those errors to
debug level, but I think that they might be important enough for users
who DO want to use the codec to keep them as dev_err.

Let me know what you think.

Thank you.

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

  reply	other threads:[~2021-05-14 18:33 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14 17:19 [PATCH v10 0/4] ASoC: codecs: add rk817 support Chris Morgan
2021-05-14 17:19 ` Chris Morgan
2021-05-14 17:19 ` Chris Morgan
2021-05-14 17:19 ` [PATCH v10 1/4] mfd: Add Rockchip rk817 audio CODEC support Chris Morgan
2021-05-14 17:19   ` Chris Morgan
2021-05-14 17:19   ` Chris Morgan
2021-05-14 17:19 ` [PATCH v10 2/4] ASoC: " Chris Morgan
2021-05-14 17:19   ` Chris Morgan
2021-05-14 17:19   ` Chris Morgan
2021-05-14 17:49   ` Mark Brown
2021-05-14 17:49     ` Mark Brown
2021-05-14 17:49     ` Mark Brown
2021-05-14 18:33     ` Chris Morgan [this message]
2021-05-14 18:33       ` Chris Morgan
2021-05-14 18:33       ` Chris Morgan
2021-05-14 19:58       ` Mark Brown
2021-05-14 19:58         ` Mark Brown
2021-05-14 19:58         ` Mark Brown
2021-05-14 21:04         ` Chris Morgan
2021-05-14 21:04           ` Chris Morgan
2021-05-14 21:04           ` Chris Morgan
2021-05-14 17:19 ` [PATCH v10 3/4] dt-bindings: " Chris Morgan
2021-05-14 17:19   ` Chris Morgan
2021-05-14 17:19   ` Chris Morgan
2021-05-14 17:19 ` [PATCH v10 4/4] arm64: dts: rockchip: add rk817 codec to Odroid Go Chris Morgan
2021-05-14 17:19   ` Chris Morgan
2021-05-14 17:19   ` Chris Morgan

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=20210514183324.GA20917@wintermute.localdomain \
    --to=macroalpha82@gmail.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=heiko@sntech.de \
    --cc=jbx6244@gmail.com \
    --cc=lee.jones@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=maccraft123mc@gmail.com \
    --cc=macromorgan@hotmail.com \
    --cc=perex@perex.cz \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=robh+dt@kernel.org \
    --cc=tiwai@suse.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.