On 11/18/20 5:24 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>
> PMB can be found on BCM4908 and many other chipsets (e.g. BCM63138).
> It's needed to power on and off SoC blocks like PCIe, SATA, USB.
>
> Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
Since this is a driver for the PMB and not the PMC, the subject should
probably reflect that.
> ---
> drivers/reset/Kconfig | 7 +
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-brcm-pmb.c | 307 +++++++++++++++++++++++++++++++++
> 3 files changed, 315 insertions(+)
> create mode 100644 drivers/reset/reset-brcm-pmb.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 84baec01aa30..af10fb92691c 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -41,6 +41,13 @@ config RESET_BERLIN
> help
> This enables the reset controller driver for Marvell Berlin SoCs.
>
> +config RESET_BRCM_PMB
> + tristate "Broadcom PMB reset controller"
> + depends on ARCH_BCM4908 || COMPILE_TEST
Not sure the depends on ARCH_BCM4908 is warranted here, but it certainly
does not hurt to scope the driver to the platform it is applicable to.
[snip]
> +static int brcm_pmb_reset_xlate(struct reset_controller_dev *rcdev,
> + const struct of_phandle_args *reset_spec)
> +{
> + u8 type = reset_spec->args[0];
> + u8 device = reset_spec->args[1];
> +
> + if (type > 0xff)
> + return -EINVAL;
> +
> + return (type << 8) | device;
Does not the device also need to be capped at 8 bits?
> +}
> +
> +static const struct reset_control_ops brcm_pmb_reset_control_ops = {
> + .assert = brcm_pmb_assert,
> + .deassert = brcm_pmb_deassert,
> +};
> +
> +static const struct of_device_id brcm_pmb_reset_of_match[] = {
> + { .compatible = "brcm,bcm4908-pmb", .data = &brcm_pmb_4908_data, },
> + { },
> +};
> +
> +static int brcm_pmb_reset_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct brcm_pmb *pmb;
> + struct resource *res;
> +
> + pmb = devm_kzalloc(dev, sizeof(*pmb), GFP_KERNEL);
> + if (!pmb)
> + return -ENOMEM;
> +
> + pmb->data = of_device_get_match_data(dev);
Not that it would likely support ACPI in the future but you can use
device_get_match_data() to be firmware (OF or ACPI) implementation
agnostic here.
Other than that, everything else looks good to me, thanks Rafal!
--
Florian
