Received-SPF: pass (google.com: domain of linux-kernel+bounces-11582-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 26 Dec 2023 16:39:11 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Petre Rodan <petre.rodan@subdimension.ro>
Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Andreas Klinger
 <ak@it-klinger.de>, Lars-Peter Clausen <lars@metafoo.de>, Andy Shevchenko
 <andriy.shevchenko@linux.intel.com>, Angel Iglesias
 <ang.iglesiasg@gmail.com>, Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v2 06/10] iio: pressure: mprls0025pa.c remove dangerous
 defaults
Message-ID: <20231226163911.35a4dba2@jic23-huawei>
In-Reply-To: <20231224143500.10940-7-petre.rodan@subdimension.ro>
References: <20231224143500.10940-1-petre.rodan@subdimension.ro>
	<20231224143500.10940-7-petre.rodan@subdimension.ro>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 24 Dec 2023 16:34:51 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> This driver supports 32*3 combinations of fixed ranges and transfer
> functions, plus custom ranges.
> 
> So statistically a user has more than 99% chance that the provided
> default configuration will generate invalid pressure readings if the
> bindings are not initialized and the driver is instantiated via sysfs.

I guess 99% is strong enough that it's unlikely we will break anyone so
fair enough to drop this attempt to guess the values.

> 
> The current patch removes this loophole making sure the driver loads
> only if the dt has been initialized.

Drop the reference to DT. It's correctly using generic firmware accessors
so should work fine with ACPI PRP0001 for example as well as DT.

Usually we just refer to the need for firmware properties.
There are lots of places they could be coming from.

> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  drivers/iio/pressure/mprls0025pa.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index 233cc1dc38ad..63c46592956f 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -375,11 +375,8 @@ static int mpr_probe(struct i2c_client *client)
>  				"honeywell,transfer-function %d invalid\n",
>  								data->function);
>  	} else {

I'd prefer the condition flipped even though patch will be more noisy.

	if (!dev_fwnode(dev))
		return dev_err_probe()...

	...

As end result will be more readable.

> -		/* when loaded as i2c device we need to use default values */
> -		dev_notice(dev, "firmware node not found; using defaults\n");
> -		data->pmin = 0;
> -		data->pmax = 172369; /* 25 psi */
> -		data->function = MPR_FUNCTION_A;
> +		return dev_err_probe(dev, -EINVAL,
> +				  "driver needs to be initialized in the dt\n");
>  	}
> 
>  	data->outmin = mpr_func_spec[data->function].output_min;
> --
> 2.41.0
>