Received-SPF: pass (google.com: domain of linux-kernel+bounces-21408-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: James Ogletree <James.Ogletree@cirrus.com>
To: Charles Keepax <ckeepax@opensource.cirrus.com>
CC: James Ogletree <jogletre@opensource.cirrus.com>,
        Fred Treven
	<Fred.Treven@cirrus.com>,
        Ben Bright <Ben.Bright@cirrus.com>,
        Dmitry Torokhov
	<dmitry.torokhov@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley
	<conor+dt@kernel.org>,
        Simon Trimmer <simont@opensource.cirrus.com>,
        Richard
 Fitzgerald <rf@opensource.cirrus.com>,
        Lee Jones <lee@kernel.org>, Liam
 Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>, Jaroslav
 Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        James Schulman
	<James.Schulman@cirrus.com>,
        David Rhodes <David.Rhodes@cirrus.com>,
        Sebastian Reichel <sebastian.reichel@collabora.com>,
        Jacky Bai
	<ping.bai@nxp.com>, Jeff LaBundy <jeff@labundy.com>,
        Peng Fan
	<peng.fan@nxp.com>, Weidong Wang <wangweidong.a@awinic.com>,
        Herve Codina
	<herve.codina@bootlin.com>,
        Arnd Bergmann <arnd@arndb.de>, Shenghao Ding
	<13916275206@139.com>,
        Ryan Lee <ryans.lee@analog.com>,
        Linus Walleij
	<linus.walleij@linaro.org>,
        Maxim Kochetkov <fido_max@inbox.ru>,
        Shuming Fan
	<shumingf@realtek.com>,
        "open list:CIRRUS LOGIC HAPTIC DRIVERS"
	<patches@opensource.cirrus.com>,
        "open list:INPUT (KEYBOARD, MOUSE, JOYSTICK,
 TOUCHSCREEN)..." <linux-input@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND
 FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>,
        open list
	<linux-kernel@vger.kernel.org>,
        "open list:SOUND - SOC LAYER / DYNAMIC AUDIO
 POWER MANAGEM..." <linux-sound@vger.kernel.org>,
        "moderated list:CIRRUS LOGIC
 AUDIO CODEC DRIVERS" <alsa-devel@alsa-project.org>
Subject: Re: [PATCH v5 3/5] mfd: cs40l50: Add support for CS40L50 core driver
Thread-Topic: [PATCH v5 3/5] mfd: cs40l50: Add support for CS40L50 core driver
Thread-Index: AQHaP168ighr5EhGj0KHpyfGBV7m37DLQVqAgAa/5gA=
Date: Tue, 9 Jan 2024 21:08:40 +0000
Message-ID: <AC98C1E6-C1A4-46DA-8964-3319CFC821FD@cirrus.com>
References: <20240104223643.876292-1-jogletre@opensource.cirrus.com>
 <20240104223643.876292-4-jogletre@opensource.cirrus.com>
 <20240105140407.GG14858@ediswmail.ad.cirrus.com>
