Re: [PATCH v4] rtc: isl12026: Add driver.

[Alexandre Belloni]

On 20/02/2018 at 11:43:47 -0800, David Daney wrote:
> On 02/20/2018 03:03 AM, Alexandre Belloni wrote:
> [...]
> 
> 
> > > diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> > > new file mode 100644
> > > index 000000000000..29e5bdf96c67
> > > --- /dev/null
> > > +++ b/drivers/rtc/rtc-isl12026.c
> > > @@ -0,0 +1,529 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * An I2C driver for the Intersil ISL 12026
> > > + *
> > > + * Copyright (c) 2018 Cavium, Inc.
> > > + */
> > > +#include <linux/bcd.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/nvmem-provider.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/rtc.h>
> > > +#include <linux/slab.h>
> > > +
> > > +/* register offsets */
> > > +#define ISL12026_REG_PWR	0x14
> > > +# define ISL12026_REG_PWR_BSW	BIT(6)
> > > +# define ISL12026_REG_PWR_SBIB	BIT(7)
> > > +#define ISL12026_REG_SC		0x30
> > > +#define ISL12026_REG_HR		0x32
> > > +# define ISL12026_REG_HR_MIL	BIT(7)	/* military or 24 hour time */
> > > +#define ISL12026_REG_SR		0x3f
> > > +# define ISL12026_REG_SR_RTCF	BIT(0)
> > > +# define ISL12026_REG_SR_WEL	BIT(1)
> > > +# define ISL12026_REG_SR_RWEL	BIT(2)
> > > +# define ISL12026_REG_SR_MBZ	BIT(3)
> > > +# define ISL12026_REG_SR_OSCF	BIT(4)
> > > +
> > > +/* The EEPROM array responds at i2c address 0x57 */
> > > +#define ISL12026_EEPROM_ADDR	0x57
> > > +
> > > +#define ISL12026_PAGESIZE 16
> > > +#define ISL12026_NVMEM_WRITE_TIME 20
> > > +
> > > +struct isl12026 {
> > > +	struct rtc_device *rtc;
> > > +	struct i2c_client *nvm_client;
> > > +	struct nvmem_config nvm_cfg;
> > > +	/*
> > > +	 * RTC write operations require that multiple messages be
> > > +	 * transmitted, we hold the lock for all accesses to the
> > > +	 * device so that these sequences cannot be disrupted.  Also,
> > > +	 * the write cycle to the nvmem takes many ms during which the
> > > +	 * device does not respond to commands, so holding the lock
> > > +	 * also prevents access during these times.
> > > +	 */
> > > +	struct mutex lock;
> > > +};
> > > +
> > > +static int isl12026_read_reg(struct i2c_client *client, int reg)
> > > +{
> > > +	struct isl12026 *priv = i2c_get_clientdata(client);
> > > +	u8 addr[] = {0, reg};
> > > +	u8 val;
> > > +	int ret;
> > > +
> > > +	struct i2c_msg msgs[] = {
> > > +		{
> > > +			.addr	= client->addr,
> > > +			.flags	= 0,
> > > +			.len	= sizeof(addr),
> > > +			.buf	= addr
> > > +		}, {
> > > +			.addr	= client->addr,
> > > +			.flags	= I2C_M_RD,
> > > +			.len	= 1,
> > > +			.buf	= &val
> > > +		}
> > > +	};
> > 
> > I'm pretty sure you can use regmap instead of open coding all the i2c
> > transfers, did you try?
> 
> I couldn't figure out how to make it do the device-atomic stores to SR.RWEL
> and SR.WEL that must precede certain register store operations. Also,
> dealing with locking across multiple i2c target addresses seems
> problematical with the regmap helpers.
> 
> The open coding doesn't clutter things up too much, and allows us to follow
> the isl12026 protocol without having to jump through too many hoops.
> 
> > 
> > > +
> > > +	mutex_lock(&priv->lock);
> > > +
> > 
> > Also, regmap will remove the need for that lock.
> 
> Since
> 
> 
> > 
> > > +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > +	if (ret != ARRAY_SIZE(msgs)) {
> > > +		dev_err(&client->dev, "read reg error, ret=%d\n", ret);
> > > +		ret = ret < 0 ? ret : -EIO;
> > > +	} else {
> > > +		ret = val;
> > > +	}
> > > +
> > > +	mutex_unlock(&priv->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int isl12026_write_reg(struct i2c_client *client, int reg, u8 val)
> > > +{
> > > +	struct isl12026 *priv = i2c_get_clientdata(client);
> > > +	int ret;
> > > +	u8 op[3];
> > > +	struct i2c_msg msg = {
> > > +		.addr	= client->addr,
> > > +		.flags	= 0,
> > > +		.len	= 1,
> > > +		.buf	= op
> > > +	};
> > > +
> > > +	mutex_lock(&priv->lock);
> > > +
> > > +	/* Set SR.WEL */
> > > +	op[0] = 0;
> > > +	op[1] = ISL12026_REG_SR;
> > > +	op[2] = ISL12026_REG_SR_WEL;
> > > +	msg.len = 3;
> > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > +	if (ret != 1) {
> > > +		dev_err(&client->dev, "write error SR.WEL, ret=%d\n", ret);
> > > +		ret = ret < 0 ? ret : -EIO;
> > > +		goto out;
> > > +	}
> > 
> > If you don't clear SR.WEL, I don't think you need to set it each time
> > you write to the RTC. I would just set SR.WEL at probe time and let it
> > there. That removes two i2c writes for each write operation.
> 
> I don't like the idea of leaving the thing partially armed when write
> operations should be rare.
> 

Ok, then, can you at least factorize the write enabling/disabling in two
functions. That would make the code smaller.

> > > +	ret = rtc_valid_tm(tm);
> > 
> > This rtc_valid_tm is unnecessary, you can simply return 0.
> 
> It may be possible for invalid values to be programmed into the RTC, this
> would catch that case.
> 

Which will be caught by the core anyway.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com