Re: [PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMB

[Florian Fainelli <f.fainelli@gmail.com>]

On 11/18/20 5:24 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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@milecki.pl>

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