On Fri, Jun 26, 2009 at 08:08:41PM -0300, Daniel Ribeiro wrote: > Changed: Removed workaround for regulator use_count issues.
The usual place for these comments is after the --- with the diffstat (the tools can then deal automatically with them). Incidentally, the code you had would only help pxamci with this particular regulator driver. > Add (partial) support for the voltage regulators on the PCAP2 PMIC. > > Signed-off-by: Daniel Ribeiro <drw...@gmail.com> Sorry, I've noticed a race condition below that I didn't spot first time round below: > +static int pcap_regulator_set_voltage(struct regulator_dev *rdev, > + int min_uV, int max_uV) > +{ > + struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)]; > + void *pcap = rdev_get_drvdata(rdev); > + int uV; > + u32 tmp; > + u8 i; > + > + if (vreg->n_voltages == 1) > + return -EINVAL; It'd be a little more friendly to check if the supported voltage is in the requested range. However, that'd only be an issue if constraints allowed voltage changes which is obviously broken so a comment to that effect would be enough. I wouldn't have mentioned it but... > +{ > + struct pcap_regulator *vreg = &vreg_table[rdev_get_id(rdev)]; > + void *pcap = rdev_get_drvdata(rdev); > + u32 tmp; > + > + if (vreg->en == NA) > + return -EINVAL; > + > + ezx_pcap_read(pcap, vreg->reg, &tmp); > + tmp |= (1 << vreg->en); > + ezx_pcap_write(pcap, vreg->reg, tmp); This read/modify/write cycle is racy; the individual regulator is locked by the core but this register is shared between all regulators on the chip so if two are being updated at once things will go wrong. Most of the MFDs have a set_bits() function that does an atomic read/modify/write cycle for cases like this.