Re: [RFC 3/4] gpio: scmi: add SCMI pinctrl based gpio driver

[AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>]

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