Received-SPF: pass (google.com: domain of linux-kernel+bounces-25865-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <94987a74-0ae2-8da5-735c-92c4da1fed09@axis.com>
Date: Mon, 15 Jan 2024 11:12:17 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v2] iio: light: vcnl4000: Set ps high definition for
 4040/4200
Content-Language: en-US
To: Jonathan Cameron <jic23@kernel.org>,
 =?UTF-8?Q?M=c3=a5rten_Lindahl?= <marten.lindahl@axis.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org, kernel@axis.com
References: <20231221-vcnl4000-ps-hd-v2-1-1b6790d30ae6@axis.com>
 <20240113150738.0de92b7f@jic23-huawei>
From: =?UTF-8?Q?M=c3=a5rten_Lindahl?= <martenli@axis.com>
In-Reply-To: <20240113150738.0de92b7f@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?T2hRLzNIMGJzcjJ4NnJZYmQ0eFcwQWJrSnRDcWZHOC92aFlJdzFFbDVHakxQ?=
 =?utf-8?B?VXYrajdDcWRvaGo4ZjhMYzBHRzdkT0pUQWJyckxTNmVHTGF6NzUxQzEzMFJk?=
 =?utf-8?B?bWp2T1I1VTR6djlNSkJoaXlIeWFvSzhtc0tZdEFaamlGQWFzbkJUL25jakk3?=
 =?utf-8?B?bVc1dUp3dmFsVzlraUlSQnhlc25nYlRkbFppcHluT1VoOWRUZUJ3b0xvL1Vr?=
 =?utf-8?B?V2U2Yks0L2ZJYXJkckJDR2cwMEpkcHdCV041KzZMdGJLQmxoRkhFZFZzdmhx?=
 =?utf-8?B?TjlaOUp0U21La25uVDErVmRaSkM5VkFqQjJ3NS9uZTJNZDFVdXJwT3RDWGJN?=
 =?utf-8?B?K0Z4NE5HVFdLaGhNSXlBWEpzYmJsK0k3SzJKaEJxbm1TN2N0WUg5M3N2RnRM?=
 =?utf-8?B?SXB2cDl3dmdhOTgwamgvbXhXbmNjNENZYkFNdVdJb3dHTmxwdGlYdnh6eW5Z?=
 =?utf-8?B?NlNmVURFcUYrajMxTWYwb0NsM1JJallMTGd1QXora2dQNXZlN2tIb0g5L210?=
 =?utf-8?B?aU8wUFlFSVRRUVRFV1ZTa1NHQ01HYnpRUC95K1RJOTl2ZFdqd2tJSE9TbFlQ?=
 =?utf-8?B?SGRnYlEvTTJQVzY3bnJRSVhEUG85VUt2ZkdMaXRSN0c2LzZEZmFOam56Qy9p?=
 =?utf-8?B?UnIzK1ZDM3ZrR1NJdENiWGtuRHlBVEpKdkdxQm50YVZianREWnRzdXlwZkxR?=
 =?utf-8?B?ODdHcVpyUkkxNkdaKys0QXA2ZUdJSm5XYUFIVzVFcW53QWd6NVZhK2RydTFo?=
 =?utf-8?B?UVlJQ2M5QllNRnpIbER5VnB3WEVlRzVjTTF5bHl5OEJGR3RRaFF5NjhRTVBQ?=
 =?utf-8?B?cDZCajkxM2VzNm9uOHRNaFBYbkg3cUltTzhVNFMyc0pFNkNta3dYeHlMenVh?=
 =?utf-8?B?Y1U1MFBna25aNmZ3Rm4vNW5DNDQ1RndHUG5UWTcrNGRqd1RGYVYydkIveHdB?=
 =?utf-8?B?eTdzSUpES25wbi9Zc244R1hHMUQyTjZGV0h2TU82WGlKbHUyeC9oUFAyendL?=
 =?utf-8?B?cWRmeDdYc2J3SThjczA5QkFEb0FQTlBXcjU1OFllcHViclQ4NFBVM3RPUzVQ?=
 =?utf-8?B?TlBZblF4Uzd3YVdWL0FwWXREZVpkYUZnenlKZHdYT0p5eDlCYkpZVi9LV1pP?=
 =?utf-8?B?cCtCNFMrQnRIYkNDWWx3OUpwWXp0Y04yVDNnaUMzdVNjVm4yZ2ttVUVvWFlV?=
 =?utf-8?B?a0xJMS9XTnVaOG9ZaEFpbFUydzRUWkt0N2lyUHdUckpyUFgyTnRKTG5NRUN3?=
 =?utf-8?B?VmFzbHhlL205TDNSZ3ZMV1QyZWhKdXZhdXJFNS9HdTYvRUFZa1VaNVV1SFpa?=
 =?utf-8?B?R2l0d01KVUlOUytZMUxZWnlCT3JTL08yZUlFNHFEaTdmSEZyKzNnMkdKMGlG?=
 =?utf-8?B?RlY5VVRITGx3NStyL1k0SGhHTXZjaXpvV05BU0RpalhnUUNYZWE1Y3Z6aE85?=
 =?utf-8?B?SlVFYUdqWHRSZGJpcnEwc0xOZnFPa3NVT1ZBSittaW5ueWtEZEdLWlhQT0M4?=
 =?utf-8?B?ZnpSeFNsRVcyZENIUUtWN1gxby92S1ZKbi9ueE5CUnIzS1A0VXR2ZHRmcTlj?=
 =?utf-8?B?SkVJNVNBU3NNWVpsOWg5UUZrcjZaNm5jcDVYKyszcWlwT1ljdFF3RWgzeW9Y?=
 =?utf-8?B?eVNSbExBd0owa0tHQnVrdWhsZzUxNmNrbkNQYnR4V2JkNC9wWVRqWC9wUlIx?=
 =?utf-8?B?UGZ1dVNsK0lBVnd4eklPLys3WWRERXdkUlBMLzhjSmsxUE5SaUVQckRZWk9M?=
 =?utf-8?B?bDlYVnNyVFR1VExtd01jaFc3cm9HWDRQRzRLSXFhNWNEam9SWmxJWTRnQnRo?=
 =?utf-8?B?MGtnUE1xQkNpdHNTVVpoTG1nTnRoUGkremNaMHpxUERqTE5TNm5lL2FDaElX?=
 =?utf-8?B?NjJrZzhkelhkdmk4Vm52R25NWHpWVWxLY0N2RElNVXByL0FHTnZXQ0Q5My9L?=
 =?utf-8?B?Y2ozdzJCU2QrZVVqS0k3SkpORm41KzdIeUE5eG5BWDlaaWpURlA2Y0pEUGV2?=
 =?utf-8?B?Q3lRQ0gweHhlWDhDRTJzdkhOR3Y4Rmd3TlNESXMyL1hCWGZPaUN2QUVraWZL?=
 =?utf-8?B?VFVML0JQeEZhcE9UN2dTdVFSSnY1clVDRFJySmhpdW5NWVJiL29GT0N6dDg4?=
 =?utf-8?Q?5p+Itcjz55y95xgsy3fpTqmgT?=
