| 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
| ||
|
Date: Thu, 16 Jul 2015 12:44:31 +0900
From: Masahiro Yamada <yamada.masahiro@...ionext.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Alexandre Courbot <gnurou@...il.com>
Subject: Re: [PATCH v3] gpio: UniPhier: add driver for UniPhier GPIO controller
Hi Linus,
2015-07-15 23:15 GMT+09:00 Linus Walleij <linus.walleij@...aro.org>:
> On Tue, Jul 14, 2015 at 4:43 AM, Masahiro Yamada
> <yamada.masahiro@...ionext.com> wrote:
>
>> This GPIO controller device is used on UniPhier SoCs.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
>> ---
>>
>> Changes in v3:
>> - Use module_platform_driver()
>>
>> Changes in v2:
>> - Fix typos in the comment block
>
> OK why no device tree bindings? Are they in a separate patch?
Sorry, I was planning to do it later.
OK. I will come back with
Documentation/devicetree/bindings/gpio/uniphier-gpio.txt in binding info in it.
>> +/*
>> + * Unfortunately, the hardware specification adopts weird GPIO pin labeling.
>> + * The ports are named as
>> + * PORT00, PORT01, PORT02, ..., PORT07,
>> + * PORT10, PORT11, PORT12, ..., PORT17,
>> + * PORT20, PORT21, PORT22, ..., PORT27,
>> + * ...
>> + * PORT90, PORT91, PORT92, ..., PORT97,
>> + * PORT100, PORT101, PORT102, ..., PORT107,
>> + * ...
>> + *
>> + * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place
>> + * is octal, while the other places are decimal. If we handle the port numbers
>> + * as seen in the hardware documents, the GPIO offsets must be non-contiguous.
>> + * It is possible to have sparse GPIO pins, but not handy for GPIO range
>> + * mappings, register accessing, etc.
>> + *
>> + * To make things simpler (for driver and device tree implementation), this
>> + * driver takes contiguously-numbered GPIO offsets. GPIO consumers should make
>> + * sure to convert the PORT number into the one that fits in this driver.
>> + * The conversion logic is very easy math, for example,
>> + * PORT15 --> GPIO offset 13 (8 * 1 + 5)
>> + * PORT123 --> GPIO offset 99 (8 * 12 + 3)
>> + */
>> +#define UNIPHIER_GPIO_PORTS_PER_BANK 8
>> +#define UNIPHIER_GPIO_BANK_MASK \
>> + ((1UL << (UNIPHIER_GPIO_PORTS_PER_BANK)) - 1)
>
>
>
>> +
>> +#define UNIPHIER_GPIO_REG_DATA 0 /* data */
>> +#define UNIPHIER_GPIO_REG_DIR 4 /* direction (1:in, 0:out) */
>> +
>> +struct uniphier_gpio_priv {
>> + struct of_mm_gpio_chip mmchip;
>> + spinlock_t lock;
>> +};
>> +
>> +static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type)
>> +{
>> + unsigned reg;
>> +
>> + reg = (bank + 1) * 8 + reg_type;
>> +
>> + /*
>> + * Unfortunately, there is a register hole at offset 0x90-0x9f.
>> + * Add 0x10 when crossing the hole.
>> + */
>> + if (reg >= 0x90)
>> + reg += 0x10;
>> +
>> + return reg;
>> +}
>> +
>> +static void uniphier_gpio_bank_write(struct gpio_chip *chip,
>> + unsigned bank, unsigned reg_type,
>> + unsigned mask, unsigned value)
>> +{
>> + struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip);
>> + struct uniphier_gpio_priv *priv;
>> + unsigned long flags;
>> + unsigned reg;
>> + u32 tmp;
>> +
>> + if (!mask)
>> + return;
>> +
>> + priv = container_of(mmchip, struct uniphier_gpio_priv, mmchip);
>> +
>> + reg = uniphier_gpio_bank_to_reg(bank, reg_type);
>> +
>> + /*
>> + * Note
>> + * regmap_update_bits() should not be used here.
>> + *
>> + * The DATA registers return the current readback of pins, not the
>> + * previously written data when they are configured as "input".
>> + * The DATA registers must be overwritten even if the data you are
>> + * going to write is the same as what readl() has returned.
>> + *
>> + * regmap_update_bits() does not write back if the data is not changed.
>> + */
>
> Why is this mentioned when the driver doesn't even use regmap?
> Development artifact?
At first, I thought regmap_update_bits() might be useful,
but it tuned out a bad idea.
Anyway, it did not use regmap in this driver, so this comment sounds a
bit weird.
I will delete it in v4.
>> +static int uniphier_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
>> +{
>> + return uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset) ?
>> + GPIOF_DIR_IN : GPIOF_DIR_OUT;
>
> Just use
> return !!uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset);
OK, will fix.
>> +static int uniphier_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> + return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA);
>
> return !!uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA);
Likewise.
>> +static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
>> + unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + unsigned bank, shift, bank_mask, bank_bits;
>> + int i;
>> +
>> + for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) {
>> + bank = i / UNIPHIER_GPIO_PORTS_PER_BANK;
>> + shift = i % BITS_PER_LONG;
>> + bank_mask = (mask[BIT_WORD(i)] >> shift) &
>> + UNIPHIER_GPIO_BANK_MASK;
>> + bank_bits = bits[BIT_WORD(i)] >> shift;
>> +
>> + uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA,
>> + bank_mask, bank_bits);
>> + }
>
> This looks like a piece of algorithm that we could make generic like in a
> static function in drivers/gpio/gpiolib.h or so, that it may be shared with
> other drivers. Do you see some clear way to break out the core of this?
> Or is it as generic as I think?
I assume this comment has no intention to block my patch.
We already have a generic handling in gpio_chip_set_multiple()
in case .set_multiple() is not supported.
Given that not many drivers support .set_multiple,
I am not sure if we should add another generalized helper func.
>> +static int uniphier_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct uniphier_gpio_priv *priv;
>> + u32 ngpio;
>> + int ret;
>> +
>> + ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio);
>> + if (ret) {
>> + dev_err(dev, "failed to get ngpio property\n");
>> + return ret;
>> + }
>
> This needs to be documented, plus I don't see why it's needed.
> The driver for this very specific hardware should already know
> how many GPIOs there are in this hardware, it should not come
> from the device tree.
I want to use this driver on various SoCs, but
the number of GPIO pins varies by SoC.
ngpio == 248 for some SoCs,
and ngpio == 136 for some, etc.
I will document it.
> ngpio is typically for the case where the hardware has 32 bits
> for GPIO pins, but only the first 11 are actually used in the hardware.
>
>> +static const struct of_device_id uniphier_gpio_match[] = {
>> + { .compatible = "socionext,uniphier-gpio" },
>> + { /* sentinel */ }
>> +};
>
> Needs a binding document.
>
> Apart from this it looks nice!
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Best Regards
Masahiro Yamada
--
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/
Powered by blists - more mailing lists