Received-SPF: pass (google.com: domain of linux-kernel+bounces-14284-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <03f469ca-c5f4-4255-90f4-6715a1455e0d@gmail.com>
Date: Tue, 2 Jan 2024 12:21:21 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] io: light: as73211: add support for as7331
To: Jonathan Cameron <jic23@kernel.org>
Cc: Christian Eggers <ceggers@arri.de>, Lars-Peter Clausen <lars@metafoo.de>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231220-as7331-v1-0-745b73c27703@gmail.com>
 <20231220-as7331-v1-2-745b73c27703@gmail.com>
 <20231226161414.47d5171e@jic23-huawei>
Content-Language: en-US
From: Javier Carrasco <javier.carrasco.cruz@gmail.com>
In-Reply-To: <20231226161414.47d5171e@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 26.12.23 17:14, Jonathan Cameron wrote:
>> Add a new device-specific data structure to account for the device
>> differences: channel types and scale of LSB per channel.
> A may not be worth doing it in this case, but usual approach to refactoring
> a driver to allow support of additional devices is to do it in two steps.
> 1) Refactor with no new support - so should be no operational changes.
> 2) Add the new device support.
> 
I considered that in the first place, but the "refactoring" was so
simple that the modification was just adding a pointer to an empty
struct (you don't know what is chip-specific until you have another
chip) and the patch alone had no real value (otherwise it could be
applied to all drivers that only support one device, just in case).

As you said, it may not be worth it in this case, but thank you for the
clarification.

>> +static int as73211_intensity_scale(struct as73211_data *data, int chan, int *val, int *val2)
>> +{
>> +	unsigned int scale;
>> +
>> +	switch (chan) {
>> +	case IIO_MOD_X:
>> +		scale = AS73211_SCALE_X;
>> +		break;
>> +	case IIO_MOD_Y:
>> +		scale = AS73211_SCALE_Y;
>> +		break;
>> +	case IIO_MOD_Z:
>> +		scale = AS73211_SCALE_Z;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	scale /= as73211_gain(data);
>> +	scale /= as73211_integration_time_1024cyc(data);
>> +	*val = scale;
>> +
>> +	return IIO_VAL_INT;
> 
> Obviously it's really a question about the original code but why not
> use IIO_VAL_FRACTIONAL here as well as below? Superficially looks
> like it should work in a similar fashion.
> 
> If not, perhaps a comment here somewhere?
> 
You are right, the use of IIO_VAL_INT comes from the original
implementation. I did not modify that because the expected precision
(according to the datasheet is 3 decimal places) is guaranteed with the
use of nW/m^2 instead of nW/cm^2 (the units used in the datasheet).

I think the best approach would have been using IIO_VAL_FRACTIONAL and
the units provided in the datasheet, but changing units now could cause
problems to current users. We could still use IIO_VAL_FRACTIONAL unless
that might affect current users in any way. Otherwise I will add a
comment as suggested.

>> @@ -355,30 +444,12 @@ static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
>>  			*val2 = AS73211_SCALE_TEMP_MICRO;
>>  			return IIO_VAL_INT_PLUS_MICRO;
>>  
>> -		case IIO_INTENSITY: {
>> -			unsigned int scale;
>> -
>> -			switch (chan->channel2) {
>> -			case IIO_MOD_X:
>> -				scale = AS73211_SCALE_X;
>> -				break;
>> -			case IIO_MOD_Y:
>> -				scale = AS73211_SCALE_Y;
>> -				break;
>> -			case IIO_MOD_Z:
>> -				scale = AS73211_SCALE_Z;
>> -				break;
>> -			default:
>> -				return -EINVAL;
>> -			}
>> -			scale /= as73211_gain(data);
>> -			scale /= as73211_integration_time_1024cyc(data);
>> -			*val = scale;
>> -			return IIO_VAL_INT;
>> +		case IIO_INTENSITY:
>> +			return data->spec_dev->intensity_scale(data, chan->channel2, val, val2);
> Where it doesn't hurt readability, I'd prefer we stayed as close to 80 chars or below
> as reasonably possible.  So here wrap so val, val2); is on the next line.
> 
In order to meet the 80-char rule, three lines will be required
(wrapping val, val2 is not enough; chan->channel2 must have its own
line). It looks a bit weird, but I have nothing against it.

On the other hand, the original code did not always follow the 80-char
rule (up to 99 chars per line are used), so using two lines with a first
one of 84 chars could be an option.

>> +	if (dev_fwnode(dev))
>> +		data->spec_dev = device_get_match_data(dev);
>> +	else
>> +		data->spec_dev = i2c_get_match_data(client);
> 
> Take a look at how i2c_get_match_data() is defined...
> https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L117
> and in particular what it calls first..
> 
Oops! I missed that one. I will simplify the code to a simple call to
i2c_get_match_data() and error check:

        data->spec_dev = i2c_get_match_data(client);

        if (!data->spec_dev)

                return -EINVAL;

> 
Thanks for your review and best regards,
Javier Carrasco