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 > <takahiro.akashi@xxxxxxxxxx> 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
