[PATCH v10 2/4] ASoC: Add Rockchip rk817 audio CODEC support

[Chris Morgan]

On Fri, May 14, 2021 at 08:58:35PM +0100, Mark Brown wrote:
> On Fri, May 14, 2021 at 01:33:24PM -0500, Chris Morgan wrote:
> > On Fri, May 14, 2021 at 06:49:58PM +0100, Mark Brown wrote:
> 
> > > > +	if (!node) {
> > > > +		dev_err(dev, "%s() dev->parent->of_node is NULL\n",
> > > > +			__func__);
> 
> > > 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.
> 
> So it is - I edited incorrectly when I went to reply.
> 
> > 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
> 
> Yes.
> 
> > 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
> 
> Why would there be any errors?

There won't be here. I'm thinking of the edge case where a user has
this driver but doesn't want to use this hardware again, but I'm
getting scatterbrained and thinking overall and not in the context of
this one section.

Once I remove these lines the last place for an error to occur is when
fetching/activating the mclk in the main probe function. A user who is
trying to use this hardware would want to get an error associated with
a missing clock or one that couldn't be activated, whereas a user who
does not want to use this hardware won't care. How do you think I
should handle that?

I'm assuming if the clock isn't present or working that's where we'd
want to stop the driver, since without the clock it's useless. I'm
thinking if the clock is not present we should simply exit out and drop
a dev_dbg message to aid in troubleshooting when someone wants to use
this hardware but forgets to define their clock. However, if either the
clock is present but fails to activate or the codec fails to register,
that should probably give an actual error message (and still prevent
the driver from binding successfully).

So I'll clean up the rk817_codec_parse_dt_property to not check for a
parent node (useless), and remove the return values from it since
neither of the remaining conditions should cause the driver to fail to
probe. I'll also remove checking for the device parent in
rk817_platform_probe, since its' not necessary. If you think it's the
best solution I'll then change the clock dev_err to a dev_dbg, and
leave everything else the same.

Thank you.