X-OriginatorOrg: axis.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 79c927c0-8b2d-422d-9dd9-08dc15b274e0
X-MS-Exchange-CrossTenant-AuthSource: AS8PR02MB9529.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jan 2024 10:12:19.0638
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 78703d3c-b907-432f-b066-88f7af9ca3af
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hEuUvSbsTuFxodziinqLCWYw5cdtmYl/HR1UlVyZhLofQbO9jE6YV90wpABXlKWSKfIWZfS4rCD2CJwzMv0YjA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR02MB8415

On 1/13/24 16:07, Jonathan Cameron wrote:
> On Wed, 10 Jan 2024 13:58:07 +0100
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
>> The vcnl4040/vcnl4200 proximity sensor defaults to 12 bit data
>> resolution, but the chip also supports 16 bit data resolution, which is
>> called proximity high definition (PS_HD).
>>
>> Make the vcnl4040/vcnl4200 proximity sensor use the high definition for
>> all data readings. Please note that in order to preserve the 12 bit
>> integer part of the in_proximity_raw output, the format is changed from
>> integer to fixed point.
>>
>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> Hi Mårten,
>
> I don't understand why we'd try to carry on with using the device if
> the additional register accesses fail.  Just fail the probe, the hardware
> is broken.
>
> Jonathan

Hi Jonathan!

Fair point. I'll fail the probe if the register accesses fail. It's 
unlikely that the device would work if these fail.

Kind regards

Mårten

