RE: [PATCH v2 1/4] reset: gpio: Add GPIO-based reset controller

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Krzysztof Kozlowski,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Saturday, January 6, 2024 3:29 PM
> Subject: Re: [PATCH v2 1/4] reset: gpio: Add GPIO-based reset controller
> 
> On 05/01/2024 17:39, Biju Das wrote:
> > Hi Krzysztof Kozlowski,
> >
> > Thanks for the patch.
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Friday, January 5, 2024 3:59 PM
> >> Subject: [PATCH v2 1/4] reset: gpio: Add GPIO-based reset controller
> >>
> >> Add a simple driver to control GPIO-based resets using the reset
> >> controller API for the cases when the GPIOs are shared and reset
> >> should be coordinated.  The driver is expected to be used by reset
> >> core framework for ad-hoc reset controllers.
> >>
> >> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> >> Cc: Sean Anderson <sean.anderson@seco.com>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> ---
> >>  MAINTAINERS                |   5 ++
> >>  drivers/reset/Kconfig      |   9 +++
> >>  drivers/reset/Makefile     |   1 +
> >>  drivers/reset/reset-gpio.c | 121
> >> +++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 136 insertions(+)
> >>  create mode 100644 drivers/reset/reset-gpio.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS index
> >> 7fe27cd60e1b..a0fbd4814bc7 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -8866,6 +8866,11 @@ F:	Documentation/i2c/muxes/i2c-mux-gpio.rst
> >>  F:	drivers/i2c/muxes/i2c-mux-gpio.c
> >>  F:	include/linux/platform_data/i2c-mux-gpio.h
> >>
> >> +GENERIC GPIO RESET DRIVER
> >> +M:	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> +S:	Maintained
> >> +F:	drivers/reset/reset-gpio.c
> >> +
> >>  GENERIC HDLC (WAN) DRIVERS
> >>  M:	Krzysztof Halasa <khc@pm.waw.pl>
> >>  S:	Maintained
> >> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index
> >> ccd59ddd7610..bb1b5a326eb7 100644
> >> --- a/drivers/reset/Kconfig
> >> +++ b/drivers/reset/Kconfig
> >> @@ -66,6 +66,15 @@ config RESET_BRCMSTB_RESCAL
> >>  	  This enables the RESCAL reset controller for SATA, PCIe0, or
> >> PCIe1 on
> >>  	  BCM7216.
> >>
> >> +config RESET_GPIO
> >> +	tristate "GPIO reset controller"
> >> +	help
> >> +	  This enables a generic reset controller for resets attached via
> >> +	  GPIOs.  Typically for OF platforms this driver expects "reset-
> >> gpios"
> >> +	  property.
> >> +
> >> +	  If compiled as module, it will be called reset-gpio.
> >> +
> >>  config RESET_HSDK
> >>  	bool "Synopsys HSDK Reset Driver"
> >>  	depends on HAS_IOMEM
> >> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index
> >> 8270da8a4baa..fd8b49fa46fc 100644
> >> --- a/drivers/reset/Makefile
> >> +++ b/drivers/reset/Makefile
> >> @@ -11,6 +11,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
> >>  obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> >>  obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> >>  obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> >> +obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
> >>  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> >>  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> >>  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o diff --git
> >> a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c new file
> >> mode
> >> 100644 index 000000000000..cf0a867cbc5f
> >> --- /dev/null
> >> +++ b/drivers/reset/reset-gpio.c
> >> @@ -0,0 +1,121 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/reset-controller.h>
> >> +
> >> +struct reset_gpio_priv {
> >> +	struct reset_controller_dev rc;
> >> +	struct gpio_desc *reset;
> >> +};
> >> +
> >> +static inline struct reset_gpio_priv *rc_to_reset_gpio(struct
> >> +reset_controller_dev *rc) {
> >> +	return container_of(rc, struct reset_gpio_priv, rc); }
> >> +
> >> +static int reset_gpio_assert(struct reset_controller_dev *rc,
> >> +unsigned long id) {
> >> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> >> +
> >> +	gpiod_set_value_cansleep(priv->reset, 1);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int reset_gpio_deassert(struct reset_controller_dev *rc,
> >> +			       unsigned long id)
> >> +{
> >> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> >> +
> >> +	gpiod_set_value_cansleep(priv->reset, 0);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int reset_gpio_status(struct reset_controller_dev *rc,
> >> +unsigned long id) {
> >> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> >> +
> >> +	return gpiod_get_value_cansleep(priv->reset);
> >> +}
> >> +
> >> +static const struct reset_control_ops reset_gpio_ops = {
> >> +	.assert = reset_gpio_assert,
> >> +	.deassert = reset_gpio_deassert,
> >> +	.status = reset_gpio_status,
> >> +};
> >> +
> >> +static void reset_gpio_of_args_put(void *data) {
> >> +	of_node_put(data);
> >> +}
> >> +
> >> +static int reset_gpio_probe(struct platform_device *pdev) {
> >> +	struct device *dev = &pdev->dev;
> >> +	struct device_node **platdata = dev_get_platdata(dev);
> >> +	struct of_phandle_args gpio_args;
> >> +	struct reset_gpio_priv *priv;
> >> +	int ret;
> >> +
> >> +	if (!platdata || !*platdata)
> >
> > Maybe, if (!(platdata && *platdata)) which reduces 1 inversion
> operation.
> 
> I would not call it easier to understand... To me !A and !*A are quite
> obvious and easy to read instantly because !A is obvious: check if it is
> not NULL. Therefore original check is obvious: is NULL or points to NULL?
> Then exit.
> 
> Now your check is a bit more complicated. It is not even frequent code
> pattern which my brain used to see. You want to check if both are not NULL
> and then negate it, wait, no, opposite, check if they are something and
> then negate? To me it is really opposite of readable code.

I agree maybe it is not readable, even though it reduces 1 extra operation.

!(Valid pointer AND points to a non NULL value)
Then exit.

Cheers,
Biju