linux-kernel - Re: [PATCH v2 3/4] ASoC: apple: mca: Start new platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <24C0ABFA-BF71-4492-8A6A-E9BE1462B403@cutebit.org>
Date:   Tue, 23 Aug 2022 09:33:36 +0200
From:   Martin Povišer <povik+lin@...ebit.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        asahi@...ts.linux.dev, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] ASoC: apple: mca: Start new platform driver


> On 22. 8. 2022, at 19:39, Mark Brown <broonie@...nel.org> wrote:
> 
> On Fri, Aug 19, 2022 at 02:54:29PM +0200, Martin Povišer wrote:
> 
> This all looks good, one style nit and a couple of requests for
> clarification below but basically this is fine.
> 
>> +++ b/sound/soc/apple/mca.c
>> @@ -0,0 +1,1149 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Apple SoCs MCA driver
>> + *
>> + * Copyright (C) The Asahi Linux Contributors
>> + *
>> + * The MCA peripheral is made up of a number of identical units called clusters.
> 
> Please make the entire comment block a C++ one so things look more
> intentional.

Is this so that it does not look like the SPDX header was added
mechanically? I will do it, just curious what the reasoning is.

> 
>> +#define USE_RXB_FOR_CAPTURE
> 
> What's this all about?

There’s something we can configure one way or the other way in the
hardware (choosing RXA or RXB unit in a cluster for capture). Since this 
driver developed along reverse-engineering the hardware, this switch
got built in. I want to keep it for future experimentation. Also, as
the driver’s side gig is documenting the hardware, this way it
documents more.

>> +static int mca_fe_enable_clocks(struct mca_cluster *cl)
>> +{
>> +	struct mca_data *mca = cl->host;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(cl->clk_parent);
>> +	if (ret) {
>> +		dev_err(mca->dev,
>> +			"cluster %d: unable to enable clock parent: %d\n",
>> +			cl->no, ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * We can't power up the device earlier than this because
>> +	 * the power state driver would error out on seeing the device
>> +	 * as clock-gated.
>> +	 */
>> +	cl->pd_link = device_link_add(mca->dev, cl->pd_dev,
>> +				      DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
>> +					      DL_FLAG_RPM_ACTIVE);
> 
> I'm not clear on this dynamically adding and removing device links stuff
> - it looks like the main (only?) purpose is to take a runtime PM
> reference to the target device which is fine but it's not clear why
> device links are involved given that the links are created and destroyed
> every time the DAI is used, AFAICT always in the same fixed
> relationship.  It's not a problem, it's just unclear.

Indeed the only purpose is powering up the cluster’s power domain (there’s
one domain for each cluster). Adding the links is the only way I know to
do it. They need to be added dynamically (as opposed to statically linking,
say, the DAI’s ->dev to the cluster’s ->pd_dev, which I guess may do
something similar), because we need to sequence the power-up/power-down
with the enablement of the clocks.

Martin