Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function

[Mark Brown]

On Tue, May 25, 2021 at 11:02:16PM +0100, Phillip Potter wrote:
> On Tue, May 25, 2021 at 10:38:45PM +0100, Mark Brown wrote:

> > Phillip, please follow the standard patch submission process,
> > this is documented in submitting-paches.rst in the kernel tree.
> > In particular please make sure that you copy the relevant

> This patch was submitted to a closed mentoring group as part of the
> University of Minnesota reversion/checking process. I was not
> responsible for the final send out to the public mailing lists etc. as
> the patches were collated first and sent out together en masse.

OK, this is really unfortunate.

> > This comment is not accurate, rt5645_remove() just resets the
> > hardware - it's not going to clean up anything to do with any of
> > the branches to error you've got above.  The core *will* clean up

> My comment was adjusted after submission for brevity's sake. This was
> what I originally wrote:
>  /*
>   * All of the above is cleaned up when we return an error here, as
>   * the caller will notice the error and invoke rt5645_remove and
>   * other cleanup routines, as it does for the snd_soc_dapm_* calls
>   * above as well.
>   */
> Happy to resubmit/rewrite as needed? Based on what you've written
> though it may be better to drop the patch?

That is a lot better yes, it accurately reflects what was going
on - the review definitely wasn't helping here.

> > Also I'm guessing this was done purely through inspection rather
> > than the code having been tested?  If there was a problem seen at
> > runtime this isn't fixing it, TBH I'm more than a little dubious

> Yes, that's correct - I did not test this directly other than making
> sure it builds, as I don't have this hardware to test with. 

OK, in that case it's going to be safer to just drop the change,
it's probably not going to cause any actual problems but it's
certainly not something that should go in as a hurried fix.

Attachment: signature.asc
Description: PGP signature