Re: [PATCH v3 2/5] iio: pressure: ms5637: Move device data struct to header

[Jonathan Cameron]

On Thu, 20 Aug 2026 10:12:17 -0400
Louis Adamian <adamianlouis@xxxxxxxxx> wrote:

> ms_tp_dev duplicated the hw pointer already in ms_tp_data. This stores a
> pointer to ms_tp_data instead which requires moving the struct
> definition from ms5637.c to ms_sensors_i2c.h.

Why is this a good thing to do?  Pointing to only the data
we happen to need in the structure seems fine to me.  Is it
that you are going to add something else in there in a later patch?

If so, mention that because as it stands this patch doesn't have
a strong justification.

Jonathan


> 
> No functional change intended
> 
> Signed-off-by: Louis Adamian <adamianlouis@xxxxxxxxx>
> ---
>  drivers/iio/common/ms_sensors/ms_sensors_i2c.c |  4 ++--
>  drivers/iio/common/ms_sensors/ms_sensors_i2c.h | 13 ++++++++++++-
>  drivers/iio/pressure/ms5637.c                  |  9 ++-------
>  3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
> index 1960a2ce82a8..f9dc7c7468c1 100644
> --- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
> +++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
> @@ -579,7 +579,7 @@ int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
>  	int i, ret;
>  	bool valid;
>  
> -	for (i = 0; i < dev_data->hw->prom_len; i++) {
> +	for (i = 0; i < dev_data->data->hw->prom_len; i++) {
>  		ret = ms_sensors_read_prom_word(
>  			dev_data->client,
>  			MS_SENSORS_TP_PROM_READ + (i << 1),
> @@ -589,7 +589,7 @@ int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
>  			return ret;
>  	}
>  
> -	if (dev_data->hw->prom_len == 8)
> +	if (dev_data->data->hw->prom_len == 8)
>  		valid = ms_sensors_tp_crc_valid_128(dev_data->prom);
>  	else
>  		valid = ms_sensors_tp_crc_valid_112(dev_data->prom);
> diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
> index f15b973f27c6..d9898098c066 100644
> --- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
> +++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
> @@ -35,6 +35,16 @@ struct ms_tp_hw_data {
>  	u8 max_res_index;
>  };
>  
> +/**
> + * struct ms_tp_data - Temperature/Pressure sensor data
> + * @name: Device name
> + * @hw: Sensor hardware data
> + */
> +struct ms_tp_data {
> +	const char *name;
> +	const struct ms_tp_hw_data *hw;
> +};
> +
>  /**
>   * struct ms_tp_dev - Temperature/Pressure sensor device structure
>   * @client:	i2c client
> @@ -42,11 +52,12 @@ struct ms_tp_hw_data {
>   * @prom:	array of PROM coefficients used for conversion. Added element
>   *              for CRC computation
>   * @res_index:	index to selected sensor resolution
> + * @data:	Temperature/Pressure sensor data
>   */
>  struct ms_tp_dev {
>  	struct i2c_client *client;
>  	struct mutex lock;
> -	const struct ms_tp_hw_data *hw;
> +	const struct ms_tp_data *data;
>  	u16 prom[MS_SENSORS_TP_PROM_WORDS_NB];
>  	u8 res_index;
>  };
> diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
> index 4f9f556bd123..6009d87f3d4c 100644
> --- a/drivers/iio/pressure/ms5637.c
> +++ b/drivers/iio/pressure/ms5637.c
> @@ -29,11 +29,6 @@
>  
>  #include "../common/ms_sensors/ms_sensors_i2c.h"
>  
> -struct ms_tp_data {
> -	const char *name;
> -	const struct ms_tp_hw_data *hw;
> -};
> -
>  static const int ms5637_samp_freq[6] = { 960, 480, 240, 120, 60, 30 };
>  
>  static ssize_t ms5637_show_samp_freq(struct device *dev, struct device_attribute *attr, char *buf)
> @@ -42,7 +37,7 @@ static ssize_t ms5637_show_samp_freq(struct device *dev, struct device_attribute
>  	struct ms_tp_dev *dev_data = iio_priv(indio_dev);
>  	int i, len = 0;
>  
> -	for (i = 0; i <= dev_data->hw->max_res_index; i++)
> +	for (i = 0; i <= dev_data->data->hw->max_res_index; i++)
>  		len += sysfs_emit_at(buf, len, "%u ", ms5637_samp_freq[i]);
>  	sysfs_emit_at(buf, len - 1, "\n");
>  
> @@ -168,7 +163,7 @@ static int ms5637_probe(struct i2c_client *client)
>  	dev_data = iio_priv(indio_dev);
>  	dev_data->client = client;
>  	dev_data->res_index = data->hw->max_res_index;
> -	dev_data->hw = data->hw;
> +	dev_data->data = data;
>  	mutex_init(&dev_data->lock);
>  
>  	indio_dev->info = &ms5637_info;