Re: [PATCH v2 03/10] mfd: Add base driver for Netronix embedded controller

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 6769 bytes --]

On Tue, Sep 08, 2020 at 02:29:34PM +0100, Lee Jones wrote:
> On Sat, 05 Sep 2020, Jonathan Neuschäfer wrote:
[...]
> > +config MFD_NTXEC
> > +	tristate "Netronix embedded controller"
> 
> Nit: "Embedded Controller (EC)"

I intentionally left it lowercase, because the term 'embedded
controller' is not used in code coming from Netronix. It's just a label
that I found fitting to assign to the device. (In the vendor kernels
it's either called 'msp430', named after the TI MSP430 microcontroller
family, or 'uc', presumably for 'microcontroller')

Adding '(EC)' seems somewhat useful.

> 
> > +	depends on I2C && OF
> 
> 	depends on (I2C && OF) || COMPILE_TEST

Okay

> > +++ b/drivers/mfd/ntxec.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> 
> Why 2 only?

No particular reason. If you prefer 2-or-later, I'll change it.

> > +/*
> > + * The Netronix embedded controller is a microcontroller found in some
> > + * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> > + * battery monitoring, system power management, and PWM functionality.
> > + *
> > + * This driver implements register access, version detection, and system
> > + * power-off/reset.
> > + *
> > + * Copyright 2020 Jonathan Neuschäfer
> 
> Email?

Alright, I'll add it

> > +static void ntxec_poweroff(void)
> > +{
> > +	int res;
> > +
> 
> Remove this line please.
> 
> > +	u8 buf[] = {
> > +		NTXEC_REG_POWEROFF,
> > +		(NTXEC_POWEROFF_VALUE >> 8) & 0xff,
> > +		NTXEC_POWEROFF_VALUE & 0xff,
> > +	};
> > +
> 
> And this one.
> 
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr = poweroff_restart_client->addr,
> > +			.flags = 0,
> > +			.len = sizeof(buf),
> > +			.buf = buf
> > +		}
> > +	};
> > +
> > +	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> > +
> 
> And this one.

Okay

> 
> > +	if (res < 0)
> > +		dev_alert(&poweroff_restart_client->dev, "Failed to power off (err = %d)\n", res);
> 
> This looks way over 80 chars.

Okay, I'll break it up.

> Did you run checkpatch.pl?

Yes, but it didn't complain about this line. - propabably because the
line length threshold in checkpatch has recently been increased to 100.

> > +	/*
> > +	 * The time from the register write until the host CPU is powered off
> > +	 * has been observed to be about 2.5 to 3 seconds. Sleep long enough to
> > +	 * safely avoid returning from the poweroff handler.
> > +	 */
> > +	msleep(5000);
> > +}
> > +
> > +static int ntxec_restart(struct notifier_block *nb,
> > +			 unsigned long action, void *data)
[...]
> 
> All as above for this function.

Alright

> 
> > +static struct notifier_block ntxec_restart_handler = {
> > +	.notifier_call = ntxec_restart,
> > +	.priority = 128
> > +};
> > +
> > +static const struct regmap_config regmap_config = {
> > +	.name = "ntxec",
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.cache_type = REGCACHE_NONE,
> > +	.val_format_endian = REGMAP_ENDIAN_BIG,
> > +};
> > +
> > +static const struct ntxec_info ntxec_known_versions[] = {
> > +	{ 0xd726 }, /* found in Kobo Aura */
> > +};
> > +
> > +static const struct ntxec_info *ntxec_lookup_info(u16 version)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ntxec_known_versions); i++) {
> > +		const struct ntxec_info *info = &ntxec_known_versions[i];
> > +
> > +		if (info->version == version)
> > +			return info;
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> Wait, what?  This is over-engineered.

I thought it would be useful when we want to attach additional
information to specific versions.... okay, it is over-engineered.

> Just have a look-up table (switch) of supported devices.

Will do.

> 
> > +static int ntxec_probe(struct i2c_client *client)
> > +{
> > +	struct ntxec *ec;
> > +	unsigned int version;
> > +	int res;
> > +
> > +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> > +	if (!ec)
> > +		return -ENOMEM;
> > +
> > +	ec->dev = &client->dev;
> > +
> > +	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
> > +	if (IS_ERR(ec->regmap)) {
> > +		dev_err(ec->dev, "Failed to set up regmap for device\n");
> > +		return res;
> > +	}
> > +
> > +	/* Determine the firmware version */
> > +	res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version);
> > +	if (res < 0) {
> > +		dev_err(ec->dev, "Failed to read firmware version number\n");
> > +		return res;
> > +	}
> > +	dev_info(ec->dev,
> > +		 "Netronix embedded controller version %04x detected.\n",
> > +		 version);
> > +
> > +	/* Bail out if we encounter an unknown firmware version */
> > +	ec->info = ntxec_lookup_info(version);
> > +	if (!ec->info)
> > +		return -EOPNOTSUPP;
> 
> #define EOPNOTSUPP      95      /* Operation not supported on transport endpoint */
> 
> I think you want:
> 
> #define ENODEV          19      /* No such device */

Indeed, EOPNOTSUPP seems quite wrong here. I think I used ENOTSUPP at
some earlier point but moved away from it because it's one of the
internal error codes (≥512).

ENODEV makes sense when I read it as "Not a device that this driver can
deal with".

> 
> > +	if (of_device_is_system_power_controller(ec->dev->of_node)) {
> > +		/*
> > +		 * Set the 'powerkeep' bit. This is necessary on some boards
> > +		 * in order to keep the system running.
> > +		 */
> > +		res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP,
> > +				   NTXEC_POWERKEEP_VALUE);
> > +		if (res < 0)
> > +			return res;
> > +
> > +		/* Install poweroff handler */
> 
> Don't think this comment is required.
> 
> > +		WARN_ON(poweroff_restart_client);
> > +		poweroff_restart_client = client;
> > +		if (pm_power_off)
> > +			dev_err(ec->dev, "pm_power_off already assigned\n");
> > +		else
> > +			pm_power_off = ntxec_poweroff;
> > +
> > +		/* Install board reset handler */
> 
> Nor this.

Alright, I'll remove them.

> > +		res = register_restart_handler(&ntxec_restart_handler);
> > +		if (res)
> > +			dev_err(ec->dev,
> > +				"Failed to register restart handler: %d\n", res);
> > +	}
> > +
> > +	i2c_set_clientdata(client, ec);
> > +
> > +	return devm_of_platform_populate(ec->dev);
> > +}
> > +
> > +static int ntxec_remove(struct i2c_client *i2c)
> 
> Why do you call it 'client' in .probe, but 'i2c' in .remove?

No particular reason. I'll make them consistent.

> > +struct ntxec_info {
> > +	u16 version;
> > +};
> > +
> > +struct ntxec {
> > +	struct device *dev;
> > +	struct regmap *regmap;
> 
> > +	const struct ntxec_info *info;
> 
> What is this used for?

At this point, nothing. I'll remove it.


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]