Re: [PATCH 06/17] reset: Add PLX Technology Reset Controller driver

From: Philipp Zabel
Date: Thu Mar 03 2016 - 09:18:36 EST


Hi Neil,

Am Donnerstag, den 03.03.2016, 12:39 +0100 schrieb Neil Armstrong:
> Add System reset controller driver for PLX Technology OXNAS SoC Family.
>
> CC: Ma Haijun <mahaijuns@xxxxxxxxx>
> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> ---
> drivers/reset/Kconfig | 4 ++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-oxnas.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 154 insertions(+)
> create mode 100644 drivers/reset/reset-oxnas.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..f0ea63b 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,9 @@ menuconfig RESET_CONTROLLER
>
> If unsure, say no.
>
> +config RESET_OXNAS
> + bool
> + select MFD_SYSCON

I'd prefer not to select MFD_SYSCON here, but rather let ARCH_OXNAS do
that.

> source "drivers/reset/sti/Kconfig"
> source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4d7178e..97e04c5 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_ARCH_STI) += sti/
> obj-$(CONFIG_ARCH_HISI) += hisilicon/
> obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
> obj-$(CONFIG_ATH79) += reset-ath79.o
> +obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
> diff --git a/drivers/reset/reset-oxnas.c b/drivers/reset/reset-oxnas.c
> new file mode 100644
> index 0000000..d0ab670
> --- /dev/null
> +++ b/drivers/reset/reset-oxnas.c
> @@ -0,0 +1,149 @@
> +/*
> + * drivers/reset/reset-oxnas.c
> + *
> + * Copyright (C) 2016 Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> + * Copyright (C) 2014 Ma Haijun <mahaijuns@xxxxxxxxx>
> + * Copyright (C) 2009 Oxford Semiconductor Ltd
> + *
> + * 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 <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/err.h>
> +#include <linux/io.h>

Is there any need to include linux/io.h ?

> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/types.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +/* Regmap offsets */
> +#define RST_SET_REGOFFSET 0x34
> +#define RST_CLR_REGOFFSET 0x38
> +
> +struct oxnas_reset {
> + struct regmap *regmap;
> + struct reset_controller_dev rcdev;
> +};
> +
> +static int oxnas_reset_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct oxnas_reset *data =
> + container_of(rcdev, struct oxnas_reset, rcdev);
> +
> + regmap_write(data->regmap, RST_SET_REGOFFSET, BIT(id));
> + msleep(50);

Is this the right delay for all of the resets in this register?
If not, I'd drop the .reset callback.

> + regmap_write(data->regmap, RST_CLR_REGOFFSET, BIT(id));
> +
> + return 0;
> +}
> +
> +static int oxnas_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct oxnas_reset *data =
> + container_of(rcdev, struct oxnas_reset, rcdev);
> +
> + regmap_write(data->regmap, RST_SET_REGOFFSET, BIT(id));
> +
> + return 0;
> +}
> +
> +static int oxnas_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct oxnas_reset *data =
> + container_of(rcdev, struct oxnas_reset, rcdev);
> +
> + regmap_write(data->regmap, RST_CLR_REGOFFSET, BIT(id));
> +
> + return 0;
> +}
> +
> +static struct reset_control_ops oxnas_reset_ops = {

const

> + .reset = oxnas_reset_reset,
> + .assert = oxnas_reset_assert,
> + .deassert = oxnas_reset_deassert,
> +};
> +
> +static const struct of_device_id oxnas_reset_dt_ids[] = {
> + { .compatible = "plxtech,nas782x-reset", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, oxnas_reset_dt_ids);
> +
> +static int oxnas_reset_probe(struct platform_device *pdev)
> +{
> + struct oxnas_reset *data;
> + struct device *parent;
> +
> + parent = pdev->dev.parent;
> + if (!parent) {
> + dev_err(&pdev->dev, "no parent\n");

Can this even happen?

> + return -ENODEV;
> + }
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->regmap = syscon_node_to_regmap(parent->of_node);
> + if (IS_ERR(data->regmap)) {
> + dev_err(&pdev->dev, "failed to get parent regmap\n");
> + return -ENODEV;

Better print the error code and return it.

> + }
> +
> + data->rcdev.owner = THIS_MODULE;
> + data->rcdev.nr_resets = 32;
> + data->rcdev.ops = &oxnas_reset_ops;
> + data->rcdev.of_node = pdev->dev.of_node;
> + reset_controller_register(&data->rcdev);

Move this down a bit:

> +
> + platform_set_drvdata(pdev, data);
> +
> + return 0;

and
return reset_controller_register(&data->rcdev);
here.

> +}
> +
> +static int oxnas_reset_remove(struct platform_device *pdev)
> +{
> + struct oxnas_reset *data = platform_get_drvdata(pdev);
> +
> + reset_controller_unregister(&data->rcdev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver oxnas_reset_driver = {
> + .probe = oxnas_reset_probe,
> + .remove = oxnas_reset_remove,
> + .driver = {
> + .name = "oxnas-reset",
> + .owner = THIS_MODULE,

The .owner field is overwritten by __platform_driver_register() anyway,
just drop it.

> + .of_match_table = oxnas_reset_dt_ids,
> + },
> +};
> +
> +static int __init oxnas_reset_init(void)
> +{
> + return platform_driver_probe(&oxnas_reset_driver,
> + oxnas_reset_probe);
> +}
> +
> +/*
> + * Reset controller does not support probe deferral, so it has to be
> + * initialized before any user, in particular, PCIE uses subsys_initcall.
> + */
> +arch_initcall(oxnas_reset_init);

That doesn't sound right. (of_)reset_control_get return -EPROBE_DEFER if
the rcdev isn't found in the list. Could you elaborate on this?

regards
Philipp