Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Sun, 22 May 2022 12:17:49 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
Cc:     lars@metafoo.de, robh+dt@kernel.org, tomas.melin@vaisala.com,
        andy.shevchenko@gmail.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH V7 5/5] iio: accel: sca3300: Add inclination channels
Message-ID: <20220522121749.01807970@jic23-huawei>
In-Reply-To: <20220522070419.409556-6-Qing-wu.Li@leica-geosystems.com.cn>
References: <20220522070419.409556-1-Qing-wu.Li@leica-geosystems.com.cn>
        <20220522070419.409556-6-Qing-wu.Li@leica-geosystems.com.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 22 May 2022 07:04:19 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> Different from SCA3300, SCL3300 can output inclination angles.
> Angles are formed from acceleration with following equations:
> ANG_X = atan2(accx , sqrt(pow(accy , 2) + pow(accz , 2)))
> ANG_Y = atan2(accy , sqrt(pow(accx , 2) + pow(accz , 2)))
> ANG_Z = atan2(accz , sqrt(pow(accx , 2) + pow(accy , 2)))
> 
> The commit adds the output of the raw value, scale
> and scale_available of angles.
> 
> New interfaces:
>   in_incli_scale
>   in_incli_scale_available
>   in_incli_x_raw
>   in_incli_y_raw
>   in_incli_z_raw
> Data converted by application of scale to degrees.
> 
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>

Hi Li,

A few small things to tidy up in this patch. See inline.


Thanks,

Jonathan

> ---
>  drivers/iio/accel/sca3300.c | 76 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index 10bedb14d4f0..9094f16458de 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -44,12 +44,18 @@
>  #define SCA3300_VALUE_RS_ERROR	0x3
>  #define SCA3300_MASK_RS_STATUS	GENMASK(1, 0)
>  
> +#define SCL3300_REG_ANG_CTRL 0x0C
> +#define SCL3300_ANG_ENABLE   0x1F
> +
>  enum sca3300_scan_indexes {
>  	SCA3300_ACC_X = 0,
>  	SCA3300_ACC_Y,
>  	SCA3300_ACC_Z,
>  	SCA3300_TEMP,
>  	SCA3300_TIMESTAMP,
> +	SCA3300_INCLI_X,
> +	SCA3300_INCLI_Y,
> +	SCA3300_INCLI_Z,
>  };
>  
>  #define SCA3300_ACCEL_CHANNEL(index, reg, axis) {			\
> @@ -73,6 +79,24 @@ enum sca3300_scan_indexes {
>  	},								\
>  }
>  
> +#define SCA3300_INCLI_CHANNEL(index, reg, axis) {			\
> +	.type = IIO_INCLI,						\
> +	.address = reg,							\
> +	.modified = 1,							\
> +	.channel2 = IIO_MOD_##axis,					\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type_available =				\
> +	BIT(IIO_CHAN_INFO_SCALE),					\

I don't think you need this line wrap.

> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = 16,						\
> +		.storagebits = 16,					\
> +		.endianness = IIO_CPU,					\
> +	},								\
> +}
> +
>  #define SCA3300_TEMP_CHANNEL(index, reg) {				\
>  		.type = IIO_TEMP,					\
>  		.address = reg,						\
> @@ -106,15 +130,36 @@ static const int sca3300_accel_scale_map[] = {0, 1, 2, 2};
>  static const int scl3300_accel_scale[][2] = {{0, 167}, {0, 333}, {0, 83}};
>  static const int scl3300_accel_scale_map[] = {0, 1, 2};
>  
> +static const int scl3300_incli_scale[][2] = {{0, 5495}};
> +static const int scl3300_incli_scale_map[] = {0, 0, 0};
> +
>  static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
>  static const int scl3300_avail_modes_map[] = {0, 1, 3};
>  
> +static const struct iio_chan_spec scl3300_channels[] = {
> +	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X),
> +	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
> +	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
> +	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
> +};
> +
>  static const unsigned long sca3300_scan_masks[] = {
>  	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
>  	BIT(SCA3300_TEMP),
>  	0
>  };
>  
> +static const unsigned long scl3300_scan_masks[] = {
> +	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
> +	BIT(SCA3300_TEMP) |
> +	BIT(SCA3300_INCLI_X) | BIT(SCA3300_INCLI_Y) | BIT(SCA3300_INCLI_Z),
> +	0
> +};
> +
>  struct sca3300_chip_info {
>  	const char *name;
>  	const unsigned long *scan_masks;
> @@ -123,6 +168,9 @@ struct sca3300_chip_info {
>  	u8 num_accel_scales;
>  	const int (*accel_scale)[2];
>  	const int *accel_scale_map;
> +	const int (*incli_scale)[2];
> +	const int *incli_scale_map;
> +	u8 num_incli_scales;
>  	u8 num_freqs;
>  	const int *freq_table;
>  	const int *freq_map;
> @@ -131,6 +179,7 @@ struct sca3300_chip_info {
>  	u8 chip_id;
>  };
>  
> +
Unrelated and unwanted white space chagne.

>  /**
>   * struct sca3300_data - device data
>   * @spi: SPI device structure
> @@ -168,12 +217,16 @@ static const struct sca3300_chip_info sca3300_chip_tbl[] = {
>  		.num_avail_modes = 4,
>  		.chip_id = SCA3300_WHOAMI_ID,
>  	},
> -	{	.scan_masks = sca3300_scan_masks,
> -		.channels = sca3300_channels,
> -		.num_channels = ARRAY_SIZE(sca3300_channels),
> +	{

ah. The whitespace change belongs in patch 4.

> +		.scan_masks = scl3300_scan_masks,
> +		.channels = scl3300_channels,
> +		.num_channels = ARRAY_SIZE(scl3300_channels),
>  		.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale)*2,
>  		.accel_scale = scl3300_accel_scale,
>  		.accel_scale_map = scl3300_accel_scale_map,
> +		.incli_scale = scl3300_incli_scale,
> +		.incli_scale_map = scl3300_incli_scale_map,
> +		.num_incli_scales =  ARRAY_SIZE(scl3300_incli_scale)*2,
>  		.num_freqs = ARRAY_SIZE(scl3300_lp_freq),
>  		.freq_table = scl3300_lp_freq,
>  		.freq_map = scl3300_lp_freq_map,
> @@ -400,6 +453,11 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  		switch (chan->type) {
> +		case IIO_INCLI:
> +			index = data->chip->incli_scale_map[index];
> +			*val  = data->chip->incli_scale[index][0];
> +			*val2 = data->chip->incli_scale[index][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
>  		case IIO_ACCEL:
>  			index = data->chip->accel_scale_map[index];
>  			*val  = data->chip->accel_scale[index][0];
> @@ -486,6 +544,13 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  
>  	sca_data->chip = &sca3300_chip_tbl[i];
>  
> +	if (value == SCL3300_WHOAMI_ID) {

I don't like feature decisions being based directly on a WHOAMI.
That tends not to scale as you add more supported parts to the driver.

I think this is the only case you now have.

Better would be to add a 'inclination_supported' bool to your chip info
and make the decision based on that.  I wouldn't use whether incli_scale
is set as that would hurt readability when compared to a new member
serving just this purpose.


> +		ret = sca3300_write_reg(sca_data, SCL3300_REG_ANG_CTRL,
> +					SCL3300_ANG_ENABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -521,6 +586,11 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
> +		case IIO_INCLI:
> +			*vals = (const int *)data->chip->incli_scale;
> +			*length = data->chip->num_incli_scales;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
>  		case IIO_ACCEL:
>  			*vals = (const int *)data->chip->accel_scale;
>  			*length = data->chip->num_accel_scales;