Re: [PATCH v5 15/15] reset: elbasr: Add AMD Pensando Elba SR Reset Controller

[Philipp Zabel <p.zabel@pengutronix.de>]

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