linux-kernel - Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5c142a1b-c413-66b2-86d9-b8c95ed46241@metafoo.de>
Date:   Sat, 1 Apr 2023 11:29:00 -0700
From:   Lars-Peter Clausen <lars@...afoo.de>
To:     Andreas Klinger <ak@...klinger.de>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

Hi,

Looks pretty good. Jonathan already covered most of it, a few additional 
comments.

On 4/1/23 02:10, Andreas Klinger wrote:
> [...]
> +struct mpr_data {
> +	struct device		*dev;
> +	void			*client;

Any reason not to use `struct i2c_client` for the type?

> +	struct mutex		lock;
> +	s32			pmin;
> +	s32			pmax;
> +	struct gpio_desc	*gpiod_reset;
> +	int			irq;
> +	struct completion	completion;
> +	s64			channel[2] __aligned(8);
> +};
> +
> [...]
> +static int mpr_read_pressure(struct mpr_data *data, s64 *press)
> +{
> +	int ret, i;
> +	u8 wdata[] = {0xAA, 0x00, 0x00};
> +	s32 status;
> +	int nloops = 10;
> +	char buf[5];
The tx buffer is `u8`, the rx buffer is `char`. This should be consistent.
> +	s64 press_cnt;
> +	s64 outputmin = 1677722;
> +	s64 outputmax = 15099494;
> +
> +	reinit_completion(&data->completion);
> +
> +	ret = i2c_master_send(data->client, wdata, sizeof(wdata));

The i2c family of transfer functions returns the number of bytes 
transferred, this can be less than what you expect if you get an early 
NACK. Its always good to check that all the data was transferred. E.g.

if (ret >= 0 && ret != sizeof(wdata))

    ret = -EIO;

Same for the receive later on.

[...]