From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 75F10E936F8 for ; Thu, 5 Oct 2023 02:43:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=l3zglDIKLarMTAHShzwcQe32nrtuzje7HsQhtk0HuKA=; b=NFtf8bmlGf9kiY 6/+Dt7s9OMATSovuloOWcYWqXhuvNjD5fIhGg0z08RsGuUkaPDAQx+/EzgsORiOns2V0qeoCm/Bst BVzbeTPfVjE0IzFXPbNNIgPmCeGSzZLSBRdlx3056ng8k11kjFY5uAbpetzzQTw7gWckg16t1WNvT kkFQbgwS5eOpdVVQ9LGyKXtA+QmH1P0DR57rShfleayjv8lfsyB8CR8rbZGwYyB6K5JVpPukbUrXp yCgbM1hGJgMMCO+avnWtxjM9SRiM+uL9+tdVlXISVwh7Ysme3iQGRUCjyW73GPNHAdWMif20wwmaO xaRqJfNMOJAuMQQEBpow==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qoEKM-001Fqb-0B; Thu, 05 Oct 2023 02:43:02 +0000 Received: from mail-pf1-x42f.google.com ([2607:f8b0:4864:20::42f]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qoEKH-001Fod-2s for linux-arm-kernel@lists.infradead.org; Thu, 05 Oct 2023 02:42:59 +0000 Received: by mail-pf1-x42f.google.com with SMTP id d2e1a72fcca58-692af7b641cso85213b3a.1 for ; Wed, 04 Oct 2023 19:42:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1696473772; x=1697078572; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=7eSqbR+iPZeaMwWM2FXXaj0hJ0gXvRe4tAkt/tdpAJ8=; b=tWmarBSsjKouuwSbB9P/8S3+qxPeILn0zf1HCXoPiLPCwXzckRcFXR/tPTYoEKzYuC 0vvxl+I9PXGgUbnG3yRIsOGknrxNB1TFzef8GOLvSOyIoiirAusSEn0reKYRFBL4cQEm TG4yua0GtORkbynaRDdBYUoNuiSVdHdleaZKXVhl88al1JhfZ1AN1cSp3sp3Gus1WNi8 jt0BQ5oGbkbl7sBbzDKmE8T7DZuZxaGQBFBSCOcuJHpybuZKqGsFEA4ypgWSNkLANBXd 3VhsqdhXdNJo7Lk0LMK2N5SlwUzJ1hafAi3z1IYdxomKbIdaDLRjDTKjor4yVsMIOzyY YdpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696473772; x=1697078572; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7eSqbR+iPZeaMwWM2FXXaj0hJ0gXvRe4tAkt/tdpAJ8=; b=ZZgCBMfaJB9f69ifBbO3/2SMfDmBAz7bxBDYB3WEX1mhYJOHwCbPvdrYACDkZdrxv6 H8CjcNIxQDppY5xzSzqTXQWXTWv1haWZGiY3yoTpa9520iyDnfjNQMgGrFhMMjyV7EhZ ubHkHj8ajUYWlYWl0tm1RCkT59NTtqg1NG6TPefR3SFcGzibbaORd8dx/Gv4Y9+w9x3w sqFOeQAJH8YW7RNESKX/ywNYRrkusnSDLfIhTvM9pPwN7f7m4RehpiiUXFIAVX549S45 WAiJO8xdmEar/KNhMt7CKcEG3tRKmvTr3R/3ifBcnG7YH1AYNbKwOrlDwoUMI9i27QDH h/Gg== X-Gm-Message-State: AOJu0YwwV9XBrqPQGTTW/X4mqCa7syBI5O+YZI/1YB/5KnKQeOCxDKYG Nf94kvftr9pchd+8Jxd3M0INAw== X-Google-Smtp-Source: AGHT+IFnnNjSS8t30msFADUxCDG59oqiybxhdToWtAwD2iUbLmzuZczi1sC1p6VvaSyzAAJNI9y5YA== X-Received: by 2002:a05:6a20:1584:b0:15d:f804:6907 with SMTP id h4-20020a056a20158400b0015df8046907mr4643492pzj.0.1696473772138; Wed, 04 Oct 2023 19:42:52 -0700 (PDT) Received: from octopus ([2400:4050:c3e1:100:a16d:fce2:497:afb7]) by smtp.gmail.com with ESMTPSA id d21-20020aa78695000000b00690c2cd7e0esm239580pfo.49.2023.10.04.19.42.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Oct 2023 19:42:51 -0700 (PDT) Date: Thu, 5 Oct 2023 11:42:47 +0900 From: AKASHI Takahiro To: Linus Walleij Cc: sudeep.holla@arm.com, cristian.marussi@arm.com, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, Oleksii_Moisieiev@epam.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org Subject: Re: [RFC 3/4] gpio: scmi: add SCMI pinctrl based gpio driver Message-ID: Mail-Followup-To: AKASHI Takahiro , Linus Walleij , sudeep.holla@arm.com, cristian.marussi@arm.com, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, Oleksii_Moisieiev@epam.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org References: <20231002021602.260100-1-takahiro.akashi@linaro.org> <20231002021602.260100-4-takahiro.akashi@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231004_194257_933809_620B3DBC X-CRM114-Status: GOOD ( 61.44 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Linus, On Wed, Oct 04, 2023 at 10:35:05AM +0200, Linus Walleij wrote: > Hi Takahiro, > > I see you are on track with this! > > Some clarifications: > > On Wed, Oct 4, 2023 at 8:53???AM AKASHI Takahiro > wrote: > > > I'm still not sure whether my approach can be applied to any other > > pinctrl-based gpio drivers, in which extra (driver-specific) operations > > might be needed around the generic pinctrl_gpio helpers (i.e. gpiolib.c). > > For instance, look at gpio-tegra.c: > > Yeah, it kind of requires a "pure" pin controller underneath that don't > want to do anything else on any operations, otherwise we are back > to a per-soc pin control driver. > > But I think it is appropriate for abstractions that strive to provide > "total abstraction behind a firmware", so such as SCMI or ACPI (heh). Right. So we are on the same page now. > > > Skip this, let's use device properties instead. They will anyways just translate > > > to OF properties in the OF case. > > > > Okay, I don't know how device properties work, though. > > They are pretty much 1-to-1 slot-ins for the corresponding of_* > functions, passing struct device * instead of struct device_node *, > if you look in include/linux/property.h you will feel at home very > quickly. > > > > > +static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) > > > > > > Rename all functions pinctrl_gpio_* > > > > Well, this change will result in name conflicts against existing > > pinctrl_gpio_direction_[in|out]out(). So use "pin_control_gpio_" prefix. > > Yeah that works, or pincontro_by_gpio_ or such. I will use "pin_control_gpio_", which still sounds confusing though. Please modify it if you don't like. > > Not sure how the last case (in_en && out_en && DRIVE_OPEN_DRAIN) works. > > I wrote some documentation! But it is hidden deep in the docs: > https://docs.kernel.org/driver-api/gpio/driver.html#gpio-lines-with-open-drain-source-support > > > In order to be able to read a value as an input pin, I think, we need > > to set the output status to Hi-Z. Then we should recognize it as "INPUT"? > > In this case, however, we cannot distinguish the other case where we want > > to use the pin as OUTPUT and drive it to (active) high. > > With open drain, on GPIO controllers that do not support a native > open drain mode, we emulate open drain output high by switching > the line into input mode. The line in this case has a pull-up resistor > (internal or external) and as input mode is high-Z the pull up resistor > will pull the signal high, to any level - could be e.g 48V which is > helpful for some serial links. I now think I see what you meant here, but still not sure why we need to assert CONFIG_INPUT and CONFIG_OUT at the same time from API viewpoint. Anyhow, I will follow the logic that you suggested. > But this case is really tricky so it can be hard to get things right, > I get a bit confused and so we need to think about it a few times. > > > > > +static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val) > > > > > > static int? > > > > Unfortunately, the function prototype of "set" in struct gpio_device is > > void (*set)(struct gpio_chip *gc, unsigned int offset, int value); > > > > So we cannot propagate an error to the caller. > > Grrr that must be my fault. Sorry about not fixing this :( > > > > No need to add & 0x01, the gpiolib core already does this. > > > > Which part of gpiolib core? > > chip->set = scmi_gpio_set; gets called like this in gpiolib: > > gpiod_direction_output_raw_commit(..., int value) > { > int val = !!value; > (...) > gc->set(gc, gpio_chip_hwgpio(desc), val); > > Notice clamping int val = !!value; will make the passed val 0 or 1. Yeah. > > > > +static u16 sum_up_ngpios(struct gpio_chip *chip) > > > > +{ > > > > + struct gpio_pin_range *range; > > > > + struct gpio_device *gdev = chip->gpiodev; > > > > + u16 ngpios = 0; > > > > + > > > > + list_for_each_entry(range, &gdev->pin_ranges, node) { > > > > + ngpios += range->range.npins; > > > > + } > > > > > > This works but isn't really the intended use case of the ranges. > > > Feel a bit uncertain about it, but I can't think of anything better. > > > And I guess these come directly out of SCMI so it's first hand > > > information about all GPIOs. > > > > I don't get your point. > > However many pins SCMI firmware (or other normal pin controllers) might > > expose, the total number of pins available by this driver is limited by > > "gpio-ranges" property. > > So the sum as "ngpios" should make sense unless a user accidentally > > specifies a wrong range of pins. > > Yes. > > And it is this fact that the same number need to appear in two places > and double-specification will sooner or later bring us to the situation > where the two do not agree, and what do we do then? > > If the ranges come from firmware, which is subject to change such > as "oops we forgot this pin", the GPIO number will just insert itself > among the already existing ones: say we have two ranges: > > 1: 0..5 > 2: 6..9 > > Ooops forgot a GPIO in the first range, it has to be bumped to > 0..6. > > But somewhere in the device tree there is: > > foo-gpios = <&scmi_gpio 7 GPIO_OUT_LOW>; > > So now this is wrong (need to be changed to 8) and we have zero tooling > to detect this, the author just has to be very careful all the time. Well, even without a change by an user, this kind of human error may happen. There is no way to verify the correct *pin number*, say, if I specify 100 instead of 7 in an above example. > But I honestly do not know any better way. One good practice to mitigate those cases might be to use a (gpio or gpio-group) name instead of a pin number, or a "virtual" gpio device. foo_gpio: gpio@0 { compatibles = "pin-control-gpio"; gpio-range = <&scmi_pinctrl 0 0 0>; gpio-range-group-name = "pins_for_foo"; } baa_gpio: gpio@1 { compatibles = "pin-control-gpio"; gpio-range = <&scmi_pinctrl 0 0 0>; gpio-range-group-name = "pins_for_baa"; } # Not sure multiple "pin-control-gpio" devices are possible. -Takahiro Akashi > > > which in turn becomes just pinctrl_gpio_set_config(), which > > > is what we want. > > > > > > The second cell in two-cell GPIOs already supports passing > > > GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE, > > > GPIO_PULL_UP, GPIO_PULL_DOWN, GPIO_PULL_DISABLE, > > > which you can this way trivially pass down to the pin control driver. > > > > > > NB: make sure the scmi pin control driver returns error for > > > unknown configs. > > > > Well, the error will be determined by SCMI firmware(server) > > not the driver itself :) > > Hehe, I think it is good that the SCMI firmware gets some exercise > from day 1! > > Yours, > Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel