Re: [PATCH 19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

Hi Bartosz,

Le mardi 05 septembre 2023 =C3=A0 20:53 +0200, Bartosz Golaszewski a =C3=A9=
crit=C2=A0:
> From: Bartosz Golaszewski <[email protected]>
>=20
> We're porting all users of gpiochip_find() to using
> gpio_device_find().
> Update the swnode GPIO code.
>=20
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> =C2=A0drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
> =C2=A01 file changed, 12 insertions(+), 17 deletions(-)
>=20
> diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-
> swnode.c
> index b5a6eaf3729b..56c8519be538 100644
> --- a/drivers/gpio/gpiolib-swnode.c
> +++ b/drivers/gpio/gpiolib-swnode.c
> @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> *con_id, char *propname,
> =C2=A0		strscpy(propname, "gpios", max_size);
> =C2=A0}
> =C2=A0
> -static int swnode_gpiochip_match_name(struct gpio_chip *chip, void
> *data)
> +static struct gpio_device *swnode_get_gpio_device(struct
> fwnode_handle *fwnode)
> =C2=A0{
> -	return !strcmp(chip->label, data);
> -}
> +	const struct software_node *gdev_node;
> +	struct gpio_device *gdev;
> =C2=A0
> -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> *fwnode)
> -{
> -	const struct software_node *chip_node;
> -	struct gpio_chip *chip;
> -
> -	chip_node =3D to_software_node(fwnode);
> -	if (!chip_node || !chip_node->name)
> +	gdev_node =3D to_software_node(fwnode);
> +	if (!gdev_node || !gdev_node->name)
> =C2=A0		return ERR_PTR(-EINVAL);
> =C2=A0
> -	chip =3D gpiochip_find((void *)chip_node->name,
> swnode_gpiochip_match_name);
> -	return chip ?: ERR_PTR(-EPROBE_DEFER);
> +	gdev =3D gpio_device_find_by_label((void *)gdev_node->name);
> +	return gdev ?: ERR_PTR(-EPROBE_DEFER);
> =C2=A0}
> =C2=A0
> =C2=A0struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> =C2=A0				=C2=A0=C2=A0 const char *con_id, unsigned int
> idx,
> =C2=A0				=C2=A0=C2=A0 unsigned long *flags)
> =C2=A0{
> +	struct gpio_device *gdev __free(gpio_device_put) =3D NULL;
> =C2=A0	const struct software_node *swnode;
> =C2=A0	struct fwnode_reference_args args;
> -	struct gpio_chip *chip;
> =C2=A0	struct gpio_desc *desc;
> =C2=A0	char propname[32]; /* 32 is max size of property name */
> =C2=A0	int error;
> @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> fwnode_handle *fwnode,
> =C2=A0		return ERR_PTR(error);
> =C2=A0	}
> =C2=A0
> -	chip =3D swnode_get_chip(args.fwnode);
> +	gdev =3D swnode_get_gpio_device(args.fwnode);
> =C2=A0	fwnode_handle_put(args.fwnode);
> -	if (IS_ERR(chip))
> -		return ERR_CAST(chip);
> +	if (IS_ERR(gdev))
> +		return ERR_CAST(gdev);

I'm a bit late to the party, sorry.

I'm looking at how __free() should be used to use it in my own
patchset, and I was wondering if this code actually works.

What happens if swnode_get_gpio_device() returns an error pointer?
Won't that cause a call to gpio_device_put() with the invalid pointer?

Cheers,
-Paul

> =C2=A0
> -	desc =3D gpiochip_get_desc(chip, args.args[0]);
> +	desc =3D gpiochip_get_desc(gdev->chip, args.args[0]);
> =C2=A0	*flags =3D args.args[1]; /* We expect native GPIO flags */
> =C2=A0
> =C2=A0	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> status (%d)\n",