Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a929309891f9f28ae71f7ee09e990dc8bc362fdf.camel@pengutronix.de>
Subject: Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset
 Controller
From:   Philipp Zabel <p.zabel@pengutronix.de>
To:     Brad Larson <brad@pensando.io>,
        linux-arm-kernel@lists.infradead.org
Cc:     linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org,
        adrian.hunter@intel.com, alcooperx@gmail.com,
        andy.shevchenko@gmail.com, arnd@arndb.de, blarson@amd.com,
        brijeshkumar.singh@amd.com, catalin.marinas@arm.com,
        gsomlo@gmail.com, gerg@linux-m68k.org, krzk@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, lee.jones@linaro.org,
        broonie@kernel.org, yamada.masahiro@socionext.com,
        piotrs@cadence.com, p.yadav@ti.com, rdunlap@infradead.org,
        robh+dt@kernel.org, samuel@sholland.org, fancer.lancer@gmail.com,
        suravee.suthikulpanit@amd.com, thomas.lendacky@amd.com,
        ulf.hansson@linaro.org, will@kernel.org, devicetree@vger.kernel.org
Date:   Tue, 14 Jun 2022 16:49:31 +0200
In-Reply-To: <20220613195658.5607-16-brad@pensando.io>
References: <20220613195658.5607-1-brad@pensando.io>
         <20220613195658.5607-16-brad@pensando.io>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.38.3-1 
MIME-Version: 1.0
Precedence: bulk

Hi Brad,

On Mo, 2022-06-13 at 12:56 -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> This patch adds the reset controller functionality for the
> AMD Pensando Elba System Resource Chip.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
[...]
> diff --git a/drivers/reset/reset-elbasr.c b/drivers/reset/reset-elbasr.c
> new file mode 100644
> index 000000000000..6e429cb11466
> --- /dev/null
> +++ b/drivers/reset/reset-elbasr.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 AMD Pensando
> + */
> +
> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +#include <dt-bindings/reset/amd,pensando-elba-reset.h>
> +
> +struct elbasr_reset {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +};
> +
> +static inline struct elbasr_reset *to_elbasr_rst(struct reset_controller_dev *rc)
> +{
> +	return container_of(rc, struct elbasr_reset, rcdev);
> +}
> +
> +static inline int elbasr_reset_shift(unsigned long id)
> +{
> +	switch (id) {
> +	case EMMC_HW_RESET:

Are there more reset controls than EMMC_HW_RESET?
If so, please list them all.
If not, why is this a function with a switch statement for a single
reset bit?

> +		return 6;
> +	default:
> +		return -EINVAL;

The error return value is never checked.
This can't be reached, since ELBASR_NR_RESETS == 1. So id will only
ever be 0.

> +static int elbasr_reset_probe(struct platform_device *pdev)
> +{
> +	struct elbasr_data *elbasr = dev_get_drvdata(pdev->dev.parent);

Peeking into the MFD driver's private data structure seems unnecessary.
Consider using dev_get_regmap() instead.


regards
Philipp