lkml.org 
[lkml]   [2016]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 2/2] reset: img: Add Pistachio reset controller driver
    Hi Philipp,

    On 01/18/16 09:11, Philipp Zabel wrote:
    > Hi,
    >
    > a few small issues.
    >
    > Am Freitag, den 15.01.2016, 18:17 +0000 schrieb James Hartley:
    >> From: "Damien.Horsley" <Damien.Horsley@imgtec.com>
    >>
    >> Add reset controller driver for Pistachio SoC
    >>
    >> Signed-off-by: Damien.Horsley <Damien.Horsley@imgtec.com>
    >> Signed-off-by: James Hartley <james.hartley@imgtec.com>
    >> ---
    >> drivers/reset/Makefile | 1 +
    >> drivers/reset/reset-pistachio.c | 162 ++++++++++++++++++++
    >> .../reset-controller/pistachio-resets.h | 36 +++++
    >> 3 files changed, 199 insertions(+)
    >> create mode 100644 drivers/reset/reset-pistachio.c
    >> create mode 100644 include/dt-bindings/reset-controller/pistachio-resets.h
    >>
    >> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
    >> index 4d7178e..a1fc8ed 100644
    >> --- a/drivers/reset/Makefile
    >> +++ b/drivers/reset/Makefile
    >> @@ -2,6 +2,7 @@ obj-y += core.o
    >> obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
    >> obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
    >> obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
    >> +obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
    >> obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
    >> obj-$(CONFIG_ARCH_STI) += sti/
    >> obj-$(CONFIG_ARCH_HISI) += hisilicon/
    >> diff --git a/drivers/reset/reset-pistachio.c b/drivers/reset/reset-pistachio.c
    >> new file mode 100644
    >> index 0000000..0bbc086
    >> --- /dev/null
    >> +++ b/drivers/reset/reset-pistachio.c
    >> @@ -0,0 +1,162 @@
    >> +/*
    >> + * Pistachio SoC Reset Controller driver
    >> + *
    >> + * Copyright (C) 2015 Imagination Technologies Ltd.
    >> + *
    >> + * Author: Damien Horsley <Damien.Horsley@imgtec.com>
    >> + *
    >> + * 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.
    >> + */
    >> +
    >> +#include <linux/module.h>
    >> +#include <linux/of.h>
    >> +#include <linux/platform_device.h>
    >> +#include <linux/regmap.h>
    >> +#include <linux/reset-controller.h>
    >> +#include <linux/slab.h>
    >> +#include <linux/mfd/syscon.h>
    >> +
    >> +#include <dt-bindings/reset-controller/pistachio-resets.h>
    > This should be:
    > #include <dt-bindings/reset/pistachio-resets.h>
    > (see below).

    Ok, done

    >> +
    >> +#define PISTACHIO_SOFT_RESET 0
    >> +
    >> +struct pistachio_reset_data {
    >> + struct reset_controller_dev rcdev;
    >> + struct regmap *periph_regs;
    >> +};
    >> +
    >> +static inline int pistachio_reset_shift(unsigned long id)
    >> +{
    >> + switch (id) {
    >> + case PISTACHIO_RESET_I2C0:
    >> + case PISTACHIO_RESET_I2C1:
    >> + case PISTACHIO_RESET_I2C2:
    >> + case PISTACHIO_RESET_I2C3:
    >> + case PISTACHIO_RESET_I2S_IN:
    >> + case PISTACHIO_RESET_PRL_OUT:
    >> + case PISTACHIO_RESET_SPDIF_OUT:
    >> + case PISTACHIO_RESET_SPI:
    >> + case PISTACHIO_RESET_PWM_PDM:
    >> + case PISTACHIO_RESET_UART0:
    >> + case PISTACHIO_RESET_UART1:
    >> + case PISTACHIO_RESET_QSPI:
    >> + case PISTACHIO_RESET_MDC:
    >> + case PISTACHIO_RESET_SDHOST:
    >> + case PISTACHIO_RESET_ETHERNET:
    >> + case PISTACHIO_RESET_IR:
    >> + case PISTACHIO_RESET_HASH:
    >> + case PISTACHIO_RESET_TIMER:
    >> + return id;
    >> + case PISTACHIO_RESET_I2S_OUT:
    >> + case PISTACHIO_RESET_SPDIF_IN:
    >> + case PISTACHIO_RESET_EVT:
    >> + return id + 6;
    >> + case PISTACHIO_RESET_USB_H:
    >> + case PISTACHIO_RESET_USB_PR:
    >> + case PISTACHIO_RESET_USB_PHY_PR:
    >> + case PISTACHIO_RESET_USB_PHY_PON:
    >> + return id + 7;
    >> + default:
    >> + return -1;
    > That's -EPERM. Please
    > return -EINVAL;
    Ok, done.
    > here, since ...
    >
    >> + }
    >> +}
    >> +
    >> +static int pistachio_reset_assert(struct reset_controller_dev *rcdev,
    >> + unsigned long id)
    >> +{
    >> + struct pistachio_reset_data *rd;
    >> + u32 mask;
    >> + int shift;
    >> +
    >> + rd = container_of(rcdev, struct pistachio_reset_data, rcdev);
    >> + shift = pistachio_reset_shift(id);
    >> + if (shift < 0)
    >> + return shift;
    > ... here we return the above error value to the caller.
    >
    >> + mask = 1UL << shift;
    > I'd prefer
    > mask = BIT(shift);
    > instead.

    Ok, done.

    >
    >> +
    >> + regmap_update_bits(rd->periph_regs, PISTACHIO_SOFT_RESET, mask, mask);
    >> +
    >> + return 0;
    > Better propagate the return value from regmap_update_bits:
    > return regmap_update_bits(rd->periph_regs, PISTACHIO_SOFT_RESET,
    > mask, mask);

    Ok, done.

    >> +}
    >> +
    >> +static int pistachio_reset_deassert(struct reset_controller_dev *rcdev,
    >> + unsigned long id)
    >> +{
    >> + struct pistachio_reset_data *rd;
    >> + u32 mask;
    >> + int shift;
    >> +
    >> + rd = container_of(rcdev, struct pistachio_reset_data, rcdev);
    >> + shift = pistachio_reset_shift(id);
    >> + if (shift < 0)
    >> + return shift;
    >> + mask = 1UL << shift;
    > Same as above.

    Ok, done.

    >
    >> +
    >> + regmap_update_bits(rd->periph_regs, PISTACHIO_SOFT_RESET, mask, 0);
    >> +
    >> + return 0;
    > Same as above.

    Ok, done.

    >
    >> +}
    >> +
    >> +static struct reset_control_ops pistachio_reset_ops = {
    >> + .assert = pistachio_reset_assert,
    >> + .deassert = pistachio_reset_deassert,
    >> +};
    >> +
    >> +static int pistachio_reset_probe(struct platform_device *pdev)
    >> +{
    >> + struct pistachio_reset_data *rd;
    >> + int ret;
    >> + struct device *dev = &pdev->dev;
    >> + struct device_node *np = pdev->dev.of_node;
    >> +
    >> + rd = devm_kzalloc(dev, sizeof(*rd), GFP_KERNEL);
    >> + if (!rd)
    >> + return -ENOMEM;
    >> +
    >> + rd->periph_regs = syscon_node_to_regmap(np->parent);
    >> + if (IS_ERR(rd->periph_regs))
    >> + return PTR_ERR(rd->periph_regs);
    >> +
    >> + rd->rcdev.owner = THIS_MODULE;
    >> + rd->rcdev.nr_resets = PISTACHIO_RESET_MAX + 1;
    >> + rd->rcdev.ops = &pistachio_reset_ops;
    >> + rd->rcdev.of_node = np;
    >> +
    >> + ret = reset_controller_register(&rd->rcdev);
    >> + if (ret)
    >> + return ret;
    >> +
    >> + return 0;
    > This is equivalent to:
    > return reset_controller_register(&rd->rcdev);
    > Just use that instead.

    Yes that's pretty ugly - done.

    >
    >> +}
    >> +
    >> +static int pistachio_reset_remove(struct platform_device *pdev)
    >> +{
    >> + struct pistachio_reset_data *data = platform_get_drvdata(pdev);
    >> +
    >> + reset_controller_unregister(&data->rcdev);
    >> +
    >> + return 0;
    >> +}
    >> +
    >> +
    >> +static const struct of_device_id pistachio_reset_dt_ids[] = {
    >> + { .compatible = "img,pistachio-reset", },
    >> + { /* sentinel */ },
    >> +};
    >> +MODULE_DEVICE_TABLE(of, pistachio_reset_dt_ids);
    >> +
    >> +static struct platform_driver pistachio_reset_driver = {
    >> + .probe = pistachio_reset_probe,
    >> + .remove = pistachio_reset_remove,
    >> + .driver = {
    >> + .name = "pistachio-reset",
    >> + .of_match_table = pistachio_reset_dt_ids,
    >> + },
    >> +};
    >> +module_platform_driver(pistachio_reset_driver);
    >> +
    >> +MODULE_AUTHOR("Damien Horsley <Damien.Horsley@imgtec.com>");
    >> +MODULE_DESCRIPTION("Pistacho Reset Controller Driver");
    >> +MODULE_LICENSE("GPL v2");
    >> diff --git a/include/dt-bindings/reset-controller/pistachio-resets.h b/include/dt-bindings/reset-controller/pistachio-resets.h
    >> new file mode 100644
    >> index 0000000..60a189b
    >> --- /dev/null
    >> +++ b/include/dt-bindings/reset-controller/pistachio-resets.h
    > Please move the header to include/dt-bindings/reset/pistachio-resets.h.

    Done.

    >
    > regards
    > Philipp
    >

    Thanks for the review! I'll send a V3 patchet.

    James.

    \
     
     \ /
      Last update: 2016-01-18 13:41    [W:3.707 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site