Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231002021602.260100-1-takahiro.akashi@linaro.org>
 <20231002021602.260100-4-takahiro.akashi@linaro.org> <CACRpkdZojfC2qr7gfzL9fj=DEYJcuPR=a1+zVWTMysK9BH_m_Q@mail.gmail.com>
 <ZR0L5Oe9oT/sEF2p@octopus>
In-Reply-To: <ZR0L5Oe9oT/sEF2p@octopus>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Wed, 4 Oct 2023 10:35:05 +0200
Message-ID: <CACRpkdY=5hYONDcXW4omcX7=r-JtH=AvOSVMkj72LKiaF_wJuA@mail.gmail.com>
Subject: Re: [RFC 3/4] gpio: scmi: add SCMI pinctrl based gpio driver
To:     AKASHI Takahiro <takahiro.akashi@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Takahiro,

I see you are on track with this!

Some clarifications:

On Wed, Oct 4, 2023 at 8:53=E2=80=AFAM AKASHI Takahiro
<takahiro.akashi@linaro.org> 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).

> > 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.

> 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-dr=
ain-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.

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 offse=
t, 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 =3D scmi_gpio_set; gets called like this in gpiolib:

 gpiod_direction_output_raw_commit(..., int value)
{
    int val =3D !!value;
(...)
    gc->set(gc, gpio_chip_hwgpio(desc), val);

Notice clamping int val =3D !!value; will make the passed val 0 or 1.

> > > +static u16 sum_up_ngpios(struct gpio_chip *chip)
> > > +{
> > > +       struct gpio_pin_range *range;
> > > +       struct gpio_device *gdev =3D chip->gpiodev;
> > > +       u16 ngpios =3D 0;
> > > +
> > > +       list_for_each_entry(range, &gdev->pin_ranges, node) {
> > > +               ngpios +=3D 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 =3D <&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.

But I honestly do not know any better way.

> > 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