linux-kernel - Re: [PATCH 6/6 v14] gpio: Add block gpio to several gpio drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 27 Jan 2013 13:18:48 +0100
From:	Stijn Devriendt <highguy@...il.com>
To:	Roland Stigge <stigge@...com.de>
Cc:	gregkh@...uxfoundation.org, grant.likely@...retlab.ca,
	linus.walleij@...aro.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, w.sang@...gutronix.de,
	jbe@...gutronix.de, plagnioj@...osoft.com,
	broonie@...nsource.wolfsonmicro.com, daniel-gl@....net,
	rmallon@...il.com, sr@...x.de, wg@...ndegger.com,
	mark.rutland@....com, nicolas.ferre@...el.com
Subject: Re: [PATCH 6/6 v14] gpio: Add block gpio to several gpio drivers

On Thu, Jan 24, 2013 at 1:19 PM, Roland Stigge <stigge@...com.de> wrote:
> On 01/24/2013 01:02 PM, Stijn Devriendt wrote:
>>> +static void lpc32xx_gpio_set_block_p3(struct gpio_chip *chip,
>>> +                                     unsigned long mask,
>>> +                                     unsigned long values)
>>> +{
>>> +       struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
>>> +       u32 set_bits = values & mask;
>>> +       u32 clr_bits = ~values & mask;
>>> +
>>> +       /* States of GPIO 0-5 start at bit 25 */
>>> +       set_bits <<= 25;
>>> +       clr_bits <<= 25;
>>> +
>>> +       /* Note: On LPC32xx, GPOs can only be set at once or cleared at once,
>>> +        *       but not set and cleared at once
>>> +        */
>>> +       if (set_bits)
>>> +               __raw_writel(set_bits, group->gpio_grp->outp_set);
>>> +       if (clr_bits)
>>> +               __raw_writel(clr_bits, group->gpio_grp->outp_clr);
>>> +}
>>> +
>>
>> In my patch, I go out of the way of this kind of thing for a simple reason:
>> You may generate incorrect timing by doing this.
>
> You are right, certain things like synchronous on+off is not really
> possible.
>
> However, the above at least supports switching on simulaneously, and
> switching off simultaneously, which is an improvement in certain cases
> (and this certain hardware part doesn't support more). Maybe this
> certain driver behaviour can be documented even better than just in the
> driver source.
>

The question here is: do you expect a user of the block-GPIO API to
go look into the base-driver code to see what will be supported?

In my version of the patch this means:
- do not provide a single GPIO-block that crosses multiple base-drivers
- only provide gpio_block_get/set for GPIO drivers that support the complete
operation in a single go. (for example, in the above example there would be
no gpio_block_set() function)

Perhaps the best approach is to make this explicit: Allow drivers to expose
their capabilities wrt timing and allow users to request strict-timing or
loose-timing. Loose-timing allows multiple gpio-drivers to be combined and
allows drivers with separate set/clear, hi/lo registers to be used.
Of course, for a first version you may as well leave it out. Perhaps the
use-cases for cross-GPIO-driver blocks are not worth the extra complexity
as of today?

Regards,
Stijn

> If there is consensus that everything else in the patch set is settled,
> and we just don't want to support behaviour like the above - well, we
> can still drop it.
>
> Thanks for the note,
>
> Roland
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/