On 27 November 2014 at 13:03, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
> On Thu, Nov 20, 2014 at 9:37 PM, Vincent Yang
> <vincent.yang.fujitsu@xxxxxxxxx> wrote:
>> Driver for Fujitsu MB86S7x SoCs that have a memory mapped GPIO controller.
>>
>> Signed-off-by: Andy Green <andy.green@xxxxxxxxxx>
>> Signed-off-by: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
>> Signed-off-by: Vincent Yang <Vincent.Yang@xxxxxxxxxxxxxx>
>> Signed-off-by: Tetsuya Nuriya <nuriya.tetsuya@xxxxxxxxxxxxxx>
>> ---
>> .../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 0000000..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.
>
OK, will add them as well.
>> +
>> +Examples:
>> + gpio0: gpio@31000000 {
>> + compatible = "fujitsu,mb86s70-gpio";
>> + reg = <0 0x31000000 0x10000>;
>> + 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?
>
Other mb86s7x drivers don't use that either and we do just fine :)
>> +
>> +#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?
>
OK
>> +
>> +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.
>
OK
>> + unsigned long flags;
>> + u32 val;
>> +
>> + spin_lock_irqsave(&gchip->lock, flags);
>> + val = readl(gchip->base + PFR(offset / 8 * 4));
>
> Same, having mb86s70_readl()/mb86s70_writel() functions would prevent
> you from explicitly doing pointer arithmetic every time you write a
> register.
>
This becomes trivial after updating macros.
>> + val &= ~(1 << (offset % 8)); /* as gpio-port */
>
> val &= ~BIT(offset % 8); ? Same everywhere it applies.
>
OK
>> +
>> +static int mb86s70_gpio_get(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct mb86s70_gpio_chip *gchip =
>> + container_of(gc, struct mb86s70_gpio_chip, gc);
>> + unsigned char val;
>> +
>> + val = readl(gchip->base + PDR(offset / 8 * 4));
>> + val &= (1 << (offset % 8));
>> + return val ? 1 : 0;
>
> Shouldn't this function be protected by the spin lock as well?
>
hmm... then we need to fix most other drivers as well :)
> These minor comments aside, the driver looks nice and simple.
Thanks for the review.
-Jassi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
