Hi, Mark Brown <[email protected]> writes:
> On Tue, Dec 17, 2013 at 08:07:28PM +0100, Arnaud Ebalard wrote: >> >> Intersil ISL12057 is an I2C RTC chip also supporting two alarms. This >> patch only adds support for basic RTC functionalities (i.e. getting and >> setting time). Tests have been performed on NETGEAR ReadyNAS 102 w/ >> startup/shutdown scripts, hwclock, ntpdate and openntpd. > > Reviewed-by: Mark Brown <[email protected]> Thanks, Mark! >> +static int isl12057_rtc_proc(struct device *dev, struct seq_file *seq) >> +{ >> + struct isl12057_rtc_data *data = dev_get_drvdata(dev); >> + int reg, ret; >> + >> + mutex_lock(&data->lock); >> + >> + /* Status register */ >> + ret = regmap_read(data->regmap, ISL12057_REG_SR, ®); >> + if (ret) { >> + dev_err(dev, "%s: reading SR failed\n", __func__); >> + goto out; >> + } >> + >> + seq_printf(seq, "status_reg\t:%s%s%s (0x%.2x)\n", >> + (reg & ISL12057_REG_SR_OSF) ? " OSF" : "", >> + (reg & ISL12057_REG_SR_A1F) ? " A1F" : "", >> + (reg & ISL12057_REG_SR_A2F) ? " A2F" : "", reg); >> + >> + /* Control register */ >> + ret = regmap_read(data->regmap, ISL12057_REG_INT, ®); >> + if (ret) { >> + dev_err(dev, "%s: reading INT failed\n", __func__); >> + goto out; >> + } >> + >> + seq_printf(seq, "control_reg\t:%s%s%s%s%s%s (0x%.2x)\n", >> + (reg & ISL12057_REG_INT_A1IE) ? " A1IE" : "", >> + (reg & ISL12057_REG_INT_A2IE) ? " A2IE" : "", >> + (reg & ISL12057_REG_INT_INTCN) ? " INTCN" : "", >> + (reg & ISL12057_REG_INT_RS1) ? " RS1" : "", >> + (reg & ISL12057_REG_INT_RS2) ? " RS2" : "", >> + (reg & ISL12057_REG_INT_EOSC) ? " EOSC" : "", reg); >> + >> + out: >> + mutex_unlock(&data->lock); > > I'm not sure the lock is achieving anything any more - the only time it > actualy protects more than one operation is the above but since the > status register is volatile anyway it might change between it being read > and the control register being read. Well, I guess the lock will be more useful when alarm code will be added back, even if it does not hurt above. Now, regarding protection of regmap_bulk_write/read() calls in set/read time helpers, I also protected those because I thought it was good practice in general to serialize concurrent accesses to both the regmap structure and the device *in the driver*. But looking at the internals of regmap functions in more details, I guess I could have relied on those (they acquire an internal lock before action). Cheers, a+ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
