Received-SPF: pass (google.com: domain of linux-kernel+bounces-135095-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <5264b2cddd5d3cbba5e9d5e55f8c7069347ec38b.camel@gmail.com>
Subject: Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Andi Shyti <andi.shyti@kernel.org>, Jonathan Cameron <jic23@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Nuno Sa
 <nuno.sa@analog.com>, Petr Mladek <pmladek@suse.com>, Lars-Peter Clausen
 <lars@metafoo.de>, Olivier Moysan <olivier.moysan@foss.st.com>, Jyoti
 Bhayana <jbhayana@google.com>, Chris Down <chris@chrisdown.name>, John
 Ogness <john.ogness@linutronix.de>, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>,  linux-kernel@vger.kernel.org,
 linux-iio@vger.kernel.org
Date: Mon, 08 Apr 2024 11:05:39 +0200
In-Reply-To: <7ejh2r6zcripnptn3nbw2sa3mlfyleg4qg74rryxlut3fysneg@kuqwo5z2l65n>
References: <20240404-dev-add_dev_errp_probe-v1-0-d18e3eb7ec3f@analog.com>
	 <20240404-dev-add_dev_errp_probe-v1-3-d18e3eb7ec3f@analog.com>
	 <Zg6bsS3WMvp-nY3S@smile.fi.intel.com>
	 <9f91388faee750e16021f2c0544e7a158a16202b.camel@gmail.com>
	 <Zg7DWQtY6SP9E_1m@smile.fi.intel.com>
	 <20240406170717.31ab7e47@jic23-huawei>
	 <7ejh2r6zcripnptn3nbw2sa3mlfyleg4qg74rryxlut3fysneg@kuqwo5z2l65n>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.0 
Precedence: bulk
MIME-Version: 1.0

On Sat, 2024-04-06 at 20:54 +0200, Andi Shyti wrote:
> Hi,
>=20
> On Sat, Apr 06, 2024 at 05:07:17PM +0100, Jonathan Cameron wrote:
> > On Thu, 4 Apr 2024 18:12:25 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >=20
> > > On Thu, Apr 04, 2024 at 04:58:27PM +0200, Nuno S=C3=A1 wrote:
> > > > On Thu, 2024-04-04 at 15:23 +0300, Andy Shevchenko wrote:=C2=A0=20
> > > > > On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:=C2=A0=
=20
> > > > > > Using dev_errp_probe() to simplify the code.=C2=A0=20
> > >=20
> > > ...
> > >=20
> > > > > > +	if (IS_ERR(fwnode))
> > > > > > +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> > > > > > +				=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "Cannot get Firmware
> > > > > > reference\n");=C2=A0=20
> > > > >=20
> > > > > ERR_CAST() seems quite good candidate to have here.
> > > > >=20
> > > > > 		return dev_errp_probe(dev, fwnode, "Cannot get
> > > > > Firmware
> > > > > reference\n");
> > > > >=20
> > > > > (Assuming dev_errp_probe() magically understands that, note you m=
ay
> > > > > have it as
> > > > > =C2=A0a macro and distinguish parameter type with _Generic() or s=
o and
> > > > > behave
> > > > > =C2=A0differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid=
_match()
> > > > > =C2=A0implementation, but also keep in mind that it doesn't disti=
nguish
> > > > > NULL/0,
> > > > > there
> > > > > =C2=A0is a patch available in the mailing list to fix that, thoug=
h.)=C2=A0=20
> > > >=20
> > > > Do we care that much for going with that trouble?=C2=A0=20
> > >=20
> > > I don't think we do. We are not supposed to be called with ret =3D=3D=
 0/NULL.
> > > That's why I pointed out to the current version.
> > >=20
> > > > I understand like this we go
> > > > PTR_ERR() to then comeback to ERR_PTR() but this for probe() which =
is
> > > > not a
> > > > fastpath. So perhaps we could just keep it simple?=C2=A0=20
> > >=20
> > > It's not about performance, it's about readability. See the differenc=
e
> > > between
> > > yours and mine.
> > >=20
> >=20
> > You are suggesting making it transparently take an error ptr or an inte=
ger?
> > Whilst clever, I'm not seeing that as a good idea for readability /
> > reviewability.
> > I expect something that looks like a function to take the same paramete=
rs
> > (other vargs)
> > always.=C2=A0 _Generic messes with that.
> >=20
> > Maybe I just don't like to learn new things!=C2=A0 If consensus comes d=
own in
> > favour
> > of _Generic trickery then I'll get used to it eventually.
>=20
> the whole point of the dev_err_...() functions is to add trickery
> in order to reduce code and brackets.
>=20

I'm not sure I'm completely convinced on having more helpers but also no st=
rong
opinion tbh. But see below...

> The way I see this is to have a combination of functions:
>=20
> =C2=A0- takes integer, returns integer -> dev_err_probe()
> =C2=A0- takes integer, returns pointer -> dev_errp_probe() (or dev_err_pt=
r_probe())
> =C2=A0- takes pointer, return integer -> ? dev_ptr_err_probe()

This is pretty much all the dev_err_probe(dev, PTR_ERR(), ...) we already h=
ave
out there. Do we really want to have this variant?

> =C2=A0- takes pointer, returns pointer -> ? dev_ptr_probe()

dev_ptr_probe() misses to be clear about being an error and think this is p=
retty
much the ERR_CAST() case right? Maybe dev_err_cast_ptr_probe()? Or
dev_err_cast_probe()?

- Nuno S=C3=A1