| Message ID | 20230605125504.2570158-1-ckeepax@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add cs42l43 PC focused SoundWire CODEC | expand |
On Fri, 16 Jun 2023, Charles Keepax wrote: > 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. Yes please. > > > + // 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? Since this table isn't providing any information which cannot be derived from the other (OF, ACPI) tables, the I2C subsystem should be able to obtain it from those sources instead. > > > +#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. Can you have a little think for another way to solve this please? > > > +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 Sure, during development. Now the driver is authored however, how often are you likely to turn it back on. Besides, this isn't real debug information with dynamically obtained values and useful information, it's a function call trace which can be obtained from other sources, such as ftrace and the like. [...] > > > + 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. Please make that clear. I don't see any documentation or pointers here. > > 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. I'd suggest not using kernel-doc formatting, but that type of thing, yes.
On Mon, Jun 19, 2023 at 09:30:05AM +0100, Lee Jones wrote: > On Fri, 16 Jun 2023, Charles Keepax wrote: > > On Thu, Jun 15, 2023 at 06:11:24PM +0100, Lee Jones wrote: > > > On Mon, 05 Jun 2023, Charles Keepax wrote: > > > > +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? > > Since this table isn't providing any information which cannot be derived > from the other (OF, ACPI) tables, the I2C subsystem should be able to > obtain it from those sources instead. > Sorry I literally just sent a v4 then saw this email. I will test removing this table and send a v5. > > > > +#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. > > Can you have a little think for another way to solve this please? > I will have another go at it, if memory serves the vague options were: 1) this approach 2) some sort of horrible #include to put the defaults array in both modules, although I would really prefer to avoid this one. 3) dynamically allocate the regmap_configs so those two fields can be filled in with non-static data. If I fail to come up with an option 4 would you prefer 1 or 3? Well or 2 but I really would prefer not to do 2. > > > 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. > > I'd suggest not using kernel-doc formatting, but that type of thing, > yes. Ok I will remove the kernel doc bits for v5. Thanks, Charles