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

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: <andy.shevchenko@gmail.com>
Cc: <broonie@kernel.org>, <lee@kernel.org>, <robh+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
	<linus.walleij@linaro.org>, <vkoul@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 v7 3/6] mfd: cs42l43: Add support for cs42l43 core driver
Date: Fri, 19 Jan 2024 11:32:03 +0000	[thread overview]
Message-ID: <20240119113203.GA16899@ediswmail.ad.cirrus.com> (raw)
In-Reply-To: <ZalU8r1OvqKOLHrf@surfacebook.localdomain>

On Thu, Jan 18, 2024 at 06:42:26PM +0200, andy.shevchenko@gmail.com wrote:
> Fri, Aug 04, 2023 at 11:45:59AM +0100, Charles Keepax kirjoitti:
> > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> > for portable applications. It provides a high dynamic range, stereo
> > DAC for headphone output, two integrated Class D amplifiers for
> > loudspeakers, and two ADCs for wired headset microphone input or
> > stereo line input. PDM inputs are provided for digital microphones.
> > 
> > The MFD component registers and initialises the device and provides
> > PM/system power management.
> 

Full disclosure probably not going to fix everything here, few
almost entirely stylistic things and the patch was like merged 4
month ago. But happy to spin through and do a few fix ups.

> ...
> 
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> 
> It seems errno is not used (as Linux kernel error codes, otherwise err.h
> already includes necessary header).
> 
> Also seems array_size.h, mod_devicetable.h are missing (at least).
> 
> ...

Thanks for spotting these will spin through and tidy the headers
up.

> 
> > +#if IS_ENABLED(CONFIG_OF)
> 
> We are trying hard to get rid of this ugly ifdefferies (ACPI as well) along
> with respective macros that are often the PITA for CIs.
> 

Fair enough, but what is the expected alternative here? Is it now
preferred to just always include both in the driver? That does
come at a small cost in driver size, but it doesn't really bother
me.

> > +static const struct of_device_id cs42l43_of_match[] = {
> > +	{ .compatible = "cirrus,cs42l43", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, cs42l43_of_match);
> > +#endif
> > +
> > +#if IS_ENABLED(CONFIG_ACPI)
> > +static const struct acpi_device_id cs42l43_acpi_match[] = {
> > +	{ "CSC4243", 0 },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, cs42l43_acpi_match);
> > +#endif
> > +

...

> > +	cs42l43->regmap = devm_regmap_init_sdw(sdw, &cs42l43_sdw_regmap);
> > +	if (IS_ERR(cs42l43->regmap)) {
> > +		ret = PTR_ERR(cs42l43->regmap);
> > +		dev_err(cs42l43->dev, "Failed to allocate regmap: %d\n", ret);
> > +		return ret;
> > +	}
> 
> Can be simplified as
> 
> 	cs42l43->regmap = devm_regmap_init_sdw(sdw, &cs42l43_sdw_regmap);
> 	if (IS_ERR(cs42l43->regmap))
> 		dev_err_probe(cs42l43->dev, PTR_ERR(cs42l43->regmap),
> 			      "Failed to allocate regmap: %d\n", ret);
> 

Oops, yeah looks like I missed the ones in sdw and i2c, will fix
those up.

> > +#define CS42L43_RESET_DELAY			20
> > +
> > +#define CS42L43_SDW_ATTACH_TIMEOUT		500
> > +#define CS42L43_SDW_DETACH_TIMEOUT		100
> > +
> > +#define CS42L43_MCU_POLL			5000
> > +#define CS42L43_MCU_CMD_TIMEOUT			20000
> 
> > +#define CS42L43_MCU_UPDATE_TIMEOUT		500000
> 
> > +#define CS42L43_VDDP_DELAY			50
> > +#define CS42L43_VDDD_DELAY			1000
> > +
> > +#define CS42L43_AUTOSUSPEND_TIME		250
> 
> Usually we use units for the macro names as suffixes...
> E.g., _US (for microseconds).
> 

Can add those, does make it clearer.
> ...
> 
> > +struct cs42l43_patch_header {
> > +	__le16 version;
> > +	__le16 size;
> > +	u8 reserved;
> > +	u8 secure;
> 
> Seems to me that __u8 is appropriate as patch is external to the kernel and
> it's kinda firmware interface.
> 

Sure.

> > +	irq_flags = irqd_get_trigger_type(irq_data);
> > +	switch (irq_flags) {
> > +	case IRQF_TRIGGER_LOW:
> > +	case IRQF_TRIGGER_HIGH:
> > +	case IRQF_TRIGGER_RISING:
> > +	case IRQF_TRIGGER_FALLING:
> > +		break;
> > +	case IRQ_TYPE_NONE:
> 
> Are you sure it's a right place to interpret no type flags as a default?
> 

I mean... no... but I might need more to go on. The chip
generates an active low IRQ by default so it seems reasonable if
nothing is specified to assume the chip is doing what it normally
would.

> > +	default:
> > +		irq_flags = IRQF_TRIGGER_LOW;
> > +		break;
> > +	}

...

> > +EXPORT_NS_GPL_DEV_PM_OPS(cs42l43_pm_ops, MFD_CS42L43) = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(cs42l43_suspend, cs42l43_resume)
> > +	SET_RUNTIME_PM_OPS(cs42l43_runtime_suspend, cs42l43_runtime_resume, NULL)
> > +};
> 
> Why do you need SET_*() versions of those macros? They are not supposed to be
> used with the new macros such as EXPORT_NS_GPL_DEV_PM_OPS().
> 

Yeah fixed that up already in an earlier fixup patch.

Thanks,
Charles