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