Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs

On Mon, Dec 21, 2020 at 08:10:15AM +0000, Lee Jones wrote:

On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:

quoted

On Fri, Dec 18, 2020 at 01:21:39PM +0000, Lee Jones wrote:

quoted

On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:

quoted

Hi Lee,

Thank you for the detailed review!

I will prepare a new revision, but there are still a couple of open
points..

Could you please snip your replies, leaving only the open points.

Scrolling through lots of empty quotes or "done" comments is quite
time consuming.  Thanks.

Sure, I'll take that into account.

quoted

[...]

quoted

quoted

quoted

+	ret = regmap_read(atc260x->regmap, atc260x->rev_reg, &chip_rev);
+	if (ret) {
+		dev_err(dev, "Failed to get chip revision\n");
+		return ret;
+	}
+
+	if (chip_rev < 0 || chip_rev > 31) {
+		dev_err(dev, "Unknown chip revision: %d\n", ret);
+		return -EINVAL;
+	}

This still seems limiting.

This is based on the vendor implementation. Unfortunately I don't have
access to a data sheet or any other source of information about the
management of the chip revisions.

So which versions does this driver work with?  All 32?

I'm not even sure there are so many revisions, I guess that's just a
rough validation for a vendor reserved range.

For the moment, the only place where the functionality is affected
by the chip revision is in the regulator driver - there is a special
handling for the ATC2603C rev.B chip variant.

I expect some additional handling might be required for new drivers
bringing support for the other functions provided by the hardware.

The current patch seems to insinuate that 32 versions are currently
supported.  What is the chip_rev for the ATC2603C rev.B?

I only own the rev.A for the ATC2603C variant, for which I read '0' from
the chip rev register.

However what really matters for the driver is not the raw value, but the
one computed via:

atc260x->ic_ver = __ffs(chip_rev + 1U);

This is basically a translation of the raw value to a chip version
that is used in the context of the special handling mentioned above:

enum atc260x_ver {
	ATC260X_A = 0,
	ATC260X_B,
	ATC260X_C,
	ATC260X_D,
	ATC260X_E,
	ATC260X_F,
	ATC260X_G,
	ATC260X_H,
};

So we actually could handle up to 8 chip versions with the current
management scheme.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog