Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 26 Feb 2021 21:41:11 +0900
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Ahmad Fatoum <a.fatoum@pengutronix.de>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        David Jander <david@protonic.nl>,
        Robin van der Gracht <robin@protonic.nl>,
        linux-iio@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        Jonathan Cameron <jic23@kernel.org>
Subject: Re: [PATCH v7 2/2] counter: add IRQ or GPIO based counter
Message-ID: <YDjsZ8hyETN0VpCM@shinobu>
References: <20210226090830.10927-1-o.rempel@pengutronix.de>
 <20210226090830.10927-3-o.rempel@pengutronix.de>
 <YDjDMBfWwdImiZxY@shinobu>
 <20210226121455.t7kz4cxtganzt2xz@pengutronix.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="zDZZKf6/pb+UPY5C"
Content-Disposition: inline
In-Reply-To: <20210226121455.t7kz4cxtganzt2xz@pengutronix.de>
Precedence: bulk


--zDZZKf6/pb+UPY5C
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 26, 2021 at 01:14:55PM +0100, Oleksij Rempel wrote:
> On Fri, Feb 26, 2021 at 06:45:20PM +0900, William Breathitt Gray wrote:
> > On Fri, Feb 26, 2021 at 10:08:30AM +0100, Oleksij Rempel wrote:
> > > +static int interrupt_cnt_signal_read(struct counter_device *counter,
> > > +				     struct counter_signal *signal,
> > > +				     enum counter_signal_value *val)
> > > +{
> > > +	struct interrupt_cnt_priv *priv =3D counter->priv;
> > > +	int ret;

I forgot about this function. Add a check here to return -EINVAL if
we're not dealing with a GPIO:

	if (!priv->gpio)
		return -EINVAL;

> > > +
> > > +	ret =3D gpiod_get_value(priv->gpio);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*val =3D ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct counter_ops interrupt_cnt_ops =3D {
> > > +	.action_get =3D interrupt_cnt_action_get,
> > > +	.count_read =3D interrupt_cnt_read,
> > > +	.count_write =3D interrupt_cnt_write,
> > > +	.function_get =3D interrupt_cnt_function_get,
> > > +	.signal_read  =3D interrupt_cnt_signal_read,
> > > +};
> > > +
> > > +static int interrupt_cnt_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev =3D &pdev->dev;
> > > +	struct interrupt_cnt_priv *priv;
> > > +	int ret;
> > > +
> > > +	priv =3D devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	priv->irq =3D platform_get_irq_optional(pdev,  0);
> > > +	if (priv->irq =3D=3D -ENXIO)
> > > +		priv->irq =3D 0;
> > > +	else if (priv->irq < 0)
> > > +		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
> > > +
> > > +	priv->gpio =3D devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
> > > +	if (IS_ERR(priv->gpio))
> > > +		return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get GPIO=
\n");
> > > +
> > > +	if (!priv->irq && !priv->gpio) {
> > > +		dev_err(dev, "IRQ and GPIO are not found. At least one source shou=
ld be provided\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	if (!priv->irq) {
> > > +		int irq =3D gpiod_to_irq(priv->gpio);
> > > +
> > > +		if (irq < 0)
> > > +			return dev_err_probe(dev, irq, "failed to get IRQ from GPIO\n");
> > > +
> > > +		priv->irq =3D irq;
> > > +	}
> > > +
> > > +	if (priv->gpio) {
> >=20
> > This if statement can be removed. There's no need to restrict this to
> > just GPIO because we're always dealing with an IRQ, so allocate the
> > "IRQ #" name unconditionally and set signals/num_signals.
>=20
> Your previous suggestion was to no assign signals if there is no gpios.
> What should I do?
>=20
> Regards,
> Oleksij
> --=20
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

I'm sorry for not being clear. I'm saying there is no need to
differentiate here because there will always be a respective IRQ line
whether there is a GPIO line or not. So removing the if statement is all
you need to do.

Instead of:

	if (priv->gpio) {
		priv->signals.name =3D devm_kasprintf(dev, GFP_KERNEL, "IRQ %d",
					    priv->irq);
		if (!priv->signals.name)
			return -ENOMEM;

		priv->counter.signals =3D &priv->signals;
		priv->counter.num_signals =3D 1;
	}
=09
	priv->synapses.actions_list =3D interrupt_cnt_synapse_actionss;
	priv->synapses.num_actions =3D ARRAY_SIZE(interrupt_cnt_synapse_actionss);
	priv->synapses.signal =3D &priv->signals;
	...

You can just have those lines execute unconditionally even if there are
no gpios:

	priv->signals.name =3D devm_kasprintf(dev, GFP_KERNEL, "IRQ %d",
				    priv->irq);
	if (!priv->signals.name)
		return -ENOMEM;

	priv->counter.signals =3D &priv->signals;
	priv->counter.num_signals =3D 1;
=09
	priv->synapses.actions_list =3D interrupt_cnt_synapse_actionss;
	priv->synapses.num_actions =3D ARRAY_SIZE(interrupt_cnt_synapse_actionss);
	priv->synapses.signal =3D &priv->signals;
	...

William Breathitt Gray

--zDZZKf6/pb+UPY5C
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAmA47EQACgkQhvpINdm7
VJL2OxAAw+LYMNXAWMCpkG64pnB+bM+4rXT547BrG7UkDPr7qvDHgVHY5cx+jTwR
Uaisl3EGDMLID45fIiDXkZ74WYJB16xrVGTi8/4msfoJFINgKR7AzQ90wUqSx1VF
3GOTy/kKTokzJQ41Hv1sJtGX5rh+5+bzt62nTpEuY4lc3lQAlgZk3/03IoR33zoX
eAQ0eQFxyjIdakF3HYlGodS8Vz5dSviBNmmtElvaGRgEdBFiz6fxaKMGHV15nWO/
BfhIf78TyPCq+KqhQLJb+L7Hvz4cv2bqLHxPOCkmXODBrylYd7TbmVA0JEl8prGM
OlwPazjT+5a8/yNh/CM09G2xiUUzwunB61Hiny64MWPeWRNM3M4W0ErxQqmp3sgz
k8QJJ+swppHzC/pLeZTJAEySwQk/eGGu9x3TL2r+GVpegsDkbeaiXqmFRy2+EiIx
royJZVa6/7HJ0xTrCO8gUHSMFIK4dwAUpQaPeyXaJ/zEdlgdYxy5a9TugNVyVaez
Zs2UtajmVUNHvqIVkbZUt8oQ6ZghMaHwZ2ffDMqG2249umcC7/RkmvpMH97XEDIj
MqKCer2gVLqkLAGOV++7MiWLNgjBtCvDkH8atWPCNPny6mcUHE9bfWF1h8/Ckhn3
5jU/aboI6HYWBfEwnmmMM7KE4J12jZeWZ/gzcFulV3j/JesOY2c=
=lFTA
-----END PGP SIGNATURE-----

--zDZZKf6/pb+UPY5C--