Re: [GIT PULL] On-demand device probing

[Alexandre Courbot]

On Tue, Oct 20, 2015 at 3:39 AM, Russell King - ARM Linux
 wrote:
> On Mon, Oct 19, 2015 at 08:27:44PM +0200, Uwe Kleine-König wrote:
>> Hello,
>>
>> On Mon, Oct 19, 2015 at 04:43:24PM +0100, Russell King - ARM Linux wrote:
>> > It's a bit ironic that you've chosen GPIO as an example there.  The
>> > "new" GPIO API (the gpiod_* stuff) only has a fwnode way to get the
>> > gpio descriptor.  There's no of_* method.
>>
>> Without following all that fwnode discussion:
>> gpiod_get et al. should work for you here, doesn't it? It just takes a
>> struct device * and I'm happy with it.
>
> What if you don't have a struct device?  I had that problem recently
> when modifying the mvebu PCIe code.  The 'struct device' node doesn't
> contain the GPIOs, it's the PCIe controller.  Individual ports on the
> controller are described in DT as sub-nodes, and the sub-nodes can
> have a GPIO for card reset purposes.  These sub-nodes don't have a
> struct device.
>
> Right now, I'm having to do this to work around this issue:
>
> reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> if (reset_gpio == -EPROBE_DEFER) {
> ret = reset_gpio;
> goto err;
> }
>
> if (gpio_is_valid(reset_gpio)) {
> unsigned long gpio_flags;
>
> port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
>   port->name);
> if (!port->reset_name) {
> ret = -ENOMEM;
> goto err;
> }
>
> if (flags & OF_GPIO_ACTIVE_LOW) {
> dev_info(dev, "%s: reset gpio is active low\n",
>  of_node_full_name(child));
> gpio_flags = GPIOF_ACTIVE_LOW |
>  GPIOF_OUT_INIT_LOW;
> } else {
> gpio_flags = GPIOF_OUT_INIT_HIGH;
> }
>
> ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> port->reset_name);
> if (ret) {
> if (ret == -EPROBE_DEFER)
> goto err;
> goto skip;
> }
>
> port->reset_gpio = gpio_to_desc(reset_gpio);
> }
>
> Not nice, is it?  Not nice to have that in lots of drivers either.
>
> However, switching to use any of_* or fwnode_* thing also carries with
> it another problem: you can't control the name appearing in the
> allocation, so you end up with a bunch of GPIOs requested with a "reset"
> name - meaning you lose any identification of which port the GPIO was
> bound to.

There are a few holes in the gpiod API. I see two solutions here:

1) extend devm_get_gpiod_from_child() to take an optional name argument
2) add a function to explicitly change a GPIO's name

2) seems to be the most generic solution, would that do the trick?

(sorry for the off-topic)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] gpio: tps65912: Add GPIO driver for the TPS65912 PMIC

[Alexandre Courbot]

+static const struct of_device_id tps65912_gpio_of_match_table[] = {
> +   { .compatible = "ti,tps65912-gpio", },
> +   { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, tps65912_gpio_of_match_table);
> +
> +static struct gpio_chip template_chip = {
> +   .label  = "tps65912-gpio",
> +   .owner  = THIS_MODULE,
> +   .direction_input= tps65912_gpio_input,
> +   .direction_output   = tps65912_gpio_output,
> +   .get= tps65912_gpio_get,
> +   .set= tps65912_gpio_set,
> +   .can_sleep  = true,
> +   .ngpio  = 5,
> +   .base   = -1,
> +};
> +
> +static int tps65912_gpio_probe(struct platform_device *pdev)
> +{
> +   struct tps65912_gpio *gpio;
> +   int ret;
> +
> +   gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +   if (!gpio)
> +   return -ENOMEM;
> +
> +   gpio->tps = dev_get_drvdata(pdev->dev.parent);
> +   gpio->gpio_chip = template_chip;
> +   ret = gpiochip_add(&gpio->gpio_chip);
> +   if (ret < 0) {
> +   dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
> +   return ret;
> +   }
> +
> +   platform_set_drvdata(pdev, gpio);
> +
> +   return 0;
> +}
> +
> +static int tps65912_gpio_remove(struct platform_device *pdev)
> +{
> +   struct tps65912_gpio *gpio = platform_get_drvdata(pdev);
> +
> +   gpiochip_remove(&gpio->gpio_chip);
> +
> +   return 0;
> +}
> +
> +static struct platform_driver tps65912_gpio_driver = {
> +   .driver = {
> +   .name = "tps65912-gpio",
> +   .of_match_table = tps65912_gpio_of_match_table,
> +   },
> +   .probe = tps65912_gpio_probe,
> +   .remove = tps65912_gpio_remove,
> +};
> +
> +module_platform_driver(tps65912_gpio_driver);
> +
> +MODULE_AUTHOR("Andrew F. Davis ");
> +MODULE_DESCRIPTION("TPS65912 GPIO driver");
> +MODULE_ALIAS("platform:tps65912-gpio");
> +MODULE_LICENSE("GPL v2");

Nice and simple driver - my remarks above should not be taken as a
call to re-spin, just consider them if you need to send another
version of this patchset.

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Userspace and Device-Tree

[Alexandre Courbot]

On Tue, Sep 22, 2015 at 10:54 PM, Chris Read  wrote:
> On Mon, Sep 21, 2015 at 4:55 PM, Linus Walleij  
> wrote:
>> On Wed, Sep 2, 2015 at 6:35 AM, Chris Read  wrote:
>>> There are some hardware aspects/parameters
>>> of exporting that aren't controllable from userspace, such as whether or not
>>> reversing the direction of a GPIO is safe.
>>
>> The original argument as to why kernel should handle hardware
>> is to keep things safe and under control.
>>
>> I don't understand this argument really, should the kernel give you
>> a gun but stop you from shooting yourself in the foot with it or
>> what do you mean? Then the stance of kernel not to give out guns
>> is better.
>
> From my embedded perspective a board designer wants to keep
> hardware safe and under control too.  He may want or need to expose
> controls or status to userspace applications, though, and what he
> wants to have exposed may vary from board to board.  I just feel that
> exposing them via the DT could be OK, whereas others do not.

It is really not a matter of feeling. The DT's job is to describe what
the hardware is and how it is connected together - not the way in
which it may be used.

In your case, the DT describes a given range of pins as not being used
(typically, any unclaimed GPIO) and the sysfs kernel option allows
these GPIOs to be exposed to user-space. It is then up to the
priviledged userspace (via init scripts and/or udev rules) to export
and configure these GPIOs in the way they are supposed to be by
regular users.

This basically covers what you wanted to do exclusively by DT, but
with a stronger role separation. IIUC a good old init script could
just do what you want, using chmod to restrict write access to a GPIO
value or the ability to change direction. I also suspect the same
could be achieved with udev (especially since this series has just
been merged: http://www.spinics.net/lists/linux-gpio/msg07844.html ,
see the link for an example of a rule).

Board differenciation can also be handled at boot time, with init
doing different things depending on the board model. Again, similarly
to what you would do with DT, but without the it being involved in
describing the system semantics.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] gpio: Use __gpiod_request directly

[Alexandre Courbot]

On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann  wrote:
> There is no reason to find out chip and hwnum to use to request a gpio
> and get another gpio descriptor. We already have the descriptor we want
> to use so we can directly use it.
>
> Signed-off-by: Markus Pargmann 
> ---
>  drivers/gpio/gpiolib.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 79a0b41ce57b..872fdd3617c1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>  int gpiod_hog(struct gpio_desc *desc, const char *name,
>   unsigned long lflags, enum gpiod_flags dflags)
>  {
> -   struct gpio_chip *chip;
> -   struct gpio_desc *local_desc;
> -   int hwnum;
> int status;
>
> -   chip = gpiod_to_chip(desc);
> -   hwnum = gpio_chip_hwgpio(desc);
> -
> -   local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> -   if (IS_ERR(local_desc)) {
> +   status = __gpiod_request(desc, name);
> +   if (status) {
> pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> -  name, chip->label, hwnum);
> -   return PTR_ERR(local_desc);
> +  name, gpiod_to_chip(desc)->label,
> +  gpio_chip_hwgpio(desc));
> +   return status;
> }
>
> status = gpiod_configure_flags(desc, name, lflags, dflags);
> if (status < 0) {
> pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> -  name, chip->label, hwnum);
> +  name, gpiod_to_chip(desc)->label, 
> gpio_chip_hwgpio(desc));
> gpiochip_free_own_desc(desc);

Mmm I should have reviewed this patch earlier, but what bothers me a
bit is that it breaks the symetry that we had by calling
request_own_desc() and free_own_desc() in the failing case (as well as
in gpiochip_free_hogs). And in the end you still need to call
gpiod_to_chip() so I am not sure what the benefit is.

Sure, the code is less verbose, but at the same time it has become
slightly harder to understand. Semantically speaking
"request_own_desc()" is exactly the action we want to convey.
__gpiod_request() is more ambiguous.

Note that this is not a reject, I just wanted to stress that "less
code" is not necessarily the same as "easier to read".
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: mention in DT binding doc that -gpio is also supported

[Alexandre Courbot]

On Sat, Sep 19, 2015 at 2:17 AM, Javier Martinez Canillas
 wrote:
> Hello Alexandre,
>
> On 09/18/2015 05:44 PM, Alexandre Courbot wrote:
>> On Thu, Sep 17, 2015 at 10:33 AM, Javier Martinez Canillas
>>  wrote:
>>> The GPIO DT binding doc mentions that GPIO are mapped by defining
>>> a -gpios property in the consumer device's node but a -gpio
>>> sufix is also supported after commit:
>>>
>>> dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names")
>>>
>>> Update the DT binding documentation to match the implementation.
>>>
>>> Signed-off-by: Javier Martinez Canillas 
>>>
>>> ---
>>> Hello,
>>>
>>> The GPIO documentation was updated to mention that the -gpio sufix
>>> is also supported in patch https://lkml.org/lkml/2015/9/1/117 that
>>> already landed in Torvalds tree.
>>>
>>> I now noticed that the DT binding also only mentions -gpios so I'm
>>> posting this patch that adds -gpio to the DT binding documentation.
>>
>> I think I remember that -gpio is considered obsolete and its use
>> should thus not be encouraged, which is the reason why the
>> documentation does not mention it. We could mention it and add a note
>> saying that it should not be used for new bindings, but all in all
>> isn't it better to keep the documentation clear of such use cases that
>> will not be accepted for new patches anyway?
>>
>
> I agree that if that's the case then it should be documented. Currently
> by reading the docs there is no way to tell if -gpio was only added to
> support old DT bindings and should not be used or if is that the docs
> were not updated when -gpio parsing was added to gpiolib.
>
> I can re-spin the patch making it clear that even when the -gpio suffix
> is supported, it's only there for compatibility reasons and should not
> be used for newer bindings.
>
> And also Documentation/gpio/board.txt has to be updated now since now it
> mentions -gpio but does not say that should not be used.

Sounds good. Thanks for taking care of this!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: mention in DT binding doc that -gpio is also supported

[Alexandre Courbot]

On Thu, Sep 17, 2015 at 10:33 AM, Javier Martinez Canillas
 wrote:
> The GPIO DT binding doc mentions that GPIO are mapped by defining
> a -gpios property in the consumer device's node but a -gpio
> sufix is also supported after commit:
>
> dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names")
>
> Update the DT binding documentation to match the implementation.
>
> Signed-off-by: Javier Martinez Canillas 
>
> ---
> Hello,
>
> The GPIO documentation was updated to mention that the -gpio sufix
> is also supported in patch https://lkml.org/lkml/2015/9/1/117 that
> already landed in Torvalds tree.
>
> I now noticed that the DT binding also only mentions -gpios so I'm
> posting this patch that adds -gpio to the DT binding documentation.

I think I remember that -gpio is considered obsolete and its use
should thus not be encouraged, which is the reason why the
documentation does not mention it. We could mention it and add a note
saying that it should not be used for new bindings, but all in all
isn't it better to keep the documentation clear of such use cases that
will not be accepted for new patches anyway?

>
> Best regards,
> Javier
>
>  Documentation/devicetree/bindings/gpio/gpio.txt | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
> b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 5788d5cf1252..8db8c7bb96c1 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -13,10 +13,10 @@ properties, each containing a 'gpio-list':
> gpio-specifier : Array of #gpio-cells specifying specific gpio
>  (controller specific)
>
> -GPIO properties should be named "[-]gpios", with  being the 
> purpose
> -of this GPIO for the device. While a non-existent  is considered valid
> -for compatibility reasons (resolving to the "gpios" property), it is not 
> allowed
> -for new bindings.
> +GPIO properties should be named "[-]gpios" or "[-]gpio" with 
> 
> +being the purpose of this GPIO for the device. While a non-existent  is
> +considered valid for compatibility reasons (resolving to the "gpios" 
> property),
> +it is not allowed for new bindings.
>
>  GPIO properties can contain one or more GPIO phandles, but only in 
> exceptional
>  cases should they contain more than one. If your device uses several GPIOs 
> with
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Userspace and Device-Tree

[Alexandre Courbot]

On Tue, Sep 1, 2015 at 4:20 AM, Chris Read  wrote:
> From what I see the standard kernel allows exposing GPIOs to userspace.
> There are kernel calls to do so.  It seems to be recognized as an important
> feature.  The standard kernel also has a regulator-consumer whose whole
> purpose is to expose control of a regulator to userspace.  So it looks like
> such userspace control is intended.
>
> My concern is that, while these features are appropriate to expose in
> userspace, I can't expose them using what are now the standard ways
> (i.e. without a board file or customized driver for each board).  I'd like
> to have a way to use the same kernel for all our boards, and have a
> configuration that can be provided that will expose the required ones.
>
> It may be possible to make it a set of kernel command line parameters,
> which would then be customized for each board.  There are, no doubt,
> still other ways to accomplish this board-specific customization.
> However, since there is already a board-specific mechanism to do
> kernel driver/module configuration (i.e. DT), it seemed like a good idea
> to piggy-back on it.
>
> Obviously exporting to userspace isn't a hardware configuration
> parameter, but the mechanism of hooking up and configuring drivers
> that DT uses looks to me like the most efficient way implement this.

Since the GPIO sysfs allows you to request any GPIO that is not
claimed by a kernel driver from userspace, is there a reason why you
cannot simply run board-specific init scripts that request and
configure the GPIOs you need? Once GPIO naming is merged, this should
be as elegant as it gets for what you want to do.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Userspace and Device-Tree

[Alexandre Courbot]

>> Similarly, I'd like to be able to signal the export of a GPIO to userspace
>> (/sys/class/gpio/gpioWXY) without a customized board file.  Since we're
>> using DT to specify the hardware in a system, it would be a convenient place
>> to specify exporting GPIOs to userspace.
>
> I believe that the GPIO subsystem maintainers (Linus Walleij and
> Alexandre Courbot) were trying to make GPIO control from userspace more
> consistent (independent of DT) for a while. However I am not all that
> familiar with the GPIO subsystem and may have misunderstood some of the
> work that's going on there.

If I properly understood what Chris wants, he may want to have a look
at the following works:

- GPIO hogging mechanism to configure a GPIO using the device tree -
not sure if you can also export at that time?
- Markus' ongoing work on GPIO naming:
http://www.spinics.net/lists/linux-gpio/msg07844.html
- Device tree overlays, to separate the configuration of GPIOs for
user-space use (which is arguably specific to one's use case) from the
rest of the board's definition

With these 3 works I think Chris' needs would be fulfilled, at least
as far as GPIOs are concerned.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Alexandre Courbot]

On Tue, Apr 28, 2015 at 3:45 PM, Uwe Kleine-König
 wrote:
> Hello,
>
> On Tue, Apr 28, 2015 at 12:31:37PM +0900, Alexandre Courbot wrote:
>> On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König
>>  wrote:
>> > On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote:
>> >> Having GPIO disabled means there is no GPIO support, including the
>> >> ability to look for GPIOs. -ENOSYS is a well-documented error-code
>> >> which meaning also applies to the gpio_*_optional functions (we don't
>> >> have support for the operation you requested). If a driver or
>> >> architecture really, really needs GPIO support they can require or
>> >> depend on CONFIG_GPIOLIB, and the problem goes away. If they can work
>> >> with and without gpiolib, then they should check for -ENOSYS when they
>> >> request GPIOs and behave accordingly.
>> > What whould be the right behaviour in your eyes? I hope it's not
>> >
>> > if (ret != -ENOSYS)
>> > return ret;
>> >
>> > /* continue and ignore error */
>>
>> If a consumer absolutely needs a GPIO (most of the drivers out there I
>> believe), then -ENOSYS can be handled like any other error. If it
>> doesn't, and the driver is fine without GPIO support as well (meaning
>> that it can somehow work even if a GPIO is declared, but GPIO support
>> is not compiled in), then it will need to explicitly handle that
>> particular error. That case should be rare though - most drivers will
>> want to propagate -ENOSYS.
> Ack.

Of course if you can think of a more elegant way to handle this, by
all means, please let us know. :)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Alexandre Courbot]

On Tue, Apr 28, 2015 at 12:21 AM, Uwe Kleine-König
 wrote:
> Hello,
>
> On Thu, Apr 09, 2015 at 11:20:55AM +0900, Alexandre Courbot wrote:
>> I should have replied one month ago, but if gpiolib is disabled, how
>> can we use gpiolib-like logic to check the existence of a GPIO?
>>
>> Having GPIO disabled means there is no GPIO support, including the
>> ability to look for GPIOs. -ENOSYS is a well-documented error-code
>> which meaning also applies to the gpio_*_optional functions (we don't
>> have support for the operation you requested). If a driver or
>> architecture really, really needs GPIO support they can require or
>> depend on CONFIG_GPIOLIB, and the problem goes away. If they can work
>> with and without gpiolib, then they should check for -ENOSYS when they
>> request GPIOs and behave accordingly.
> What whould be the right behaviour in your eyes? I hope it's not
>
> if (ret != -ENOSYS)
> return ret;
>
> /* continue and ignore error */

If a consumer absolutely needs a GPIO (most of the drivers out there I
believe), then -ENOSYS can be handled like any other error. If it
doesn't, and the driver is fine without GPIO support as well (meaning
that it can somehow work even if a GPIO is declared, but GPIO support
is not compiled in), then it will need to explicitly handle that
particular error. That case should be rare though - most drivers will
want to propagate -ENOSYS.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: let gpiod_get_optional et all return NULL when GPIOLIB is not enabled

[Alexandre Courbot]

On Fri, Mar 6, 2015 at 5:59 PM, Uwe Kleine-König
 wrote:
> Hello,
>
> On Fri, Mar 06, 2015 at 09:26:26AM +0100, Linus Walleij wrote:
>> On Thu, Feb 12, 2015 at 10:03 AM, Uwe Kleine-König
>>  wrote:
>>
>> > I wonder if gpiod_get_optional et all should be changed to return NULL
>> > instead.
>>
>> This is actually the norm in most subsystems returning cookie
>> pointers, clk, regulator, pinctrl... a NULL pointer is a functional
>> noop. But normally then NULL is returned from all stubs, not
>> just optional.
>>
>> Alexandre, what do you say?
>>
>> > The obvious downside is that if the device tree specifies a
>> > reset-gpio and the kernel just fails to use it because there is some
>> > code missing, this should better be an error. (The adau1977 code has
>> > this problem already know, but when changing devm_gpiod_get_optional all
>> > callers are affected.)
>>
>> Device Tree-specific problems is not something we design
>> subsystems for, we try to just accomodate them. I'm not
>> sure I fully understand what you mean here.
> Consider you have a device that has:
>
> enable-gpio = <&gpio3 12 0>;
>
> and you do in your driver:
>
> enablegpio = devm_gpiod_get_optional(... "enable" ...);
>
> . If GPIOLIB is off, enablegpio gets assigned NULL and the driver
> continues happily without enabling the device which most likely is a
> bug.
>
> So IMHO the logic in devm_gpiod_get_optional for the GPIOLIB=n case
> should be:
>
> if (device_has_gpio())
> return ERR_PTR(-ENOSYS);
> else
> return NULL;
>
> . device_has_gpio should use similar logic like gpiod_get_index to check
> if there is a gpio. If this is considered to be too complicated for a
> disabled subsystem, returning -ENOSYS unconditionally is better than
> NULL.

I should have replied one month ago, but if gpiolib is disabled, how
can we use gpiolib-like logic to check the existence of a GPIO?

Having GPIO disabled means there is no GPIO support, including the
ability to look for GPIOs. -ENOSYS is a well-documented error-code
which meaning also applies to the gpio_*_optional functions (we don't
have support for the operation you requested). If a driver or
architecture really, really needs GPIO support they can require or
depend on CONFIG_GPIOLIB, and the problem goes away. If they can work
with and without gpiolib, then they should check for -ENOSYS when they
request GPIOs and behave accordingly.

Moving the interpretation of what the absence of gpiolib means down to
the GPIO functions themselves is actually what might lead consumers to
not know the result of their request. For this reason I would say that
-ENOSYS is appropriate here.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On Wed, Mar 18, 2015 at 3:27 PM, Tomeu Vizoso
 wrote:
> On 18 March 2015 at 06:10, MyungJoo Ham  wrote:
>>> Hello,
>>>
>>> something happened during the last cycle and an old version of the devfreq
>>> driver was merged.
>>>
>>> This thread contains patches that bring it up to date to the last submitted
>>> version and also incorporates the feedback that that version received, plus
>>> some other small fixes and improvements that came up during rebase and
>>> testing.
>>>
>>> These patches implement support for setting the rate of the EMC clock based 
>>> on
>>> stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
>>> memory accesses (among others).
>>>
>>> It depends on the following in-flight patches:
>>>
>>> * EMC driver: http://thread.gmane.org/gmane.linux.kernel/1907035
>>> * CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1897078
>>>
>>> I have pushed a branch here for testing:
>>>
>>> http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=actmon-v6
>>>
>>> Regards,
>>>
>>> Tomeu
>>>
>>> Tomeu Vizoso (8):
>>>   of: Add binding for NVIDIA Tegra ACTMON node
>>>   PM / devfreq: tegra: Update to v5 of the submitted patches
>>>   clk: tegra: Have EMC clock implement determine_rate()
>>>   PM / devfreq: tegra: Use clock rate constraints
>>>   PM / devfreq: tegra: remove operating-points
>>>   PM / devfreq: tegra: Set drvdata before enabling the irq
>>>   PM / devfreq: tegra: Enable interrupts after resuming the devfreq
>>> monitor
>>>   ARM: tegra: Add Tegra124 ACTMON support
>>
>> Acked-by: MyungJoo Ham 
>> for all PM / devfreq patches (2, 4, 5, 6, 7)
>> And merged in for-rc tree with a little modification.
>
> Thanks. Though that's fine with me, I was wondering if Mikko or
> Alexandre would have any comments on the changes, even if they are
> small regarding what they already reviewed.

After a quick look I think I'm good with it. I will try to look in
further detail. If there is anything wrong, we can fix it with fixup
patches, since what has been merged is good stuff already!

I guess it would now be interesting to look at the watermark support
for devfreq. I am not sure if anything is happening to it? Arto?

https://lkml.org/lkml/2014/12/5/262
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] pinctrl: tegra: add a driver for Tegra210

[Alexandre Courbot]

On Wed, Feb 25, 2015 at 6:00 AM, Stephen Warren  wrote:
> From: Stephen Warren 
>
> Tegra210's pinmux supports a different set of pins/options than earlier
> SoCs, so requires its own driver (well, table of pin-specific data).
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Stephen Warren 

The series, and this patch in particular,

Tested-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 06/15] of: Add Tegra124 EMC bindings

[Alexandre Courbot]

On Thu, Feb 12, 2015 at 11:06 PM, Tomeu Vizoso
 wrote:
> From: Mikko Perttunen 
>
> Add binding documentation for the nvidia,tegra124-emc device tree node.
>
> Signed-off-by: Mikko Perttunen 
> Signed-off-by: Tomeu Vizoso 
>
> ---
>
> v5: * Add a short description for each of the register properties
>
> v4: * Remove mandatory naming of the timings subnode
> * Remove constraint on the unit-address of the timings and timing 
> subnodes
> * Add some more information about nvidia,emc-configuration
> * Make the example complete
>
> v2: * Specify the unit addresses for the timings and timing nodes
> ---
>  .../bindings/memory-controllers/tegra-emc.txt  | 379 
> +
>  1 file changed, 379 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
>
> diff --git 
> a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt 
> b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
> new file mode 100644
> index 000..da923b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
> @@ -0,0 +1,379 @@
> +NVIDIA Tegra124 SoC EMC (external memory controller)
> +
> +
> +Required properties :
> +- compatible : Should be "nvidia,tegra124-emc".
> +- reg : physical base address and length of the controller's registers.
> +- nvidia,memory-controller : phandle of the MC driver.
> +
> +The node should contain a "emc-timings" subnode for each supported RAM type 
> (see field RAM_CODE in
> +register PMC_STRAPPING_OPT_A), with its unit address being its RAM_CODE.
> +
> +Required properties for "emc-timings" nodes :
> +- nvidia,ram-code : Should contain the value of RAM_CODE this timing set is 
> used for.
> +
> +Each "emc-timings" node should contain a "timing" subnode for every 
> supported EMC clock rate. The
> +"timing" subnodes should have the clock rate in Hz as their unit address.
> +
> +Required properties for "timing" nodes :
> +- clock-frequency : Should contain the memory clock rate in Hz.
> +- The following properties contain EMC timing characterization values 
> (specified in the board
> +documentation) :
> +  - nvidia,emc-auto-cal-config : EMC_AUTO_CAL_CONFIG
> +  - nvidia,emc-auto-cal-config2 : EMC_AUTO_CAL_CONFIG2
> +  - nvidia,emc-auto-cal-config3 : EMC_AUTO_CAL_CONFIG3
> +  - nvidia,emc-auto-cal-interval : EMC_AUTO_CAL_INTERVAL
> +  - nvidia,emc-bgbias-ctl0 : EMC_BGBIAS_CTL0
> +  - nvidia,emc-cfg : EMC_CFG
> +  - nvidia,emc-cfg-2 : EMC_CFG_2
> +  - nvidia,emc-cfg-dig-dll : EMC_CFG_DIG_DLL
> +  - nvidia,emc-ctt-term-ctrl : EMC_CTT_TERM_CTRL
> +  - nvidia,emc-mode-1 : Mode Register 1
> +  - nvidia,emc-mode-2 : Mode Register 2
> +  - nvidia,emc-mode-4 : Mode Register 4
> +  - nvidia,emc-mode-reset : Mode Register 0
> +  - nvidia,emc-mrs-wait-cnt : EMC_MRS_WAIT_CNT
> +  - nvidia,emc-sel-dpd-ctrl : EMC_SEL_DPD_CTRL
> +  - nvidia,emc-xm2dqspadctrl2 : EMC_XM2DQSPADCTRL2
> +  - nvidia,emc-zcal-cnt-long : EMC_ZCAL_WAIT_CNT
> +  - nvidia,emc-zcal-interval : EMC_ZCAL_INTERVAL
> +- nvidia,emc-configuration : EMC timing characterization data. These are the 
> registers (see section
> +"15.6.2 EMC Registers" in the TRM) whose values need to be specified, 
> according to the board
> +documentation:

I'm a little bit confused by this. On the one hand, some registers are
defined by dedicated DT properties, and on the other some are simply
specified in the nvidia,emc-configuration array. I guess the rationale
for this is to isolate the registers whose value may control the
driver vs. those that simply needs to be written as-is.

But in this case, why are some registers (like EMC_CFG_DIG_DLL)
present in both lists, sometimes with different values? In the case of
EMC_CFG_DIG_DLL, I also see that the read value of
"nvidia,emc-cfg-dig-dll" seems to never be used anywhere in
tegra124-emc.c. Should this property exist at all? Note that I have
only checked this one, there might be others in the same case.

Or maybe I completely misunderstood the intent here, in which case,
please enlighten me. :)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 00/11] Improvements to Tegra-based Chromebook support

[Alexandre Courbot]

On Thu, Feb 12, 2015 at 5:50 PM, Tomeu Vizoso
 wrote:
> v5: * Moved to use gpio-restart for reboots, had to make tegra_pmc_restart
> a notification handler
>
> v4: * Added support for the system reset GPIO, for proper reboots
> * Moved out changes to ASOC to their own series, as requested by Mark
> Brown
> * Added patch to reset the SOR, to make sure it's in a known state
> * Changed nvidia,model property of the sound nodes to GoogleNyanBig
> and GoogleNyanBlaze so they can be told apart in userspace
>
> v3: * Added bindings for the LTN140AT29 panel
> * Removed the delay in pwrseq, as what was actually needed was to add
> a dependency on the power supplies of the host
> * Uses the pinmux for the Blaze as generated by tegra-pinmux-scripts
> * Uses the pinmux for the Big as in the last patch from Simon Glass
>
> Hello,
>
> this series adds support for the Tegra-based HP Chromebook 14 (aka nyan
> blaze), which is very similar to the Acer Chromebook 13 (aka nyan big).
> Because they both include tegra124-nyan.dtsi, some improvements to Blaze
> support have also benefitted the Big. I have tested that USB2, the panels,
> HDMI, the trackpad, Wifi and sound work on both.
>
> The leaf DTs contain the whole pinmux configuration as generated by
> tegra-pinmux-scripts. I chose to not put the common configuration in the
> common dtsi so we can paste the output as is and be sure that the kernel
> doesn't diverge from the canonical data.

FWIW, the series:

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v6 0/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

Linus, this looks good to me in its current form, what are your thoughts?

On Tue, Feb 3, 2015 at 2:44 AM, Benoit Parrot  wrote:
> This patch set re-introduces the gpio hogging concept first
> presented by Boris Brezillion.
> This patch set provides a way to initially configure specific GPIO
> when the GPIO controller is probed.
>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of of_gpiochip_add().
>
> The purpose of this is to allow specific GPIOs to be configured
> without any driver specific code.
> This is particularly useful because board design are getting
> increasingly complex and given SoC pins can now have more
> than 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes.
>
> Specific drivers should not necessarily need to be aware of
> what accounts to a specific board implementation. This board level
> "description" should be best kept as part of the dts file.
>
> Changes since v5:
>  * Addressed review comment from Linus Walleij
>  * Replace "state" property back with separate boolean properties
>  * Renamed helper function
>  * Refactored pr_* calls to remove "__func__"
>
> Changes since v4:
>  * Addressed review comments from Alexandre Courbot
>
> Changes since v3:
>  * Relocated the non-DT "hog" function to gpiolib.c.
>  * Rename some of the function to be clearer and remove _ prefixes.
>  * Replace the gpiod_request/gpiod_put usage with
>gpiochip_request_own_desc/free_own_desc version instead.
>  * Refactor some of the logic to better handle error condition/reporting
>  * Renamed the "direction" DT properties to "state".
>
> Changes since v2:
>  * Refactor the gpio-hog mechanism to split the DT related action
>from the actual "hogging" operation.
>  * This allows non-DT providers to implement hogs as well.
>  * Added FLAG_IS_HOGGED to mark hogged gpio and make gpiochip removal
>able to release hogged gpio.
>  * Similarly to the hogging, the cleanup is performed as part of
>of_gpiochip_remove
>
> Changes since v1:
>  * Split the devicetree bindings documentation in its own patch.
>  * Refactor the gpio-hog mechanism as private functions meant to
>be to invoked from of_gpiochip_add().
>
>
> Benoit Parrot (2):
>   gpio: add GPIO hogging mechanism
>   gpio: Document GPIO hogging mechanism
>
>  Documentation/devicetree/bindings/gpio/gpio.txt |  30 ++
>  drivers/gpio/gpiolib-of.c   | 111 +
>  drivers/gpio/gpiolib.c  | 124 
> 
>  drivers/gpio/gpiolib.h  |   3 +
>  4 files changed, 249 insertions(+), 19 deletions(-)
>
> --
> 1.8.5.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v6 0/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Feb 19, 2015 at 2:23 AM, Benoit Parrot  wrote:
> Gentle ping.
>
> Is there any chance this will make it in 3.21?