>
>> ---
>> Changes in v2:
>> - Update registers for sample rate to align it with 16 bit sensor readings.
>> - Change sysfs output from IIO_VAL_INT to IIO_VAL_FRACTIONAL and scale by 16.
>> - Link to v1: https://lore.kernel.org/r/20231221-vcnl4000-ps-hd-v1-1-a024bfb28896@axis.com
>> ---
>>   drivers/iio/light/vcnl4000.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
>> index fdf763a04b0b..a078c573131e 100644
>> --- a/drivers/iio/light/vcnl4000.c
>> +++ b/drivers/iio/light/vcnl4000.c
>> @@ -90,6 +90,7 @@
>>   #define VCNL4040_PS_CONF1_PS_SHUTDOWN	BIT(0)
>>   #define VCNL4040_PS_CONF2_PS_IT	GENMASK(3, 1) /* Proximity integration time */
>>   #define VCNL4040_CONF1_PS_PERS	GENMASK(5, 4) /* Proximity interrupt persistence setting */
>> +#define VCNL4040_PS_CONF2_PS_HD		BIT(11)	/* Proximity high definition */
>>   #define VCNL4040_PS_CONF2_PS_INT	GENMASK(9, 8) /* Proximity interrupt mode */
>>   #define VCNL4040_PS_CONF3_MPS		GENMASK(6, 5) /* Proximity multi pulse number */
>>   #define VCNL4040_PS_MS_LED_I		GENMASK(10, 8) /* Proximity current */
>> @@ -114,6 +115,13 @@
>>   #define VCNL4010_INT_DRDY \
>>   	(BIT(VCNL4010_INT_PROXIMITY) | BIT(VCNL4010_INT_ALS))
>>   
>> +#define VCNL4040_CONF3_PS_MPS_16BITS	3	/* 8 multi pulses */
>> +#define VCNL4040_CONF3_PS_LED_I_16BITS	3	/* 120 mA */
>> +
>> +#define VCNL4040_CONF3_PS_SAMPLE_16BITS \
>> +	(FIELD_PREP(VCNL4040_PS_CONF3_MPS, VCNL4040_CONF3_PS_MPS_16BITS) | \
>> +	 FIELD_PREP(VCNL4040_PS_MS_LED_I, VCNL4040_CONF3_PS_LED_I_16BITS))
>> +
>>   static const int vcnl4010_prox_sampling_frequency[][2] = {
>>   	{1, 950000},
>>   	{3, 906250},
>> @@ -195,6 +203,7 @@ struct vcnl4000_data {
>>   	enum vcnl4000_device_ids id;
>>   	int rev;
>>   	int al_scale;
>> +	int ps_scale;
>>   	u8 ps_int;		/* proximity interrupt mode */
>>   	u8 als_int;		/* ambient light interrupt mode*/
>>   	const struct vcnl4000_chip_spec *chip_spec;
>> @@ -345,6 +354,7 @@ static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on)
>>   static int vcnl4200_init(struct vcnl4000_data *data)
>>   {
>>   	int ret, id;
>> +	u16 regval;
>>   
>>   	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
>>   	if (ret < 0)
>> @@ -386,9 +396,36 @@ static int vcnl4200_init(struct vcnl4000_data *data)
>>   		break;
>>   	}
>>   	data->al_scale = data->chip_spec->ulux_step;
>> +	data->ps_scale = 16;
>>   	mutex_init(&data->vcnl4200_al.lock);
>>   	mutex_init(&data->vcnl4200_ps.lock);
>>   
>> +	/* Use 16 bits proximity sensor readings */
>> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
>> +	if (ret >= 0) {
>> +		regval = (ret | VCNL4040_PS_CONF2_PS_HD);
> Trivial but those brackets are unnecessary.
>
>> +		ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
>> +						regval);
>> +	}
>> +
>> +	if (ret < 0) {
>> +		dev_info(&data->client->dev, "Default to 12 bits sensor data");
> Silly question - how can we get here?  If it's a case of a read or a write failing
> then that's a critical failure and we have very little idea what the state is.
> If that happens I'd just fail to probe the driver rather than carrying on with
> 12 bit assumed.
>
>> +		data->ps_scale = 1;
>> +		goto out;
>> +	}
>> +
>> +	/* Align proximity sensor sample rate to 16 bits data width */
>> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF3);
>> +	if (ret >= 0) {
> If the read fails, something is very wrong. Fail the probe.
>
>> +		regval = (ret | VCNL4040_CONF3_PS_SAMPLE_16BITS);
>> +		ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF3,
>> +						regval);
>> +	}
>> +
>> +	if (ret < 0)
>> +		dev_warn(&data->client->dev, "Sample rate may not be accurate");
> If this fails, fail the probe.  Don't try to carry on anyway.
>
>
>> +
>> +out:
>>   	ret = data->chip_spec->set_power_state(data, true);
>>   	if (ret < 0)
>>   		return ret;
>> @@ -901,8 +938,9 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
>>   			break;
>>   		case IIO_PROXIMITY:
>>   			ret = data->chip_spec->measure_proximity(data, val);
>> +			*val2 = data->ps_scale;
>>   			if (!ret)
>> -				ret = IIO_VAL_INT;
>> +				ret = IIO_VAL_FRACTIONAL;
>>   			break;
>>   		default:
>>   			ret = -EINVAL;
>>
>> ---
>> base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
>> change-id: 20231221-vcnl4000-ps-hd-863f4f8fcea7
>>
>> Best regards,