On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:

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?

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