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
