Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5c142a1b-c413-66b2-86d9-b8c95ed46241@metafoo.de>
Date:   Sat, 1 Apr 2023 11:29:00 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
Content-Language: en-US
To:     Andreas Klinger <ak@it-klinger.de>, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Angel Iglesias <ang.iglesiasg@gmail.com>,
        linux-kernel@vger.kernel.org
References: <ZCf085W4XL2PtQf6@arbad>
From:   Lars-Peter Clausen <lars@metafoo.de>
In-Reply-To: <ZCf085W4XL2PtQf6@arbad>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

[...]