In-Reply-To: <20240105140407.GG14858@ediswmail.ad.cirrus.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?tgO/kjJebvPO21Wd5ije9K3eQM7spDehzUo/Tk1tPxxH/djYX5VxWXPD3zgf?=
 =?us-ascii?Q?DzO9yLMVI/fEO9W2QGyXnqQG5Eks8hFOzRG9N6O1KvfvnO74Tag6Xu2gBg60?=
 =?us-ascii?Q?O/5+RKkc2Pdm7qkHfOME5cx6LKxqaG4FpN9okaY/fSqj1no8F2EPfvCj1eSA?=
 =?us-ascii?Q?Fo3HwX6prvelyye4R/4OYfrpVcaA5e54QZzh2D+x8ZNAeCb+QkbWbqV6eEmH?=
 =?us-ascii?Q?OWVX2RrOrQIARDj0M4FyHBWswgNOgfPvEroWayoOzEtOAYOCWfzpQLR8S3wE?=
 =?us-ascii?Q?obGwlXo7XC6D/Rr97dOWhLwjXPPN2+e7EcrA+J+dVi6IiiNVb5Zr9UZm4vYt?=
 =?us-ascii?Q?mAtGlqX1ub8hGWfIWL64jm2U3kaW4eZxzI4ghdpPkVigp0BlU/3McPW7lfQx?=
 =?us-ascii?Q?iJ/MtfYyLtAfuLC+hrE2hMKg+TzJ54YaHwsLrHYfLC8P4ZYUqzxfIhF5UPF5?=
 =?us-ascii?Q?DCpKIF6yQQpvAt4+ENGKi5YElqBa/3rTxV8WWvFPRDbs4XJr2izln+LgL2X6?=
 =?us-ascii?Q?n6V3e8Ml127+VGNw1Wg9ytCzWxypY2YvVE4G3785t0uC6oUsyCdOK6mZa0aw?=
 =?us-ascii?Q?XLbNzFD4njQyRf/CodhT8lw10xW58wP5CViJk45Dh/r9EnisYrvZYAbVzvo1?=
 =?us-ascii?Q?UErfL2Qujy8O6YfLE0PTXvvPXWRFTjInADbVUP/XpgDaQN0jquUcc+c36nks?=
 =?us-ascii?Q?SYIQBAnKprTIeNZqEJAnktTK5qhvwd6CQbdga6WfOzHuEbinaNQFHNjpmh+J?=
 =?us-ascii?Q?lXe9mwA8sNPgu33ApdQPWf0rvTIo8Ja36KA3N8nO1uU6ILcDDeuwUYWajMg6?=
 =?us-ascii?Q?KyrMiRzRLoHXrESuK6gI8Zm1B9Ctny4/FpoN+WPhTMW+Mooxz6n/hf6Z0FvO?=
 =?us-ascii?Q?c+A9h5Jmn/m/o5AUvCmp3Yv0KqCxLIEfcUk14EDT0zQfQ/CY2qR/X1jqCheN?=
 =?us-ascii?Q?8ry4An++nzlb+MrQ98tM5EHcRy1ouRtD0U7U5V2Ly5rWdTk86vOqI5paukpd?=
 =?us-ascii?Q?30x54KDaHpecETbIzwxBNTgbJbI/Mhc8UG6eu62VNt/GS5Tfcc29eL+voD2H?=
 =?us-ascii?Q?8zzPX0G9UwoeWYJNwj8gUk0QUttALDPh1LR3+Cwb3yg0M3cka8AkyfUUczzu?=
 =?us-ascii?Q?6arsdfZv7TKPTjVHWYHPofwiMCxZdmh0v+gaL9jL2+TfloU/xmAnA5QXNcqU?=
 =?us-ascii?Q?OPzXV6bfLLYzm0M47Czdi5SAGB+kVM8OFjFeMlB2uZCsqR4tGgFG1fynnUJW?=
 =?us-ascii?Q?l0WD7+2sqNA6IsDpG13/HQl7YdAo6Qv4n3Iq0Ci6rzJ4RgTBwmtWzoTruTtS?=
 =?us-ascii?Q?p9DQX4PHpC5HzFbb/7OwGdnfe2phLcVdZFA2TcrhHN2ITw8L8U3scH+iE4Wa?=
 =?us-ascii?Q?3U/PmrAXjnxlgHkdaUeIBTY/zzagThGS7R8sE0n1UyU+CDwNcfM7mt2GMV/i?=
 =?us-ascii?Q?gTdzOXon8mLmMpvXiy5Y2EZszAJX/pxkdnwGhZeG28O7RKxo6R9WRAbJp+mr?=
 =?us-ascii?Q?k5/EiD894W6qe8/WlEipUvPvXFJG79BMq5U869DsrQwheIPJb0iVzftDs43D?=
 =?us-ascii?Q?Y3rJCwkOA4Gy1k3QEM9d9GOJezanEeKKDRdlc8CncF0ggcw5IZKZmEiPcF8Q?=
 =?us-ascii?Q?fg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <CB5F5974BAC31C42B8A6526C02630EBB@namprd19.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: cirrus.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DS7PR19MB5688.namprd19.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3a78c868-b776-4070-402c-08dc115727c3
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Jan 2024 21:08:40.6435
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: bec09025-e5bc-40d1-a355-8e955c307de8
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: VY5kzEZuzpOi2p1cSLc3JcyBU2gDfofjA5KrYcpXadOJ/F8Vc8GGNdkxqfRuDjL5lzoBV9bCmEBrSnWfI9lOa4Gq8APjwReYgPfPXqQLuKI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR19MB6891
X-Proofpoint-GUID: utK7TKBzX2H1rdOcT1l3JPxYywfWKeqT
X-Proofpoint-ORIG-GUID: utK7TKBzX2H1rdOcT1l3JPxYywfWKeqT
X-Proofpoint-Spam-Reason: safe

Hi Charles,

Thank you for your excellent review. Anything not replied to will be
adopted as-is in the next version.

