Re: [PATCH v3 3/6] mfd: cs42l43: Add support for cs42l43 core driver

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: Lee Jones <lee@kernel.org>
Cc: <broonie@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
	<linus.walleij@linaro.org>, <vkoul@kernel.org>,
	<robh+dt@kernel.org>, <conor+dt@kernel.org>,
	<lgirdwood@gmail.com>, <yung-chuan.liao@linux.intel.com>,
	<sanyog.r.kale@intel.com>, <pierre-louis.bossart@linux.intel.com>,
	<alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
	<devicetree@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
	<linux-spi@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 3/6] mfd: cs42l43: Add support for cs42l43 core driver
Date: Fri, 16 Jun 2023 08:34:04 +0000	[thread overview]
Message-ID: <20230616083404.GR68926@ediswmail.ad.cirrus.com> (raw)
In-Reply-To: <20230615171124.GL3635807@google.com>

On Thu, Jun 15, 2023 at 06:11:24PM +0100, Lee Jones wrote:
> On Mon, 05 Jun 2023, Charles Keepax wrote:
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// CS42L43 I2C driver
> > +//
> > +// Copyright (C) 2022-2023 Cirrus Logic, Inc. and
> > +//                         Cirrus Logic International Semiconductor Ltd.
> > +
> 
> I realise there is some precedent for this already in MFD.
> 
> However, I'd rather headers used C style multi-line comments.
> 

Apologies but just to be super clear you want this to look like:

// SPDX-License-Identifier: GPL-2.0
/*
 * CS42L43 I2C driver
 *
 * Copyright (C) 2022-2023 Cirrus Logic, Inc. and
 *                         Cirrus Logic International Semiconductor Ltd.
 */

Just clarifying since you want to get rid of all the // comments,
but the SPDX stuff specifically requires one according to
Documentation/process/license-rules.rst.

> > +	// I2C is always attached by definition
> 
> C please.  And everywhere else.
> 

Can do.

> > +static struct i2c_device_id cs42l43_i2c_id[] = {
> > +	{ "cs42l43", 0 },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, cs42l43_i2c_id);
> 
> Is this required anymore?
> 

I was not aware of it not being required, I think it will still
be used for the purposes of module naming. Perhaps someone more
knowledgable than me can comment?

> > +#if IS_ENABLED(CONFIG_MFD_CS42L43_I2C)
> > +const struct regmap_config cs42l43_i2c_regmap = {
> > +	.reg_bits		= 32,
> > +	.reg_stride		= 4,
> > +	.val_bits		= 32,
> > +	.reg_format_endian	= REGMAP_ENDIAN_BIG,
> > +	.val_format_endian	= REGMAP_ENDIAN_BIG,
> > +
> > +	.max_register		= CS42L43_MCU_RAM_MAX,
> > +	.readable_reg		= cs42l43_readable_register,
> > +	.volatile_reg		= cs42l43_volatile_register,
> > +	.precious_reg		= cs42l43_precious_register,
> > +
> > +	.cache_type		= REGCACHE_RBTREE,
> > +	.reg_defaults		= cs42l43_reg_default,
> > +	.num_reg_defaults	= ARRAY_SIZE(cs42l43_reg_default),
> > +};
> > +EXPORT_SYMBOL_NS_GPL(cs42l43_i2c_regmap, MFD_CS42L43);
> > +#endif
> 
> We don't tend to like #ifery in C files.
> 
> Why is it required?
> 
> And why not just put them were they're consumed?

The trouble is the cs42l43_reg_default array and the array size.
There is no good way to statically initialise those two fields
from a single array in both the I2C and SDW modules.

> > +static int cs42l43_soft_reset(struct cs42l43 *cs42l43)
> > +{
> > +	static const struct reg_sequence reset[] = {
> > +		{ CS42L43_SFT_RESET, 0x5A000000 },
> > +	};
> > +	unsigned long time;
> > +
> > +	dev_dbg(cs42l43->dev, "Soft resetting\n");
> 
> How often are you realistically going to enable these?  Can you do
> without them and just add some printks when debugging?  Seems a shame to
> dirty the code-base with seldom used / questionably helpful LoC.

I mean I use them all the time they are very helpful. But yeah I
can just add them each time I need them its just a pain, but I
guess since you are the second person to comment I will accept
that wanting to easily turn on debug is not a feature we are keen
on.

> > +	reinit_completion(&cs42l43->device_detach);
> > +
> > +	/* apply cache only as the device will also fall off the soundwire bus */
> 
> Grammar please.  Some capital letters missing there.
> 

No problem.

> > +	regcache_cache_only(cs42l43->regmap, true);
> > +	regmap_multi_reg_write_bypassed(cs42l43->regmap, reset, ARRAY_SIZE(reset));
> > +
> > +	msleep(20);
> 
> Why 20?
> 

Because that is what the hardware needs, I assume this will be
covered when I turn all number in to defines.

> > +static int cs42l43_mcu_stage_2_3(struct cs42l43 *cs42l43, bool shadow)
> > +{
> > +	unsigned int need_reg = CS42L43_NEED_CONFIGS;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	if (shadow)
> 
> What's shadow?  Comment please.
> 

Yeah can add a comment for that.

> > +		need_reg = CS42L43_FW_SH_BOOT_CFG_NEED_CONFIGS;
> > +
> > +	regmap_write(cs42l43->regmap, need_reg, 0x0);
> > +
> > +	ret = regmap_read_poll_timeout(cs42l43->regmap, CS42L43_BOOT_STATUS,
> > +				       val, (val == 3), 5000, 20000);
> 
> Defines are easier to read than magic numbers.
> 

Can do.

> > +	if (ret) {
> > +		dev_err(cs42l43->dev, "Failed to move to stage 3: %d, 0x%x\n", ret, val);
> 
> Stage 3 what?
> 

Of the MCU boot.

> Perhaps some simple function headers would help?
> 

You mean add some kernel doc for these functions, right? Assuming
that is what you mean, will do.

Thanks,
Charles