I'm good with it - Linus will probably come to it after the 3.20 merge
window closes.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 3/4] mmc: pwrseq: Initial support for the simple MMC power sequence provider

[Alexandre Courbot]

On Wed, Feb 11, 2015 at 1:28 PM, Ulf Hansson  wrote:
> [...]
>
>>> +int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
>>> +{
>>> +   struct mmc_pwrseq_simple *pwrseq;
>>> +
>>> +   pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL);
>>> +   if (!pwrseq)
>>> +   return -ENOMEM;
>>> +
>>> +   pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>>> +   host->pwrseq = &pwrseq->pwrseq;
>>
>> How about making this function return a struct mmc_pwrseq * so this
>> last line can be moved to mmc_pwrseq_alloc() instead of requiring all
>> power sequences to do it?
>>
>> The same applies to
>>
>> host->pwrseq = NULL;
>>
>> in mmc_pwrseq_simple_free(), which could be done in mmc_pwrseq_free() it 
>> seems.
>
> Thanks for reviewing!
>
> I like you suggestion, though $subject patch is already part of the PR for 
> 3.20.
>
> Feel free to send a patch, I would happily apply it.

Damn, why am I always so late to review patches...

I will send a fixup patch - better to have this done early.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 4/4] mmc: pwrseq_simple: Add support for a reset GPIO pin

[Alexandre Courbot]

On Mon, Jan 19, 2015 at 6:13 PM, Ulf Hansson  wrote:
> The need for reset GPIOs has several times been pointed out from
> erlier posted patchsets. Especially some WLAN chips which are
> attached to an SDIO interface may use a GPIO reset.
>
> The reset GPIO is asserted at initialization and prior we start the
> power up procedure. The GPIO will be de-asserted right after the power
> has been provided to the card, from the ->post_power_on() callback.
>
> Note, the reset GPIO is optional. Thus we don't return an error even if
> we can't find a GPIO for the consumer.
>
> Signed-off-by: Ulf Hansson 

Excellent, with this I can probe the wifi chip on NVIDIA SHIELD which
requires a GPIO to be high for reset to be de-asserted.

The series:

Tested-by: Alexandre Courbot 

> ---
>
> Changes in v4:
> - Fixed call to kfree().
>
> ---
>  drivers/mmc/core/pwrseq_simple.c | 38 ++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/mmc/core/pwrseq_simple.c 
> b/drivers/mmc/core/pwrseq_simple.c
> index 61c991e..0958c69 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -11,6 +11,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>
> @@ -18,31 +19,68 @@
>
>  struct mmc_pwrseq_simple {
> struct mmc_pwrseq pwrseq;
> +   struct gpio_desc *reset_gpio;
>  };
>
> +static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
> +{
> +   struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
> +   struct mmc_pwrseq_simple, pwrseq);
> +
> +   if (!IS_ERR(pwrseq->reset_gpio))
> +   gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
> +}
> +
> +static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
> +{
> +   struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
> +   struct mmc_pwrseq_simple, pwrseq);
> +
> +   if (!IS_ERR(pwrseq->reset_gpio))
> +   gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
> +}
> +
>  static void mmc_pwrseq_simple_free(struct mmc_host *host)
>  {
> struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
> struct mmc_pwrseq_simple, pwrseq);
>
> +   if (!IS_ERR(pwrseq->reset_gpio))
> +   gpiod_put(pwrseq->reset_gpio);
> +
> kfree(pwrseq);
> host->pwrseq = NULL;
>  }
>
>  static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
> +   .pre_power_on = mmc_pwrseq_simple_pre_power_on,
> +   .post_power_on = mmc_pwrseq_simple_post_power_on,
> +   .power_off = mmc_pwrseq_simple_pre_power_on,
> .free = mmc_pwrseq_simple_free,
>  };
>
>  int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
>  {
> struct mmc_pwrseq_simple *pwrseq;
> +   int ret = 0;
>
> pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL);
> if (!pwrseq)
> return -ENOMEM;
>
> +   pwrseq->reset_gpio = gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH);

gpiod_get() will translate to exactly the same, with less characters.

Actually I see that the version in -next has support for multiple
GPIOs. You will probably want to look at Rojhalat's latest work on
GPIO arrays:

http://permalink.gmane.org/gmane.linux.kernel.gpio/6126

This code would be a great candidate to use this GPIO array API, but
since it is not in -next yet (should happen soon though) you might
want to consider doing it later.

Btw, I wasted a considerable amount of time on one of the defunct
previous attempts at power sequences, so I'm interested in reviewing
future versions of this patchset if you don't mind adding me to the CC
list. :)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 3/4] mmc: pwrseq: Initial support for the simple MMC power sequence provider

[Alexandre Courbot]

On Mon, Jan 19, 2015 at 6:13 PM, Ulf Hansson  wrote:
> To add the core part for the MMC power sequence, let's start by adding
> initial support for the simple MMC power sequence provider.
>
> In this initial step, the MMC power sequence node are fetched and the
> compatible string for the simple MMC power sequence provider are
> verified.
>
> At this point we don't parse the node for any properties, but instead
> that will be handled from following patches. Since there are no
> properties supported yet, let's just implement the ->alloc() and the
> ->free() callbacks.
>
> Signed-off-by: Ulf Hansson 
> ---
>
> Changes in v4:
> - Fixed call to kfree().
>
> ---
>  drivers/mmc/core/Makefile|  2 +-
>  drivers/mmc/core/pwrseq.c| 61 
> +++-
>  drivers/mmc/core/pwrseq.h|  2 ++
>  drivers/mmc/core/pwrseq_simple.c | 48 +++
>  4 files changed, 111 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/mmc/core/pwrseq_simple.c
>
> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> index ccdd35f..b39cbd2 100644
> --- a/drivers/mmc/core/Makefile
> +++ b/drivers/mmc/core/Makefile
> @@ -8,5 +8,5 @@ mmc_core-y  := core.o bus.o host.o \
>sdio.o sdio_ops.o sdio_bus.o \
>sdio_cis.o sdio_io.o sdio_irq.o \
>quirks.o slot-gpio.o
> -mmc_core-$(CONFIG_OF)  += pwrseq.o
> +mmc_core-$(CONFIG_OF)  += pwrseq.o pwrseq_simple.o
>  mmc_core-$(CONFIG_DEBUG_FS)+= debugfs.o
> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
> index bd08772..2cea00e 100644
> --- a/drivers/mmc/core/pwrseq.c
> +++ b/drivers/mmc/core/pwrseq.c
> @@ -7,14 +7,73 @@
>   *
>   *  MMC power sequence management
>   */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
>  #include 
>
>  #include "pwrseq.h"
>
> +struct mmc_pwrseq_match {
> +   const char *compatible;
> +   int (*alloc)(struct mmc_host *host, struct device *dev);
> +};
> +
> +static struct mmc_pwrseq_match pwrseq_match[] = {
> +   {
> +   .compatible = "mmc-pwrseq-simple",
> +   .alloc = mmc_pwrseq_simple_alloc,
> +   },
> +};
> +
> +static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
> +{
> +   struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
> +   int i;
> +
> +   for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
> +   if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
> +   match = &pwrseq_match[i];
> +   break;
> +   }
> +   }
> +
> +   return match;
> +}
>
>  int mmc_pwrseq_alloc(struct mmc_host *host)
>  {
> -   return 0;
> +   struct platform_device *pdev;
> +   struct device_node *np;
> +   struct mmc_pwrseq_match *match;
> +   int ret = 0;
> +
> +   np = of_parse_phandle(host->parent->of_node, "mmc-pwrseq", 0);
> +   if (!np)
> +   return 0;
> +
> +   pdev = of_find_device_by_node(np);
> +   if (!pdev) {
> +   ret = -ENODEV;
> +   goto err;
> +   }
> +
> +   match = mmc_pwrseq_find(np);
> +   if (IS_ERR(match)) {
> +   ret = PTR_ERR(match);
> +   goto err;
> +   }
> +
> +   ret = match->alloc(host, &pdev->dev);
> +   if (!ret)
> +   dev_info(host->parent, "allocated mmc-pwrseq\n");
> +
> +err:
> +   of_node_put(np);
> +   return ret;
>  }
>
>  void mmc_pwrseq_pre_power_on(struct mmc_host *host)
> diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
> index 12aaf2b..bd860d8 100644
> --- a/drivers/mmc/core/pwrseq.h
> +++ b/drivers/mmc/core/pwrseq.h
> @@ -27,6 +27,8 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host);
>  void mmc_pwrseq_power_off(struct mmc_host *host);
>  void mmc_pwrseq_free(struct mmc_host *host);
>
> +int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev);
> +
>  #else
>
>  static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
> diff --git a/drivers/mmc/core/pwrseq_simple.c 
> b/drivers/mmc/core/pwrseq_simple.c
> new file mode 100644
> index 000..61c991e
> --- /dev/null
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -0,0 +1,48 @@
> +/*
> + *  Copyright (C) 2014 Linaro Ltd
> + *
> + * Author: Ulf Hansson 
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + *
> + *  Simple MMC power sequence management
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "pwrseq.h"
> +
> +struct mmc_pwrseq_simple {
> +   struct mmc_pwrseq pwrseq;
> +};
> +
> +static void mmc_pwrseq_simple_free(struct mmc_host *host)
> +{
> +   struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
> +   struct mmc_pwrseq_simple, pwrseq);
> +
> +   kfree(pwrse

Re: gpio-pxa: getting GPIOs by devicetree phandle broken

[Alexandre Courbot]

On Tue, Feb 10, 2015 at 2:38 AM, Robert Jarzmik  wrote:
> Tyler Hall  writes:
>
>>> The issue with multiple gpiochips per of-node could be worked around as 
>>> followed I believe, comments?
>>>
>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>> index 08261f2..43984ab 100644
>>> --- a/drivers/gpio/gpiolib-of.c
>>> +++ b/drivers/gpio/gpiolib-of.c
>>> @@ -47,11 +47,12 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip 
>>> *gc, void *data)
>>> ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
>>> if (ret < 0) {
>>> /* We've found the gpio chip, but the translation failed.
>>> -* Return true to stop looking and return the translation
>>> -* error via out_gpio
>>> +* Store translation error in out_gpio.
>>> +* Return false to keep looking, as more than one GPIO chip
>>> +* could be registered per of-node.
>>>  */
>>> gg_data->out_gpio = ERR_PTR(ret);
>>> -   return true;
>>> +   return false;
>>>  }
>>>
>>> gg_data->out_gpio = gpiochip_get_desc(gc, ret);
>>
>> As long as we're ok with multiple gpiochips per of-node, this would
>> work for me. It'll change the preference of which chip returns the
>> error in the case of multiple chips, but that's already undefined
>> behavior.
>
> Looks good to me too, this will solve my issue, and the global behavior would 
> be
> consistent with the former one.
>
> Would you care submitting a proper patch so that we can apply our Reviewed-by,
> Tested-by etc ... ?

Looks ok to me too, if only a little bit hackish. A patch would be
appreciated so we can send it for -fixes as well.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: gpio-pxa: getting GPIOs by devicetree phandle broken

[Alexandre Courbot]

Adding Robert who reported the same thing.

On Sat, Feb 7, 2015 at 6:28 AM, Tyler Hall  wrote:
> Hi,
>
> Commit 7b8792b ("gpiolib: of: Correct error handling in
> of_get_named_gpiod_flags") seems to break the ability to use DT
> bindings to reference this driver's GPIOs by phandle for banks above
> the first.
>
> The issue is that gpio-pxa registers multiple gpio chips - one for
> each bank - but they're all associated with the same DT node. The new
> behavior in of_gpiochip_find_and_xlate() causes gpiochip_find() to
> bail after the first chip matches and its xlate function fails.
> Previously it would try all chips associated with the phandle and
> pxa_gpio_of_xlate() would fail until it was called with the correct
> gpiochip.
>
> I think the new behavior of of_gpiochip_find_and_xlate() is reasonable,
> so I see a couple ways of fixing gpio-pxa.
>
> 1. Require child nodes in DT for each bank

This would break DT compatibility.

> 2. Refactor gpio-pxa to only register one gpiochip

Sounds better, especially since this would reflect the hardware more
accurately. One DT node should translate into one GPIO chip. The
problem is that I'm afraid several other drivers are doing the same
thing and thus are now similarly broken.

The following is also likely to work as a workaround, but I would not
go as far as saying this should be taken as a fix. Hans, since you
wrote 7b8792b, could we have your input on this?

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 08261f2b3a82..a09095531b00 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -45,14 +45,16 @@ static int of_gpiochip_find_and_xlate(struct
gpio_chip *gc, void *data)
return false;

ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
-   if (ret < 0) {
+   if (ret == -EPROBE_DEFER) {
/* We've found the gpio chip, but the translation failed.
 * Return true to stop looking and return the translation
 * error via out_gpio
 */
gg_data->out_gpio = ERR_PTR(ret);
return true;
-}
+   } else if (ret < 0) {
+   return false;
+   }

gg_data->out_gpio = gpiochip_get_desc(gc, ret);
return true;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] gpio: add parameter to allow the use named gpios

[Alexandre Courbot]

On Fri, Jan 30, 2015 at 10:46 PM, Linus Walleij
 wrote:
> On Wed, Jan 21, 2015 at 10:33 PM, Olliver Schinagl
>  wrote:
>
>> From: Olliver Schinagl 
>>
>> The gpio binding document says that new code should always use named
>> gpios. Patch 40b73183 added support to parse a list of gpios from child
>> nodes, but does not make it possible to use named gpios. This patch adds
>> the con_id property and implements it is done in gpiolib.c, where the
>> old-style of using unnamed gpios still works.
>>
>> Signed-off-by: Olliver Schinagl 
>> ---
>>  drivers/gpio/devres.c | 18 +-
>>  drivers/input/keyboard/gpio_keys_polled.c |  2 +-
>>  drivers/leds/leds-gpio.c  |  2 +-
>>  include/linux/gpio/consumer.h |  1 +
>
> Alexandre: does this match your vision of how it should work, i.e. ACK?

Pretty much, yes - as I mentioned in the previous versions there may
be shortcomings for ACPI, but we need a refactor of the whole thing -
nothing that this patch should address by itself.

So this patch:

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Alexandre Courbot]

On Thu, Jan 22, 2015 at 6:44 AM, Olliver Schinagl  wrote:
> Hey Alexandre,
>
> On 01/19/2015 05:04 AM, Alexandre Courbot wrote:
>>
>> On Wed, Jan 7, 2015 at 6:08 PM, Olliver Schinagl
>>  wrote:
>>>
>>> From: Olliver Schinagl 
>>>
>>> The gpio binding document says that new code should always use named
>>> gpios. Patch 40b73183 added support to parse a list of gpios from child
>>> nodes, but does not make it possible to use named gpios. This patch adds
>>> the con_id property and implements it is done in gpiolib.c, where the
>>> old-style of using unnamed gpios still works.
>>
>> This is absolutely correct - thanks for spotting this.
>>
>> 
>> ... since it looks like this part has been mostly copy/pasted from
>> of_find_gpio(), can you add another patch that fixes it there as well?
>
> Yeah, since it has the same functionality, i copy pasted it. Wasn't sure if
> it was worth to macro it or anything. I've sent a v2 with that patch added
> to the mix :)
>>
>>
>> Also in the case of ACPI this will prove to be an incomplete lookup
>> since acpi_find_gpio() has an additional fallback if the named lookup
>> fails.
>
> I'm not very familiar (or at all) how ACPI falls into all of this, I'm just
> starting to get a hang of the DT, but since this is how the dts is being
> parsed, where is the relation here? Or did I misunderstand?

GPIO mappings can be provided by DT or by ACPI. In the latter case
there is a fallback method if the name does not match (see
acpi_find_gpio()). fwnode_get_named_gpiod() does not check it though,
so maybe we can just ignore that.

>> In that respect, I wonder if it would not be better for
>> devm_get_gpiod_from_child() to call of_find_gpio() and
>> acpi_find_gpio() (after making them non-static) followed by
>> gpiod_request() instead of calling fwnode_get_named_gpiod(). But in
>> that case it will have to do the OF/ACPI handling by itself.
>>
>> I'm not really sure about which way is better. I'd appreciate if you
>> could give a thought to a possible refactoring that would improve the
>> situation ; otherwise feel free to ignore what I have written above
>> and to duplicate the property name building code.
>
> I'm afraid I'm a little too inexperienced to follow exactly what you say ;)

Don't worry about it then, this patch is already an improvement. GPIO
mappings lookup from firmware has become kind of messy, and I'm just
trying to enroll people to clean it instead of doing it myself. ;)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Alexandre Courbot]

On Sun, Jan 18, 2015 at 8:39 PM, Ricardo Ribalda Delgado
 wrote:
> This way we do not need to transverse the device tree manually.

... and this makes things much more legible.

Acked-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Alexandre Courbot]

On Mon, Jan 19, 2015 at 12:43 PM, Alexandre Courbot  wrote:
> On Wed, Jan 14, 2015 at 10:20 PM, Olliver Schinagl  wrote:
>>
>> On 14-01-15 13:45, Linus Walleij wrote:
>>>
>>> On Thu, Jan 8, 2015 at 11:12 PM, Dmitry Torokhov
>>>  wrote:
>>>>
>>>> On Thu, Jan 08, 2015 at 08:40:20AM -0600, Rob Herring wrote:
>>>>>
>>>>> On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl 
>>>>> wrote:
>>>>>>>>
>>>>>>>> --- a/drivers/leds/leds-gpio.c
>>>>>>>> +++ b/drivers/leds/leds-gpio.c
>>>>>>>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv
>>>>>>>> *gpio_leds_create(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>  struct gpio_led led = {};
>>>>>>>>  const char *state = NULL;
>>>>>>>>- led.gpiod = devm_get_gpiod_from_child(dev, NULL,
>>>>>>>> child);
>>>>>>>> +   led.gpiod = devm_get_gpiod_from_child(dev, "led",
>>>>>>>> child);
>>>>>>>
>>>>>>> Would not this break existing boards using old bindings? You need to
>>>>>>> handle both cases: if you can't located "led-gpios" then you will have
>>>>>>> to
>>>>>>> try just "gpios".
>>>>>>
>>>>>> Very true. I was rather even hoping we could update all bindings, I
>>>>>> don't
>>>>>> mind going through the available dts files to fix them ... But need to
>>>>>> know
>>>>>> that that's the proper way to go before doing the work ;)
>>>>>
>>>>> That will not work. You cannot make changes that require a new dtb
>>>>> with a new kernel. This would also break for the other way around
>>>>> (i.e. a new dtb and old kernel).
>>>>>
>>>>> You would have to search for both led-gpios and gpios. I'm not sure if
>>>>> we can do that generically for all GPIOs. If you had a node with both
>>>>> "blah-gpios" and "gpios", it would break. I would hope there are no
>>>>> such cases like that. We also now have to consider how ACPI identifies
>>>>> GPIOs and whether this makes sense.
>>>>
>>>> I think only the driver itself can know about such "legacy" mappings and
>>>> make a decision.
>>>
>>> Yeah leds-gpio.c will need to be patched to check for "led-gpios" first
>>> and then fall back to "gpios" if not found.
>>
>> yeah I've done the work, just need to double check it and resend a v2.
>>
>> Linus, I assume you want the already applied patches omitted from v2?
>
> Yes, please base new revisions on Linus' devel branch, omitting any
> patches that he has already accepted.

Actually on top of breaking backward compatibility, I think the case
of LED GPIOs is one of thoses where it makes sense to not have a name
(the GPIO usage is obvious from the DT hierarchy, and there cannot be
another one). So I don't feel like this change is really needed (other
patches in this series are still useful though).
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Alexandre Courbot]

On Wed, Jan 7, 2015 at 6:08 PM, Olliver Schinagl
 wrote:
> From: Olliver Schinagl 
>
> The gpio binding document says that new code should always use named
> gpios. Patch 40b73183 added support to parse a list of gpios from child
> nodes, but does not make it possible to use named gpios. This patch adds
> the con_id property and implements it is done in gpiolib.c, where the
> old-style of using unnamed gpios still works.

This is absolutely correct - thanks for spotting this.

>
> Signed-off-by: Olliver Schinagl 
> ---
>  drivers/gpio/devres.c | 16 +++-
>  drivers/input/keyboard/gpio_keys_polled.c |  2 +-
>  drivers/leds/leds-gpio.c  |  2 +-
>  include/linux/gpio/consumer.h |  1 +
>  4 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c
> index 13dbd3d..b7fbe1c 100644
> --- a/drivers/gpio/devres.c
> +++ b/drivers/gpio/devres.c
> @@ -111,23 +111,37 @@ EXPORT_SYMBOL(__devm_gpiod_get_index);
>  /**
>   * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child 
> node
>   * @dev:   GPIO consumer
> + * @con_id:function within the GPIO consumer
>   * @child: firmware node (child of @dev)
>   *
>   * GPIO descriptors returned from this function are automatically disposed on
>   * driver detach.
>   */
>  struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
> +   const char *con_id,
> struct fwnode_handle *child)
>  {
> +   static const char const *suffixes[] = { "gpios", "gpio" };
> +   char prop_name[32]; /* 32 is max size of property name */
> struct gpio_desc **dr;
> struct gpio_desc *desc;
> +   unsigned int i;
>
> dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
>   GFP_KERNEL);
> if (!dr)
> return ERR_PTR(-ENOMEM);
>
> -   desc = fwnode_get_named_gpiod(child, "gpios");
> +   for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
> +   if (con_id)
> +   snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);
> +   else
> +   snprintf(prop_name, 32, "%s", suffixes[i]);

Same remark as Dmitry about the hardcoded length of the string, but ...

> +
> +   desc = fwnode_get_named_gpiod(child, prop_name);
> +   if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
> +   break;
> +   }

... since it looks like this part has been mostly copy/pasted from
of_find_gpio(), can you add another patch that fixes it there as well?

Also in the case of ACPI this will prove to be an incomplete lookup
since acpi_find_gpio() has an additional fallback if the named lookup
fails.

In that respect, I wonder if it would not be better for
devm_get_gpiod_from_child() to call of_find_gpio() and
acpi_find_gpio() (after making them non-static) followed by
gpiod_request() instead of calling fwnode_get_named_gpiod(). But in
that case it will have to do the OF/ACPI handling by itself.

I'm not really sure about which way is better. I'd appreciate if you
could give a thought to a possible refactoring that would improve the
situation ; otherwise feel free to ignore what I have written above
and to duplicate the property name building code.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 10:20 PM, Olliver Schinagl  wrote:
>
> On 14-01-15 13:45, Linus Walleij wrote:
>>
>> On Thu, Jan 8, 2015 at 11:12 PM, Dmitry Torokhov
>>  wrote:
>>>
>>> On Thu, Jan 08, 2015 at 08:40:20AM -0600, Rob Herring wrote:

 On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl 
 wrote:
>>>
>>> --- a/drivers/leds/leds-gpio.c
>>> +++ b/drivers/leds/leds-gpio.c
>>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv
>>> *gpio_leds_create(struct
>>> platform_device *pdev)
>>>  struct gpio_led led = {};
>>>  const char *state = NULL;
>>>- led.gpiod = devm_get_gpiod_from_child(dev, NULL,
>>> child);
>>> +   led.gpiod = devm_get_gpiod_from_child(dev, "led",
>>> child);
>>
>> Would not this break existing boards using old bindings? You need to
>> handle both cases: if you can't located "led-gpios" then you will have
>> to
>> try just "gpios".
>
> Very true. I was rather even hoping we could update all bindings, I
> don't
> mind going through the available dts files to fix them ... But need to
> know
> that that's the proper way to go before doing the work ;)

 That will not work. You cannot make changes that require a new dtb
 with a new kernel. This would also break for the other way around
 (i.e. a new dtb and old kernel).

 You would have to search for both led-gpios and gpios. I'm not sure if
 we can do that generically for all GPIOs. If you had a node with both
 "blah-gpios" and "gpios", it would break. I would hope there are no
 such cases like that. We also now have to consider how ACPI identifies
 GPIOs and whether this makes sense.
>>>
>>> I think only the driver itself can know about such "legacy" mappings and
>>> make a decision.
>>
>> Yeah leds-gpio.c will need to be patched to check for "led-gpios" first
>> and then fall back to "gpios" if not found.
>
> yeah I've done the work, just need to double check it and resend a v2.
>
> Linus, I assume you want the already applied patches omitted from v2?

Yes, please base new revisions on Linus' devel branch, omitting any
patches that he has already accepted.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: basic-mmio-gpio: add DT support

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 8:20 PM, Álvaro Fernández Rojas
 wrote:
> El 14/01/2015 a las 6:32, Alexandre Courbot escribió:
>> On Wed, Dec 17, 2014 at 7:41 AM, Álvaro Fernández Rojas
>>  wrote:
>>> Add DT support while keeping legacy support.
>>>
>>> Signed-off-by: Álvaro Fernández Rojas 
>>> ---
>>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
>>
>> There is no documentation for these new bindings?
>
> Actually, I was waiting for this patch (by kamlakant.pa...@linaro.org) to be 
> accepted before: "[PATCH v1 5/5] gpio: document basic mmio gpio library".
> Anyway, I will add documentation on the next version of this patch.
>
>>
>>> index 16f6115..9792783 100644
>>> --- a/drivers/gpio/gpio-generic.c
>>> +++ b/drivers/gpio/gpio-generic.c
>>> @@ -61,6 +61,9 @@ o` \___/` 
>>> controller in FPGA is ,.`
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>> +#include 
>>> +#include 
>>>
>>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>>>  {
>>> @@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device 
>>> *pdev,
>>> return ret;
>>>  }
>>>
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id bgpio_dt_ids[] = {
>>> +   { .compatible = "basic-mmio-gpio" },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
>>> +
>>> +static int bgpio_probe_dt(struct platform_device *pdev)
>>> +{
>>> +   u32 tmp;
>>> +   struct bgpio_pdata *pdata;
>>> +   struct device_node *np;
>>> +
>>> +   np = pdev->dev.of_node;
>>> +   if (!np)
>>> +   return 0;
>>> +
>>> +   pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>> +   if (!pdata)
>>> +   return -ENOMEM;
>>> +
>>> +   pdata->label = dev_name(&pdev->dev);
>>> +   pdata->base = -1;
>>> +   if (of_find_property(np, "byte-be", NULL)) {
>>> +   pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
>>> +   }
>>> +   if (of_find_property(np, "bit-be", NULL)) {
>>> +   pdata->flags |= BGPIOF_BIG_ENDIAN;
>>> +   }
>>> +   if (of_find_property(np, "regset-nr", NULL)) {
>>> +   pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
>>> +   }
>>> +   if (of_find_property(np, "regdir-nr", NULL)) {
>>> +   pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
>>> +   }
>>> +   if (!of_property_read_u32(np, "num-gpios", &tmp)) {
>>> +   pdata->ngpio = tmp;
>>> +   }
>>
>> I don't think this is acceptable. gpio-generic is designed to be used
>> as a framework for other drivers to build upon. These drivers should
>> have their own compatible strings, which should be enough to infer all
>> the properties you defined here.
>>
>
> gpio-generic is not only designed as a framework for other drivers, it can 
> also be used directly by registering the driver through platform device 
> (basic-mmio-gpio/basic-mmio-gpio-be).

This works for platform drivers which stay confined to the kernel, but
DT is a firmware definition where such generic bindings are much less
desirable.

> My intention is to make it DT compatible as a generic driver, which can be 
> used for different hardware, by selecting different DT properties as 
> configuration.
>
>> Device Tree identifies hardware precisely (vendor and model), and this
>> new binding is just not that. You *could* however have a very simple
>> driver that associates compatible strings to static tables containing
>> the values of the properties you wanted to see passed through the DT,
>> and have one single driver that covers many mmio-based GPIO devices.
>> But I'm afraid "basic-mmio-gpio" is *way* to vague to describe
>> hardware.
>>
>
> I don't think making a new driver mapping different compatible strings to 
> static tables containing the values of the properties passed through DT is a 
> sane way of doing it, since it will require multiple combinations to cover 
> all the possibilites.
>
> My plan is to be able to do something like this (btw, I actually tested this 
> patch on BCM63xx and works perfectly):
> gpio-controller@1084 {
&g

Re: [PATCH v4 3/4] gpio: Add find GPIO base in increasing order

[Alexandre Courbot]

On Sat, Jan 17, 2015 at 12:24 AM, Linus Walleij
 wrote:
> On Wed, Jan 14, 2015 at 9:17 AM, Alexandre Courbot  wrote:
>> On Wed, Jan 14, 2015 at 5:13 PM, Linus Walleij  
>> wrote:
>>> On Wed, Dec 10, 2014 at 9:51 AM, Alexandre Courbot  wrote:
>>>> On Fri, Dec 5, 2014 at 12:38 PM, Zhou Wang  wrote:
>>>>> In function gpiochip_find_base, base number of a GPIO controller
>>>>> is found in decreasing order. ARCH_NR_GPIOS is used to define from
>>>>> which number we begin to search for base number of a GPIO controller.
>>>>>
>>>>> In fact, ARCH_NR_GPIOS brings us some multiplatform problems, like:
>>>>> http://www.spinics.net/lists/devicetree/msg60433.html
>>>>>
>>>>> This patch adds the support to find base number of a GPIO controller
>>>>> in increasing order. It will assign base number from 0.
>>>>> A new dts property called gpio-number-forward must be add to the related
>>>>> GPIO dts nodes if you want it works well.
>>>>
>>>> Global GPIO numbers are a Linux-only concept, so your property should
>>>> be named linux,gpio-number-forward.
>>>>
>>>> But even that way I don't think I like this idea. This just adds some
>>>> more mess to how the GPIO number space is constructed, and it is
>>>> already quite messy as it is. You have no guarantee over the probe
>>>> order of your GPIO controllers, so they may very well be probed in a
>>>> different order and end up with different base numbers anytime.
>>>
>>> Yup.
>>>
>>> The way stuff is usually forced ordered in device tree is to use
>>> aliases, e.g.:
>>>
>>> aliases {
>>> serial0 = &pb1176_serial0;
>>> serial1 = &pb1176_serial1;
>>> serial2 = &pb1176_serial2;
>>> serial3 = &pb1176_serial3;
>>> serial4 = &fpga_serial;
>>> };
>>>
>>> By getting the alias ID with of_alias_get_id(np, "serial")
>>> a certain serial port is assigned to be
>>> 0, 1, 2 ...
>>>
>>> I think I could accept a tweak of this patch that will register
>>> GPIOs beginning from 0 if and only if the alias mechanism is
>>> used.
>>>
>>> aliases {
>>> gpio0 = &foo_gpio0;
>>> gpio1 = &expander;
>>> };
>>>
>>>
>>> The actual Linux-specific integer base will *NOT* be in the
>>> device tree, but if you know how many GPIOs are on each
>>> controller the number space is still stable and predictable
>>> beginning from 0. If one want to keep track of the Linux
>>> number space one can do so with comments in the device
>>> tree or something.
>>>
>>>> I know we pushed back against this kind of solution in the past, but
>>>> it is still more reliable than having a property that affects how the
>>>> kernel's base finding function works, and will offer us a way to
>>>> eventually put ARCH_NR_GPIOS and gpiochip_find_base() to rest (at the
>>>> cost of backwards-compatibility, but we just cannot do without it).
>>>> Linus, do you agree?
>>>
>>> What do you think about the above?
>>
>> It would definitely help with the probe order, but unfortunately not
>> with the fact that GPIO bases are allocated in decreasing order
>> starting from the potentially varying ARCH_NR_GPIOS. So that alone
>> will not be able to ensure stable GPIO numbers.
>
> What I mean is that if and only if aliases are used, we
> allocate GPIOs in ascending order starting at 0.
>
> Let us remember why this is done: dynamic GPIO numbers are
> allocated in descending order because it was assumed that some
> static GPIOs are already allocated starting from 0, like on-chip
> GPIOs.
>
> We can also make it a Kconfig option, because for a large chunk
> of systems only using device tree we can
> actually allocate from zero. Notably any ARCH_MULTIPLATFORM
> should just do this if we can make a quick survey of such systems
> and see that they don't fool around setting .base in their GPIO
> chips.

Ah, in that case yes, I agree it would probably work fine.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] gpio: Add find GPIO base in increasing order

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 5:13 PM, Linus Walleij  wrote:
> On Wed, Dec 10, 2014 at 9:51 AM, Alexandre Courbot  wrote:
>> On Fri, Dec 5, 2014 at 12:38 PM, Zhou Wang  wrote:
>>> In function gpiochip_find_base, base number of a GPIO controller
>>> is found in decreasing order. ARCH_NR_GPIOS is used to define from
>>> which number we begin to search for base number of a GPIO controller.
>>>
>>> In fact, ARCH_NR_GPIOS brings us some multiplatform problems, like:
>>> http://www.spinics.net/lists/devicetree/msg60433.html
>>>
>>> This patch adds the support to find base number of a GPIO controller
>>> in increasing order. It will assign base number from 0.
>>> A new dts property called gpio-number-forward must be add to the related
>>> GPIO dts nodes if you want it works well.
>>
>> Global GPIO numbers are a Linux-only concept, so your property should
>> be named linux,gpio-number-forward.
>>
>> But even that way I don't think I like this idea. This just adds some
>> more mess to how the GPIO number space is constructed, and it is
>> already quite messy as it is. You have no guarantee over the probe
>> order of your GPIO controllers, so they may very well be probed in a
>> different order and end up with different base numbers anytime.
>
> Yup.
>
> The way stuff is usually forced ordered in device tree is to use
> aliases, e.g.:
>
> aliases {
> serial0 = &pb1176_serial0;
> serial1 = &pb1176_serial1;
> serial2 = &pb1176_serial2;
> serial3 = &pb1176_serial3;
> serial4 = &fpga_serial;
> };
>
> By getting the alias ID with of_alias_get_id(np, "serial")
> a certain serial port is assigned to be
> 0, 1, 2 ...
>
> I think I could accept a tweak of this patch that will register
> GPIOs beginning from 0 if and only if the alias mechanism is
> used.
>
> aliases {
> gpio0 = &foo_gpio0;
> gpio1 = &expander;
> };
>
>
> The actual Linux-specific integer base will *NOT* be in the
> device tree, but if you know how many GPIOs are on each
> controller the number space is still stable and predictable
> beginning from 0. If one want to keep track of the Linux
> number space one can do so with comments in the device
> tree or something.
>
>> I know we pushed back against this kind of solution in the past, but
>> it is still more reliable than having a property that affects how the
>> kernel's base finding function works, and will offer us a way to
>> eventually put ARCH_NR_GPIOS and gpiochip_find_base() to rest (at the
>> cost of backwards-compatibility, but we just cannot do without it).
>> Linus, do you agree?
>
> What do you think about the above?

It would definitely help with the probe order, but unfortunately not
with the fact that GPIO bases are allocated in decreasing order
starting from the potentially varying ARCH_NR_GPIOS. So that alone
will not be able to ensure stable GPIO numbers.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v5 2/2] gpio: Document GPIO hogging mechanism

[Alexandre Courbot]

On Tue, Jan 13, 2015 at 1:39 AM, Benoit Parrot  wrote:
> Linus Walleij  wrote on Mon [2015-Jan-12 11:20:14 
> +0100]:
>> On Fri, Dec 19, 2014 at 9:07 PM, Benoit Parrot  wrote:
>>
>> > Add GPIO hogging documentation to gpio.txt
>> >
>> > Signed-off-by: Benoit Parrot 
>> > Reviewed-by: Alexandre Courbot 
>>
>> This is starting to look good ...
>>
>> > +   line_b {
>> > +   gpio-hog;
>> > +   gpios = <6 0>;
>> > +   state = "output-low";
>>
>> I don't like the state string.
>>
>> Instead have boolean properties for all states.
>>
>> line_b {
>> gpio-hog;
>> gpios = <6 0>;
>> output-low;
>> line-name = "foo-bar-gpio";
>> }
>>
>> Then use of_property_read_bool() in the code to check which
>> state is to be selected intially. You can check that no mutually
>> exclusive state are selected, I don't like that an arbitrary string
>> select the state like that, if we do it that way an enumerator would
>> be better, I prefer bools.
>
> I am sorry but that is how it was originally in the first patch.
> Alexandre's review comment suggested this method in [1] and [2] (below).
>
> Alexandre, any comments?
>
> [1] http://marc.info/?l=linux-gpio&m=141456662426151&w=2
>
> [2] http://marc.info/?l=linux-gpio&m=141715982424744&w=2

When Linus and I are in conflict, follow Linus. Arnd's suggestion of
having enums defined in (IIUC) include/dt-bindings/gpio and using them
sounds good to me too and might make everyone happy (no possibility of
conflicting definitions + no strings). Linus, could you comment on it?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] ARM: tegra: Add Tegra124 ACTMON support

[Alexandre Courbot]

On Tue, Dec 16, 2014 at 5:41 PM, Tomeu Vizoso
 wrote:
> Add device node for the ACTMON block to the Tegra124 device tree.

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor

[Alexandre Courbot]

> +   unsigned int i;
> +
> +   for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> +   dev = &tegra->devices[i];
> +
> +   val = device_readl(dev, ACTMON_DEV_CTRL);
> +   val |= ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
> +   val |= ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
> +   val |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +   val |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
> +   device_writel(dev, val, ACTMON_DEV_CTRL);
> +   }
> +
> +   actmon_write_barrier(tegra);
> +}
> +
> +static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra)
> +{
> +   struct tegra_devfreq_device *dev;
> +   u32 val;
> +   unsigned int i;
> +
> +   for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
> +   dev = &tegra->devices[i];
> +
> +   val = device_readl(dev, ACTMON_DEV_CTRL);
> +   val &= ~ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN;
> +   val &= ~ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN;
> +   val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +   val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
> +   device_writel(dev, val, ACTMON_DEV_CTRL);
> +   }
> +
> +   actmon_write_barrier(tegra);
> +}
> +
> +static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> + struct tegra_devfreq_device *dev)
> +{
> +   u32 val = 0;
> +
> +   dev->target_freq = tegra->cur_freq;
> +
> +   dev->avg_count = tegra->cur_freq * ACTMON_SAMPLING_PERIOD;
> +   device_writel(dev, dev->avg_count, ACTMON_DEV_INIT_AVG);
> +
> +   tegra_devfreq_update_avg_wmark(tegra, dev);
> +   tegra_devfreq_update_wmark(tegra, dev);
> +
> +   device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
> +   device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> +
> +   val |= ACTMON_DEV_CTRL_ENB_PERIODIC;
> +   val |= (ACTMON_AVERAGE_WINDOW_LOG2 - 1)
> +   << ACTMON_DEV_CTRL_K_VAL_SHIFT;
> +   val |= (ACTMON_BELOW_WMARK_WINDOW - 1)
> +   << ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_NUM_SHIFT;
> +   val |= (ACTMON_ABOVE_WMARK_WINDOW - 1)
> +   << ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT;
> +   val |= ACTMON_DEV_CTRL_ENB;
> +
> +   device_writel(dev, val, ACTMON_DEV_CTRL);
> +
> +   actmon_write_barrier(tegra);
> +}
> +
> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> +   u32 flags)
> +{
> +   struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> +   struct dev_pm_opp *opp;
> +   unsigned long rate = *freq * KHZ;
> +
> +   rcu_read_lock();
> +   opp = devfreq_recommended_opp(dev, &rate, flags);
> +   if (IS_ERR(opp)) {
> +   rcu_read_unlock();
> +   dev_err(dev, "Failed to find opp for %lu KHz\n", *freq);
> +   return PTR_ERR(opp);
> +   }
> +   rate = dev_pm_opp_get_freq(opp);
> +   rcu_read_unlock();
> +
> +   /* TODO: Once we have per-user clk constraints, set a floor */
> +   clk_set_rate(tegra->emc_clock, rate);
> +
> +   /* TODO: Set voltage as well */
> +
> +   return 0;
> +}
> +
> +static int tegra_devfreq_get_dev_status(struct device *dev,
> +   struct devfreq_dev_status *stat)
> +{
> +   struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> +   struct tegra_devfreq_device *actmon_dev;
> +
> +   stat->current_frequency = tegra->cur_freq;
> +
> +   /* To be used by the tegra governor */
> +   stat->private_data = tegra;
> +
> +   /* The below are to be used by the other governors */
> +
> +   actmon_dev = &tegra->devices[MCALL];
> +
> +   /* Number of cycles spent on memory access */
> +   stat->busy_time = device_readl(actmon_dev, ACTMON_DEV_AVG_COUNT);
> +
> +   /* The bus can be considered to be saturated way before 100% */
> +   stat->busy_time *= 100 / BUS_SATURATION_RATIO;
> +
> +   /* Number of cycles in a sampling period */
> +   stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
> +
> +   stat->busy_time = min(stat->busy_time, stat->total_time);
> +
> +   return 0;
> +}
> +
> +static struct devfreq_dev_profile tegra_devfreq_profile = {
> +   .polling_ms = 0,
> +   .target = tegra_devfreq_target,
> + 

Re: [PATCH v5 1/3] of: Add binding for NVIDIA Tegra ACTMON node

[Alexandre Courbot]

On Tue, Dec 16, 2014 at 5:41 PM, Tomeu Vizoso
 wrote:
> This block gathers statistics about various counters and can be configured to
> fire interrupts when thresholds are crossed.
>
> Signed-off-by: Tomeu Vizoso 
>
> ---
>
> v2: * Add operating-points property
> ---
>  .../devicetree/bindings/arm/tegra/actmon.txt   | 38 
> ++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/actmon.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/tegra/actmon.txt 
> b/Documentation/devicetree/bindings/arm/tegra/actmon.txt
> new file mode 100644
> index 000..b4069df
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/actmon.txt
> @@ -0,0 +1,38 @@
> +Tegra124 Activity Monitor driver

Device Tree describes hardware and is supposed to be
driver-independant, so that "driver" qualifier sounds weird to me -
maybe remove it?

> +
> +Required properties:
> +
> +- compatible: should be "nvidia,tegra124-actmon"
> +- reg: offset and length of the register set for the device
> +- interrupts: standard interrupt property
> +- clocks: Must contain a phandle and clock specifier pair for each entry in 
> clock-names. See ../clock/clock-bindings.txt for details.

Mmm, shouldn't this line be wrapper at character 80? Same throughout this file.

Also from this file the correct patch to clock-bindings.txt is
../../clock/clock-bindings.txt (same for reset.txt later).

Otherwise,

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v4 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Dec 19, 2014 at 2:08 AM, Benoit Parrot  wrote:
> Alexandre Courbot  wrote on Wed [2014-Dec-17 17:41:51 
> +0900]:
>> Looks ok to me. I have a few nits below, but otherwise I am happy with
>> how it turned out:
>>
>> Reviewed-by: Alexandre Courbot 
> I'll add this to the next version.
>
>>
>> Thanks for your patience with this!
>>
>> On Sat, Dec 13, 2014 at 7:07 AM, Benoit Parrot  wrote:
>> > Based on Boris Brezillion's work this is a reworked patch
>> > of his initial GPIO hogging mechanism.
>> > This patch provides a way to initally configure specific GPIO
>>
>> s/initally/initially
>>
>> > when the gpio controller is probed.
>>
>> GPIO in capitals please.
>>
>> >
>> > The actual DT scanning to collect the GPIO specific data is performed
>> > as part of the gpiochip_add().
>>
>> d/the
>>
>> >
>> > The purpose of this is to allows specific GPIOs to be configured
>>
>> s/allows/allow
>>
>> > without any driver specific code.
>> > This particularly useful because board design are getting
>>
>> This is particularly
>>
>> > increasingly complex and given SoC pins can now have upward
>> > of
>>
>> s/upward of/up to?
>
> well i meant more than 10 I think in some cases we have 13.
>
>>
>> > 10 mux values a lot of connections are now dependent on
>>
>> seems like a comma is missing here.
>>
>> > external IO muxes to switch various modes and combination.
>>
>> not sure what a combination is in that context.
>>
>> >
>> > Specific drivers should not necessarily need to be aware of
>> > what accounts to a specific board implementation. This board level
>> > "description" should be best kept as part of the dts file.
>> >
>> > Signed-off-by: Benoit Parrot 
>> > ---
>> > Changes since v3:
>> >  * Relocated the non-DT "hog" function to gpiolib.c.
>> >  * Rename some of the function to be clearer and remove _ prefixes.
>> >  * Replace the gpiod_request/gpiod_put usage with
>> >gpiochip_request_own_desc/free_own_desc version instead.
>> >  * Refactor some of the logic to better handle error condition/reporting
>> >  * Renamed the "direction" DT properties to "state".
>> >
>> >  drivers/gpio/gpiolib-of.c | 119 ++
>> >  drivers/gpio/gpiolib.c| 128 
>> > ++
>> >  drivers/gpio/gpiolib.h|   3 ++
>> >  3 files changed, 230 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> > index 604dbe6..5422216 100644
>> > --- a/drivers/gpio/gpiolib-of.c
>> > +++ b/drivers/gpio/gpiolib-of.c
>> > @@ -22,6 +22,7 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> >
>> >  #include "gpiolib.h"
>> >
>> > @@ -111,6 +112,122 @@ int of_get_named_gpio_flags(struct device_node *np, 
>> > const char *list_name,
>> >  EXPORT_SYMBOL(of_get_named_gpio_flags);
>> >
>> >  /**
>> > + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for 
>> > GPIO API
>> > + * @np:device node to get GPIO from
>> > + * @name:  GPIO line name
>> > + * @flags: a flags pointer to fill in
>> > + *
>> > + * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
>> > + * value on the error condition.
>> > + */
>> > +
>> > +static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
>> > + const char **name,
>> > + enum gpio_lookup_flags *lflags,
>> > + enum gpiod_flags *dflags)
>> > +{
>> > +   struct device_node *chip_np;
>> > +   enum of_gpio_flags xlate_flags;
>> > +   struct gpio_desc *desc;
>> > +   const char *dir_val;
>> > +   struct gg_data gg_data = {
>> > +   .flags = &xlate_flags,
>> > +   .out_gpio = NULL,
>>
>> out_gpio will be set to NULL if you don't specify it, so you don't
>> need this last line.
>
> ok
>
>>
>> > +   };
>> > +   u32 tmp;
>> > +   int i, ret;
>

Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

[Alexandre Courbot]

On Wed, Dec 17, 2014 at 7:26 PM, Arnd Bergmann  wrote:
> On Wednesday 17 December 2014 11:45:01 Alexandre Courbot wrote:
>>
>> Actually we are not that far from being able to do completely without
>> any GPIO number, and maybe that's what we should aim for. I think the
>> only remaining offender is the sysfs interface. If we could reach GPIO
>> controllers through a fixed path and just export their GPIOs there, I
>> believe we would have fixed the whole issue.
>
> What about the hundreds of board files and device drivers that still
> reference hardcoded gpio numbers? The problem seems mostly solved for
> anything that uses DT, but there are some architectures and a number
> of ARM platforms that don't use DT and probably never will.
>
> I would assume they could all be changed to use gpiod_lookup tables,
> but that's a lot of work.

Indeed, that's not something to expect, as I replied to Russell. Sorry
about the confusion.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

[Alexandre Courbot]

On Wed, Dec 17, 2014 at 7:44 PM, Russell King - ARM Linux
 wrote:
> On Wed, Dec 17, 2014 at 11:45:01AM +0900, Alexandre Courbot wrote:
>> Actually we are not that far from being able to do completely without
>> any GPIO number, and maybe that's what we should aim for. I think the
>> only remaining offender is the sysfs interface.
>
> And that is a user API, and there's lots of users of it (eg, on Raspberry
> Pi platforms.)  So changing it isn't going to be easy - I'd say that it's
> impractical.
>
> What you're suggesting would be like re-numbering Linux syscalls.

Uh, I expressed myself poorly. What I intended to say is that once we
have a sysfs alternative that does not rely on GPIO numbers (and thus
have the same feature coverage as the integer interface), we can
require new platforms to exclusively rely on gpiod/sysfs2, and
encourage older users to switch to it if they have an issue with the
way integers are handled or need one of the new features.

I don't foresee that we will ever be able to retire the integer
interface, however I would like to be able to say "your problem will
be solved if you switch to gpiod" instead of having to juggle with
potentially conflicting integer range requirements from different
platforms. Right now the only thing that prevents us to say that is
the lack of a consistent sysfs interface.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v4 2/2] gpio: Document GPIO hogging mechanism

[Alexandre Courbot]

A few nits:

On Sat, Dec 13, 2014 at 7:07 AM, Benoit Parrot  wrote:
> Add GPIO hogging documentation to gpio.txt
>
> Signed-off-by: Benoit Parrot 
> ---
> Changes since v3:
>  * Renamed the "direction" DT properties to "state".
>
>  Documentation/devicetree/bindings/gpio/gpio.txt | 23 +++
>  1 file changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
> b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 3fb8f53..a38da91 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -103,6 +103,22 @@ Every GPIO controller node must contain both an empty 
> "gpio-controller"
>  property, and a #gpio-cells integer property, which indicates the number of
>  cells in a gpio-specifier.
>
> +The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> +providing automatic GPIO request and configuration as part of the
> +gpio-controller's driver probe function.
> +
> +Each GPIO hog definition is represented as a child node of the GPIO 
> controller.
> +Required properties:
> +- gpio-hog:  A property specifying that this child node represent a gpio-hog.

... represent a GPIO hog.

> +- gpios: Store the gpio information (id, flags, ...). Shall contain the
> +number of cells specified in its parent node (GPIO controller
> +node).
> +- state: A property specifying the direction/value needed. This property
> +    can take the folowing values: input, output-high, output-low.

s/folowing/following

This aside,

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v4 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

Looks ok to me. I have a few nits below, but otherwise I am happy with
how it turned out:

Reviewed-by: Alexandre Courbot 

Thanks for your patience with this!

On Sat, Dec 13, 2014 at 7:07 AM, Benoit Parrot  wrote:
> Based on Boris Brezillion's work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO

s/initally/initially

> when the gpio controller is probed.

GPIO in capitals please.

>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().

d/the

>
> The purpose of this is to allows specific GPIOs to be configured

s/allows/allow

> without any driver specific code.
> This particularly useful because board design are getting

This is particularly

> increasingly complex and given SoC pins can now have upward
> of

s/upward of/up to?

> 10 mux values a lot of connections are now dependent on

seems like a comma is missing here.

> external IO muxes to switch various modes and combination.

not sure what a combination is in that context.

>
> Specific drivers should not necessarily need to be aware of
> what accounts to a specific board implementation. This board level
> "description" should be best kept as part of the dts file.
>
> Signed-off-by: Benoit Parrot 
> ---
> Changes since v3:
>  * Relocated the non-DT "hog" function to gpiolib.c.
>  * Rename some of the function to be clearer and remove _ prefixes.
>  * Replace the gpiod_request/gpiod_put usage with
>gpiochip_request_own_desc/free_own_desc version instead.
>  * Refactor some of the logic to better handle error condition/reporting
>  * Renamed the "direction" DT properties to "state".
>
>  drivers/gpio/gpiolib-of.c | 119 ++
>  drivers/gpio/gpiolib.c| 128 
> ++
>  drivers/gpio/gpiolib.h|   3 ++
>  3 files changed, 230 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 604dbe6..5422216 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "gpiolib.h"
>
> @@ -111,6 +112,122 @@ int of_get_named_gpio_flags(struct device_node *np, 
> const char *list_name,
>  EXPORT_SYMBOL(of_get_named_gpio_flags);
>
>  /**
> + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO 
> API
> + * @np:device node to get GPIO from
> + * @name:  GPIO line name
> + * @flags: a flags pointer to fill in
> + *
> + * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
> + * value on the error condition.
> + */
> +
> +static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
> + const char **name,
> + enum gpio_lookup_flags *lflags,
> + enum gpiod_flags *dflags)
> +{
> +   struct device_node *chip_np;
> +   enum of_gpio_flags xlate_flags;
> +   struct gpio_desc *desc;
> +   const char *dir_val;
> +   struct gg_data gg_data = {
> +   .flags = &xlate_flags,
> +   .out_gpio = NULL,

out_gpio will be set to NULL if you don't specify it, so you don't
need this last line.

> +   };
> +   u32 tmp;
> +   int i, ret;
> +
> +   chip_np = np->parent;
> +   if (!chip_np)
> +   return ERR_PTR(-EINVAL);
> +
> +   xlate_flags = 0;
> +   *lflags = 0;
> +   *dflags = 0;
> +
> +   ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> +   if (ret)
> +   return ERR_PTR(ret);
> +
> +   if (tmp > MAX_PHANDLE_ARGS)
> +   return ERR_PTR(-EINVAL);
> +
> +   gg_data.gpiospec.args_count = tmp;
> +   gg_data.gpiospec.np = chip_np;
> +   for (i = 0; i < tmp; i++) {
> +   ret = of_property_read_u32_index(np, "gpios", i,
> +  &gg_data.gpiospec.args[i]);
> +   if (ret)
> +   return ERR_PTR(ret);
> +   }
> +
> +   gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +   if (!gg_data.out_gpio) {
> +   if (np->parent == np)
> +   return ERR_PTR(-ENXIO);
> +   else
> +   return ERR_PTR(-EPROBE_DEFER);
> +   }
> +
> +   if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> +   *lflags |= GPIO_ACTIVE_LOW;

Re: [PATCH] ARM: dts: tegra20: fix GR3D, DSI unit and reg base addresses

[Alexandre Courbot]

On Sat, Dec 13, 2014 at 12:19 AM, Dmitry Osipenko  wrote:
> Commit 58ecb23f64ee ("ARM: tegra: add missing unit addresses to DT") added
> unit address and changed reg base for GR3D and DSI host1x modules, but these
> addresses belongs to GR2D and TVO modules respectively. Fix it by changing
> modules unit and reg base addresses to proper ones.

Checked with the TRM, you are absolutely correct.

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/3] Add gpio support to Broadcom Cygnus SoC

[Alexandre Courbot]

On Tue, Dec 16, 2014 at 11:18 AM, Ray Jui  wrote:
> This patchset contains the initial GPIO support for the Broadcom Cygnus SoC.
> Cygnus has 3 GPIO controllers: 1) the ASIU GPIO; 2) the chipCommonG GPIO;
> and 3) the ALWAYS-ON GPIO. All 3 types of GPIO controllers are supported by
> the same Cygnus GPIO driver

No objections for this v6. The series,

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] gpio: Add find GPIO base in increasing order

[Alexandre Courbot]

On Mon, Dec 15, 2014 at 1:01 PM, Zhou Wang  wrote:
> On 2014年12月10日 16:51, Alexandre Courbot wrote:
>>
>> On Fri, Dec 5, 2014 at 12:38 PM, Zhou Wang  wrote:
>>>
>>> In function gpiochip_find_base, base number of a GPIO controller
>>> is found in decreasing order. ARCH_NR_GPIOS is used to define from
>>> which number we begin to search for base number of a GPIO controller.
>>>
>>> In fact, ARCH_NR_GPIOS brings us some multiplatform problems, like:
>>> http://www.spinics.net/lists/devicetree/msg60433.html
>>>
>>> This patch adds the support to find base number of a GPIO controller
>>> in increasing order. It will assign base number from 0.
>>> A new dts property called gpio-number-forward must be add to the related
>>> GPIO dts nodes if you want it works well.
>>
>>
>> Global GPIO numbers are a Linux-only concept, so your property should
>> be named linux,gpio-number-forward.
>>
>> But even that way I don't think I like this idea. This just adds some
>> more mess to how the GPIO number space is constructed, and it is
>> already quite messy as it is. You have no guarantee over the probe
>> order of your GPIO controllers, so they may very well be probed in a
>> different order and end up with different base numbers anytime.
>>
>> Not that this is your fault - the number namespace is broken by design
>> and I don't think there is a way to fix it. The long-term solution is
>> to stop using it by switching to the gpiod interface.
>>
>> First a few questions to understand why you need your GPIOs numbered
>> this way, and if you need it at all:
>> - Can't you use the gpiod interface instead so you don't need to rely
>> on GPIO numbers?
>
>
> Hi Alexandre,
>
> Sorry for late. Could you give me more clue about the gpiod interface? Don't
> we also call gpio_request() which uses GPIO number to request a
> GPIO?

See Documentation/gpio/consumer.txt and Documentation/gpio/board.txt.
You can obtain a GPIO descriptor without using a number by calling
gpiod_get(). Prior to that, individual GPIOs need to be bound to
devices and functions using either DT (preferred) or the platform
interface. The old integer-based GPIO interface is considered
deprecated, although still widely used. But new code should rely
exclusively on gpiod, and we encourage people to convert existing code
to it too.

>
>> - Do you plan to use the sysfs interface? If so then we are screwed,
>> because there is no way to use it without using global GPIO numbers.
>
>
> I am now enabling GPIO in Hip04-d01. Maybe, I can just use
> the default ARCH_NR_GPIOS, then users can manage GPIO through sysfs.
> However if so, GPIO 0~127 will be mapped to GPIO 384~511.

Yeah, I know that's not ideal. As a workaround, users can maybe
identify the right gpiochip by parsing /sys/class/gpio/gpiochip* and
comparing the "label" node. Once the right chip is found, its "base"
entry gives the base GPIO number which can be used to export the
desired GPIO.

We are also taking steps to come with a better sysfs interface. I will
keep you in the loop.

>
>> This is something we should/will fix with named exported GPIOs, but we
>> are not here yet.
>>
>> As to how we can solve your problem: if you must use GPIO integers
>> (because you need to use the sysfs interface for instance), and need
>> them affected consistently, the only way I can think of is to
>> introduce a "linux,gpio-base" property that will set gpiochip->base to
>> a fixed number. It still kind of sucks, but at least it will enforce
>
>
> Thanks for your suggestion. But setting "linux,gpio-base" will bring
> gpio base implementation specific, and in face there is no place to gain
> this gpio base info, not appropriate both in gpio arch code and dwapb
> code.

Yeah, besides this property did not receive a warm reception. So my
suggestion for now is to workaround the issue using the technique
described above, until we come with a better sysfs interface that does
not rely on GPIO numbers. Sorry for that inconvenience.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

[Alexandre Courbot]

On Tue, Dec 16, 2014 at 6:57 AM, Arnd Bergmann  wrote:
> On Monday 15 December 2014 13:35:47 Ray Jui wrote:
>>
>> Like I said previously, dynamic GPIO allocation works fine in the
>> kernel, as long as all of our GPIO clients in the kernel use gpiod based
>> API, which is what we will enforce going forward. The only problem is
>> with some of our customers who use GPIO through sysfs and expect fixed
>> global GPIO numbers. Thinking about this more, it's probably not that
>> difficult to add a script for those customers to convert/map the GPIO
>> numbers based on readings parsed from sysfs, so I guess that's fine.
>>
>
> I think we discussed the user space interface a number of times
> in the past, but I forgot the outcome. Either there is already
> a way to name gpio lines uniquely in sysfs, or there should be
> one.
>
> Can you reach the gpio interfaces using 
> /sys/devices/0001234.bus/1234566.gpiocontroller/...?

No, but it seems like this is exactly the solution we need. We could
have an "export" node there that takes a relative GPIO number and
exports it under
/sys/devices/0001234.bus/1234566.gpiocontroller/exported/ the same way
the current sysfs exporter does. Then for convenience we could also
allow exported GPIOs to take names to be used under the shorter
/sys/class/gpio/ (named GPIOs is another request we pushed back many
times but that keeps coming).

Let's see if I can come with a patch. That would at least give us
something to reply to the many people that hit this issue.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

[Alexandre Courbot]

On Sat, Dec 13, 2014 at 12:28 AM, Arnd Bergmann  wrote:
> On Friday 12 December 2014 22:05:37 Alexandre Courbot wrote:
>> On Fri, Dec 12, 2014 at 9:08 PM, Arnd Bergmann  wrote:
>> > On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
>> >> +
>> >> +- linux,gpio-base:
>> >> +Base GPIO number of this controller
>> >> +
>> >>
>> >
>> > We've NAK'ed properties like this multiple times before, and it
>> > doesn't get any better this time. What are you trying to achieve
>> > here?
>>
>> I am to blame for suggesting using this property to Ray, and I am
>> fully aware that this has been rejected before, but look at what
>> people came with recently to palliate the lack of control over the
>> GPIO number space for DT platforms:
>>
>> http://www.spinics.net/lists/arm-kernel/msg384847.html
>> https://lkml.org/lkml/2014/12/10/133
>>
>> Right now GPIO numbering for platforms using DT is a very inconsistent
>> process, subject to change by the simple action of adjusting the value
>> of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO
>> controller, or changing the probe order of devices. For users of the
>> integer or sysfs interfaces, this results in GPIO numbers that change,
>> and drivers and/or user-space programs that behave incorrectly.
>> Ironically, the only way to have consistent numbers is to use the old
>> platform files, where you can specify the base number of a gpio_chip.
>>
>> DT is actually probably not such a bad place to provide consistency in
>> GPIO numbering. It has a global vision of the system layout, including
>> all GPIO controllers and the number of GPIOs they include, and thus
>> can make informed decisions. It provides a consistent result
>> regardless of probe order. And allowing it to assign GPIO bases to
>> controllers will free us from the nonsensical dependency of some
>> arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on
>> us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it
>> since we don't need it anymore after the removal of the global
>> gpio_descs array. This will again interfere with the numbering of GPIO
>> chips that do not have a base number provided.
>>
>> Note that I don't really like this, either - but the problem is the
>> GPIO integer interface. Until everyone has upgraded to gpiod and we
>> have a replacement for the current sysfs interface (this will take a
>> while) we have to cope with this. This issue has been bothering users
>> for years, so this time I'd like to try and solve it the less ugly
>> way. If there is a better solution, of course I'm all for it.
>
> I think the scheme will fail if you ever get gpio controllers that are
> not part of the DT: We have hotpluggable devices (PCI, USB, ...) that
> are not represented in DT and that may also provide GPIOs for internal
> uses.
>
> The current state of affairs is definitely problematic, but defining
> the GPIO numbers in DT properties would only be a relative improvement,
> not a solution, and I fear it would make it harder to change the kernel
> to remove the gpio numbers eventually.

You are absolutely right that this would be only a partial solution.
However this is a situation where there is no absolute fix (besides
dropping the GPIO numbers completely) and the relief this property
would brings makes it up for its shortcomings IMHO.

> I wonder if we could instead come up with an approach that completely
> randomizes the gpio numbers (as a compile-time option) to find any
> places that still rely on specific numbers.

A.k.a. Linus and Alex' hate mail generator. :P

Actually we are not that far from being able to do completely without
any GPIO number, and maybe that's what we should aim for. I think the
only remaining offender is the sysfs interface. If we could reach GPIO
controllers through a fixed path and just export their GPIOs there, I
believe we would have fixed the whole issue.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

[Alexandre Courbot]

On Fri, Dec 12, 2014 at 9:08 PM, Arnd Bergmann  wrote:
> On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
>> +
>> +- linux,gpio-base:
>> +Base GPIO number of this controller
>> +
>>
>
> We've NAK'ed properties like this multiple times before, and it
> doesn't get any better this time. What are you trying to achieve
> here?

I am to blame for suggesting using this property to Ray, and I am
fully aware that this has been rejected before, but look at what
people came with recently to palliate the lack of control over the
GPIO number space for DT platforms:

http://www.spinics.net/lists/arm-kernel/msg384847.html
https://lkml.org/lkml/2014/12/10/133

Right now GPIO numbering for platforms using DT is a very inconsistent
process, subject to change by the simple action of adjusting the value
of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO
controller, or changing the probe order of devices. For users of the
integer or sysfs interfaces, this results in GPIO numbers that change,
and drivers and/or user-space programs that behave incorrectly.
Ironically, the only way to have consistent numbers is to use the old
platform files, where you can specify the base number of a gpio_chip.

DT is actually probably not such a bad place to provide consistency in
GPIO numbering. It has a global vision of the system layout, including
all GPIO controllers and the number of GPIOs they include, and thus
can make informed decisions. It provides a consistent result
regardless of probe order. And allowing it to assign GPIO bases to
controllers will free us from the nonsensical dependency of some
arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on
us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it
since we don't need it anymore after the removal of the global
gpio_descs array. This will again interfere with the numbering of GPIO
chips that do not have a base number provided.

Note that I don't really like this, either - but the problem is the
GPIO integer interface. Until everyone has upgraded to gpiod and we
have a replacement for the current sysfs interface (this will take a
while) we have to cope with this. This issue has been bothering users
for years, so this time I'd like to try and solve it the less ugly
way. If there is a better solution, of course I'm all for it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v3 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Dec 11, 2014 at 8:48 AM, Benoit Parrot  wrote:
> Alexandre Courbot  wrote on Wed [2014-Dec-10 20:19:51 
> +0900]:
>> On Fri, Dec 5, 2014 at 6:02 AM, Benoit Parrot  wrote:
>> > Based on Boris Brezillion's work this is a reworked patch
>> > of his initial GPIO hogging mechanism.
>> > This patch provides a way to initally configure specific GPIO
>> > when the gpio controller is probed.
>> >
>> > The actual DT scanning to collect the GPIO specific data is performed
>> > as part of the gpiochip_add().
>> >
>> > The purpose of this is to allows specific GPIOs to be configured
>> > without any driver specific code.
>> > This particularly useful because board design are getting
>> > increasingly complex and given SoC pins can now have upward
>> > of 10 mux values a lot of connections are now dependent on
>> > external IO muxes to switch various modes and combination.
>> >
>> > Specific drivers should not necessarily need to be aware of
>> > what accounts to a specific board implementation. This board level
>> > "description" should be best kept as part of the dts file.
>> >
>> > Signed-off-by: Benoit Parrot 
>> > ---
>> > Changes since v2:
>> >  * Refactor the gpio-hog mechanism to split the DT related action
>> >from the actual "hogging" operation.
>> >  * This allows non-DT providers to implement hogs as well.
>> >  * Added FLAG_IS_HOGGED to mark hogged gpio and make gpiochip removal
>> >able to release hogged gpio.
>> >  * Similarly to the hogging, the cleanup is performed as part of
>> >of_gpiochip_remove
>> >  * Refactor the gpio-hog mechanism as private functions meant to
>> >be to invoked from of_gpiochip_add().
>> >
>> > Changes since v1:
>> >  * Refactor the gpio-hog mechanism as private functions meant to
>> >be to invoked from of_gpiochip_add().
>> >
>> >  drivers/gpio/gpiolib-of.c | 127 
>> > ++
>> >  drivers/gpio/gpiolib.c| 118 
>> > ++-
>> >  drivers/gpio/gpiolib.h|   1 +
>> >  include/linux/gpio/consumer.h |   9 +++
>> >  4 files changed, 229 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> > index 604dbe6..e13134d 100644
>> > --- a/drivers/gpio/gpiolib-of.c
>> > +++ b/drivers/gpio/gpiolib-of.c
>> > @@ -22,6 +22,7 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> >
>> >  #include "gpiolib.h"
>> >
>> > @@ -111,6 +112,128 @@ int of_get_named_gpio_flags(struct device_node *np, 
>> > const char *list_name,
>> >  EXPORT_SYMBOL(of_get_named_gpio_flags);
>> >
>> >  /**
>> > + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for 
>> > GPIO API
>> > + * @np:device node to get GPIO from
>> > + * @name:  GPIO line name
>> > + * @flags: a flags pointer to fill in
>> > + *
>> > + * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
>> > + * value on the error condition.
>> > + */
>> > +
>> > +static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
>> > + const char **name,
>> > + enum gpio_lookup_flags *lflags,
>> > + enum gpiod_flags *dflags)
>> > +{
>> > +   struct device_node *chip_np;
>> > +   enum of_gpio_flags xlate_flags;
>> > +   struct gpio_desc *desc;
>> > +   const char *dir_val;
>> > +   struct gg_data gg_data = {
>> > +   .flags = &xlate_flags,
>> > +   .out_gpio = NULL,
>> > +   };
>> > +   u32 tmp;
>> > +   int i, ret;
>> > +
>> > +   chip_np = np->parent;
>> > +   if (!chip_np)
>> > +   return ERR_PTR(-EINVAL);
>> > +
>> > +   xlate_flags = 0;
>> > +   *lflags = 0;
>> > +   *dflags = 0;
>> > +
>> > +   ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
>> > +   if (ret)
>> > +   return ERR_PTR(ret);
>> > +
>> > +   if (tmp > MAX_PHANDLE_ARGS

Re: [Patch v3 2/2] gpio: Document GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Dec 11, 2014 at 7:50 AM, Benoit Parrot  wrote:
> Alexandre Courbot  wrote on Wed [2014-Dec-10 19:56:17 
> +0900]:
>> On Fri, Dec 5, 2014 at 6:02 AM, Benoit Parrot  wrote:
>> > Add GPIO hogging documentation to gpio.txt
>> >
>> > Signed-off-by: Benoit Parrot 
>> > ---
>> > Changes since v2:
>> >  * Updated to the latest hog syntax.
>> >
>> > Changes since v1:
>> >  * Split the devicetree bindings documentation in its own patch.
>> >
>> >  Documentation/devicetree/bindings/gpio/gpio.txt | 23 
>> > +++
>> >  1 file changed, 23 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
>> > b/Documentation/devicetree/bindings/gpio/gpio.txt
>> > index 3fb8f53..6d88133 100644
>> > --- a/Documentation/devicetree/bindings/gpio/gpio.txt
>> > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
>> > @@ -103,6 +103,22 @@ Every GPIO controller node must contain both an empty 
>> > "gpio-controller"
>> >  property, and a #gpio-cells integer property, which indicates the number 
>> > of
>> >  cells in a gpio-specifier.
>> >
>> > +The GPIO chip may contain GPIO hog definitions. GPIO hogging is a 
>> > mechanism
>> > +providing automatic GPIO request and configuration as part of the
>> > +gpio-controller's driver probe function.
>> > +
>> > +Each GPIO hog definition is represented as a child node of the GPIO 
>> > controller.
>> > +Required properties:
>> > +- gpio-hog:  A property specifying that this child node represent a 
>> > gpio-hog.
>> > +- gpios: Store the gpio information (id, flags, ...). Shall contain 
>> > the
>> > +number of cells specified in its parent node (GPIO controller
>> > +node).
>>
>> Since this property will only describe one GPIO, why use the plural
>> form? Would it not be confusing?
>
> I would tend to agree in this case but based on this original comment from 
> Linus on this thread:

Ok, good for me then.

>
> http://marc.info/?l=linux-gpio&m=138917388625221&w=2
> where on 08/01/2014 10:37, Linus Walleij wrote:
>
>   > +Each GPIO hog definition is represented as a child node of the GPIO 
> controller.
>   > +Required properties:
>   > +- gpio: store the gpio informations (id, flags, ...). Shall contain the
>   > +  number of cells specified in its parent node (GPIO controller node).
>
>   This property is alway plural "gpios".
>
> As long as everyone agree, I am fine with it.
>
>>
>> > +- direction: A property specifying the direction/value needed. This 
>> > property
>> > +can take the folowing values: input, output-high, output-low.
>>
>> nit: since this property not only describes the direction but also the
>> (potential) value, I suggest to rename it to "state".
>
> Since you are the one who suggested "direction" in the first place, as long 
> as you agree with yourself :)

/me of today is not the same person as /me of yesterday... :P

This is just a suggestion though, please pick the name *you* think is
the most adequate.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor

[Alexandre Courbot]

Hi Tomeu,

On Tue, Dec 9, 2014 at 10:15 PM, Tomeu Vizoso
 wrote:
> The ACTMON block can monitor several counters, providing averaging and firing
> interrupts based on watermarking configuration. This implementation monitors
> the MCALL and MCCPU counters to choose an appropriate frequency for the
> external memory clock.
>
> This patch is based on work by Alex Frid  and Mikko
> Perttunen .
>
> Signed-off-by: Tomeu Vizoso 
>
> ---
>
> v3: * Address misc. style issues found by Thierry and Alexander
> * Added helpers for register i/o
> * Further documented the structs
> * Enable the ACTMON after the IRQ handler has been installed
> * Disable the ACTMON before removing the IRQ handler
> * Add governor in a subsys initcall
> * Rename tegra-devfreq.c to tegra-actmon-devfreq.c
>
> v2: * Use devfreq

This is taking shape, and although it will be desirable to port it to
the more suitable watermark model once it gets merged, I am for
merging this first version in the meantime as it provides a useful
feature. Then we could use it to test-drive watermarking and argue the
advantages of that model with impressive negative line count patches.
;)

A few comments below but I am close to giving my ack to this.

> ---
>  drivers/devfreq/Kconfig|   9 +
>  drivers/devfreq/Makefile   |   1 +
>  drivers/devfreq/tegra-actmon-devfreq.c | 777 
> +
>  3 files changed, 787 insertions(+)
>  create mode 100644 drivers/devfreq/tegra-actmon-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index faf4e70..76be33a 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -87,4 +87,13 @@ config ARM_EXYNOS5_BUS_DEVFREQ
>   It reads PPMU counters of memory controllers and adjusts the
>   operating frequencies and voltages with OPP support.
>
> +config ARM_TEGRA_ACTMON_DEVFREQ
> +   bool "Tegra ACTMON DEVFREQ Driver"
> +   depends on ARCH_TEGRA
> +   select PM_OPP
> +   help
> + This adds the DEVFREQ driver for the Tegra family of SoCs.
> + It reads ACTMON counters of memory controllers and adjusts the
> + operating frequencies and voltages with OPP support.
> +
>  endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 16138c9..c9159c5 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += 
> governor_userspace.o
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)  += exynos/
>  obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)  += exynos/
> +obj-$(CONFIG_ARM_TEGRA_ACTMON_DEVFREQ) += tegra-actmon-devfreq.o
> diff --git a/drivers/devfreq/tegra-actmon-devfreq.c 
> b/drivers/devfreq/tegra-actmon-devfreq.c
> new file mode 100644
> index 000..8d2ea22f
> --- /dev/null
> +++ b/drivers/devfreq/tegra-actmon-devfreq.c
> @@ -0,0 +1,777 @@
> +/*
> + * A devfreq driver for NVIDIA Tegra SoCs
> + *
> + * Copyright (c) 2014 NVIDIA CORPORATION. All rights reserved.
> + * Copyright (C) 2014 Google, Inc
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see .
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "governor.h"
> +
> +#define ACTMON_GLB_STATUS  0x0
> +#define ACTMON_GLB_PERIOD_CTRL 0x4
> +
> +#define ACTMON_DEV_CTRL0x0
> +#define ACTMON_DEV_CTRL_K_VAL_SHIFT10
> +#define ACTMON_DEV_CTRL_ENB_PERIODIC   BIT(18)
> +#define ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN BIT(20)
> +#define ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN BIT(21)
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_NUM_SHIFT  23
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT  26
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN BIT(29)
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN BIT(30)
> +#define ACTMON_DEV_CTRL_ENBBIT(31)
> +
> +#define ACTMON_DEV_UPPER_WMARK 0x4
> +#define ACTMON_DEV_LOWER_WMARK 0x8
> +#define ACTMON_DEV_INIT_AVG  

Re: [Patch v3 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 6:02 AM, Benoit Parrot  wrote:
> Based on Boris Brezillion's work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO
> when the gpio controller is probed.
>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().
>
> The purpose of this is to allows specific GPIOs to be configured
> without any driver specific code.
> This particularly useful because board design are getting
> increasingly complex and given SoC pins can now have upward
> of 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes and combination.
>
> Specific drivers should not necessarily need to be aware of
> what accounts to a specific board implementation. This board level
> "description" should be best kept as part of the dts file.
>
> Signed-off-by: Benoit Parrot 
> ---
> Changes since v2:
>  * Refactor the gpio-hog mechanism to split the DT related action
>from the actual "hogging" operation.
>  * This allows non-DT providers to implement hogs as well.
>  * Added FLAG_IS_HOGGED to mark hogged gpio and make gpiochip removal
>able to release hogged gpio.
>  * Similarly to the hogging, the cleanup is performed as part of
>of_gpiochip_remove
>  * Refactor the gpio-hog mechanism as private functions meant to
>be to invoked from of_gpiochip_add().
>
> Changes since v1:
>  * Refactor the gpio-hog mechanism as private functions meant to
>be to invoked from of_gpiochip_add().
>
>  drivers/gpio/gpiolib-of.c | 127 
> ++
>  drivers/gpio/gpiolib.c| 118 ++-
>  drivers/gpio/gpiolib.h|   1 +
>  include/linux/gpio/consumer.h |   9 +++
>  4 files changed, 229 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 604dbe6..e13134d 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "gpiolib.h"
>
> @@ -111,6 +112,128 @@ int of_get_named_gpio_flags(struct device_node *np, 
> const char *list_name,
>  EXPORT_SYMBOL(of_get_named_gpio_flags);
>
>  /**
> + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO 
> API
> + * @np:device node to get GPIO from
> + * @name:  GPIO line name
> + * @flags: a flags pointer to fill in
> + *
> + * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
> + * value on the error condition.
> + */
> +
> +static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
> + const char **name,
> + enum gpio_lookup_flags *lflags,
> + enum gpiod_flags *dflags)
> +{
> +   struct device_node *chip_np;
> +   enum of_gpio_flags xlate_flags;
> +   struct gpio_desc *desc;
> +   const char *dir_val;
> +   struct gg_data gg_data = {
> +   .flags = &xlate_flags,
> +   .out_gpio = NULL,
> +   };
> +   u32 tmp;
> +   int i, ret;
> +
> +   chip_np = np->parent;
> +   if (!chip_np)
> +   return ERR_PTR(-EINVAL);
> +
> +   xlate_flags = 0;
> +   *lflags = 0;
> +   *dflags = 0;
> +
> +   ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> +   if (ret)
> +   return ERR_PTR(ret);
> +
> +   if (tmp > MAX_PHANDLE_ARGS)
> +   return ERR_PTR(-EINVAL);
> +
> +   gg_data.gpiospec.args_count = tmp;
> +   gg_data.gpiospec.np = chip_np;
> +   for (i = 0; i < tmp; i++) {
> +   ret = of_property_read_u32_index(np, "gpios", i,
> +  &gg_data.gpiospec.args[i]);
> +   if (ret)
> +   return ERR_PTR(ret);
> +   }
> +
> +   gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> +   if (!gg_data.out_gpio) {
> +   if (np->parent == np)
> +   return ERR_PTR(-ENXIO);
> +   else
> +   return ERR_PTR(-EPROBE_DEFER);
> +   }
> +
> +   if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> +   *lflags |= GPIO_ACTIVE_LOW;
> +
> +   if (!of_property_read_string(np, "direction", &dir_val)) {
> +   if (!strcmp(dir_val, "input"))
> +   *dflags |= GPIOD_IN;
> +   else if (!strcmp(dir_val, "output-low"))
> +   *dflags |= GPIOD_OUT_LOW;
> +   else if (!strcmp(dir_val, "output-high"))
> +   *dflags |= GPIOD_OUT_HIGH;
> +   }

... else?

We should probably return an error if the property is not specified -
is there a point in hogging a GPIO without a direction? E.g:

if (of_property_read_string(np, "direction", &dir_val))
return ERR_PTR(-EINVAL);

if (!

Re: [Patch v3 2/2] gpio: Document GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 6:02 AM, Benoit Parrot  wrote:
> Add GPIO hogging documentation to gpio.txt
>
> Signed-off-by: Benoit Parrot 
> ---
> Changes since v2:
>  * Updated to the latest hog syntax.
>
> Changes since v1:
>  * Split the devicetree bindings documentation in its own patch.
>
>  Documentation/devicetree/bindings/gpio/gpio.txt | 23 +++
>  1 file changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
> b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 3fb8f53..6d88133 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -103,6 +103,22 @@ Every GPIO controller node must contain both an empty 
> "gpio-controller"
>  property, and a #gpio-cells integer property, which indicates the number of
>  cells in a gpio-specifier.
>
> +The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> +providing automatic GPIO request and configuration as part of the
> +gpio-controller's driver probe function.
> +
> +Each GPIO hog definition is represented as a child node of the GPIO 
> controller.
> +Required properties:
> +- gpio-hog:  A property specifying that this child node represent a gpio-hog.
> +- gpios: Store the gpio information (id, flags, ...). Shall contain the
> +number of cells specified in its parent node (GPIO controller
> +node).

Since this property will only describe one GPIO, why use the plural
form? Would it not be confusing?

> +- direction: A property specifying the direction/value needed. This property
> +can take the folowing values: input, output-high, output-low.

nit: since this property not only describes the direction but also the
(potential) value, I suggest to rename it to "state".
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] gpio: Cygnus: add GPIO driver

[Alexandre Courbot]

On Tue, Dec 9, 2014 at 5:41 AM, Ray Jui  wrote:
> This GPIO driver supports all 3 GPIO controllers in the Broadcom Cygnus
> SoC. The 3 GPIO controllers are 1) the ASIU GPIO controller, 2) the
> chipCommonG GPIO controller, and 3) the ALWAYS-ON GPIO controller
>
> Signed-off-by: Ray Jui 
> Reviewed-by: Scott Branden 
> ---
>  drivers/gpio/Kconfig   |   11 +
>  drivers/gpio/Makefile  |1 +
>  drivers/gpio/gpio-bcm-cygnus.c |  705 
> 
>  3 files changed, 717 insertions(+)
>  create mode 100644 drivers/gpio/gpio-bcm-cygnus.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 633ec21..3e3b0342 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -126,6 +126,17 @@ config GPIO_74XX_MMIO
> 8 bits: 74244 (Input), 74273 (Output)
> 16 bits:741624 (Input), 7416374 (Output)
>
> +config GPIO_BCM_CYGNUS
> +   bool "Broadcom Cygnus GPIO support"
> +   depends on ARCH_BCM_CYGNUS && OF_GPIO
> +   help
> + Say yes here to turn on GPIO support for Broadcom Cygnus SoC
> +
> + The Broadcom Cygnus SoC has 3 GPIO controllers including the ASIU
> + GPIO controller (ASIU), the chipCommonG GPIO controller (CCM), and
> + the always-ON GPIO controller (CRMU). All 3 GPIO controllers are
> + supported by this driver
> +
>  config GPIO_CLPS711X
> tristate "CLPS711X GPIO support"
> depends on ARCH_CLPS711X || COMPILE_TEST
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 81755f1..31eb7e0 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_GPIO_ADP5520)+= gpio-adp5520.o
>  obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
>  obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
>  obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
> +obj-$(CONFIG_GPIO_BCM_CYGNUS)  += gpio-bcm-cygnus.o
>  obj-$(CONFIG_GPIO_BCM_KONA)+= gpio-bcm-kona.o
>  obj-$(CONFIG_GPIO_BT8XX)   += gpio-bt8xx.o
>  obj-$(CONFIG_GPIO_CLPS711X)+= gpio-clps711x.o
> diff --git a/drivers/gpio/gpio-bcm-cygnus.c b/drivers/gpio/gpio-bcm-cygnus.c
> new file mode 100644
> index 000..4fd9b73
> --- /dev/null
> +++ b/drivers/gpio/gpio-bcm-cygnus.c
> @@ -0,0 +1,705 @@
> +/*
> + * Copyright (C) 2014 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CYGNUS_GPIO_DATA_IN_OFFSET   0x00
> +#define CYGNUS_GPIO_DATA_OUT_OFFSET  0x04
> +#define CYGNUS_GPIO_OUT_EN_OFFSET0x08
> +#define CYGNUS_GPIO_IN_TYPE_OFFSET   0x0c
> +#define CYGNUS_GPIO_INT_DE_OFFSET0x10
> +#define CYGNUS_GPIO_INT_EDGE_OFFSET  0x14
> +#define CYGNUS_GPIO_INT_MSK_OFFSET   0x18
> +#define CYGNUS_GPIO_INT_STAT_OFFSET  0x1c
> +#define CYGNUS_GPIO_INT_MSTAT_OFFSET 0x20
> +#define CYGNUS_GPIO_INT_CLR_OFFSET   0x24
> +#define CYGNUS_GPIO_PAD_RES_OFFSET   0x34
> +#define CYGNUS_GPIO_RES_EN_OFFSET0x38
> +
> +/* drive strength control for ASIU GPIO */
> +#define CYGNUS_GPIO_ASIU_DRV0_CTRL_OFFSET 0x58
> +
> +/* drive strength control for CCM GPIO */
> +#define CYGNUS_GPIO_CCM_DRV0_CTRL_OFFSET  0x00
> +
> +#define GPIO_BANK_SIZE 0x200
> +#define NGPIOS_PER_BANK 32
> +#define GPIO_BIT(pin) ((pin) % NGPIOS_PER_BANK)
> +#define GPIO_BANK(pin) ((pin) / NGPIOS_PER_BANK)
> +
> +#define GPIO_FLAG_BIT_MASK   0x
> +#define GPIO_PULL_BIT_SHIFT  16
> +#define GPIO_PULL_BIT_MASK   0x3
> +
> +#define GPIO_DRV_STRENGTH_BIT_SHIFT  20
> +#define GPIO_DRV_STRENGTH_BITS   3
> +#define GPIO_DRV_STRENGTH_BIT_MASK   ((1 << GPIO_DRV_STRENGTH_BITS) - 1)
> +
> +/*
> + * For GPIO internal pull up/down registers
> + */
> +enum gpio_pull {
> +   GPIO_PULL_NONE = 0,
> +   GPIO_PULL_UP,
> +   GPIO_PULL_DOWN,
> +   GPIO_PULL_INVALID,
> +};
> +
> +/*
> + * GPIO drive strength
> + */
> +enum gpio_drv_strength {
> +   GPIO_DRV_STRENGTH_2MA = 0,
> +   GPIO_DRV_STRENGTH_4MA,
> +   GPIO_DRV_STRENGTH_6MA,
> +   GPIO_DRV_STRENGTH_8MA,
> +   GPIO_DRV_STRENGTH_10MA,
> +   GPIO_DRV_STRENGTH_12MA,
> +   GPIO_DRV_STRENGTH_14MA,
> +   GPIO_DRV_STRENGTH_16MA,
> +   GPIO_DRV_STRENGTH_INVALID,
> +};
> +
> +struct bcm_cygnus_gpio {
> +   struct device *dev;
> +   void __iomem *base;
> +   void __iomem *io_ctrl;
> +   spinlock_t lock;
> +   struct gpio_chip gc;
> +   unsigned num_banks;
> +   i

Re: [PATCH v4 3/4] gpio: Add find GPIO base in increasing order

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 12:38 PM, Zhou Wang  wrote:
> In function gpiochip_find_base, base number of a GPIO controller
> is found in decreasing order. ARCH_NR_GPIOS is used to define from
> which number we begin to search for base number of a GPIO controller.
>
> In fact, ARCH_NR_GPIOS brings us some multiplatform problems, like:
> http://www.spinics.net/lists/devicetree/msg60433.html
>
> This patch adds the support to find base number of a GPIO controller
> in increasing order. It will assign base number from 0.
> A new dts property called gpio-number-forward must be add to the related
> GPIO dts nodes if you want it works well.

Global GPIO numbers are a Linux-only concept, so your property should
be named linux,gpio-number-forward.

But even that way I don't think I like this idea. This just adds some
more mess to how the GPIO number space is constructed, and it is
already quite messy as it is. You have no guarantee over the probe
order of your GPIO controllers, so they may very well be probed in a
different order and end up with different base numbers anytime.

Not that this is your fault - the number namespace is broken by design
and I don't think there is a way to fix it. The long-term solution is
to stop using it by switching to the gpiod interface.

First a few questions to understand why you need your GPIOs numbered
this way, and if you need it at all:
- Can't you use the gpiod interface instead so you don't need to rely
on GPIO numbers?
- Do you plan to use the sysfs interface? If so then we are screwed,
because there is no way to use it without using global GPIO numbers.
This is something we should/will fix with named exported GPIOs, but we
are not here yet.

As to how we can solve your problem: if you must use GPIO integers
(because you need to use the sysfs interface for instance), and need
them affected consistently, the only way I can think of is to
introduce a "linux,gpio-base" property that will set gpiochip->base to
a fixed number. It still kind of sucks, but at least it will enforce
that different controllers get the same base number reliably, and the
firmware is the best placed to know how many GPIOs each controller has
and decide an appropriate layout.

I know we pushed back against this kind of solution in the past, but
it is still more reliable than having a property that affects how the
kernel's base finding function works, and will offer us a way to
eventually put ARCH_NR_GPIOS and gpiochip_find_base() to rest (at the
cost of backwards-compatibility, but we just cannot do without it).
Linus, do you agree?

In the medium term, we need to offer a solution for user-space to
*not* rely on GPIO numbers, and that will necessarily go through a new
sysfs interface, since at the moment even GPIO chips use their base
number in their exported name.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On Tue, Dec 9, 2014 at 11:14 PM, Tomeu Vizoso
 wrote:
> On 12/09/2014 06:38 AM, Alexandre Courbot wrote:
>> On Fri, Dec 5, 2014 at 1:14 AM, Tomeu Vizoso  
>> wrote:
>>> Hello,
>>>
>>> this v3 addresses the comments that the devfreq implementation got, namely:
>>>
>>> * Address misc. style issues found by Thierry and Alexander
>>> * Added helpers for register i/o
>>> * Further documented the structs
>>> * Enable the ACTMON after the IRQ handler has been installed
>>> * Disable the ACTMON before removing the IRQ handler
>>> * Add governor in a subsys initcall
>>>
>>> There's an open question on whether some functionality currently in this
>>> devfreq driver should be moved into the devfreq framework, but without 
>>> knowing
>>> of other SoC family that would benefit from it, I'm reticent. It would be
>>> great to hear from the devfreq maintainers if they have any plans regarding
>>> this, or if they have any suggestion.
>>
>> I cannot make a thorough review because of the problem I mentioned in
>> patch 3/3, but I am guessing this series is converging towards what we
>> want. Now the main question will be how we can leverage Arto's
>> watermark series for this one. I am ready to bet that doing so can
>> reduce quite a lot of code.
>>
>> Since you are likely to be the first user of the watermarking feature,
>> could you comment on its potential shortcomings and whatever needs to
>> be fixed to best implement ACTMON support using it? I will try to push
>> it myself, but you are obviously in a better position to understand
>> what is needed.
>
> Sure, I'm still playing with the idea, but I have sent a few questions
> to that thread already.

Seen that, thanks! After seeing your comments I come to thing that
maybe we should merge ACTMON support first and then adapt it to use
watermarking afterwards. I'm all for incremental changes and after 4
iterations this series should be close to completion now - would be
sad to go back to the design board at this stage.

Reviewing your v4 is on my schedule for tomorrow, thanks for your patience!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 1:14 AM, Tomeu Vizoso  wrote:
> Hello,
>
> this v3 addresses the comments that the devfreq implementation got, namely:
>
> * Address misc. style issues found by Thierry and Alexander
> * Added helpers for register i/o
> * Further documented the structs
> * Enable the ACTMON after the IRQ handler has been installed
> * Disable the ACTMON before removing the IRQ handler
> * Add governor in a subsys initcall
>
> There's an open question on whether some functionality currently in this
> devfreq driver should be moved into the devfreq framework, but without knowing
> of other SoC family that would benefit from it, I'm reticent. It would be
> great to hear from the devfreq maintainers if they have any plans regarding
> this, or if they have any suggestion.

I cannot make a thorough review because of the problem I mentioned in
patch 3/3, but I am guessing this series is converging towards what we
want. Now the main question will be how we can leverage Arto's
watermark series for this one. I am ready to bet that doing so can
reduce quite a lot of code.

Since you are likely to be the first user of the watermarking feature,
could you comment on its potential shortcomings and whatever needs to
be fixed to best implement ACTMON support using it? I will try to push
it myself, but you are obviously in a better position to understand
what is needed.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] ARM: tegra: Add Tegra124 ACTMON support

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 1:14 AM, Tomeu Vizoso  wrote:
> Add device node for the ACTMON block to the Tegra124 device tree.
>
> Signed-off-by: Tomeu Vizoso 
>
> ---
>
> v3: * Address misc. style issues found by Thierry and Alexander
> * Added helpers for register i/o
> * Further documented the structs
> * Enable the ACTMON after the IRQ handler has been installed
> * Disable the ACTMON before removing the IRQ handler
> * Add governor in a subsys initcall
>
> v2: * Add operating-points property
> ---
>  arch/arm/boot/dts/tegra124.dtsi|  23 +
>  drivers/devfreq/Kconfig|   7 +-
>  drivers/devfreq/Makefile   |   2 +-
>  drivers/devfreq/tegra-actmon-devfreq.c | 777 
> +
>  drivers/devfreq/tegra-devfreq.c| 718 --

In patch 2/3 you create tegra-devfreq.c, to apparently move it to
tegra-actmon-devfreq.c and (seemingly) add somemore lines to it. This
makes things difficult to review, and I am almost sure this is a
naming mistake - could you fix this and resend?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 7:24 PM, Maxime Ripard
 wrote:
> On Thu, Dec 04, 2014 at 11:49:19PM +0900, Alexandre Courbot wrote:
>> On Thu, Dec 4, 2014 at 11:27 PM, Maxime Ripard
>>  wrote:
>> > Hi,
>> >
>> > On Thu, Dec 04, 2014 at 11:15:38PM +0900, Alexandre Courbot wrote:
>> >> On Wed, Dec 3, 2014 at 1:12 AM, Maxime Ripard
>> >>  wrote:
>> >> > On Tue, Dec 02, 2014 at 03:29:46PM +0100, Linus Walleij wrote:
>> >> >> On Tue, Dec 2, 2014 at 3:13 PM, Alexandre Courbot  
>> >> >> wrote:
>> >> >> > On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
>> >> >> >  wrote:
>> >> >>
>> >> >> >> The only thing I'd like to have would be that the request here would
>> >> >> >> be non-exclusive, so that a later driver would still be allowed 
>> >> >> >> later
>> >> >> >> on to request that GPIO later on and manage it itself (ideally using
>> >> >> >> the usual gpiod_request function).
>> >> >> >
>> >> >> > Actually we have a plan (and I have some code too) to allow multiple
>> >> >> > consumers per GPIO. Although like Benoit I wonder why you would want
>> >> >> > to hog a GPIO and then request it properly later. Also, that probably
>> >> >> > means we should abandon the hog since it actively drives the line and
>> >> >> > would interfere with the late requested. How to do that correctly is
>> >> >> > not really clear to me.
>> >> >>
>> >> >> I don't get the usecase. A hogged GPIO is per definition hogged.
>> >> >> This sounds more like "initial settings" or something, which is another
>> >> >> usecase altogether.
>> >> >
>> >> > We do have one board where we have a pin (let's say GPIO14 of the bank
>> >> > A) that enables a regulator that will provide VCC the bank B.
>> >> >
>> >> > Now, both banks are handled by the same driver, but in order to have a
>> >> > working output on the bank B, we do need to set GPIO14 as soon as
>> >> > we're probed.
>> >> >
>> >> > Just relying on the usual deferred probing introduces a circular
>> >> > dependency between the gpio-regulator that needs to grab its GPIO from
>> >> > a driver not there yet, and the gpio driver that needs to enable its
>> >> > gpio-regulator.
>> >>
>> >> I don't get it. According to what you said, the following order should
>> >> go through IIUC:
>> >>
>> >> 1) bank A is probed, gpio 14 is available
>> >> 2) gpio-regulator is probed, acquires GPIO 14, regulator for Bank B is 
>> >> available
>> >> 3) bank B is probed, grabs its regulator and turn it on, probes.
>> >>
>> >> What am I missing?
>> >
>> > It would be true if bank A and B were exposed through different
>> > drivers (or at least different instances of the same driver), which is
>> > not the case.
>> >
>> > In our case, banks A and B are handled by the same instance.
>>
>> Ok, so both banks A and B are part of the same device/DT node. Now I
>> think I understand the issue. You need to hog the pin so that bank B
>> will work right after the device is probed.
>
> Exactly.
>
>> But you will still have the problem that the regulator device will
>> *not* be available when your device is probed, so you cannot call
>> regulator_get() for bank B anyway. I guess your only choice is to hog
>> that pin and leave it active ad vitam eternam.
>
> Hmmm, indeed.
>
> I'll stop boring you with this then :)

Please *keep* bothering us with any doubt you may have until they are
all cleared and you are sure this feature will be useful to you.
Especially since we are designing DT bindings that will have to be
carried over forever. We really want to get them right, and need input
of potential users for that.

Having a few design arguments is a small thing compared to the hassle
of having to work with unadapted features and bindings.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 12:22 AM, Pantelis Antoniou
 wrote:
> Hi Alexandre,
>
>> On Dec 4, 2014, at 17:10 , Alexandre Courbot  wrote:
>>
>> On Fri, Dec 5, 2014 at 12:02 AM, Pantelis Antoniou
>>  wrote:
>>> Hi Alexandre,
>>>
>>>> On Dec 4, 2014, at 16:58 , Alexandre Courbot  wrote:
>>>>
>>>> On Thu, Dec 4, 2014 at 11:47 PM, Pantelis Antoniou
>>>>  wrote:
>>>>> Hi Alexandre,
>>>>>
>>>>>> On Dec 4, 2014, at 16:41 , Alexandre Courbot  wrote:
>>>>>>
>>>>>> On Thu, Dec 4, 2014 at 11:27 PM, Pantelis Antoniou
>>>>>>  wrote:
>>>>>>> Hi Alexandre,
>>>>>>>
>>>>>>> I tried to stay away while things are being fleshed out but…
>>>>>>>
>>>>>>>> On Dec 4, 2014, at 16:15 , Alexandre Courbot  wrote:
>>>>>>>>
>>>>>>>> On Wed, Dec 3, 2014 at 1:12 AM, Maxime Ripard
>>>>>>>>  wrote:
>>>>>>>>> On Tue, Dec 02, 2014 at 03:29:46PM +0100, Linus Walleij wrote:
>>>>>>>>>> On Tue, Dec 2, 2014 at 3:13 PM, Alexandre Courbot  
>>>>>>>>>> wrote:
>>>>>>>>>>> On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
>>>>>>>>>>>  wrote:
>>>>>>>>>>
>>>>>>>>>>>> The only thing I'd like to have would be that the request here 
>>>>>>>>>>>> would
>>>>>>>>>>>> be non-exclusive, so that a later driver would still be allowed 
>>>>>>>>>>>> later
>>>>>>>>>>>> on to request that GPIO later on and manage it itself (ideally 
>>>>>>>>>>>> using
>>>>>>>>>>>> the usual gpiod_request function).
>>>>>>>>>>>
>>>>>>>>>>> Actually we have a plan (and I have some code too) to allow multiple
>>>>>>>>>>> consumers per GPIO. Although like Benoit I wonder why you would want
>>>>>>>>>>> to hog a GPIO and then request it properly later. Also, that 
>>>>>>>>>>> probably
>>>>>>>>>>> means we should abandon the hog since it actively drives the line 
>>>>>>>>>>> and
>>>>>>>>>>> would interfere with the late requested. How to do that correctly is
>>>>>>>>>>> not really clear to me.
>>>>>>>>>>
>>>>>>>>>> I don't get the usecase. A hogged GPIO is per definition hogged.
>>>>>>>>>> This sounds more like "initial settings" or something, which is 
>>>>>>>>>> another
>>>>>>>>>> usecase altogether.
>>>>>>>>>
>>>>>>>>> We do have one board where we have a pin (let's say GPIO14 of the bank
>>>>>>>>> A) that enables a regulator that will provide VCC the bank B.
>>>>>>>>>
>>>>>>>>> Now, both banks are handled by the same driver, but in order to have a
>>>>>>>>> working output on the bank B, we do need to set GPIO14 as soon as
>>>>>>>>> we're probed.
>>>>>>>>>
>>>>>>>>> Just relying on the usual deferred probing introduces a circular
>>>>>>>>> dependency between the gpio-regulator that needs to grab its GPIO from
>>>>>>>>> a driver not there yet, and the gpio driver that needs to enable its
>>>>>>>>> gpio-regulator.
>>>>>>>>
>>>>>>>> I don't get it. According to what you said, the following order should
>>>>>>>> go through IIUC:
>>>>>>>>
>>>>>>>> 1) bank A is probed, gpio 14 is available
>>>>>>>> 2) gpio-regulator is probed, acquires GPIO 14, regulator for Bank B is 
>>>>>>>> available
>>>>>>>> 3) bank B is probed, grabs its regulator and turn it on, probes.
>>>>>>>>
>>>>>>>> What am I missing?
>>>>>>>>
>>>>>>>>>
>>

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 12:02 AM, Pantelis Antoniou
 wrote:
> Hi Alexandre,
>
>> On Dec 4, 2014, at 16:58 , Alexandre Courbot  wrote:
>>
>> On Thu, Dec 4, 2014 at 11:47 PM, Pantelis Antoniou
>>  wrote:
>>> Hi Alexandre,
>>>
>>>> On Dec 4, 2014, at 16:41 , Alexandre Courbot  wrote:
>>>>
>>>> On Thu, Dec 4, 2014 at 11:27 PM, Pantelis Antoniou
>>>>  wrote:
>>>>> Hi Alexandre,
>>>>>
>>>>> I tried to stay away while things are being fleshed out but…
>>>>>
>>>>>> On Dec 4, 2014, at 16:15 , Alexandre Courbot  wrote:
>>>>>>
>>>>>> On Wed, Dec 3, 2014 at 1:12 AM, Maxime Ripard
>>>>>>  wrote:
>>>>>>> On Tue, Dec 02, 2014 at 03:29:46PM +0100, Linus Walleij wrote:
>>>>>>>> On Tue, Dec 2, 2014 at 3:13 PM, Alexandre Courbot  
>>>>>>>> wrote:
>>>>>>>>> On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
>>>>>>>>>  wrote:
>>>>>>>>
>>>>>>>>>> The only thing I'd like to have would be that the request here would
>>>>>>>>>> be non-exclusive, so that a later driver would still be allowed later
>>>>>>>>>> on to request that GPIO later on and manage it itself (ideally using
>>>>>>>>>> the usual gpiod_request function).
>>>>>>>>>
>>>>>>>>> Actually we have a plan (and I have some code too) to allow multiple
>>>>>>>>> consumers per GPIO. Although like Benoit I wonder why you would want
>>>>>>>>> to hog a GPIO and then request it properly later. Also, that probably
>>>>>>>>> means we should abandon the hog since it actively drives the line and
>>>>>>>>> would interfere with the late requested. How to do that correctly is
>>>>>>>>> not really clear to me.
>>>>>>>>
>>>>>>>> I don't get the usecase. A hogged GPIO is per definition hogged.
>>>>>>>> This sounds more like "initial settings" or something, which is another
>>>>>>>> usecase altogether.
>>>>>>>
>>>>>>> We do have one board where we have a pin (let's say GPIO14 of the bank
>>>>>>> A) that enables a regulator that will provide VCC the bank B.
>>>>>>>
>>>>>>> Now, both banks are handled by the same driver, but in order to have a
>>>>>>> working output on the bank B, we do need to set GPIO14 as soon as
>>>>>>> we're probed.
>>>>>>>
>>>>>>> Just relying on the usual deferred probing introduces a circular
>>>>>>> dependency between the gpio-regulator that needs to grab its GPIO from
>>>>>>> a driver not there yet, and the gpio driver that needs to enable its
>>>>>>> gpio-regulator.
>>>>>>
>>>>>> I don't get it. According to what you said, the following order should
>>>>>> go through IIUC:
>>>>>>
>>>>>> 1) bank A is probed, gpio 14 is available
>>>>>> 2) gpio-regulator is probed, acquires GPIO 14, regulator for Bank B is 
>>>>>> available
>>>>>> 3) bank B is probed, grabs its regulator and turn it on, probes.
>>>>>>
>>>>>> What am I missing?
>>>>>>
>>>>>>>
>>>>>>> GPIO hogging needs to be the ideal solution for that, since we can
>>>>>>> just enforce the GPIO14 value as the driver is probed, which provides
>>>>>>> the guarantee that any driver using the bank B will actually drive the
>>>>>>> GPIO it might use.
>>>>>>
>>>>>> At this point I start wondering if such initial setup should not be
>>>>>> the job of the bootloader? GPIO hogging ought to be simple and
>>>>>> definitive, adding the possibility to have it just as an initial value
>>>>>> would considerably complexify it. E.g. when is the gpio chip driver
>>>>>> supposed to release the hogged descriptor in such a case?
>>>>>>
>>>>>
>>>>> Do not count on the bootloader setting up anything. The trend is
>>>>> for the bootl

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Dec 4, 2014 at 11:52 PM, Maxime Ripard
 wrote:
> Hi again,
>
> It looks like I missed some part of it.
>
> On Thu, Dec 04, 2014 at 11:15:38PM +0900, Alexandre Courbot wrote:
>> > GPIO hogging needs to be the ideal solution for that, since we can
>> > just enforce the GPIO14 value as the driver is probed, which provides
>> > the guarantee that any driver using the bank B will actually drive the
>> > GPIO it might use.
>>
>> At this point I start wondering if such initial setup should not be
>> the job of the bootloader? GPIO hogging ought to be simple and
>> definitive, adding the possibility to have it just as an initial value
>> would considerably complexify it. E.g. when is the gpio chip driver
>> supposed to release the hogged descriptor in such a case?
>
> Relying on the bootloader for such trivial (and critical) things may
> not work at all. You don't always have the option to replace it,
> either because you physically can't, or just because you don't have
> any alternative.
>
> I agree that in general this is something that should go in the
> bootloader, but we should have a way to deal with the case where it's
> not done.
>
>> Note that if the multiple GPIO consumer feature we are planning goes
>> through, you should be able to use both hogging *and* a regulator on
>> the same GPIO and achieve what you want. The expectation of multiple
>> consumers is that the board designers know what they are doing, and
>> this case would certainly fit (chip hogs the line and doesn't touch
>> the value after that, letting the regulator control it without any
>> conflict afterwards), although it would of course be better to solve
>> the issue through regular probing...
>
> If such an effort is on-going, then I'm totally fine waiting for it
> and leaving that outside the hogging mechanism. As long as it works,
> I'm happy.

Ok. I just want to wait until the next -rc1 to make sure that the
large GPIO array removal patch (on which the multiple consumers
feature depend) did not break anything important, then I will submit
it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Dec 4, 2014 at 11:47 PM, Pantelis Antoniou
 wrote:
> Hi Alexandre,
>
>> On Dec 4, 2014, at 16:41 , Alexandre Courbot  wrote:
>>
>> On Thu, Dec 4, 2014 at 11:27 PM, Pantelis Antoniou
>>  wrote:
>>> Hi Alexandre,
>>>
>>> I tried to stay away while things are being fleshed out but…
>>>
>>>> On Dec 4, 2014, at 16:15 , Alexandre Courbot  wrote:
>>>>
>>>> On Wed, Dec 3, 2014 at 1:12 AM, Maxime Ripard
>>>>  wrote:
>>>>> On Tue, Dec 02, 2014 at 03:29:46PM +0100, Linus Walleij wrote:
>>>>>> On Tue, Dec 2, 2014 at 3:13 PM, Alexandre Courbot  
>>>>>> wrote:
>>>>>>> On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
>>>>>>>  wrote:
>>>>>>
>>>>>>>> The only thing I'd like to have would be that the request here would
>>>>>>>> be non-exclusive, so that a later driver would still be allowed later
>>>>>>>> on to request that GPIO later on and manage it itself (ideally using
>>>>>>>> the usual gpiod_request function).
>>>>>>>
>>>>>>> Actually we have a plan (and I have some code too) to allow multiple
>>>>>>> consumers per GPIO. Although like Benoit I wonder why you would want
>>>>>>> to hog a GPIO and then request it properly later. Also, that probably
>>>>>>> means we should abandon the hog since it actively drives the line and
>>>>>>> would interfere with the late requested. How to do that correctly is
>>>>>>> not really clear to me.
>>>>>>
>>>>>> I don't get the usecase. A hogged GPIO is per definition hogged.
>>>>>> This sounds more like "initial settings" or something, which is another
>>>>>> usecase altogether.
>>>>>
>>>>> We do have one board where we have a pin (let's say GPIO14 of the bank
>>>>> A) that enables a regulator that will provide VCC the bank B.
>>>>>
>>>>> Now, both banks are handled by the same driver, but in order to have a
>>>>> working output on the bank B, we do need to set GPIO14 as soon as
>>>>> we're probed.
>>>>>
>>>>> Just relying on the usual deferred probing introduces a circular
>>>>> dependency between the gpio-regulator that needs to grab its GPIO from
>>>>> a driver not there yet, and the gpio driver that needs to enable its
>>>>> gpio-regulator.
>>>>
>>>> I don't get it. According to what you said, the following order should
>>>> go through IIUC:
>>>>
>>>> 1) bank A is probed, gpio 14 is available
>>>> 2) gpio-regulator is probed, acquires GPIO 14, regulator for Bank B is 
>>>> available
>>>> 3) bank B is probed, grabs its regulator and turn it on, probes.
>>>>
>>>> What am I missing?
>>>>
>>>>>
>>>>> GPIO hogging needs to be the ideal solution for that, since we can
>>>>> just enforce the GPIO14 value as the driver is probed, which provides
>>>>> the guarantee that any driver using the bank B will actually drive the
>>>>> GPIO it might use.
>>>>
>>>> At this point I start wondering if such initial setup should not be
>>>> the job of the bootloader? GPIO hogging ought to be simple and
>>>> definitive, adding the possibility to have it just as an initial value
>>>> would considerably complexify it. E.g. when is the gpio chip driver
>>>> supposed to release the hogged descriptor in such a case?
>>>>
>>>
>>> Do not count on the bootloader setting up anything. The trend is
>>> for the bootloader to setup the minimal environment to load your kernel
>>> and jump to it.
>>>
>>> http://www.denx.de/wiki/pub/U-Boot/MiniSummitELCE2013/2013-ELCE-U-Boot-Falcon-Boot.pdf
>>
>> Just wondering. :)
>>
>> But yeah, there are some use-cases (such as this one or
>> Linux-as-a-bootloader) for which this would not play nicely.
>>
>>>
>>>
>>>> Note that if the multiple GPIO consumer feature we are planning goes
>>>> through, you should be able to use both hogging *and* a regulator on
>>>> the same GPIO and achieve what you want. The expectation of multiple
>>>> consumers is that the board designers know what they are doing, a

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Dec 4, 2014 at 11:27 PM, Maxime Ripard
 wrote:
> Hi,
>
> On Thu, Dec 04, 2014 at 11:15:38PM +0900, Alexandre Courbot wrote:
>> On Wed, Dec 3, 2014 at 1:12 AM, Maxime Ripard
>>  wrote:
>> > On Tue, Dec 02, 2014 at 03:29:46PM +0100, Linus Walleij wrote:
>> >> On Tue, Dec 2, 2014 at 3:13 PM, Alexandre Courbot  
>> >> wrote:
>> >> > On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
>> >> >  wrote:
>> >>
>> >> >> The only thing I'd like to have would be that the request here would
>> >> >> be non-exclusive, so that a later driver would still be allowed later
>> >> >> on to request that GPIO later on and manage it itself (ideally using
>> >> >> the usual gpiod_request function).
>> >> >
>> >> > Actually we have a plan (and I have some code too) to allow multiple
>> >> > consumers per GPIO. Although like Benoit I wonder why you would want
>> >> > to hog a GPIO and then request it properly later. Also, that probably
>> >> > means we should abandon the hog since it actively drives the line and
>> >> > would interfere with the late requested. How to do that correctly is
>> >> > not really clear to me.
>> >>
>> >> I don't get the usecase. A hogged GPIO is per definition hogged.
>> >> This sounds more like "initial settings" or something, which is another
>> >> usecase altogether.
>> >
>> > We do have one board where we have a pin (let's say GPIO14 of the bank
>> > A) that enables a regulator that will provide VCC the bank B.
>> >
>> > Now, both banks are handled by the same driver, but in order to have a
>> > working output on the bank B, we do need to set GPIO14 as soon as
>> > we're probed.
>> >
>> > Just relying on the usual deferred probing introduces a circular
>> > dependency between the gpio-regulator that needs to grab its GPIO from
>> > a driver not there yet, and the gpio driver that needs to enable its
>> > gpio-regulator.
>>
>> I don't get it. According to what you said, the following order should
>> go through IIUC:
>>
>> 1) bank A is probed, gpio 14 is available
>> 2) gpio-regulator is probed, acquires GPIO 14, regulator for Bank B is 
>> available
>> 3) bank B is probed, grabs its regulator and turn it on, probes.
>>
>> What am I missing?
>
> It would be true if bank A and B were exposed through different
> drivers (or at least different instances of the same driver), which is
> not the case.
>
> In our case, banks A and B are handled by the same instance.

Ok, so both banks A and B are part of the same device/DT node. Now I
think I understand the issue. You need to hog the pin so that bank B
will work right after the device is probed.

But you will still have the problem that the regulator device will
*not* be available when your device is probed, so you cannot call
regulator_get() for bank B anyway. I guess your only choice is to hog
that pin and leave it active ad vitam eternam.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Dec 4, 2014 at 11:27 PM, Pantelis Antoniou
 wrote:
> Hi Alexandre,
>
> I tried to stay away while things are being fleshed out but…
>
>> On Dec 4, 2014, at 16:15 , Alexandre Courbot  wrote:
>>
>> On Wed, Dec 3, 2014 at 1:12 AM, Maxime Ripard
>>  wrote:
>>> On Tue, Dec 02, 2014 at 03:29:46PM +0100, Linus Walleij wrote:
>>>> On Tue, Dec 2, 2014 at 3:13 PM, Alexandre Courbot  wrote:
>>>>> On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
>>>>>  wrote:
>>>>
>>>>>> The only thing I'd like to have would be that the request here would
>>>>>> be non-exclusive, so that a later driver would still be allowed later
>>>>>> on to request that GPIO later on and manage it itself (ideally using
>>>>>> the usual gpiod_request function).
>>>>>
>>>>> Actually we have a plan (and I have some code too) to allow multiple
>>>>> consumers per GPIO. Although like Benoit I wonder why you would want
>>>>> to hog a GPIO and then request it properly later. Also, that probably
>>>>> means we should abandon the hog since it actively drives the line and
>>>>> would interfere with the late requested. How to do that correctly is
>>>>> not really clear to me.
>>>>
>>>> I don't get the usecase. A hogged GPIO is per definition hogged.
>>>> This sounds more like "initial settings" or something, which is another
>>>> usecase altogether.
>>>
>>> We do have one board where we have a pin (let's say GPIO14 of the bank
>>> A) that enables a regulator that will provide VCC the bank B.
>>>
>>> Now, both banks are handled by the same driver, but in order to have a
>>> working output on the bank B, we do need to set GPIO14 as soon as
>>> we're probed.
>>>
>>> Just relying on the usual deferred probing introduces a circular
>>> dependency between the gpio-regulator that needs to grab its GPIO from
>>> a driver not there yet, and the gpio driver that needs to enable its
>>> gpio-regulator.
>>
>> I don't get it. According to what you said, the following order should
>> go through IIUC:
>>
>> 1) bank A is probed, gpio 14 is available
>> 2) gpio-regulator is probed, acquires GPIO 14, regulator for Bank B is 
>> available
>> 3) bank B is probed, grabs its regulator and turn it on, probes.
>>
>> What am I missing?
>>
>>>
>>> GPIO hogging needs to be the ideal solution for that, since we can
>>> just enforce the GPIO14 value as the driver is probed, which provides
>>> the guarantee that any driver using the bank B will actually drive the
>>> GPIO it might use.
>>
>> At this point I start wondering if such initial setup should not be
>> the job of the bootloader? GPIO hogging ought to be simple and
>> definitive, adding the possibility to have it just as an initial value
>> would considerably complexify it. E.g. when is the gpio chip driver
>> supposed to release the hogged descriptor in such a case?
>>
>
> Do not count on the bootloader setting up anything. The trend is
> for the bootloader to setup the minimal environment to load your kernel
> and jump to it.
>
> http://www.denx.de/wiki/pub/U-Boot/MiniSummitELCE2013/2013-ELCE-U-Boot-Falcon-Boot.pdf

Just wondering. :)

But yeah, there are some use-cases (such as this one or
Linux-as-a-bootloader) for which this would not play nicely.

>
>
>> Note that if the multiple GPIO consumer feature we are planning goes
>> through, you should be able to use both hogging *and* a regulator on
>> the same GPIO and achieve what you want. The expectation of multiple
>> consumers is that the board designers know what they are doing, and
>> this case would certainly fit (chip hogs the line and doesn't touch
>> the value after that, letting the regulator control it without any
>> conflict afterwards), although it would of course be better to solve
>> the issue through regular probing...
>
>
> That’s why I was advocating a simple probing driver for all this.
> Figure out a way for this driver to be probed first would be an easier
> solution that what’s going on here.

Do you mean, a driver whose sole job is to probe other drivers in the
right order? :/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Wed, Dec 3, 2014 at 1:12 AM, Maxime Ripard
 wrote:
> On Tue, Dec 02, 2014 at 03:29:46PM +0100, Linus Walleij wrote:
>> On Tue, Dec 2, 2014 at 3:13 PM, Alexandre Courbot  wrote:
>> > On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
>> >  wrote:
>>
>> >> The only thing I'd like to have would be that the request here would
>> >> be non-exclusive, so that a later driver would still be allowed later
>> >> on to request that GPIO later on and manage it itself (ideally using
>> >> the usual gpiod_request function).
>> >
>> > Actually we have a plan (and I have some code too) to allow multiple
>> > consumers per GPIO. Although like Benoit I wonder why you would want
>> > to hog a GPIO and then request it properly later. Also, that probably
>> > means we should abandon the hog since it actively drives the line and
>> > would interfere with the late requested. How to do that correctly is
>> > not really clear to me.
>>
>> I don't get the usecase. A hogged GPIO is per definition hogged.
>> This sounds more like "initial settings" or something, which is another
>> usecase altogether.
>
> We do have one board where we have a pin (let's say GPIO14 of the bank
> A) that enables a regulator that will provide VCC the bank B.
>
> Now, both banks are handled by the same driver, but in order to have a
> working output on the bank B, we do need to set GPIO14 as soon as
> we're probed.
>
> Just relying on the usual deferred probing introduces a circular
> dependency between the gpio-regulator that needs to grab its GPIO from
> a driver not there yet, and the gpio driver that needs to enable its
> gpio-regulator.

I don't get it. According to what you said, the following order should
go through IIUC:

1) bank A is probed, gpio 14 is available
2) gpio-regulator is probed, acquires GPIO 14, regulator for Bank B is available
3) bank B is probed, grabs its regulator and turn it on, probes.

What am I missing?

>
> GPIO hogging needs to be the ideal solution for that, since we can
> just enforce the GPIO14 value as the driver is probed, which provides
> the guarantee that any driver using the bank B will actually drive the
> GPIO it might use.

At this point I start wondering if such initial setup should not be
the job of the bootloader? GPIO hogging ought to be simple and
definitive, adding the possibility to have it just as an initial value
would considerably complexify it. E.g. when is the gpio chip driver
supposed to release the hogged descriptor in such a case?

Note that if the multiple GPIO consumer feature we are planning goes
through, you should be able to use both hogging *and* a regulator on
the same GPIO and achieve what you want. The expectation of multiple
consumers is that the board designers know what they are doing, and
this case would certainly fit (chip hogs the line and doesn't touch
the value after that, letting the regulator control it without any
conflict afterwards), although it would of course be better to solve
the issue through regular probing...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Tue, Dec 2, 2014 at 1:36 AM, Maxime Ripard
 wrote:
> Hi,
>
> On Fri, Nov 28, 2014 at 04:30:01PM +0900, Alexandre Courbot wrote:
>> > +/**
>> > + * do_gpio_hog - Given node is a GPIO hog configuration, handle it
>> > + * @np:device node to get GPIO from
>> > + *
>> > + * This is only used by of_gpiochip_add to request/set GPIO initial
>> > + * configuration.
>> > + */
>> > +static int do_gpio_hog(struct device_node *np)
>> > +{
>> > +   struct gpio_desc *desc = NULL;
>> > +   int err;
>> > +   const char *name;
>> > +   enum gpio_lookup_flags lflags;
>> > +   enum gpiod_flags dflags;
>> > +
>> > +   desc = of_get_gpio_hog(np, &name, &lflags, &dflags);
>> > +   if (!desc)
>> > +   return -ENOTSUPP;
>> > +   else if (IS_ERR(desc))
>> > +   return PTR_ERR(desc);
>> > +
>> > +   err = gpiod_request(desc, name);
>>
>> Using this function means that a GPIO chip module cannot be unloaded
>> if it uses GPIO hogs. Is it the intended behavior? If not, please use
>> gpiochip_request_own_desc() instead, and make sure to call
>> gpiochip_free_own_desc() for each hog when the driver is unloaded.
>
> The only thing I'd like to have would be that the request here would
> be non-exclusive, so that a later driver would still be allowed later
> on to request that GPIO later on and manage it itself (ideally using
> the usual gpiod_request function).

Actually we have a plan (and I have some code too) to allow multiple
consumers per GPIO. Although like Benoit I wonder why you would want
to hog a GPIO and then request it properly later. Also, that probably
means we should abandon the hog since it actively drives the line and
would interfere with the late requested. How to do that correctly is
not really clear to me.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Tue, Dec 2, 2014 at 9:22 AM, Benoit Parrot  wrote:
>> > +   }
>> > +
>> > +   if (tmp > MAX_PHANDLE_ARGS) {
>> > +   desc = ERR_PTR(-EINVAL);
>> > +   goto out;
>> > +   }
>> > +
>> > +   gg_data.gpiospec.args_count = tmp;
>> > +   gg_data.gpiospec.np = chip_np;
>> > +   for (i = 0; i < tmp; i++) {
>> > +   ret = of_property_read_u32(np, "gpios",
>> > +  &gg_data.gpiospec.args[i]);
>> > +   if (ret) {
>> > +   desc = ERR_PTR(ret);
>> > +   goto out;
>> > +   }
>> > +   }
>> > +
>> > +   gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
>>
>> This seems to work but only supports one GPIO per hog node. It would
>> be nice to be able to specify several GPIOs to which the same settings
>> need to be applied.
>
> This is on purpose following Linus Walleij's comment.

Could you point me to his comment? My bad for not remembering what he
said, but I'd like to understand why.

>> Using this function means that a GPIO chip module cannot be unloaded
>> if it uses GPIO hogs. Is it the intended behavior? If not, please use
>> gpiochip_request_own_desc() instead, and make sure to call
>> gpiochip_free_own_desc() for each hog when the driver is unloaded.
>
> So I guess we could add a undo_gpio_hog() function and hook it up under 
> of_gpiochip_remove().
> Now instead of maintaining a seperate structure just to keep track of hogged 
> descriptor,
> would it be acceptable to add a new "gpio_desc.flags" value in gpiolib.h says:
>
>#define FLAG_GPIO_IS_HOGGED 10
>
> And key on that at removal time instead of creating a list and having to 
> maintain that?

Definitely, that would be even better I think.

>> I would suggest to factorize this code that is similar to the one
>> found in __gpiod_get_index(). Do all the DT parsing in a function that
>> just returns a descriptor and the
>
> I would tend to agree.
> But as Linus suggested I was trying to contain the changes to gpiolib_of.c 
> only.

If we add a FLAG_GPIO_IS_HOGGED and undo the hogs when the chip is
unloaded, I would say that this becomes a gpiolib feature. Moving it
here would also allow non-DT GPIO providers to implement hogs (it
should be particularly easy to implement for platform data). Linus, do
you agree?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 2/2] gpio: Document GPIO hogging mechanism

[Alexandre Courbot]

On Tue, Dec 2, 2014 at 7:57 AM, Benoit Parrot  wrote:
> Alexandre Courbot  wrote on Fri [2014-Nov-28 16:31:19 
> +0900]:
>> On Fri, Nov 21, 2014 at 8:54 AM, Benoit Parrot  wrote:
>> > Add GPIO hogging documentation to gpio.txt
>> >
>> > Signed-off-by: Benoit Parrot 
>> > ---
>> > Changes since v1:
>> >  * Split the devicetree bindings documentation in its own patch.
>> >
>> >  Documentation/devicetree/bindings/gpio/gpio.txt | 25 
>> > +
>> >  1 file changed, 25 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
>> > b/Documentation/devicetree/bindings/gpio/gpio.txt
>> > index 3fb8f53..82755e2 100644
>> > --- a/Documentation/devicetree/bindings/gpio/gpio.txt
>> > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
>> > @@ -103,6 +103,24 @@ Every GPIO controller node must contain both an empty 
>> > "gpio-controller"
>> >  property, and a #gpio-cells integer property, which indicates the number 
>> > of
>> >  cells in a gpio-specifier.
>> >
>> > +The GPIO chip may contain GPIO hog definitions. GPIO hogging is a 
>> > mechanism
>> > +providing automatic GPIO request and configuration as part of the
>> > +gpio-controller's driver probe function.
>> > +
>> > +Each GPIO hog definition is represented as a child node of the GPIO 
>> > controller.
>> > +Required properties:
>> > +- gpio-hog: a property specifying that this child node represent a 
>> > gpio-hog.
>> > +- gpios: store the gpio information (id, flags, ...). Shall contain the
>> > +  number of cells specified in its parent node (GPIO controller node).
>> > +- input: a property specifying to set the GPIO direction as input.
>> > +- output-high: a property specifying to set the GPIO direction to output 
>> > with
>> > +  the value high.
>> > +- output-low: a property specifying to set the GPIO direction to output 
>> > with
>> > +  the value low.
>> > +
>> > +Optional properties:
>> > +- line-name: the GPIO label name. If not present the node name is used.
>> > +
>> >  Example of two SOC GPIO banks defined as gpio-controller nodes:
>> >
>> > qe_pio_a: gpio-controller@1400 {
>> > @@ -110,6 +128,13 @@ Example of two SOC GPIO banks defined as 
>> > gpio-controller nodes:
>> > reg = <0x1400 0x18>;
>> > gpio-controller;
>> > #gpio-cells = <2>;
>> > +
>> > +   line_b: line_b {
>>
>> Mmm what is the label used for? Can this node ever be referenced from
>> somewhere else?
>
> It's not used for anything else as far as I know other than as the line-name 
> to be assigned to the gpio being hogged.
> I guess you agree with Linus and should make the line-name mandatory and 
> remove the label altogether?
>
> I was trying to keep the verbosity to a minimum so as to have the possibilty 
> to keep everything on a single line when possible.

It's just that when you see a label, you expect it to be referenced
somewhere, which is obviously not the case here. Just having

line_b {

would work just as well, wouldn't it?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] ARM: hip04: set ARCH_NR_GPIO to 128

[Alexandre Courbot]

On Sat, Nov 29, 2014 at 6:16 AM, Arnd Bergmann  wrote:
> On Friday 28 November 2014 16:54:44 Linus Walleij wrote:
>> On Fri, Nov 28, 2014 at 10:33 AM, Arnd Bergmann  wrote:
>> > On Friday 28 November 2014 14:29:47 Zhou Wang wrote:
>>
>> >> Set ARCH_NR_GPIO for Hisilicon Soc Hip04, which has 4 GPIO
>> >> controllers with 32 GPIOs each.
>> >>
>> >> Signed-off-by: Zhou Wang 
>> >> ---
>> >>  arch/arm/Kconfig|1 +
>> >>  arch/arm/configs/hisi_defconfig |3 +++
>> >>  2 files changed, 4 insertions(+)
>> >>
>> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> >> index 89c4b5c..26aae1e 100644
>> >> --- a/arch/arm/Kconfig
>> >> +++ b/arch/arm/Kconfig
>> >> @@ -1509,6 +1509,7 @@ config ARCH_NR_GPIO
>> >> default 352 if ARCH_VT8500
>> >> default 288 if ARCH_ROCKCHIP
>> >> default 264 if MACH_H4700
>> >> +   default 128 if ARCH_HIP04
>> >> default 0
>> >> help
>> >>   Maximum number of GPIOs in the system.
>> >>
>> >
>> > If I remember correctly, you don't actually need to set this if all gpio
>> > clients are using the new gpio descriptor interfaces instead of gpio
>> > numbers.
>>
>> Unfortunately you still have to. We are working on removing the
>> dependency on ARCH_NR_GPIO, old habits die hard.
>
> Ok, I see.
>
>> But I just
>> merged this patch:
>> http://marc.info/?l=linux-gpio&m=141638350328535&w=2
>>
>> Which makes the situation better.
>
> Ah, cool!
>
>> There is however some other use of this define, so there is
>> some work required still to get rid of it.
>
> These are the only ones I could find:
>
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 9f0682534e2f..fe05d8b375ca 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -224,9 +224,6 @@ static int davinci_gpio_probe(struct platform_device 
> *pdev)
> return -EINVAL;
> }
>
> -   if (WARN_ON(ARCH_NR_GPIOS < ngpio))
> -   ngpio = ARCH_NR_GPIOS;
> -
> chips = devm_kzalloc(dev,
>  ngpio * sizeof(struct davinci_gpio_controller),
>  GFP_KERNEL);
> diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c 
> b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
> index 746db6acf648..4f70024c2f9a 100644
> --- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c
> +++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
> @@ -281,12 +281,12 @@ struct nmk_pinctrl {
> void __iomem *prcm_base;
>  };
>
> -static struct nmk_gpio_chip *
> -nmk_gpio_chips[DIV_ROUND_UP(ARCH_NR_GPIOS, NMK_GPIO_PER_CHIP)];
> +#define NUM_BANKS 9 /* increase if necessary */
> +
> +static struct nmk_gpio_chip *nmk_gpio_chips[NUM_BANKS];
>
>  static DEFINE_SPINLOCK(nmk_gpio_slpm_lock);
>
> -#define NUM_BANKS ARRAY_SIZE(nmk_gpio_chips)
>
>  static void __nmk_gpio_set_mode(struct nmk_gpio_chip *nmk_chip,
> unsigned offset, int gpio_mode)
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index 383ade1a211b..b53bcf5c58e7 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -13,23 +13,6 @@
>  #include 
>  #include 
>
> -/* Platforms may implement their GPIO interface with library code,
> - * at a small performance cost for non-inlined operations and some
> - * extra memory (for code and for per-GPIO table entries).
> - *
> - * While the GPIO programming interface defines valid GPIO numbers
> - * to be in the range 0..MAX_INT, this library restricts them to the
> - * smaller range 0..ARCH_NR_GPIOS-1.
> - *
> - * ARCH_NR_GPIOS is somewhat arbitrary; it usually reflects the sum of
> - * builtin/SoC GPIOs plus a number of GPIOs on expanders; the latter is
> - * actually an estimate of a board-specific value.
> - */
> -
> -#ifndef ARCH_NR_GPIOS
> -#define ARCH_NR_GPIOS  512
> -#endif
> -
>  /*
>   * "valid" GPIO numbers are nonnegative and may be passed to
>   * setup routines like gpio_request().  only some valid numbers
> @@ -41,7 +24,11 @@
>
>  static inline bool gpio_is_valid(int number)
>  {
> -   return number >= 0 && number < ARCH_NR_GPIOS;
> +#ifdef ARCH_NR_GPIOS
> +   if (number > ARCH_NR_GPIOS)
> +   return 0;
> +#endif
> +   return number >= 0;
>  }
>
>  struct device;
>
>> > However if one builds a kernel that just enables OMAP4 and HIP04, I suspect
>> > it can't work on OMAP4 for any gpio line above 128, which seems to be
>> > a fundamental multiplatform problem.
>>
>> Yes I guess you are right :(
>>
>> It's probably just so that so many platforms converge on 512.
>>
>> > Do we neet to increase the default to 512 for all ARCH_MULTIPLATFORM
>> > configurations and just leave ARCH_SHMOBILE, ARCH_TEGRA and MACH_H4700
>> > here as special cases?
>>
>> That'd be good while we are working to kill off
>> ARCH_NR_GPIO for good.
>>
>> I guess there could be arch-specific problems with trying
>> to get rid of ARCH_NR_GPIO for good, do you have some
>> input on th

Re: [Patch v2 2/2] gpio: Document GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Nov 21, 2014 at 8:54 AM, Benoit Parrot  wrote:
> Add GPIO hogging documentation to gpio.txt
>
> Signed-off-by: Benoit Parrot 
> ---
> Changes since v1:
>  * Split the devicetree bindings documentation in its own patch.
>
>  Documentation/devicetree/bindings/gpio/gpio.txt | 25 
> +
>  1 file changed, 25 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
> b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 3fb8f53..82755e2 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -103,6 +103,24 @@ Every GPIO controller node must contain both an empty 
> "gpio-controller"
>  property, and a #gpio-cells integer property, which indicates the number of
>  cells in a gpio-specifier.
>
> +The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> +providing automatic GPIO request and configuration as part of the
> +gpio-controller's driver probe function.
> +
> +Each GPIO hog definition is represented as a child node of the GPIO 
> controller.
> +Required properties:
> +- gpio-hog: a property specifying that this child node represent a gpio-hog.
> +- gpios: store the gpio information (id, flags, ...). Shall contain the
> +  number of cells specified in its parent node (GPIO controller node).
> +- input: a property specifying to set the GPIO direction as input.
> +- output-high: a property specifying to set the GPIO direction to output with
> +  the value high.
> +- output-low: a property specifying to set the GPIO direction to output with
> +  the value low.
> +
> +Optional properties:
> +- line-name: the GPIO label name. If not present the node name is used.
> +
>  Example of two SOC GPIO banks defined as gpio-controller nodes:
>
> qe_pio_a: gpio-controller@1400 {
> @@ -110,6 +128,13 @@ Example of two SOC GPIO banks defined as gpio-controller 
> nodes:
> reg = <0x1400 0x18>;
> gpio-controller;
> #gpio-cells = <2>;
> +
> +   line_b: line_b {

Mmm what is the label used for? Can this node ever be referenced from
somewhere else?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Fri, Nov 21, 2014 at 8:54 AM, Benoit Parrot  wrote:
> Based on Boris Brezillion's work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO
> when the gpio controller is probed.
>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().
>
> The purpose of this is to allows specific GPIOs to be configured
> without any driver specific code.
> This particularly useful because board design are getting
> increasingly complex and given SoC pins can now have upward
> of 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes and combination.
>
> Specific drivers should not necessarily need to be aware of
> what accounts to a specific board implementation. This board level
> "description" should be best kept as part of the dts file.
>
> Signed-off-by: Benoit Parrot 
> ---
> Changes since v1:
>  * Refactor the gpio-hog mechanism as private functions meant to
>be to invoked from of_gpiochip_add().
>
>  drivers/gpio/gpiolib-of.c | 188 
> ++
>  1 file changed, 188 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 604dbe6..3caed76 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "gpiolib.h"
>
> @@ -111,6 +112,184 @@ int of_get_named_gpio_flags(struct device_node *np, 
> const char *list_name,
>  EXPORT_SYMBOL(of_get_named_gpio_flags);
>
>  /**
> + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO 
> API
> + * @np:device node to get GPIO from
> + * @name:  GPIO line name
> + * @flags: a flags pointer to fill in
> + *
> + * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
> + * value on the error condition.
> + */
> +
> +static struct gpio_desc *of_get_gpio_hog(struct device_node *np,
> + const char **name,
> + enum gpio_lookup_flags *lflags,
> + enum gpiod_flags *dflags)
> +{
> +   struct device_node *chip_np;
> +   enum of_gpio_flags xlate_flags;
> +   struct gpio_desc *desc;
> +   struct gg_data gg_data = {
> +   .flags = &xlate_flags,
> +   .out_gpio = NULL,
> +   };
> +   u32 tmp;
> +   int i, in, outlo, outhi;
> +   int ret;
> +
> +   if (!np)
> +   return ERR_PTR(-EINVAL);

This function is being called from a perfectly mastered context in
this file, so maybe we can avoid this check.

> +
> +   chip_np = np->parent;
> +   if (!chip_np) {
> +   desc = ERR_PTR(-EINVAL);
> +   goto out;
> +   }
> +
> +   if (!lflags || !dflags) {
> +   desc = ERR_PTR(-EINVAL);
> +   goto out;
> +   }

Same for this one.

> +
> +   *lflags = 0;
> +   *dflags = 0;
> +   in = 0;
> +   outlo = 0;
> +   outhi = 0;
> +
> +   ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> +   if (ret) {
> +   desc = ERR_PTR(ret);
> +   goto out;

Please use "return ERR_PTR(ret);" directly, since you do absolutely no
cleanup in out:. Same remark everywhere it applies.

> +   }
> +
> +   if (tmp > MAX_PHANDLE_ARGS) {
> +   desc = ERR_PTR(-EINVAL);
> +   goto out;
> +   }
> +
> +   gg_data.gpiospec.args_count = tmp;
> +   gg_data.gpiospec.np = chip_np;
> +   for (i = 0; i < tmp; i++) {
> +   ret = of_property_read_u32(np, "gpios",
> +  &gg_data.gpiospec.args[i]);
> +   if (ret) {
> +   desc = ERR_PTR(ret);
> +   goto out;
> +   }
> +   }
> +
> +   gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);

This seems to work but only supports one GPIO per hog node. It would
be nice to be able to specify several GPIOs to which the same settings
need to be applied.

> +   if (!gg_data.out_gpio) {
> +   if (np->parent == np)
> +   desc = ERR_PTR(-ENXIO);
> +   else
> +   desc = ERR_PTR(-EPROBE_DEFER);
> +   goto out;
> +   }
> +
> +   if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> +   *lflags |= GPIOF_ACTIVE_LOW;
> +
> +   if (of_property_read_bool(np, "input")) {
> +   *dflags |= GPIOD_IN;
> +   in = 1;
> +   }
> +   if (of_property_read_bool(np, "output-low")) {
> +   *dflags |= GPIOD_OUT_LOW;
> +   outlo = 1;
> +   }
> +   if (of_property_read_bool(np, "output-high")) {
> +   *dflags |= GPIOD_OUT_HIGH;
> +   outhi = 1;
> +   }

I thought we agreed that this should be a "directio

Re: [PATCH 5/9] gpio: Add Fujitsu MB86S7x GPIO driver

[Alexandre Courbot]

On Thu, Nov 20, 2014 at 9:37 PM, Vincent Yang
 wrote:
> Driver for Fujitsu MB86S7x SoCs that have a memory mapped GPIO controller.
>
> Signed-off-by: Andy Green 
> Signed-off-by: Jassi Brar 
> Signed-off-by: Vincent Yang 
> Signed-off-by: Tetsuya Nuriya 
> ---
>  .../bindings/gpio/fujitsu,mb86s70-gpio.txt |  18 ++
>  drivers/gpio/Kconfig   |   6 +
>  drivers/gpio/Makefile  |   1 +
>  drivers/gpio/gpio-mb86s7x.c| 211 
> +
>  4 files changed, 236 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/gpio/fujitsu,mb86s70-gpio.txt
>  create mode 100644 drivers/gpio/gpio-mb86s7x.c
>
> diff --git a/Documentation/devicetree/bindings/gpio/fujitsu,mb86s70-gpio.txt 
> b/Documentation/devicetree/bindings/gpio/fujitsu,mb86s70-gpio.txt
> new file mode 100644
> index 000..38300ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/fujitsu,mb86s70-gpio.txt
> @@ -0,0 +1,18 @@
> +Fujitsu MB86S7x GPIO Controller
> +---
> +
> +Required properties:
> +- compatible: Should be "fujitsu,mb86s70-gpio"
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be <2>. The first cell is the pin number and the
> +  second cell is used to specify optional parameters:
> +   - bit 0 specifies polarity (0 for normal, 1 for inverted).

According to the example below and the code, "reg" and "clocks" are
also required properties.

> +
> +Examples:
> +   gpio0: gpio@3100 {
> +   compatible = "fujitsu,mb86s70-gpio";
> +   reg = <0 0x3100 0x1>;
> +   gpio-controller;
> +   #gpio-cells = <2>;
> +   clocks = <&clk_alw_2_1>;

Maybe add a "clocks-names" property so the clock can be given a meaningful name?

> +   };
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 0959ca9..493b7fe 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -181,6 +181,12 @@ config GPIO_F7188X
>   To compile this driver as a module, choose M here: the module will
>   be called f7188x-gpio.
>
> +config GPIO_MB86S7X
> +   bool "GPIO support for Fujitsu MB86S7x Platforms"
> +   depends on ARCH_MB86S7X
> +   help
> + Say yes here to support the GPIO controller in LSI ZEVIO SoCs.
> +
>  config GPIO_MOXART
> bool "MOXART GPIO support"
> depends on ARCH_MOXART
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index e5d346c..eec0664 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_GPIO_MAX730X)+= gpio-max730x.o
>  obj-$(CONFIG_GPIO_MAX7300) += gpio-max7300.o
>  obj-$(CONFIG_GPIO_MAX7301) += gpio-max7301.o
>  obj-$(CONFIG_GPIO_MAX732X) += gpio-max732x.o
> +obj-$(CONFIG_GPIO_MB86S7X) += gpio-mb86s7x.o
>  obj-$(CONFIG_GPIO_MC33880) += gpio-mc33880.o
>  obj-$(CONFIG_GPIO_MC9S08DZ60)  += gpio-mc9s08dz60.o
>  obj-$(CONFIG_GPIO_MCP23S08)+= gpio-mcp23s08.o
> diff --git a/drivers/gpio/gpio-mb86s7x.c b/drivers/gpio/gpio-mb86s7x.c
> new file mode 100644
> index 000..f0b2dc0
> --- /dev/null
> +++ b/drivers/gpio/gpio-mb86s7x.c
> @@ -0,0 +1,211 @@
> +/*
> + *  linux/drivers/gpio/gpio-mb86s7x.c
> + *
> + *  Copyright (C) 2014 Fujitsu Semiconductor Limited
> + *  Copyright (C) 2014 Linaro Ltd.
> + *
> + *  This program is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PDR(x) (0x0 + x)
> +#define DDR(x) (0x10 + x)
> +#define PFR(x) (0x20 + x)

Everytime these macros are used in the code, they are called in the
form PFR(offset / 8 * 4). How about doing this operation in the macros
instead of repeating it at every call?

> +
> +struct mb86s70_gpio_chip {
> +   spinlock_t lock;
> +   struct clk *clk;
> +   void __iomem *base;
> +   struct gpio_chip gc;
> +};
> +
> +static int mb86s70_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> +   struct mb86s70_gpio_chip *gchip = container_of(gc,
> +   struct mb86s70_gpio_chip, gc);

Please have a chip_to_mb86s70() macro to avoid that cumbersome call to
container_of in every function. Also I suggest you move the gpio_chip
to the top of your mb86s70_gpio_chip structure so the container_of
translates to a simple recast without any arithmetic.

> +   uns

Re: [PATCH v2 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor

[Alexandre Courbot]

On Mon, Nov 24, 2014 at 9:28 PM, Tomeu Vizoso
 wrote:
> The ACTMON block can monitor several counters, providing averaging and firing
> interrupts based on watermarking configuration. This implementation monitors
> the MCALL and MCCPU counters to choose an appropriate frequency for the
> external memory clock.
>
> This patch is based on work by Alex Frid  and Mikko
> Perttunen .

Thanks for taking the time to adapt this driver to use devfreq.
Looking at it, I am more and more convinced that's the correct way to
do.

I made some comments inline, but I'd like to bring Arto Merilainen
into this discussion. Arto thought (and might even have some code)
about adding watermarks support to devfreq's core and using a generic
"watermark" governor which I believe would greatly benefit this patch
set. Arto, do you have some concrete code you could submit here? If
you lack the time for doing so, some guidance so we could implement
this support ourselves would be great.

>
> Signed-off-by: Tomeu Vizoso 
>
> ---
>
> v2: * Use devfreq
> ---
>  drivers/devfreq/Kconfig |  10 +
>  drivers/devfreq/Makefile|   1 +
>  drivers/devfreq/tegra-devfreq.c | 718 
> 
>  3 files changed, 729 insertions(+)
>  create mode 100644 drivers/devfreq/tegra-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index faf4e70..4aab799 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -87,4 +87,14 @@ config ARM_EXYNOS5_BUS_DEVFREQ
>   It reads PPMU counters of memory controllers and adjusts the
>   operating frequencies and voltages with OPP support.
>
> +config ARM_TEGRA_DEVFREQ
> +   tristate "Tegra DEVFREQ Driver"
> +   depends on ARCH_TEGRA_124_SOC
> +   select DEVFREQ_GOV_SIMPLE_ONDEMAND
> +   select PM_OPP
> +   help
> + This adds the DEVFREQ driver for the Tegra family of SoCs.
> + It reads ACTMON counters of memory controllers and adjusts the
> + operating frequencies and voltages with OPP support.
> +
>  endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 16138c9..0ea991f 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += 
> governor_userspace.o
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)  += exynos/
>  obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)  += exynos/
> +obj-$(CONFIG_ARM_TEGRA_DEVFREQ)+= tegra-devfreq.o
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> new file mode 100644
> index 000..3479096
> --- /dev/null
> +++ b/drivers/devfreq/tegra-devfreq.c

This file should probably be named tegra-actmon-devfreq.c, for nothing
guarantees that we will not have more devfreq devices for Tegra in the
future.

> @@ -0,0 +1,718 @@
> +/*
> + * A devfreq driver for NVIDIA Tegra SoCs
> + *
> + * Copyright (c) 2014 NVIDIA CORPORATION. All rights reserved.
> + * Copyright (C) 2014 Google, Inc
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see .
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "governor.h"
> +
> +#define ACTMON_GLB_STATUS  0x0
> +#define ACTMON_GLB_PERIOD_CTRL 0x4
> +
> +#define ACTMON_DEV_CTRL0x0
> +#define ACTMON_DEV_CTRL_K_VAL_SHIFT10
> +#define ACTMON_DEV_CTRL_ENB_PERIODIC   BIT(18)
> +#define ACTMON_DEV_CTRL_AVG_BELOW_WMARK_EN BIT(20)
> +#define ACTMON_DEV_CTRL_AVG_ABOVE_WMARK_EN BIT(21)
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_NUM_SHIFT  23
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_NUM_SHIFT  26
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN BIT(29)
> +#define ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN BIT(30)
> +#define ACTMON_DEV_CTRL_ENBBIT(31)
> +
> +#define ACTMON_DEV_UPPER_WMARK 0x4
> +#define ACTMON_DEV_LOWER_WMARK 0x8
> +#define ACTMON_DEV_INIT_AVG0xc
> +#define ACTMON_DEV_AVG_UPPER_WMARK 

Re: [PATCHv4 1/1] thermal: of: improve of-thermal sensor registration API

[Alexandre Courbot]

On Tue, Nov 18, 2014 at 11:39 PM, Eduardo Valentin  wrote:
> Different drivers request API extensions in of-thermal. For this reason,
> additional callbacks are required to fit the new drivers needs.
>
> The current API implementation expects the registering sensor driver
> to provide a get_temp and get_trend callbacks as function parameters.
> As the amount of callbacks is growing, this patch changes the existing
> implementation to use a .ops field to hold all the of thermal callbacks
> to sensor drivers.
>
> This patch also changes the existing of-thermal users to fit the new
> API design. No functional change is introduced in this patch.

A good idea even if no ops were to be added!

Reviewed-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] gpio: em: Use dynamic allocation of GPIOs

[Alexandre Courbot]

On Mon, Nov 17, 2014 at 11:30 PM, Geert Uytterhoeven
 wrote:
> Use dynamic allocation of GPIOs instead of looking at the gpio%u alias
> in DT.
> ---
>   - Is this correct? Not having to care about the alias would simplify the
> to-be-written DT binding documentation.
>   - Completely untested.
>
> Signed-off-by: Geert Uytterhoeven 
>
>  drivers/gpio/gpio-em.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-em.c b/drivers/gpio/gpio-em.c
> index 21d34d4d473dcefe..c3434146f605748b 100644
> --- a/drivers/gpio/gpio-em.c
> +++ b/drivers/gpio/gpio-em.c
> @@ -330,12 +330,7 @@ static int em_gio_probe(struct platform_device *pdev)
> goto err0;
> }
>
> -   ret = of_alias_get_id(pdev->dev.of_node, "gpio");
> -   if (ret < 0) {
> -   dev_err(&pdev->dev, "Couldn't get OF id\n");
> -   goto err0;
> -   }
> -   pdata->gpio_base = ret * 32; /* 32 GPIOs per instance */
> +   pdata->gpio_base = -1;

User-space might break because of GPIO renumbering. Why not setting
gpio_base to -1 when the property is not present (instead of
triggering an error), keeping support for the property so existing
boards remain safe, and marking the property as deprecated in the
bindings documentation?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Alexandre Courbot]

On Fri, Nov 14, 2014 at 5:58 PM, Linus Walleij  wrote:
> On Mon, Nov 3, 2014 at 11:56 PM, Rafael J. Wysocki  wrote:
>> On Monday, November 03, 2014 02:22:10 PM Linus Walleij wrote:
>
>>> With that change:
>>> Reviewed-by: Linus Walleij 
>>
>> OK, made the changes and added your Reviewed-by.
>>
>> One semi-related question though.  Alexandre ACKed the patch before, so
>> what did that mean from the GPIO subsystem's perspective?  Was the patch
>> approved, not approved, semi-approved?
>
> It just means we are not always entirely aligned, it's not like
> Alexandre and I have meetings on the side and decide what to
> say, it's just these mails. I do trust him, I wouldn't get mad if it
> had been merged just wanted some polishing when I could get
> it...

In case of conflicting feedback between me and Linus, it goes without
saying that Linus' opinion should prevail. I am trying to help a bit
and take some of the burden off his shoulders, but have neither his
skill nor his experience (which shows when you compare our patch
reviews). So for important stuff it is a good idea to wait until both
of us give a ack - the most likely conflict scenario being that I give
a pass overlooking something that Linus will catch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] of: replace Asahi Kasei Corp venter prefix

[Alexandre Courbot]

On 11/14/2014 10:43 AM, Kuninori Morimoto wrote:

From: Kuninori Morimoto 

Current vendor-prefixes.txt already has
"ak" prefix for Asahi Kasei Corp by
ae8c4209af2cec065fef15d200a42a04130799f7
(of: Add vendor prefix for Asahi Kasei Corp.)

It went through the appropriate review process.
But, almost all Asahi Kasei chip drivers are
using "asahi-kasei" prefix today.
(arch/arm/boot/dts/tegra20-seaboard.dts only is
using "ak,ak8975", but there are instances of
"asahi-kasei,ak8975" in other dts files.
And drivers/iio/magnetometer/ak8975.c
doesn't support "ak,ak8975")
So, we made a mistake there.

In addition, checkpatch.pl reports WARNING
if it is using "asahi-kasei" prerfix in DT file.
(DT compatible string vendor "asahi-kasei" appears un-documented)

Marking it deprecated and warning with checkpatch
is certainly preferable.
So, this patch replace "ak" to "asahi-kasei" in
vendor-prefixes.txt. (and fixup tegra20-seaboard)

OTOH, Asahi Kasei is usually referred to as "AKM",
but this patch doesn't care about it.
Because no DT is using that today.

Signed-off-by: Kuninori Morimoto 
---
  .../devicetree/bindings/vendor-prefixes.txt|2 +-
  arch/arm/boot/dts/tegra20-seaboard.dts |2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 723999d..ddcb4cd 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -9,7 +9,6 @@ ad  Avionic Design GmbH
  adapteva  Adapteva, Inc.
  adi   Analog Devices, Inc.
  aeroflexgaisler   Aeroflex Gaisler AB
-ak Asahi Kasei Corp.
  allwinner Allwinner Technology Co., Ltd.
  altr  Altera Corp.
  amcc  Applied Micro Circuits Corporation (APM, formally AMCC)
@@ -20,6 +19,7 @@ amstaos   AMS-Taos Inc.
  apm   Applied Micro Circuits Corporation (APM)
  arm   ARM Ltd.
  armadeus  ARMadeus Systems SARL
+asahi-kaseiAsahi Kasei Corp.
  atmel Atmel Corporation
  auo   AU Optronics Corporation
  avago Avago Technologies
diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts 
b/arch/arm/boot/dts/tegra20-seaboard.dts
index a1d4bf9..7f5cf80 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -405,7 +405,7 @@
clock-frequency = <40>;

magnetometer@c {
-   compatible = "ak,ak8975";
+   compatible = "asahi-kasei,ak8975";


Mmm. So does this mean this device was never probed because the driver 
did not recognize its compatible property? I cannot find "ak,ak8975" 
anywhere else in the kernel.


If so,

Acked-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On 11/11/2014 03:57 PM, Terje Bergström wrote:

On 11.11.2014 06:29, Alexandre Courbot wrote:

I think (after a quick look at devfreq's source) that you can avoid
polling altogether if you set polling_ms to 0 in your
devfreq_dev_profile instance. Then it is up to you to call
update_devfreq() from your custom governor whenever it sees fit.

ACTMON support seems to overlap between being a governor (which reacts
to ACTMON interrupts and calls update_devfreq() when needed) and part of
a devfreq_dev_profile (get_dev_status() needs to use the actmon
counters). If we keep your current design where the driver simply
controls a clock, you could have the ACTMON driver obtain that clock,
register its governor, create a non-polling devfreq_dev_profile that
controls that clock, and just call devfreq_add_device() with both. Then
we will have the benefit of being able to use ACTMON as well as the
performance and powersave governors on EMC, and switch policies
dynamically.


Another way to use it is that governor is just a governor. It takes in
load values and generates new target frequencies, and doesn't know
anything about HW.

Device profile is the one that enables threshold interrupts and disables
polling. Device profile receives the interrupt, retrieves new load
number from hardware, and calls update_devfreq().

This way we keep all HW specific code in device profile, and there's
potential to use a generic governor instead of writing your own.


I see several obstacles with this approach:

1) update_devfreq() is a governor private function (defined in 
drivers/devfreq/governor.h) and it would need to be called from the 
device profile.


2) devfreq events (start monitoring, stop monitoring, suspend, resume, 
...) are processed by the governor. So it would still need access to the 
actmon registers to honor these requests.


3) simple monitors like performance and powersave set the frequency (max 
or min) once and for all when they are started, and don't need to be 
called again afterwards. But this is probably what will happen if you 
let the devfreq_dev_profile handle the ACTMON registers, since it can 
make no assumption on when the governor expects to be invoked.


It would be good to have the feedback of devfreq's maintainers about 
this (added them). How could an IP capable of monitoring activity 
through counters and firing interrupts when these counters reach a 
certain threshold be integrated nicely into devfreq? The functions seem 
to overlap between the governor (reacting to specific events, in this 
case ACTMON interrupts) and dev_profile (querying current status through 
the counters), and it seems difficult to come with a design where the 
governor and dev_profile are not tightly intertwined.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On 11/07/2014 09:35 PM, Tomeu Vizoso wrote:

On 11/07/2014 10:07 AM, Alexandre Courbot wrote:

On 10/29/2014 11:50 PM, Tomeu Vizoso wrote:

Hello,

these patches implement support for setting the rate of the EMC clock based on
stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
memory accesses (among others).

It depends on the following in-flight patches:

* MC driver: http://thread.gmane.org/gmane.linux.ports.tegra/19623
* EMC driver: http://thread.gmane.org/gmane.linux.ports.arm.kernel/365125
* CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1812962

I have pushed a branch here for testing:


I am not too familiar with DVFS, but after going through this series it
really seems to me that this could use devfreq. In its current form this
driver mixes control and policy and lacks flexibility, preventing e.g.
to switch to a performance or power-saving profile. Could you study the
feasibility of using devfreq for this?


Yeah, I started writing a devfreq driver, but then I looked in more
detail to the downstream driver and realized that most of the
functionality that devfreq provides overlaps with the hw.

The ACTMON can be configured to fire an interrupt when a set of
thresholds are crossed, similar to the simple-ondemand governor but a
bit more sophisticated.


I think (after a quick look at devfreq's source) that you can avoid 
polling altogether if you set polling_ms to 0 in your 
devfreq_dev_profile instance. Then it is up to you to call 
update_devfreq() from your custom governor whenever it sees fit.


ACTMON support seems to overlap between being a governor (which reacts 
to ACTMON interrupts and calls update_devfreq() when needed) and part of 
a devfreq_dev_profile (get_dev_status() needs to use the actmon 
counters). If we keep your current design where the driver simply 
controls a clock, you could have the ACTMON driver obtain that clock, 
register its governor, create a non-polling devfreq_dev_profile that 
controls that clock, and just call devfreq_add_device() with both. Then 
we will have the benefit of being able to use ACTMON as well as the 
performance and powersave governors on EMC, and switch policies dynamically.


Another benefit is that you will have a placeholder in the governor to 
implement suspend/resume for the ACTMON IP, in case this becomes 
necessary in the future.


Do you think that would work? Apart from the polling which doesn't seem 
to be an issue, have you seen any other redundancy between devfreq and 
ACTMON?



The only functionality of the governors that
isn't covered by the ACTMON hw is determining the new frequency after a
threshold has been crossed, but if we want to retain the flexibility of
the downstream solution, we would need to write a new governor anyway.


Yes, and that's fine. Actually if the parameters of the ACTMON governor 
could be fine-tuned via sysfs, that would just be perfect.



I realize that it would be cool to reuse the code in devfreq, but being
able to let the hw sample the counters, calculating averages and
checking if a threshold has been crossed without the cpu having to
intervene gives this SoC quite an edge when compared to its competitors.


AFAICT we can keep that edge while getting the benefit of using a common 
framework. Also I expect quite some resistance against the merge of this 
driver if it is not using devfreq. You probably can tell us better 
whether it is fit or not, but can you examine my points above and let us 
know what you think? After a quick look, it actually looks quite 
exploitable for our use-case.


Cheers,
Alex.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Add support for Tegra Activity Monitor

[Alexandre Courbot]

On 10/29/2014 11:50 PM, Tomeu Vizoso wrote:

Hello,

these patches implement support for setting the rate of the EMC clock based on
stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
memory accesses (among others).

It depends on the following in-flight patches:

* MC driver: http://thread.gmane.org/gmane.linux.ports.tegra/19623
* EMC driver: http://thread.gmane.org/gmane.linux.ports.arm.kernel/365125
* CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1812962

I have pushed a branch here for testing:


I am not too familiar with DVFS, but after going through this series it 
really seems to me that this could use devfreq. In its current form this 
driver mixes control and policy and lacks flexibility, preventing e.g. 
to switch to a performance or power-saving profile. Could you study the 
feasibility of using devfreq for this?


I also wonder if this driver could not be made more flexible generally 
speaking - right now it is hardcoded that you can only control EMC 
frequency with it. I can imagine that we could want to control several 
clocks using the same counter information, and that e.g. a notifier 
block might help with that. But let's keep that for later - whether to 
use devfreq or not seems to be the most important question for now.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 04/13] of: document new emc-timings subnode in nvidia,tegra124-car

[Alexandre Courbot]

On 11/07/2014 12:12 AM, Rob Herring wrote:

On Thu, Nov 6, 2014 at 12:37 AM, Alexandre Courbot  wrote:

On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:


The EMC clock needs some extra information for changing its rate.

Signed-off-by: Tomeu Vizoso 
---
   .../bindings/clock/nvidia,tegra124-car.txt | 46
+-
   1 file changed, 44 insertions(+), 2 deletions(-)

diff --git
a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
index ded5d62..42e0588 100644
--- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
@@ -19,12 +19,35 @@ Required properties :
 In clock consumers, this cell represents the bit number in the CAR's
 array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.

+The node should contain a "emc-timings" subnode for each supported RAM
type (see
+field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
being its
+RAM_CODE.
+
+Required properties for "emc-timings" nodes :
+- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
+  is used for.
+
+Each "emc-timings" node should contain a "timing" subnode for every
supported
+EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
their
+unit address.



This seems to be a quite liberal use of unit addresses (same in the next
patch) - is this allowed by DT?


No, unit address should match a reg property.


Mmm, would you have any suggestion as to how this can be fixed? Right 
now what I can think of is either to replace the "clock-frequency" 
property by "reg" (which would be confusing), or to use a different 
naming scheme, e.g. timing-1275. IIUC the naming is not essential 
for properly parsing these nodes, so maybe the second solution is the 
way to go?





+
+Required properties for "timing" nodes :
+- clock-frequency : Should contain the memory clock rate to which this
timing
+relates.
+- nvidia,parent-clock-frequency : Should contain the rate at which the
current
+parent of the EMC clock should be running at this timing.
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries:
+  - emc-parent : the clock that should be the parent of the EMC clock at
this
+timing.
+
   Example SoC include file:

   / {
-   tegra_car: clock {
+   tegra_car: clock@0,60006000 {


The comma here is wrong. Commas should be used when you have something
like PCI bus:dev:func for addressing.


 compatible = "nvidia,tegra124-car";
-   reg = <0x60006000 0x1000>;
+   reg = <0x0 0x60006000 0x0 0x1000>;


The number of cell's is really irrelevant to the example.


 #clock-cells = <1>;
 #reset-cells = <1>;
 };
@@ -60,4 +83,23 @@ Example board file:
 &tegra_car {
 clocks = <&clk_32k> <&osc>;
 };
+
+   clock@0,60006000 {
+   emc-timings@3 {
+   nvidia,ram-code = <3>;
+
+   timing@1275 {
+   clock-frequency = <1275>;
+   nvidia,parent-clock-frequency =
<40800>;
+   clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
+   clock-names = "emc-parent";


Why do you need both clocks and hardcoded values? clock-frequency is
the desired freq you want to set TEGRA124_CLK_PLL_P to?


That would be nvidia,parent-clock-frequency IIUC, while clock-frequency 
is the resulting EMC clock.




The clocks property really belongs as part of the memory controller
node or a memory device node.


I would tend to agree here. Tomeu, does it make sense to move these 
properties to the EMC driver instead?

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 04/13] of: document new emc-timings subnode in nvidia,tegra124-car

[Alexandre Courbot]

On 11/06/2014 06:11 PM, Tomeu Vizoso wrote:

On 6 November 2014 07:37, Alexandre Courbot  wrote:

On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:


The EMC clock needs some extra information for changing its rate.

Signed-off-by: Tomeu Vizoso 
---
   .../bindings/clock/nvidia,tegra124-car.txt | 46
+-
   1 file changed, 44 insertions(+), 2 deletions(-)

diff --git
a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
index ded5d62..42e0588 100644
--- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
@@ -19,12 +19,35 @@ Required properties :
 In clock consumers, this cell represents the bit number in the CAR's
 array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.

+The node should contain a "emc-timings" subnode for each supported RAM
type (see
+field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
being its
+RAM_CODE.
+
+Required properties for "emc-timings" nodes :
+- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
+  is used for.
+
+Each "emc-timings" node should contain a "timing" subnode for every
supported
+EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
their
+unit address.



This seems to be a quite liberal use of unit addresses (same in the next
patch) - is this allowed by DT?


Well, it's not that different from using the register address. I think
it helps with readability more than an arbitrary index.


+
+Required properties for "timing" nodes :
+- clock-frequency : Should contain the memory clock rate to which this
timing
+relates.
+- nvidia,parent-clock-frequency : Should contain the rate at which the
current
+parent of the EMC clock should be running at this timing.
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries:
+  - emc-parent : the clock that should be the parent of the EMC clock at
this
+timing.
+
   Example SoC include file:

   / {
-   tegra_car: clock {
+   tegra_car: clock@0,60006000 {
 compatible = "nvidia,tegra124-car";
-   reg = <0x60006000 0x1000>;
+   reg = <0x0 0x60006000 0x0 0x1000>;
 #clock-cells = <1>;
 #reset-cells = <1>;
 };
@@ -60,4 +83,23 @@ Example board file:
 &tegra_car {
 clocks = <&clk_32k> <&osc>;
 };
+
+   clock@0,60006000 {
+   emc-timings@3 {
+   nvidia,ram-code = <3>;
+
+   timing@1275 {
+   clock-frequency = <1275>;
+   nvidia,parent-clock-frequency =
<40800>;
+   clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
+   clock-names = "emc-parent";
+   };
+   timing@2040 {
+   clock-frequency = <2040>;
+   nvidia,parent-clock-frequency =
<40800>;
+   clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
+   clock-names = "emc-parent";
+   };
+   };
+   };



At first it seems confusing to see a top-level node without a compatible
property, until you realize it has already been defined before.

In patch 05, you put "Example board file:" above a similar node, which is
enough to lift that ambiguity - could you do the same here?


Oh, actually it was already in the file. I took this idea from here
for patch 05.


My bad, missed it!

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 05/13] of: Document timings subnode of nvidia,tegra-mc

[Alexandre Courbot]

On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:

The MC driver needs some timing-specific information to program the EMEM during
a rate change of the EMC clock.

Signed-off-by: Tomeu Vizoso 
---
  .../memory-controllers/nvidia,tegra-mc.txt | 46 +-
  1 file changed, 44 insertions(+), 2 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt 
b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
index f3db93c..8467b8c 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
@@ -15,9 +15,26 @@ Required properties:
  This device implements an IOMMU that complies with the generic IOMMU binding.
  See ../iommu/iommu.txt for details.

-Example:
-
+The node should contain a "timings" subnode for each supported RAM type (see
+field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address being 
its
+RAM_CODE.

+Required properties for "timings" nodes :
+- nvidia,ram-code : Should contain the value of RAM_CODE this timing set is 
used
+for.
+
+Each "timings" node should contain a "timing" subnode for every supported EMC
+clock rate. The "timing" subnodes should have the clock rate in Hz as their 
unit
+address.


In patch 04, a similar sub-node is named "emc-timings". Shouldn't the 
same name be used here for consistency?


Also, it seems like there is a need for timing nodes in the MC to have 
match timing nodes in the CAR. It would be nice to add that information 
in all related bindings files.



+
+Required properties for "timing" nodes :
+- clock-frequency : Should contain the memory clock rate in Hz.
+- nvidia,emem-configuration : Values to be written to the EMEM register block,
+as specified by the board documentation.


Could you add some more information about which range of registers we 
are talking about, and whether they must all be specified or only part 
of them?



+
+Example SoC include file:
+
+/ {
mc: memory-controller@0,70019000 {
compatible = "nvidia,tegra124-mc";
reg = <0x0 0x70019000 0x0 0x1000>;
@@ -34,3 +51,28 @@ Example:
...
iommus = <&mc TEGRA_SWGROUP_SDMMC1A>;
};
+};
+
+Example board file:
+
+/ {
+   memory-controller@0,70019000 {
+   timings@3 {
+   nvidia,ram-code = <3>;
+
+   timing@1275 {
+   clock-frequency = <1275>;
+
+   nvidia,emem-configuration = <
+   0x40040001 /* MC_EMEM_ARB_CFG */
+   0x800a /* 
MC_EMEM_ARB_OUTSTANDING_REQ */
+   0x0001 /* MC_EMEM_ARB_TIMING_RCD */
+   0x0001 /* MC_EMEM_ARB_TIMING_RP */
+   0x0002 /* MC_EMEM_ARB_TIMING_RC */
+   0x /* MC_EMEM_ARB_TIMING_RAS */
+   0x0002 /* MC_EMEM_ARB_TIMING_FAW */
+   >;


Looking at the actual board files I suppose this example here is 
incomplete. It would be nice to make this explicit, maybe by adding 
"..." on a new line to indicate more registers are expected.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 10/13] memory: tegra: Add EMC (external memory controller) driver

[Alexandre Courbot]

On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:

From: Mikko Perttunen 

Implements functionality needed to change the rate of the memory bus clock.

Signed-off-by: Mikko Perttunen 
Signed-off-by: Tomeu Vizoso 

---

v2: * Use subsys_initcall(), so it gets probed after the MC driver and
  before the CAR driver
---
  drivers/memory/Kconfig  |   10 +
  drivers/memory/Makefile |1 -
  drivers/memory/tegra/Makefile   |1 +
  drivers/memory/tegra/tegra124-emc.c | 1133 +++
  include/soc/tegra/memory.h  |2 +
  5 files changed, 1146 insertions(+), 1 deletion(-)
  create mode 100644 drivers/memory/tegra/tegra124-emc.c

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 6d91c27..3ba3aef 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -51,6 +51,16 @@ config MVEBU_DEVBUS
  Armada 370 and Armada XP. This controller allows to handle flash
  devices such as NOR, NAND, SRAM, and FPGA.

+config TEGRA124_EMC
+   bool "Tegra124 External Memory Controller driver"
+   default y
+   depends on ARCH_TEGRA_124_SOC
+   help
+ This driver is for the External Memory Controller (EMC) found on
+ Tegra124 chips. The EMC controls the external DRAM on the board.
+ This driver is required to change memory timings / clock rate for
+ external memory.
+
  config TEGRA20_MC
bool "Tegra20 Memory Controller(MC) driver"
default y
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 1c932e7..8e5ec8d 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -12,5 +12,4 @@ obj-$(CONFIG_FSL_CORENET_CF)  += fsl-corenet-cf.o
  obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
  obj-$(CONFIG_MVEBU_DEVBUS)+= mvebu-devbus.o
  obj-$(CONFIG_TEGRA20_MC)  += tegra20-mc.o
-
  obj-$(CONFIG_ARCH_TEGRA)  += tegra/
diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
index 51b9e8f..4e84bef 100644
--- a/drivers/memory/tegra/Makefile
+++ b/drivers/memory/tegra/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += tegra30-mc.o
  obj-$(CONFIG_ARCH_TEGRA_114_SOC)  += tegra114-mc.o
  obj-$(CONFIG_ARCH_TEGRA_124_SOC)  += tegra124-mc.o
  obj-$(CONFIG_ARCH_TEGRA_132_SOC)  += tegra124-mc.o
+obj-$(CONFIG_TEGRA124_EMC) += tegra124-emc.o
diff --git a/drivers/memory/tegra/tegra124-emc.c 
b/drivers/memory/tegra/tegra124-emc.c
new file mode 100644
index 000..8438de2
--- /dev/null
+++ b/drivers/memory/tegra/tegra124-emc.c
@@ -0,0 +1,1133 @@
+/*
+ * drivers/memory/tegra124-emc.c
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author:
+ * Mikko Perttunen 
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#define EMC_FBIO_CFG5  0x104
+#defineEMC_FBIO_CFG5_DRAM_TYPE_MASK0x3
+#defineEMC_FBIO_CFG5_DRAM_TYPE_SHIFT   0
+
+#define EMC_INTSTATUS  0x0
+#define EMC_INTSTATUS_CLKCHANGE_COMPLETE   BIT(4)
+
+#define EMC_CFG0xc
+#define EMC_CFG_DRAM_CLKSTOP_PDBIT(31)
+#define EMC_CFG_DRAM_CLKSTOP_SRBIT(30)
+#define EMC_CFG_DRAM_ACPD  BIT(29)
+#define EMC_CFG_DYN_SREF   BIT(28)
+#define EMC_CFG_PWR_MASK   ((0xF << 28) | BIT(18))
+#define EMC_CFG_DSR_VTTGEN_DRV_EN  BIT(18)
+
+#define EMC_REFCTRL0x20
+#define EMC_REFCTRL_DEV_SEL_SHIFT  0
+#define EMC_REFCTRL_ENABLE BIT(31)
+
+#define EMC_TIMING_CONTROL 0x28
+#define EMC_RC 0x2c
+#define EMC_RFC0x30
+#define EMC_RAS0x34
+#define EMC_RP 0x38
+#define EMC_R2W0x3c
+#define EMC_W2R0x40
+#define EMC_R2P0x44
+#define EMC_W2P0x48
+#define EMC_RD_RCD 0x4c
+#define EMC_WR_RCD 0x50
+#define EMC_RRD0x54
+#define EMC_REXT   0x58
+#define 

Re: [PATCH v3 04/13] of: document new emc-timings subnode in nvidia,tegra124-car

[Alexandre Courbot]

On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:

The EMC clock needs some extra information for changing its rate.

Signed-off-by: Tomeu Vizoso 
---
  .../bindings/clock/nvidia,tegra124-car.txt | 46 +-
  1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt 
b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
index ded5d62..42e0588 100644
--- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
@@ -19,12 +19,35 @@ Required properties :
In clock consumers, this cell represents the bit number in the CAR's
array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.

+The node should contain a "emc-timings" subnode for each supported RAM type 
(see
+field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address being 
its
+RAM_CODE.
+
+Required properties for "emc-timings" nodes :
+- nvidia,ram-code : Should contain the value of RAM_CODE this timing set
+  is used for.
+
+Each "emc-timings" node should contain a "timing" subnode for every supported
+EMC clock rate. The "timing" subnodes should have the clock rate in Hz as their
+unit address.


This seems to be a quite liberal use of unit addresses (same in the next 
patch) - is this allowed by DT?



+
+Required properties for "timing" nodes :
+- clock-frequency : Should contain the memory clock rate to which this timing
+relates.
+- nvidia,parent-clock-frequency : Should contain the rate at which the current
+parent of the EMC clock should be running at this timing.
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries:
+  - emc-parent : the clock that should be the parent of the EMC clock at this
+timing.
+
  Example SoC include file:

  / {
-   tegra_car: clock {
+   tegra_car: clock@0,60006000 {
compatible = "nvidia,tegra124-car";
-   reg = <0x60006000 0x1000>;
+   reg = <0x0 0x60006000 0x0 0x1000>;
#clock-cells = <1>;
#reset-cells = <1>;
};
@@ -60,4 +83,23 @@ Example board file:
&tegra_car {
clocks = <&clk_32k> <&osc>;
};
+
+   clock@0,60006000 {
+   emc-timings@3 {
+   nvidia,ram-code = <3>;
+
+   timing@1275 {
+   clock-frequency = <1275>;
+   nvidia,parent-clock-frequency = <40800>;
+   clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
+   clock-names = "emc-parent";
+   };
+   timing@2040 {
+   clock-frequency = <2040>;
+   nvidia,parent-clock-frequency = <40800>;
+   clocks = <&tegra_car TEGRA124_CLK_PLL_P>;
+   clock-names = "emc-parent";
+   };
+   };
+   };


At first it seems confusing to see a top-level node without a compatible 
property, until you realize it has already been defined before.


In patch 05, you put "Example board file:" above a similar node, which 
is enough to lift that ambiguity - could you do the same here?

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Alexandre Courbot]

On Fri, Oct 31, 2014 at 4:51 PM, Krzysztof Kozlowski
 wrote:
> On pią, 2014-10-31 at 12:31 +0900, Alexandre Courbot wrote:
>> On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
>>  wrote:
>> > On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
>> >> Hi, and thanks for bringing this issue to us!
>> >>
>> >> On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
>> >>  wrote:
>> >> > [adding Linus and Alexandre to the cc list]
>> >> >
>> >> > Hello Krzysztof,
>> >> >
>> >> > On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
>> >> >> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
>> >> >>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
>> >> >>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
>> >> >>> > > Hello Krzysztof,
>> >> >>> > >
>> >> >>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
>> >> >>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
>> >> >>> > > >struct max77686_regulator_data *regulators;
>> >> >>> > > >int num_regulators;
>> >> >>> > > >
>> >> >>> > > > +  /* Array of size num_regulators with GPIOs for external 
>> >> >>> > > > control. */
>> >> >>> > > > +  int *ext_control_gpio;
>> >> >>> > > > +
>> >> >>> > >
>> >> >>> > > The integer-based GPIO API is deprecated in favor of the 
>> >> >>> > > descriptor-based GPIO
>> >> >>> > > interface (Documentation/gpio/consumer.txt). Could you please use 
>> >> >>> > > the later?
>> >> >>> >
>> >> >>> > Sure, I can. Please have in mind that regulator core still accepts 
>> >> >>> > old
>> >> >>> > GPIO so I will have to use desc_to_gpio(). That should work... and
>> >> >>> > should be future-ready.
>> >> >>>
>> >> >>> It seems I was too hasty... I think usage of the new gpiod API implies
>> >> >>> completely different bindings.
>> >> >>>
>> >> >>> The gpiod_get() gets GPIO from a device level, not from given sub-node
>> >> >>> pointer. This means that you cannot have DTS like this:
>> >> >>> ldo21_reg: ldo21 {
>> >> >>>  regulator-compatible = "LDO21";
>> >> >>>  regulator-name = "VTF_2.8V";
>> >> >>>  regulator-min-microvolt = <280>;
>> >> >>>  regulator-max-microvolt = <280>;
>> >> >>>  ec-gpio = <&gpy2 0 0>;
>> >> >>> };
>> >> >>>
>> >> >>> ldo22_reg: ldo22 {
>> >> >>>  regulator-compatible = "LDO22";
>> >> >>>  regulator-name = "VMEM_VDD_2.8V";
>> >> >>>  regulator-min-microvolt = <280>;
>> >> >>>  regulator-max-microvolt = <280>;
>> >> >>>  ec-gpio = <&gpk0 2 0>;
>> >> >>> };
>> >> >>>
>> >> >>>
>> >> >>> I could put GPIOs in device node:
>> >> >>>
>> >> >>> max77686_pmic@09 {
>> >> >>>  compatible = "maxim,max77686";
>> >> >>>  interrupt-parent = <&gpx0>;
>> >> >>>  interrupts = <7 0>;
>> >> >>>  reg = <0x09>;
>> >> >>>  #clock-cells = <1>;
>> >> >>>  ldo21-gpio = <&gpy2 0 0>;
>> >> >>>  ldo22-gpio = <&gpk0 2 0>;
>> >> >>>
>> >> >>>  ldo21_reg: ldo21 {
>> >> >>>  regulator-compatible = "LDO21";
>> >> >>>  regulator-name = "VTF_2.8V";
>> >> >>>  regulator-min-microvolt = <280>;
>> >> >>>  regulator-max-microvolt = <280>;
>> >> >>>   

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Alexandre Courbot]

On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
 wrote:
> On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
>> Hi, and thanks for bringing this issue to us!
>>
>> On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
>>  wrote:
>> > [adding Linus and Alexandre to the cc list]
>> >
>> > Hello Krzysztof,
>> >
>> > On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
>> >> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
>> >>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
>> >>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
>> >>> > > Hello Krzysztof,
>> >>> > >
>> >>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
>> >>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
>> >>> > > >struct max77686_regulator_data *regulators;
>> >>> > > >int num_regulators;
>> >>> > > >
>> >>> > > > +  /* Array of size num_regulators with GPIOs for external 
>> >>> > > > control. */
>> >>> > > > +  int *ext_control_gpio;
>> >>> > > > +
>> >>> > >
>> >>> > > The integer-based GPIO API is deprecated in favor of the 
>> >>> > > descriptor-based GPIO
>> >>> > > interface (Documentation/gpio/consumer.txt). Could you please use 
>> >>> > > the later?
>> >>> >
>> >>> > Sure, I can. Please have in mind that regulator core still accepts old
>> >>> > GPIO so I will have to use desc_to_gpio(). That should work... and
>> >>> > should be future-ready.
>> >>>
>> >>> It seems I was too hasty... I think usage of the new gpiod API implies
>> >>> completely different bindings.
>> >>>
>> >>> The gpiod_get() gets GPIO from a device level, not from given sub-node
>> >>> pointer. This means that you cannot have DTS like this:
>> >>> ldo21_reg: ldo21 {
>> >>>  regulator-compatible = "LDO21";
>> >>>  regulator-name = "VTF_2.8V";
>> >>>  regulator-min-microvolt = <280>;
>> >>>  regulator-max-microvolt = <280>;
>> >>>  ec-gpio = <&gpy2 0 0>;
>> >>> };
>> >>>
>> >>> ldo22_reg: ldo22 {
>> >>>  regulator-compatible = "LDO22";
>> >>>  regulator-name = "VMEM_VDD_2.8V";
>> >>>  regulator-min-microvolt = <280>;
>> >>>  regulator-max-microvolt = <280>;
>> >>>  ec-gpio = <&gpk0 2 0>;
>> >>> };
>> >>>
>> >>>
>> >>> I could put GPIOs in device node:
>> >>>
>> >>> max77686_pmic@09 {
>> >>>  compatible = "maxim,max77686";
>> >>>  interrupt-parent = <&gpx0>;
>> >>>  interrupts = <7 0>;
>> >>>  reg = <0x09>;
>> >>>  #clock-cells = <1>;
>> >>>  ldo21-gpio = <&gpy2 0 0>;
>> >>>  ldo22-gpio = <&gpk0 2 0>;
>> >>>
>> >>>  ldo21_reg: ldo21 {
>> >>>  regulator-compatible = "LDO21";
>> >>>  regulator-name = "VTF_2.8V";
>> >>>  regulator-min-microvolt = <280>;
>> >>>  regulator-max-microvolt = <280>;
>> >>>  };
>> >>>
>> >>>  ldo22_reg: ldo22 {
>> >>>  regulator-compatible = "LDO22";
>> >>>  regulator-name = "VMEM_VDD_2.8V";
>> >>>  regulator-min-microvolt = <280>;
>> >>>  regulator-max-microvolt = <280>;
>> >>>  };
>> >>>
>> >>> This would work but I don't like it. The properties of a regulator are
>> >>> above the node configuring that regulator.
>> >>>
>> >>> Any ideas?
>> >>>
>> >>
>> >> Continuing talking to myself... I found another problem - GPIO cannot be
>> >> requested more than once (-EBUSY). In c

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Alexandre Courbot]

Hi, and thanks for bringing this issue to us!

On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
 wrote:
> [adding Linus and Alexandre to the cc list]
>
> Hello Krzysztof,
>
> On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
>> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
>>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
>>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
>>> > > Hello Krzysztof,
>>> > >
>>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
>>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
>>> > > >struct max77686_regulator_data *regulators;
>>> > > >int num_regulators;
>>> > > >
>>> > > > +  /* Array of size num_regulators with GPIOs for external 
>>> > > > control. */
>>> > > > +  int *ext_control_gpio;
>>> > > > +
>>> > >
>>> > > The integer-based GPIO API is deprecated in favor of the 
>>> > > descriptor-based GPIO
>>> > > interface (Documentation/gpio/consumer.txt). Could you please use the 
>>> > > later?
>>> >
>>> > Sure, I can. Please have in mind that regulator core still accepts old
>>> > GPIO so I will have to use desc_to_gpio(). That should work... and
>>> > should be future-ready.
>>>
>>> It seems I was too hasty... I think usage of the new gpiod API implies
>>> completely different bindings.
>>>
>>> The gpiod_get() gets GPIO from a device level, not from given sub-node
>>> pointer. This means that you cannot have DTS like this:
>>> ldo21_reg: ldo21 {
>>>  regulator-compatible = "LDO21";
>>>  regulator-name = "VTF_2.8V";
>>>  regulator-min-microvolt = <280>;
>>>  regulator-max-microvolt = <280>;
>>>  ec-gpio = <&gpy2 0 0>;
>>> };
>>>
>>> ldo22_reg: ldo22 {
>>>  regulator-compatible = "LDO22";
>>>  regulator-name = "VMEM_VDD_2.8V";
>>>  regulator-min-microvolt = <280>;
>>>  regulator-max-microvolt = <280>;
>>>  ec-gpio = <&gpk0 2 0>;
>>> };
>>>
>>>
>>> I could put GPIOs in device node:
>>>
>>> max77686_pmic@09 {
>>>  compatible = "maxim,max77686";
>>>  interrupt-parent = <&gpx0>;
>>>  interrupts = <7 0>;
>>>  reg = <0x09>;
>>>  #clock-cells = <1>;
>>>  ldo21-gpio = <&gpy2 0 0>;
>>>  ldo22-gpio = <&gpk0 2 0>;
>>>
>>>  ldo21_reg: ldo21 {
>>>  regulator-compatible = "LDO21";
>>>  regulator-name = "VTF_2.8V";
>>>  regulator-min-microvolt = <280>;
>>>  regulator-max-microvolt = <280>;
>>>  };
>>>
>>>  ldo22_reg: ldo22 {
>>>  regulator-compatible = "LDO22";
>>>  regulator-name = "VMEM_VDD_2.8V";
>>>  regulator-min-microvolt = <280>;
>>>  regulator-max-microvolt = <280>;
>>>  };
>>>
>>> This would work but I don't like it. The properties of a regulator are
>>> above the node configuring that regulator.
>>>
>>> Any ideas?
>>>
>>
>> Continuing talking to myself... I found another problem - GPIO cannot be
>> requested more than once (-EBUSY). In case of this driver (and board:
>> Trats2) one GPIO is connected to regulators. The legacy GPIO API and
>> regulator core handle this.
>>
>> With new GPIO API I would have to implement some additional steps in
>> such case...
>>
>> So there are 2 issues:
>> 1. Cannot put GPIO property in regulator node.

For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:

https://lkml.org/lkml/2014/10/6/529

It should reach -next soon now.

>> 2. Cannot request some GPIO more than once.

We have been confronted to this problem with the regulator core as well:

http://marc.info/?l=linux-arm-kernel&m=140417649119733&w=1

I have a draft patch that allows GPIOs to be requested by several
clients. What prevented me from submitting it was that I wanted to
make sure the different requested configurations were compatible, but
maybe I am overthinking this. There are also a couple of other patches
that this depends on (like removal of the big descs array), so I don't
think it will be available too soon, sadly.

So maybe your best shot for now is to keep using the integer API, as
much as I hate it. Once we become able to request the same GPIO
several times, you should be good to switch to descriptors. Sorry this
has not been done faster.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Oct 30, 2014 at 1:42 AM, Pantelis Antoniou
 wrote:
> Hi Benoit,
>
>> On Oct 29, 2014, at 18:34 , Benoit Parrot  wrote:
>>
>> Pantelis,
>>
>> Thanks for the feedback.
>>
>> Pantelis Antoniou  wrote on Wed [2014-Oct-29 
>> 10:53:44 +0200]:
>>> Hi Benoit,
>>>
 On Oct 21, 2014, at 23:09 , Benoit Parrot  wrote:

 Based on Boris Brezillion work this is a reworked patch
 of his initial GPIO hogging mechanism.
 This patch provides a way to initally configure specific GPIO
 when the gpio controller is probe.

 The actual DT scanning to collect the GPIO specific data is performed
 as part of the gpiochip_add().

 The purpose of this is to allows specific GPIOs to be configured
 without any driver specific code.
 This particularly usueful because board design are getting
 increassingly complex and given SoC pins can now have upward
 of 10 mux values a lot of connections are now dependent on
 external IO muxes to switch various modes and combination.

 Specific drivers should not necessarily need to be aware of
 what accounts to a specific board implementation. This board level
 "description" should be best kept as part of the dts file.

>>>
>>> This look like it’s going to the right direction. I have a few general
>>> comments at first.
>>>
>>> 1) It relies on dubious DT binding of having sub-nodes of the
>>> gpio device implicitly defining hogs.
>>
>> I think in this instance the nodes are explicitly defining hogs.
>> Please clarify. What would you like to see here?
>>>
>
> Any subnodes are implicitly taken as hog definitions. This is not right 
> because
> gpio controllers might have subnodes that they use for another purpose.
>
>>> 2) There is no way for having hogs inserted dynamically as far as I can 
>>> tell, and
>>> no way to remove a hog either.
>>
>> The original patch was allowing that but, Linus's review comment suggested 
>> this feature be
>> part of the gpio-controller's gpiochip_add() hook only.
>>
>
> If it’s not possible to remove a hog, then it’s no good for my use case in 
> which
> the gpios get exported and then removed.

Why would you want to remove a GPIO hog at runtime, and what is the
point of having it set in the DT in that case?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

On Thu, Oct 30, 2014 at 1:21 AM, Benoit Parrot  wrote:
>> > +
>> > if (status)
>> > goto fail;
>> >
>> > @@ -1742,6 +1757,72 @@ struct gpio_desc *__must_check 
>> > __gpiod_get_index_optional(struct device *dev,
>> >  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>> >
>> >  /**
>> > + * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog
>> > + * @dev:   GPIO consumer
>> > + * @idx:   index of the GPIO to obtain
>> > + *
>> > + * This should only be used by the gpiochip_add to request/set GPIO 
>> > initial
>> > + * configuration.
>> > + *
>> > + * Return a valid GPIO descriptor, or an IS_ERR() condition in case of 
>> > error.
>> > + */
>> > +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
>> > +  unsigned int idx)
>> > +{
>> > +   struct gpio_desc *desc = NULL;
>> > +   int err;
>> > +   unsigned long flags;
>> > +   const char *name;
>> > +
>> > +   /* Using device tree? */
>> > +   if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
>> > +   desc = of_get_gpio_hog(dev->of_node, idx, &name, &flags);
>> > +
>> > +   if (!desc)
>> > +   return ERR_PTR(-ENOTSUPP);
>> > +   else if (IS_ERR(desc))
>> > +   return desc;
>> > +
>> > +   dev_dbg(dev, "gpio-hog: GPIO:%d (%s) as %s%s\n",
>> > +   desc_to_gpio(desc), name, 
>> > (flags&GPIOF_DIR_IN)?"input":"output",
>> > +   
>> > (flags&GPIOF_DIR_IN)?"":(flags&GPIOF_INIT_HIGH)?"/high":"/low");
>> > +
>>
>> ...
>
> I guess this means to remove the dev_dbg code?

No, it was just to delimitate the code which I suggested to factorize
into its own function below. :) This dev_dbg  is fine IMHO.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] Documentation: gpio: guidelines for bindings

[Alexandre Courbot]

Now that ACPI supports named GPIO properties, either through ACPI 5.1 or
the per-driver ACPI GPIO mappings, we can be more narrow about the way
GPIOs should be specified in Device Tree bindings.

This patch updates the GPIO DT bindings documentation to highlight the
following rules for new GPIO bindings:

- All new bindings must have a meaningful name (e.g. the "gpios"
  property must not be used)
- The only suffix allowed is "-gpios", no matter the number of
  descriptors in the property
- GPIOs can only be grouped under the same property when they serve the
  same purpose, a case that should remain exceptional (e.g. bit-banged
  data lines).

Signed-off-by: Alexandre Courbot 
CC: Linus Walleij 
CC: Arnd Bergmann 
CC: Rafael J. Wysocki 
CC: Mika Westerberg 
---
Changes since v1:
- Rewritten in OS-neutral manner.

 Documentation/devicetree/bindings/gpio/gpio.txt | 40 -
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
b/Documentation/devicetree/bindings/gpio/gpio.txt
index 3fb8f53071b8..b9bd1d64cfa6 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -13,13 +13,22 @@ properties, each containing a 'gpio-list':
gpio-specifier : Array of #gpio-cells specifying specific gpio
 (controller specific)
 
-GPIO properties should be named "[-]gpios". The exact
-meaning of each gpios property must be documented in the device tree
-binding for each device.
+GPIO properties should be named "[-]gpios", with  being the purpose
+of this GPIO for the device. While a non-existent  is considered valid
+for compatibility reasons (resolving to the "gpios" property), it is not 
allowed
+for new bindings.
 
-For example, the following could be used to describe GPIO pins used
-as chip select lines; with chip selects 0, 1 and 3 populated, and chip
-select 2 left empty:
+GPIO properties can contain one or more GPIO phandles, but only in exceptional
+cases should they contain more than one. If your device uses several GPIOs with
+distinct functions, reference each of them under its own property, giving it a
+meaningful name. The only case where an array of GPIOs is accepted is when
+several GPIOs serve the same function (e.g. a parallel data line).
+
+The exact purpose of each gpios property must be documented in the device tree
+binding of the device.
+
+The following example could be used to describe GPIO pins used as device enable
+and bit-banged data signals:
 
gpio1: gpio1 {
gpio-controller
@@ -30,10 +39,12 @@ select 2 left empty:
 #gpio-cells = <1>;
};
[...]
-chipsel-gpios = <&gpio1 12 0>,
-<&gpio1 13 0>,
-<0>, /* holes are permitted, means no GPIO 2 */
-<&gpio2 2>;
+
+   enable-gpios = <&gpio2 2>;
+   data-gpios = <&gpio1 12 0>,
+<&gpio1 13 0>,
+<&gpio1 14 0>,
+<&gpio1 15 0>;
 
 Note that gpio-specifier length is controller dependent.  In the
 above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
@@ -42,16 +53,17 @@ only uses one.
 gpio-specifier may encode: bank, pin position inside the bank,
 whether pin is open-drain and whether pin is logically inverted.
 Exact meaning of each specifier cell is controller specific, and must
-be documented in the device tree binding for the device.
+be documented in the device tree binding for the device. Use the macros
+defined in include/dt-bindings/gpio/gpio.h whenever possible:
 
 Example of a node using GPIOs:
 
node {
-   gpios = <&qe_pio_e 18 0>;
+   enable-gpios = <&qe_pio_e 18 GPIO_ACTIVE_HIGH>;
};
 
-In this example gpio-specifier is "18 0" and encodes GPIO pin number,
-and GPIO flags as accepted by the "qe_pio_e" gpio-controller.
+GPIO_ACTIVE_HIGH is 0, so in this example gpio-specifier is "18 0" and encodes
+GPIO pin number, and GPIO flags as accepted by the "qe_pio_e" gpio-controller.
 
 1.1) GPIO specifier best practices
 --
-- 
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC Patch] gpio: add GPIO hogging mechanism

[Alexandre Courbot]

Sorry for the delay in reviewing. Adding Jiri and Pantelis who might
want to extend over this patch and thus will likely have interesting
comments to make.

On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot  wrote:
> Based on Boris Brezillion work this is a reworked patch
> of his initial GPIO hogging mechanism.
> This patch provides a way to initally configure specific GPIO
> when the gpio controller is probe.

Typo nit: s/probe/probed

>
> The actual DT scanning to collect the GPIO specific data is performed
> as part of the gpiochip_add().
>
> The purpose of this is to allows specific GPIOs to be configured
> without any driver specific code.
> This particularly usueful because board design are getting

s/suseful/useful

> increassingly complex and given SoC pins can now have upward

s/increassingly/increasingly

> of 10 mux values a lot of connections are now dependent on
> external IO muxes to switch various modes and combination.
>
> Specific drivers should not necessarily need to be aware of
> what accounts to a specific board implementation. This board level
> "description" should be best kept as part of the dts file.
>
> Signed-off-by: Benoit Parrot 
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt | 33 +
>  drivers/gpio/gpiolib-of.c   | 99 
> +
>  drivers/gpio/gpiolib.c  | 81 
>  include/linux/of_gpio.h | 11 +++
>  4 files changed, 224 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
> b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 3fb8f53..a9700d8 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt

Note: changes to DT bindings documentation should generally come as a
separate patch.

> @@ -103,6 +103,26 @@ Every GPIO controller node must contain both an empty 
> "gpio-controller"
>  property, and a #gpio-cells integer property, which indicates the number of
>  cells in a gpio-specifier.
>
> +It might contain GPIO hog definitions. GPIO hogging is a mechanism providing
> +automatic GPIO request and configuration as part of the gpio-controller's
> +driver probe function.
> +
> +Each GPIO hog definition is represented as a child node of the GPIO 
> controller.
> +Required properties:
> +- gpios: store the gpio information (id, flags, ...). Shall contain the
> +  number of cells specified in its parent node (GPIO controller node).

If would be nice to be able to specify several GPIO references to that
they can be all set to the same configuration in one row instead of
requiring one node each.

> +- input: a property specifying to set the GPIO direction as input.
> +- output-high: a property specifying to set the GPIO direction to output with
> +  the value high.
> +- output-low: a property specifying to set the GPIO direction to output with
> +  the value low.

Wouldn't a "direction" property taking one of the values "input",
"output-low" or "output-high" be easier to parse and generally
clearer?

> +
> +Optional properties:
> +- line-name: the GPIO label name. If not present the node name is used.

I guess we also could use this for naming the GPIO during its export
if we decide to allow this in a near-future patch.

> +
> +A GPIO controller can request GPIO hogs using the "gpio-hogs" property, which
> +encodes a list of requested GPIO hogs.
> +
>  Example of two SOC GPIO banks defined as gpio-controller nodes:
>
> qe_pio_a: gpio-controller@1400 {
> @@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller 
> nodes:
> reg = <0x1400 0x18>;
> gpio-controller;
> #gpio-cells = <2>;
> +   gpio-hogs = <&line_b>;
> +
> +   /* line_a hog is defined but not enabled in this example*/
> +   line_a: line_a {
> +   gpios = <5 0>;
> +   input;
> +   };
> +
> +   line_b: line_b {
> +   gpios = <6 0>;
> +   output-low;
> +   line-name = "foo-bar-gpio";
> +   };

I think it might be good to group the hog nodes such as to allow
complex configurations to be set in one go, à la pinmux:

gpio-controller;
#gpio-cells = <2>;
+   gpio-hogs = <&line_b>;
+
+   line_a: line_a {
+  line_a {
+   gpios = <5 0>;
+   input;
+  };
+  line_c {
+   gpios = <7 0>;
+   inputl
+  };
+   };
+
+   line_b: line_b {
+   line_b {
+   gpios = <6 0>;
+   output-low;
+   line-name = "foo-bar-gpio";
+  

Re: [PATCH RESEND V4 0/9] Tegra xHCI support

[Alexandre Courbot]

On Wed, Oct 29, 2014 at 7:27 AM, Andrew Bresticker
 wrote:
> This series adds support for xHCI on NVIDIA Tegra SoCs.  This includes:
>  - patches 1 and 2: adding a driver for the mailbox used to communicate
>with the xHCI controller's firmware,
>  - patches 3 and 4: extending the XUSB pad controller driver to support
>the USB PHY types (UTMI, HSIC, and USB3),
>  - patches 5 and 6: adding a xHCI host-controller driver, and
>  - patches 7, 8, and 9: updating the relevant DT files.
>
> The PHY and host drivers have compile-time dependencies on the mailbox
> driver, and the host driver has compile-time dependencies on the PHY
> driver.  It is probably best if these all get merged through the Tegra
> tree.  This series still needs ACKs from the relevant maintainers for
> the mailbox and xHCI host drivers and their device-tree bindings.
>
> Tested on Venice2, Jetson TK1, and Big with a variety of USB2.0 and
> USB3.0 memory sticks and ethernet dongles using controller firmware
> recently posted by Andrew Chew [0].
>
> Based on v3.18-rc2.  All other dependencies (xHCI modules and mailbox
> series) have been merged.

I do not feel comfortable enough with xHCI to give a reviewed-by tag,
but FWIW I have found nothing wrong with these patches.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: gpio: guidelines for bindings

[Alexandre Courbot]

On Tue, Oct 28, 2014 at 6:08 PM, Arnd Bergmann  wrote:
> On Tuesday 28 October 2014 13:20:34 Alexandre Courbot wrote:
>> +GPIO properties should be named "[-]gpios", with  being the 
>> con_id
>> +argument that is passed to gpiod_get(). While a NULL con_id is accepted by 
>> the
>> +GPIO API for compatibility reasons (resolving to the "gpios" property), it 
>> is
>> +not allowed for new bindings.
>> +
>> +GPIO properties can contain one or more GPIO phandles, but only in 
>> exceptional
>> +cases should they contain more than one. If your device uses several GPIOs 
>> with
>> +distinct functions, reference each of them under its own property, giving 
>> it a
>> +meaningful name. The only case where an array of GPIOs is accepted is when
>> +several GPIOs serve the same function (e.g. a parallel data line). In that 
>> case
>> +individual GPIOs can be retrieved using gpiod_get_index().
>> +
>> +The exact meaning of each gpios property must be documented in the device 
>> tree
>>  binding for each device.
>
> The binding should be written in an OS neutral way, so it would be better to 
> avoid
> direct references to Linux APIs in the part that specifies the allowed 
> properties.
>
> Could you reword this so the Linux interfaces are described in an 
> "implementation
> notes" section that is separate from the main part?

You're right - will fix this and resubmit. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/10] ARM: dts: tegra: Use standard power-off property in tps65911 for tegra30 apalis

[Alexandre Courbot]

On Tue, Oct 28, 2014 at 1:42 AM, Felipe Balbi  wrote:
> On Mon, Oct 27, 2014 at 04:26:52PM +, Romain Perier wrote:
>> Signed-off-by: Romain Perier 
>> ---
>>  arch/arm/boot/dts/tegra30-apalis.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi 
>> b/arch/arm/boot/dts/tegra30-apalis.dtsi
>> index a5446cb..ced4436 100644
>> --- a/arch/arm/boot/dts/tegra30-apalis.dtsi
>> +++ b/arch/arm/boot/dts/tegra30-apalis.dtsi
>> @@ -412,7 +412,7 @@
>>   #interrupt-cells = <2>;
>>   interrupt-controller;
>>
>> - ti,system-power-controller;
>> + system-power-controller;
>
> this board is broken until this patch is applied.

It should not. Supporting a new property in the tps65911 is fine, but
the old property must keep being supported to preserve DT
compatibility. If a change removing support for
"ti,system-power-controller" has landed in -next, then it should be
removed.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Documentation: gpio: guidelines for bindings

[Alexandre Courbot]

Now that ACPI supports named GPIO properties, either through ACPI 5.1 or
the per-driver ACPI GPIO mappings, we can be more narrow about the way
GPIOs should be specified in Device Tree bindings.

This patch updates the GPIO DT bindings documentation to highlight the
following rules for new GPIO bindings:

- All new bindings must have a meaningful name (e.g. the "gpios"
  property must not be used)
- The only suffix allowed is "-gpios", no matter the number of
  descriptors in the property
- GPIOs can only be grouped under the same property when they serve the
  same purpose, a case that should remain exceptional (e.g. bit-banged
  data lines).

Signed-off-by: Alexandre Courbot 
CC: Linus Walleij 
CC: Arnd Bergmann 
CC: Rafael J. Wysocki 
CC: Mika Westerberg 
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 41 +
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt 
b/Documentation/devicetree/bindings/gpio/gpio.txt
index 3fb8f53071b8..1043f8b47433 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -13,13 +13,23 @@ properties, each containing a 'gpio-list':
gpio-specifier : Array of #gpio-cells specifying specific gpio
 (controller specific)
 
-GPIO properties should be named "[-]gpios". The exact
-meaning of each gpios property must be documented in the device tree
+GPIO properties should be named "[-]gpios", with  being the con_id
+argument that is passed to gpiod_get(). While a NULL con_id is accepted by the
+GPIO API for compatibility reasons (resolving to the "gpios" property), it is
+not allowed for new bindings.
+
+GPIO properties can contain one or more GPIO phandles, but only in exceptional
+cases should they contain more than one. If your device uses several GPIOs with
+distinct functions, reference each of them under its own property, giving it a
+meaningful name. The only case where an array of GPIOs is accepted is when
+several GPIOs serve the same function (e.g. a parallel data line). In that case
+individual GPIOs can be retrieved using gpiod_get_index().
+
+The exact meaning of each gpios property must be documented in the device tree
 binding for each device.
 
 For example, the following could be used to describe GPIO pins used
-as chip select lines; with chip selects 0, 1 and 3 populated, and chip
-select 2 left empty:
+as device enable and bit-banged data signals:
 
gpio1: gpio1 {
gpio-controller
@@ -30,10 +40,16 @@ select 2 left empty:
 #gpio-cells = <1>;
};
[...]
-chipsel-gpios = <&gpio1 12 0>,
-<&gpio1 13 0>,
-<0>, /* holes are permitted, means no GPIO 2 */
-<&gpio2 2>;
+
+   enable-gpios = <&gpio2 2>;
+   data-gpios = <&gpio1 12 0>,
+<&gpio1 13 0>,
+<&gpio1 14 0>,
+<&gpio1 15 0>;
+
+The "enable" GPIO can then be retrieved using gpiod_get(dev, "enable", ...),
+and the data GPIOs are reached by calling gpiod_get_index(dev, "data", idx, 
...)
+where idx can go from 0 to 3 included.
 
 Note that gpio-specifier length is controller dependent.  In the
 above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
@@ -42,16 +58,17 @@ only uses one.
 gpio-specifier may encode: bank, pin position inside the bank,
 whether pin is open-drain and whether pin is logically inverted.
 Exact meaning of each specifier cell is controller specific, and must
-be documented in the device tree binding for the device.
+be documented in the device tree binding for the device. Use the macros
+defined in include/dt-bindings/gpio/gpio.h whenever possible:
 
 Example of a node using GPIOs:
 
node {
-   gpios = <&qe_pio_e 18 0>;
+   enable-gpios = <&qe_pio_e 18 GPIO_ACTIVE_HIGH>;
};
 
-In this example gpio-specifier is "18 0" and encodes GPIO pin number,
-and GPIO flags as accepted by the "qe_pio_e" gpio-controller.
+GPIO_ACTIVE_HIGH is 0, so in this example gpio-specifier is "18 0" and encodes
+GPIO pin number, and GPIO flags as accepted by the "qe_pio_e" gpio-controller.
 
 1.1) GPIO specifier best practices
 --
-- 
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Alexandre Courbot]

On Tue, Oct 28, 2014 at 7:34 AM, Rafael J. Wysocki  wrote:
> On Monday, October 27, 2014 02:21:23 PM Alexandre Courbot wrote:
>> On Sat, Oct 25, 2014 at 7:05 AM, Rafael J. Wysocki  
>> wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > Provide a way for device drivers using GPIOs described by ACPI
>> > GpioIo resources in _CRS to tell the GPIO subsystem what names
>> > (connection IDs) to associate with specific GPIO pins defined
>> > in there.
>> >
>> > To do that, a driver needs to define a mapping table as a
>> > NULL-terminated array of struct acpi_gpio_mapping objects
>> > that each contain a name, a pointer to an array of pin data
>> > (struct acpi_gpio_params) objects and the size of that array.
>> >
>> > Each struct acpi_gpio_params object consists of three fields,
>> > crs_entry_index, pin_index, active_low, representing the index of
>> > the target GpioIo()/GpioInt() resource in _CRS starting from zero,
>> > the index of the target pin in that resource starting from zero,
>> > and the active-low flag for that pin, respectively.
>> >
>> > Next, the mapping table needs to be passed as the second argument to
>> > acpi_dev_add_driver_gpios() that will register it with the ACPI device
>> > object pointed to by its first argument.  That object must represent
>> > the ACPI namespace node containing the _CRS object referred to by the
>> > GPIO mapping.  That should be done in the driver's .probe() routine.
>> >
>> > On removal, the driver should unregister its GPIO mapping table
>> > by calling acpi_dev_remove_driver_gpios() on the ACPI device
>> > object where that table was previously registered.
>> >
>> > Included are fixes from Mika Westerberg.
>>
>> Acked-by: Alexandre Courbot 
>>
>> As we discussed already, this is a great idea. The only thing that is
>> missing is a paragraph in Documentation/gpio/consumer.txt with an
>> explanation of the global mechanism and a simple example to illustrate
>> how and when this should be used.
>
> Yes, I'm going to provide documentation.
>
> One question, though.  We're already adding gpio-properties.txt under
> Documentation/acpi/ containing analogous information for _DSD-based
> mappings.  I thought I'd add a section to that file and a short paragraph
> pointing to it into consumer.txt, would that work?

Definitely yes, since this is ACPI-specific. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Alexandre Courbot]

On Sat, Oct 25, 2014 at 5:45 AM, Andrew Lunn  wrote:
>> > +   switch (mvchip->soc_variant) {
>> > +   case MVEBU_GPIO_SOC_VARIANT_ORION:
>> > +   mvchip->edge_mask_regs[0] =
>> > +   readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
>> > +   mvchip->level_mask_regs[0] =
>> > +   readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
>> > +   break;
>> > +   case MVEBU_GPIO_SOC_VARIANT_MV78200:
>> > +   for (i = 0; i < 2; i++) {
>> > +   mvchip->edge_mask_regs[i] =
>> > +   readl(mvchip->membase +
>> > + GPIO_EDGE_MASK_MV78200_OFF(i));
>> > +   mvchip->level_mask_regs[i] =
>> > +   readl(mvchip->membase +
>> > + GPIO_LEVEL_MASK_MV78200_OFF(i));
>> > +   }
>> > +   break;
>> > +   case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
>> > +   for (i = 0; i < 4; i++) {
>> > +   mvchip->edge_mask_regs[i] =
>> > +   readl(mvchip->membase +
>> > + GPIO_EDGE_MASK_ARMADAXP_OFF(i));
>> > +   mvchip->level_mask_regs[i] =
>> > +   readl(mvchip->membase +
>> > + GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
>> > +   }
>> > +   break;
>> > +   default:
>> > +   BUG();
>>
>> Isn't it too severe? Is the platform going too unstable if driver
>> reaches this case?
>> I'd consider a WARN() instead.
>
> This is a common pattern in this driver. So i guess Thomas just
> cut/pasted the switch statement from _probe(), which also has the
> BUG().
>
> Given that _probe() should of thrown a BUG() in this situation, if it
> happens here, it means mvchip->soc_variant has been corrupted, and so
> bad things are happening. So a BUG() is maybe called for?

I agree that BUG() is adequate here. probe() should recognize the
exact same set of chips - if we reach this point this means that
either the data has been corrupted or we added support for a new chip
in probe() and forgot suspend/resume. In both cases the driver should
express its discontent.

Acked-by: Alexandre Courbot 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / GPIO: Driver GPIO mappings for ACPI GPIOs

[Alexandre Courbot]

On Sat, Oct 25, 2014 at 7:05 AM, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> Provide a way for device drivers using GPIOs described by ACPI
> GpioIo resources in _CRS to tell the GPIO subsystem what names
> (connection IDs) to associate with specific GPIO pins defined
> in there.
>
> To do that, a driver needs to define a mapping table as a
> NULL-terminated array of struct acpi_gpio_mapping objects
> that each contain a name, a pointer to an array of pin data
> (struct acpi_gpio_params) objects and the size of that array.
>
> Each struct acpi_gpio_params object consists of three fields,
> crs_entry_index, pin_index, active_low, representing the index of
> the target GpioIo()/GpioInt() resource in _CRS starting from zero,
> the index of the target pin in that resource starting from zero,
> and the active-low flag for that pin, respectively.
>
> Next, the mapping table needs to be passed as the second argument to
> acpi_dev_add_driver_gpios() that will register it with the ACPI device
> object pointed to by its first argument.  That object must represent
> the ACPI namespace node containing the _CRS object referred to by the
> GPIO mapping.  That should be done in the driver's .probe() routine.
>
> On removal, the driver should unregister its GPIO mapping table
> by calling acpi_dev_remove_driver_gpios() on the ACPI device
> object where that table was previously registered.
>
> Included are fixes from Mika Westerberg.

Acked-by: Alexandre Courbot 

As we discussed already, this is a great idea. The only thing that is
missing is a paragraph in Documentation/gpio/consumer.txt with an
explanation of the global mechanism and a simple example to illustrate
how and when this should be used.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: GPIO bindings guidelines (Was: Re: [PATCH v5 10/12] gpio: Support for unified device properties interface)

[Alexandre Courbot]

On Fri, Oct 24, 2014 at 6:51 AM, Rafael J. Wysocki  wrote:
> On Thursday, October 23, 2014 03:56:55 PM Mika Westerberg wrote:
>> On Thu, Oct 23, 2014 at 01:21:06AM +0200, Rafael J. Wysocki wrote:
>> > OK, would the below make sense, then (completely untested, on top of the v6
>> > of the device properties patchset)?
>>
>> Yes it does :-)
>>
>> With the the below fix it works nicely with the modified rfkill-gpio.c
>> driver.
>>
>> > +static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
>> > + const char *name,
>> > + struct acpi_reference_args *args)
>> > +{
>> > +   struct acpi_gpio_data *gd;
>> > +
>> > +   mutex_lock(&driver_gpio_data_lock);
>> > +
>> > +   list_for_each_entry(gd, &adev->driver_gpios, item)
>> > +   if (!strcmp(name, gd->name)) {
>> > +   args->adev = adev;
>> > +   args->args[0] = gd->entry_index;
>> > +   args->args[1] = gd->pin_index;
>> > +   args->args[2] = gd->active_low;
>> > +   args->nargs = 3;
>>
>>   mutex_unlock(&driver_gpio_data_lock);
>>
>> > +   return true;
>> > +   }
>> > +
>> > +   mutex_unlock(&driver_gpio_data_lock);
>> > +
>> > +   return false;
>> > +}
>>
>> Also I think the two functions should be exported to modules as well.
>>
>> Here are the modifications needed for rfkill-gpio.c driver. I think it
>> is not that bad considering that now the driver supports both ACPI _DSD
>> and non-_DSD at the same time.
>
> OK, let's try to take that a bit farther. :-)
>
> With the (untested) patch below applied (which is a replacement for the one
> sent previously), the driver can use static tables like these:
>
> static struct acpi_gpio_params reset_gpios = { 0, 0, false };
> statuc struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
>
> static struct acpi_gpio_mapping my_gpio_mapping[] = {
> { "reset-gpios", &reset_gpios, 1 },
> { "shutdown-gpios", &shutdown_gpios, 1 },
> NULL,
> };
>
> ->
>
>>
>> diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
>> index 0f62326c0f5e..f8754e7d81ea 100644
>> --- a/net/rfkill/rfkill-gpio.c
>> +++ b/net/rfkill/rfkill-gpio.c
>> @@ -67,6 +67,8 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
>> struct rfkill_gpio_data *rfkill)
>>  {
>>   const struct acpi_device_id *id;
>> + struct acpi_device *adev = ACPI_COMPANION(dev);
>> + int ret;
>>
>>   id = acpi_match_device(dev->driver->acpi_match_table, dev);
>>   if (!id)
>> @@ -75,6 +77,20 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
>>   rfkill->name = dev_name(dev);
>>   rfkill->type = (unsigned)id->driver_data;
>>
>> + /*
>> +  * Add default mapping for ACPI _DSD properties now just in case
>> +  * there is no _DSD for this device.
>> +  */
>> + ret = acpi_dev_add_driver_gpio(adev, "reset-gpios", 0, 0, false);
>> + if (ret)
>> + return ret;
>> +
>> + ret = acpi_dev_add_driver_gpio(adev, "shutdown-gpios", 1, 0, false);
>> + if (ret) {
>> + acpi_dev_remove_driver_gpios(adev);
>> + return ret;
>> + }
>
> -> and then simply do
>
> ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(dev), 
> &my_gpio_mapping);
>
> instead of the above.
>
>>   return 0;
>>  }
>>
>> @@ -110,7 +126,7 @@ static int rfkill_gpio_probe(struct platform_device 
>> *pdev)
>>   rfkill->reset_gpio = gpio;
>>   }
>>
>> - gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 1);
>> + gpio = devm_gpiod_get_index(&pdev->dev, "shutdown", 0);
>>   if (!IS_ERR(gpio)) {
>>   ret = gpiod_direction_output(gpio, 0);
>>   if (ret)
>> @@ -150,6 +166,9 @@ static int rfkill_gpio_remove(struct platform_device 
>> *pdev)
>>   rfkill_unregister(rfkill->rfkill_dev);
>>   rfkill_destroy(rfkill->rfkill_dev);
>>
>> + if (ACPI_COMPANION(&pdev->dev))
>> + acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
>> +
>
> And here simply
>
> acpi_dev_remove_driver_gpios(ACPI_COMPANION(&pdev->dev));
>
>>   return 0;
>>  }
>
> and then we can even cover situations in which there is one name for a list of
> GPIOs.

That looks just perfect.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: add gpio_of_helper

[Alexandre Courbot]

Added the DT maintainers and fixed the DT mailing-list's address so
this discussion becomes visible to them as well.

On Thu, Oct 23, 2014 at 8:23 PM, Pantelis Antoniou
 wrote:
> Hi Alexandre,
>
>> On Oct 23, 2014, at 11:53 AM, Alexandre Courbot  wrote:
>>
>> On Thu, Oct 23, 2014 at 3:23 PM, Jiří Prchal  wrote:
>>>
>>>
>>> Dne 23.10.2014 v 07:08 Alexandre Courbot napsal(a):
>>>
>>>> On Wed, Oct 22, 2014 at 6:58 PM, Pantelis Antoniou
>>>>  wrote:
>>>>>
>>>>> Hi Alexandre,
>>>>>
>>>>>> On Oct 22, 2014, at 12:41 PM, Alexandre Courbot 
>>>>>> wrote:
>>>>>>
>>>>>> On Wed, Oct 22, 2014 at 6:24 PM, Pantelis Antoniou
>>>>>>  wrote:
>>>>>>>
>>>>>>> Hi Alexandre,
>>>>>>>
>>>>>>>> On Oct 22, 2014, at 12:18 PM, Alexandre Courbot 
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On Wed, Oct 22, 2014 at 5:26 PM, Jiri Prchal 
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> This patch adds new driver "gpio-of-helper", witch has possibility to
>>>>>>>>> export
>>>>>>>>> gpios defined in dt. It exports them in defined name under
>>>>>>>>> /sysfs/class/gpio/name.
>>>>>>>>> It's rebased from Pantelis Antoniou patch to new kernel.
>>>>>>>>> Usage example:
>>>>>>>>>   gpio {
>>>>>>>>>   compatible = "gpio-of-helper";
>>>>>>>>>   status = "okay";
>>>>>>>>>   pinctrl-names = "default";
>>>>>>>>>   pinctrl-0 = <&pinctrl_gpio>;
>>>>>>>>>
>>>>>>>>>   gsm_on {
>>>>>>>>>   gpio-name = "gsm_on";
>>>>>>>>>   gpio = <&pioB 13 GPIO_ACTIVE_HIGH>;
>>>>>>>>>   output;
>>>>>>>>>   init-low;
>>>>>>>>>   };
>>>>>>>>>   };
>>>>>>>>>
>>>>>>>>> This patch needs Alexey Ignatov [PATCH] gpiolib: allow exporting
>>>>>>>>> gpios with
>>>>>>>>> custom names.
>>>>>>>>
>>>>>>>>
>>>>>>>> We will need to see whether the pre-requisite patch can get merged
>>>>>>>> first, but there are a couple of things that are wrong with your patch
>>>>>>>> anyway:
>>>>>>>>
>>>>>>>> - it is missing a bindings documentation
>>>>>>>> - it is using the legacy integer GPIOs instead of the descriptor
>>>>>>>> interface (see include/linux/gpio/consumer.h). Since this is DT-based,
>>>>>>>> there is absolutely no reason to not use the descriptors interface.
>>>>>>>> - it seems quite long for what it needs to do
>>>>>>>> - the MODULE_AUTHOR has not signed-off this patch (?)
>>>>>>>>
>>>>>>>> But what makes me nervous is that this encourages more usage of the
>>>>>>>> sysfs interface, an in a way that is potentially harmful.
>>>>>>>>
>>>>>>>> Also, I don't know if the DT people will be happy with this idea.
>>>>>>>> Since this concerns DT, please also add the devicetree list and get a
>>>>>>>> Acked-by for the bindings you want to push by a DT maintainer.
>>>>>>>
>>>>>>>
>>>>>>> Since I’m the original perpetrator, let me put a few words here for
>>>>>>> posterity.
>>>>>>>
>>>>>>> This patch was meant as a quick and dirty method for doing the
>>>>>>> automatic export
>>>>>>> and pinmux configuration from DT on a 3.8 kernel. The kernel APIs have
>>>>>>> moved on
>>>>>>> since. It wasn’t meant to be submitted for mainline right as it is.
>>>>>>>
>>>>&g