> On Jan 5, 2024, at 8:04 AM, Charles Keepax <ckeepax@opensource.cirrus.com=
> wrote:
>=20
> On Thu, Jan 04, 2024 at 10:36:36PM +0000, James Ogletree wrote
>=20
>> +static int cs40l50_dsp_init(struct cs40l50 *cs40l50)
>> +{
>> + int err;
>> +
>> + cs40l50->dsp.num =3D 1;
>> + cs40l50->dsp.type =3D WMFW_HALO;
>> + cs40l50->dsp.dev =3D cs40l50->dev;
>> + cs40l50->dsp.regmap =3D cs40l50->regmap;
>> + cs40l50->dsp.base =3D CS40L50_CORE_BASE;
>> + cs40l50->dsp.base_sysinfo =3D CS40L50_SYS_INFO_ID;
>> + cs40l50->dsp.mem =3D cs40l50_dsp_regions;
>> + cs40l50->dsp.num_mems =3D ARRAY_SIZE(cs40l50_dsp_regions);
>> + cs40l50->dsp.no_core_startstop =3D true;
>> +
>> + err =3D cs_dsp_halo_init(&cs40l50->dsp);
>> + if (err)
>> + return err;
>> +
>> + return devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_remove,
>> + &cs40l50->dsp);
>=20
> Hmm... I notice you use this for both dsp_remove and
> dsp_power_down. Are you sure devm will guarantee those are called
> in the right order? Its not immediately clear to me that would be
> have to be the case.

On my inspection of the devm code, actions are always added to the
tail, and played back from head to tail on driver detach.

>=20
>> +static int cs40l50_power_up_dsp(struct cs40l50 *cs40l50)
>> +{
>> + int err;
>> +
>> + mutex_lock(&cs40l50->lock);
>> +
>> + if (cs40l50->patch) {
>> + /* Stop core if loading patch file */
>> + err =3D regmap_multi_reg_write(cs40l50->regmap, cs40l50_stop_core,
>> +     ARRAY_SIZE(cs40l50_stop_core));
>> + if (err)
>> + goto err_mutex;
>> + }
>> +
>> + err =3D cs_dsp_power_up(&cs40l50->dsp, cs40l50->patch, "cs40l50.wmfw",
>> +      cs40l50->bin, "cs40l50.bin", "cs40l50");
>> + if (err)
>> + goto err_mutex;
>> +
>> + err =3D devm_add_action_or_reset(cs40l50->dev, cs40l50_dsp_power_down,
>> +       &cs40l50->dsp);
>> + if (err)
>> + goto err_mutex;
>> +
>> + if (cs40l50->patch) {
>> + /* Resume core after loading patch file */
>> + err =3D regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
>> +   CS40L50_CLOCK_ENABLE);
>=20
> This feels like this needs a comment, why are we skipping the
> normal DSP init and doing it manually (this appears to be the
> same writes start_core would have done)? I assume its something to
> do with what you are really doing is you don't want lock_memory
> to run?

The dsp struct uses cs_dsp_halo_ao_ops, made for self-booting
DSPs, which has none of the ops used in cs_dsp_run(). The
manual stop is because it is self-booting (already running you could
say) but we need to stop the clock to patch the firmware. Please
correct me if that is not right.

>> +static int cs40l50_configure_dsp(struct cs40l50 *cs40l50)
>> +{
>> + u32 nwaves;
>> + int err;
>> +
>> + if (cs40l50->bin) {
>> + /* Log number of effects if wavetable was loaded */
>> + err =3D regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves);
>> + if (err)
>> + return err;
>> +
>> + dev_info(cs40l50->dev, "Loaded with %u RAM waveforms\n", nwaves);
>=20
> Kinda nervous about the fact we access all these DSP controls
> directly through address, rather than using the DSP control
> accessors, we have the accessors for a reason. They manage things
> like access permissions etc. and historically, the firmware
> guys have not been able to guarantee these remain in consistent
> locations between firmware versions.
>=20
> I guess this is so you can access them even in the case of the
> ROM firmware, but you could have a meta-data only firmware file
> that you load in that case to give you the controls.  I don't
> feel the need to NAK the driver based on this but please think
> about this very carefully it's a strange way to use the DSP
> controls, and feels likely to cause problems to me. It is also
> quite hostile to fixing it in the future since as you are not
> using the controls no one will be checking that things like the
> access flags in the firmware are set correctly, which is annoying
> if the decision has to be reversed later since there will likely
> be a bunch of broken firmwares already in the field.

Noting here that we discussed this offline. The driver is going to
stay with a static register design for now, but the write sequence
interface is being modified to be control based.

Best,
James