Re: [PATCH 06/10] mfd: cs42l43: Add support for cs42l43 core driver

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Fri, May 12, 2023 at 05:16:42PM +0200, Krzysztof Kozlowski wrote:
> On 12/05/2023 14:28, Charles Keepax wrote:
> > +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");
> 
> Drop simple debug statements for function entry/exit. There are other
> tools in kernel to do such debugging.

I mean I guess I can begrudingly drop them, there sure are other
tools but often just firing on debug is nice/simple/easy and
they are not really marking function entry/exit as much as they
are marking important events.

> > +struct cs42l43_patch_header {
> > +	__le16 version;
> > +	__le16 size;
> > +	u8 reserved;
> > +	u8 secure;
> > +	__le16 bss_size;
> > +	__le32 apply_addr;
> > +	__le32 checksum;
> > +	__le32 sha;
> > +	__le16 swrev;
> > +	__le16 patchid;
> > +	__le16 ipxid;
> > +	__le16 romver;
> > +	__le32 load_addr;
> > +} __packed;
> 
> Put all structs together at the top.

Can do.

> > +	hdr = (void *)&firmware->data[0];
> 
> Aren't you dropping here const? Why? That's not recommended programming.

Yeah that is fair will fix that up.

> > +	ret = regmap_register_patch(cs42l43->regmap, cs42l43_reva_patch,
> > +				    ARRAY_SIZE(cs42l43_reva_patch));
> > +	if (ret) {
> > +		dev_err(cs42l43->dev, "Failed to apply register patch: %d\n", ret);
> > +		goto err;
> > +	}
> > +
> > +	pm_runtime_mark_last_busy(cs42l43->dev);
> > +	pm_runtime_put_autosuspend(cs42l43->dev);
> > +
> > +	ret = devm_mfd_add_devices(cs42l43->dev, PLATFORM_DEVID_NONE,
> > +				   cs42l43_devs, ARRAY_SIZE(cs42l43_devs),
> 
> I don't why adding devices is not in probe. They use the same regmap
> right? So there will be no problem in probing them from MFD probe.

Well except SoundWire is a bit of a special boy, the hardware is
not necessarily available in probe, the hardware is only available
at some point later when the device attaches. Doing it this way all
of the attaching (and various detach/attach cycles the device needs
during configuration) are over by the time the child drivers bind, so
they don't all need special code to handle that.

> > +	cs42l43->reset = devm_gpiod_get_optional(cs42l43->dev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(cs42l43->reset)) {
> > +		ret = PTR_ERR(cs42l43->reset);
> > +		dev_err(cs42l43->dev, "Failed to get reset: %d\n", ret);
> 
> return dev_err_probe

Yeah will put those in.

> > +	cs42l43->vdd_p = devm_regulator_get(cs42l43->dev, "VDD_P");
> 
> Why these are not part of bulk get?

The comment right above explains this, VDD_P needs to be on for at
least 50uS before any other supply.

Thanks,
Charles