Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754637AbeAJNRN (ORCPT + 1 other); Wed, 10 Jan 2018 08:17:13 -0500 Received: from mail-io0-f195.google.com ([209.85.223.195]:33027 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754384AbeAJNRH (ORCPT ); Wed, 10 Jan 2018 08:17:07 -0500 X-Google-Smtp-Source: ACJfBou6OBpmiCiXQnZi76IvkJnxsKuL0KkI9hoxgGt/wydNGnizqltihjCWhWYequfIv0hBQKvi+IDio1+Whn8OTaE= MIME-Version: 1.0 In-Reply-To: <1515521301-4375-1-git-send-email-geert+renesas@glider.be> References: <1515521301-4375-1-git-send-email-geert+renesas@glider.be> From: Linus Walleij Date: Wed, 10 Jan 2018 14:17:06 +0100 Message-ID: Subject: Re: [PATCH] gpio: Add missing open drain/source handling to gpiod_set_value_cansleep() To: Geert Uytterhoeven Cc: Wolfram Sang , =?UTF-8?B?SmFuIEt1bmRyw6F0?= , =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org, Linux-Renesas , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Tue, Jan 9, 2018 at 7:08 PM, Geert Uytterhoeven wrote: > Since commit f11a04464ae57e8d ("i2c: gpio: Enable working over slow > can_sleep GPIOs"), probing the i2c RTC connected to an i2c-gpio bus on > r8a7740/armadillo fails with: > > rtc-s35390a 0-0030: error resetting chip > rtc-s35390a: probe of 0-0030 failed with error -5 > > More debug code reveals: > > i2c i2c-0: master_xfer[0] R, addr=0x30, len=1 > i2c i2c-0: NAK from device addr 0x30 msg #0 > s35390a_get_reg: ret = -6 > > Commit 02e479808b5d62f8 ("gpio: Alter semantics of *raw* operations to > actually be raw") moved open drain/source handling from > gpiod_set_raw_value_commit() to gpiod_set_value(), but forgot to take > into account that gpiod_set_value_cansleep() also needs this handling. > The i2c protocol mandates that i2c signals are open drain, hence i2c > communication fails. > > Fix this by adding the missing handling to gpiod_set_value_cansleep(), > using a new common helper __gpiod_set_value_nocheck(). > > Fixes: 02e479808b5d62f8 ("gpio: Alter semantics of *raw* operations to actually be raw") > Signed-off-by: Geert Uytterhoeven > --- > The bad gpio commit is in v4.15-rc1 and later. > The i2c commit exposing this is not yet upstream, but in i2c/for-next. Whoops! It appears Meltdown and Spectre saves my ass because now I have time to get this upstream before v4.15... Applied for fixes with the following tweaks: I removed the __prefix (I don't like them) and added kerneldoc. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 1693bb9ab72c..14532d9576e4 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2892,7 +2892,16 @@ void gpiod_set_raw_value(struct gpio_desc *desc, int value) } EXPORT_SYMBOL_GPL(gpiod_set_raw_value); -static void __gpiod_set_value_nocheck(struct gpio_desc *desc, int value) +/** + * gpiod_set_value_nocheck() - set a GPIO line value without checking + * @desc: the descriptor to set the value on + * @value: value to set + * + * This sets the value of a GPIO line backing a descriptor, applying + * different semantic quirks like active low and open drain/source + * handling. + */ +static void gpiod_set_value_nocheck(struct gpio_desc *desc, int value) { if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) value = !value; @@ -2919,7 +2928,7 @@ void gpiod_set_value(struct gpio_desc *desc, int value) { VALIDATE_DESC_VOID(desc); WARN_ON(desc->gdev->chip->can_sleep); - __gpiod_set_value_nocheck(desc, value); + gpiod_set_value_nocheck(desc, value); } EXPORT_SYMBOL_GPL(gpiod_set_value); @@ -3247,7 +3256,7 @@ void gpiod_set_value_cansleep(struct gpio_desc *desc, int value) { might_sleep_if(extra_checks); VALIDATE_DESC_VOID(desc); - __gpiod_set_value_nocheck(desc, value); + gpiod_set_value_nocheck(desc, value); } EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep); Yours, Linus Walleij