Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1539795757.4729.15.camel@pengutronix.de>
Subject: Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support
From:   Philipp Zabel <p.zabel@pengutronix.de>
To:     Maxime Ripard <maxime.ripard@bootlin.com>
Cc:     Cheng-yi Chiang <cychiang@chromium.org>,
        Mark Brown <broonie@kernel.org>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Ryan Lee <ryans.lee@maximintegrated.com>,
        alsa-devel@alsa-project.org, Dylan Reid <dgreid@chromium.org>,
        tzungbi@chromium.org
Date:   Wed, 17 Oct 2018 19:02:37 +0200
In-Reply-To: <20181012134620.fhhkyvvdgoq4sxqi@flea>
References: <20180912121955.33048-1-cychiang@chromium.org>
         <20180912121955.33048-2-cychiang@chromium.org>
         <20180920161905.GM2471@sirena.org.uk>
         <CAFv8NwLpS9r=jD2wyBsVgJJofkS1xJNo6YssgYXuVHDmGDLogg@mail.gmail.com>
         <1539338716.6204.2.camel@pengutronix.de>
         <20181012134620.fhhkyvvdgoq4sxqi@flea>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Maxime,

On Fri, 2018-10-12 at 15:46 +0200, Maxime Ripard wrote:
> On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:
[...]
> > What I would like better would be to let the consumers keep their reset-
> > gpios bindings, but add an optional hold time override in the DT:
> > 
> > 	c1 {
> > 		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> > 		reset-delays-us = <10000>;
> > 	};
> > 
> > 	c2 {
> > 		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> > 		reset-delays-us = <10000>;
> > 	};
> > 
> > The reset framework could create a reset_control backed by a gpio
> > instead of a rcdev. I have an unfinished patch for this, but without the
> > sharing requirement adding the reset framework abstraction between gpiod
> > and drivers never seemed really worth it.
> 
> I don't remember the exact details of our past discussion, but I
> (still) don't really think that this would work well.

It has been a while :) Thanks for jumping back in.

> I see two main shortcomings with that approach:
> 
>   - I guess that the main reason you want to do that would be to have
>     easy DT backward compatibility.

Yes, that is true. The other reason is that I'd like devices to have a
single binding, regardless of whether somebody decided to put them onto
a board with shared reset lines.

I'd find it hard to advocate for changing the thankfully common case
of device-exclusive reset gpios from:

	some-device {
		reset-gpios = <&gpiox y>;
	};

to:

	rstc: reset-controller {
		compatible = "gpio-reset";
		reset-gpios <&gpiox y>;
	};

	some-device {
		resets = <&rstc 0>;
	};

even for new bindings.

If the reset framework only supports the latter, and not the former,
drivers for devices which already have reset-gpios would have to handle
both reset_control and gpiod functionality, which I think should not be
necessary.

Note that this is not really an argument against a "gpio-reset"
controller driver, but an argument for reset-gpios support integrated
into the reset framework.
My argument against a "gpio-reset" device node in the DT is only that it
is basically a virtual device that would only be used to work around
missing reset-gpios support in the reset framework. Physically, this
"device" consists of no more than a few PCB traces.

>     Yet, the addition of the
>     reset-delay-us would break that compatibility, since drivers that
>     would have been converted now need to have it somehow, but older
>     DTs wouldn't have it

That is why such a property would have to be optional, and the drivers
would have to keep providing the reset delay themselves, as they
currently do when handling GPIOs directly.
It would only serve to override the driver default in case of additional
delay requirements due to board specifics (such as delay elements, or
shared resets).

>   - There's so many variations of the reset-gpios property name that
>     you wouldn't be able to cover all the cases anyhow, at least
>     without breaking the compatibility (again).

This is true, and I do not have a solution for this. Especially for
those cases that don't use gpiod yet.

	of_get_named_gpio(node, "reset-gpio")

is still quite common, but I'd really like on gpiolib for polarity
support.

> But I also see your point, and you're right that converting everyone
> to a gpio-reset node will not happen (even though I'd still really
> like to have that binding).

See above. I'd be a lot less reluctant to support this binding if
somebody could demonstrate a real gpio controlled reset controller
device of some kind. And even then I would not want to have to use
this device just to connect a single GPIO with a single reset input.

> What about having a function that would be called by the consumer to
> instantiate that reset controller from a GPIO for us, instead of
> trying to do it automatically? That function could take the property
> name that holds the GPIO, which would cover the second drawback, and
> the delay, which would cover the first.

I like this idea.

I'd like to avoid having to fall back from gpiod to of_get_named_gpio if
possible, so maybe we'd have to extend gpiolib-of.c with an
of_find_reset_gpio function, as is done for SPI and regulators already.

We probably would have to support delay ranges. I see a lot of
usleep_range calls between reset GPIO toggles in the drivers.

> And in order to cover shared GPIO reset lines, we could just look at
> the one already created by other drivers, and if one has been created,
> we just give the reference to that one instead of creating it.
> 
> Does that make sense?

I'm not quite sure how to match an already requested reset control from
the list given of_node and gpio property name at the moment - this might
involve exporting gpiod_find from gpiolib, but API wise I think this is
a sane proposal.

I would not want to add reset controller devices for each of these GPIO
reset controls, but rather store the gpio_desc reference in the
reset_control instead of the rcdev reference.

regards
Philipp