From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Wed, 19 Jun 2013 16:40:48 +0200 Subject: [U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c In-Reply-To: References: <1371649453.2126.2.camel@phoenix> <201306191549.01906.marex@denx.de> Message-ID: <51C1C2F0.60909@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 19.06.2013 16:01, Axel Lin wrote: >>> - writel(1 << gpio, ®s->gpiodata[DATA_REG_ADDR(gpio)]); >>> + if (value) >>> + writel(1 << gpio, ®s->gpiodata[DATA_REG_ADDR(gpio)]); >>> + else >>> + writel(0, ®s->gpiodata[DATA_REG_ADDR(gpio)]); >> >> >> You might want to use clrbits_le32() and setbits_le32() here, no? >> > > Honestly, I ask this because I cannot find the detail datasheet for the gpio > control of this device. ( Just found the code looks strange). > > Usually we can use clrbits_le32() and setbits_le32() helpers to set/clear > register bits. But if the only meaningful bit is "1 << gpio", simply use > "write 1 << gpio" and "write 0" saves a register read operation. > > So maybe Stefan or someone from ST can provide the information about gpio > control on this hardware. I honestly have no idea how I tested this GPIO driver. That was too long ago. And I don't have the time to dig out the hardware to do some re-testing. But the code definitely looks wrong, so thanks for catching this. Let's wait what the ST engineers have to comment here. Thanks, Stefan