Message-ID: <aSSV4lxzatAFds5e@lipo.home.arpa>
Date: Mon, 24 Nov 2025 19:29:06 +0200
From: Petre Rodan <petre.rodan@...dimension.ro>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver


hello Andy.

thank you for the review.

On Mon, Nov 24, 2025 at 01:48:08PM +0200, Andy Shevchenko wrote:
> On Sat, Nov 22, 2025 at 11:42:45PM +0200, Petre Rodan wrote:
[..]

> > +/*
> > + * transfer function A: 10%   to 90%   of 2^24
> 
> Too many spaces, also this may be a one-line comment.

it was intentional to have the comment multiline.
in case we need to add additional transfer functions in the future for compatible ICs the diff will be a few lines smaller.

> > + */
> > +static const struct abp2_func_spec abp2_func_spec[] = {
> > +	[ABP2_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 },
> > +};
> 
> ...
> 
> > +enum abp2_variants {
> 
> Why explicit assignments? Is it related to some HW register?

no registers, I just need to ensure the two arrays

static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
	[ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA", ..

static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
	[ABP2001BA] = { .pmin =       0, .pmax =  100000 },
   	[ABP21_6BA] = { .pmin =       0, .pmax =  160000 }, ..

keep being consistent and are resistant to later editing.

I feel like I had a better implementation two years ago when I used a single struct containing all this information and had a custom/NIH search function, but you kindly asked me [1] to use device_property_match_property_string() instead and split my single struct into this three parts mess.

[1] https://lore.kernel.org/linux-iio/ZWcUPkzfGqxYsysp@smile.fi.intel.com/

> Can be done easier with a macro with more robustness against typos:
> 
> #define ABP2_VARIANT(v)		[ABP2 ## v] = ##v
> 
> static const char * const abp2_triplet_variants[] = {
> 	ABP2_VARIANT(001BA), ABP2_VARIANT(1_6BA), ABP2_VARIANT(2_5BA), ABP2_VARIANT(004BA),
> 	...
> };
> 
> but this will loose the possibility to make '.' in the name. Up to you.

thanks, but I need '.' in the name. the dot is used in the part number (and thus in the pressure triplet).

> > +static int abp2_get_measurement(struct abp2_data *data)
> > +{
> > +	struct device *dev = data->dev;
> > +	int ret;
> > +
> > +	memset(data->buffer, 0, sizeof(data->buffer));
> > +	reinit_completion(&data->completion);
> > +
> > +	ret = data->ops->write(data, ABP2_CMD_SYNC, ABP2_PKT_SYNC_LEN);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (data->irq > 0) {
> > +		ret = wait_for_completion_timeout(&data->completion, HZ);
> 
> Where is HZ defined? Include that.
> 
> > +		if (!ret) {
> > +			dev_err(dev, "timeout waiting for EOC interrupt\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +	} else
> > +		fsleep(5000);
> 
> Better to have 5 * USEC_PER_MSEC. Also missed comment why this long delay
> is needed (will require time.h).
> 
> Missed {} as well.

I'm not sure I understand where are braces needed/not needed in this context.

> > +	ret = data->ops->read(data, ABP2_CMD_NOP, ABP2_PKT_NOP_LEN);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * status byte flags
> > +	 *  bit7 SANITY_CHK     - must always be 0
> > +	 *  bit6 ABP2_ST_POWER  - 1 if device is powered
> > +	 *  bit5 ABP2_ST_BUSY   - 1 if device has no new conversion ready
> > +	 *  bit4 SANITY_CHK     - must always be 0
> > +	 *  bit3 SANITY_CHK     - must always be 0
> > +	 *  bit2 MEMORY_ERR     - 1 if integrity test has failed
> > +	 *  bit1 SANITY_CHK     - must always be 0
> > +	 *  bit0 MATH_ERR       - 1 during internal math saturation err
> > +	 */
> > +
> > +	if (data->buffer[0] == (ABP2_ST_POWER | ABP2_ST_BUSY))
> > +		return -ETIMEDOUT;
> > +
> > +	if (data->buffer[0] != ABP2_ST_POWER) {
> > +		dev_err(data->dev,
> > +			"unexpected status byte 0x%02x\n", data->buffer[0]);
> > +		return -ETIMEDOUT;
> > +	}
> 
> I am not sure the chosen error code in both cases is good enough.

I'm open to recommendations on better error codes.

first error: chip reports it's busy 5ms after start conversion command. based on the datasheet the conversion should have been ready at this point in time. this sounds to me like a timeout error.

second error: status byte contains unexpected flags being set - either an internal error - see table above or a bus read error. yes, timeout is not good here but what should it be?

I'm using two conditionals because I want to log only invalid statuses and ignore simple 'device busy' errors.

> > +struct abp2_ops {
> > +	int (*init)(struct device *dev);
> > +	int (*read)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> > +	int (*write)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> 
> What is the meaning of const for the POD type parameters? I mean this gives
> really a little protection if any. I do not see a point here to have them being const.

I read a few books about C programming a few decades back and there was a consensus on using const in function prototypes wherever a parameter was supposed to not be changed.
of course it's not bulletproof, but why do you feel I should stop following that advice for functions that are not tied to any pre-existing kernel API?

> > +int abp2_common_probe(struct device *dev, const struct abp2_ops *ops, int irq);
> > +
> > +#endif
> 
> ...
> 
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> 
> > +static int abp2_i2c_init(struct device *dev)
> > +{
> > +	return 0;
> > +}
> 
> Is this stub required?

do I have a 100% guarantee that the kernel will not try to execute a null pointer function in abp2_common_probe()?

	ret = data->ops->init(data->dev); // needed only for SPI.

later edit:
since I will remove devm_kzalloc(), the _init will probably go away entirely together with the stub.

> > +static int abp2_i2c_read(struct abp2_data *data, const u8 unused, const u8 cnt)
> > +{
> > +	struct i2c_client *client = to_i2c_client(data->dev);
> > +	int ret;
> > +
> > +	if (cnt > ABP2_MEASUREMENT_RD_SIZE)
> > +		return -EOVERFLOW;
> > +
> > +	ret = i2c_master_recv(client, data->buffer, cnt);
> > +	if (ret < 0)
> > +		return ret;
> 
> > +	else if (ret != cnt)
> 
> Redundant 'else'.
> 
> > +		return -EIO;
> > +
> > +	return 0;
> > +}

are you implying that __i2c_transfer() errors out if the number of bytes transfered is not cnt?

> > +static const struct abp2_ops abp2_i2c_ops = {
> > +	.init = abp2_i2c_init,
> > +	.read = abp2_i2c_read,
> > +	.write = abp2_i2c_write,
> > +};
> 
> So, why can't regmap I²C be used?
[..]
> So, why can't regmap SPI be used?

there are no registers, no memory map, just a 'start conversion' and the equivalent of a 'read conversion' command.
any reason one would use the regmap API in this case?

best regards,
peter

Powered by blists - more mailing lists