RE: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties

[Sa, Nuno]

Hi Andy,

Good that we waited to test this patch. The fundamental logic change
for fetching and writing the custom tables are fine. That said, there
some issues that I had to fix to test the patch. See below...

> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Thursday, February 10, 2022 2:55 PM
> To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>
> Subject: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device
> properties
> 
> [External]
> 
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
> 
> The conversion slightly changes the logic behind property reading for
> the configuration values. Original code allocates just as much memory
> as needed. Then for each separate 32- or 64-bit value it reads it from
> the property and converts to a raw one which will be fed to the sensor.
> In the new code we allocate the amount of memory needed to
> retrieve all
> values at once from the property and then convert them as required.
> 
> Signed-off-by: Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> v3: no changes
>  drivers/iio/temperature/ltc2983.c | 203 +++++++++++++++-------------
> --
>  1 file changed, 102 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c
> b/drivers/iio/temperature/ltc2983.c
> index 636bbc9011b9..d20f957ca0d9 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -12,11 +12,15 @@
>  #include <linux/iio/iio.h>
>  #include <linux/interrupt.h>
>  #include <linux/list.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> -#include <linux/of_gpio.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/spi/spi.h>
> 
> +#include <asm/byteorder.h>
> +#include <asm/unaligned.h>
> +
>  /* register map */
>  #define LTC2983_STATUS_REG			0x0000
>  #define LTC2983_TEMP_RES_START_REG		0x0010
> @@ -219,7 +223,7 @@ struct ltc2983_sensor {
> 
>  struct ltc2983_custom_sensor {
>  	/* raw table sensor data */
> -	u8 *table;
> +	void *table;
>  	size_t size;
>  	/* address offset */
>  	s8 offset;
> @@ -377,25 +381,25 @@ static int
> __ltc2983_chan_custom_sensor_assign(struct ltc2983_data *st,
>  	return regmap_bulk_write(st->regmap, reg, custom->table,
> custom->size);
>  }
> 
> -static struct ltc2983_custom_sensor
> *__ltc2983_custom_sensor_new(
> -						struct ltc2983_data *st,
> -						const struct
> device_node *np,
> -						const char *propname,
> -						const bool is_steinhart,
> -						const u32 resolution,
> -						const bool has_signed)
> +static struct ltc2983_custom_sensor *
> +__ltc2983_custom_sensor_new(struct ltc2983_data *st, const struct
> fwnode_handle *fn,
> +			    const char *propname, const bool
> is_steinhart,
> +			    const u32 resolution, const bool has_signed)
>  {
>  	struct ltc2983_custom_sensor *new_custom;
> -	u8 index, n_entries, tbl = 0;
>  	struct device *dev = &st->spi->dev;
>  	/*
>  	 * For custom steinhart, the full u32 is taken. For all the others
>  	 * the MSB is discarded.
>  	 */
>  	const u8 n_size = is_steinhart ? 4 : 3;
> -	const u8 e_size = is_steinhart ? sizeof(u32) : sizeof(u64);
> +	u8 index, n_entries;
> +	int ret;
> 
> -	n_entries = of_property_count_elems_of_size(np, propname,
> e_size);
> +	if (is_steinhart)
> +		n_entries = fwnode_property_count_u32(fn,
> propname);
> +	else
> +		n_entries = fwnode_property_count_u64(fn,
> propname);
>  	/* n_entries must be an even number */
>  	if (!n_entries || (n_entries % 2) != 0) {
>  		dev_err(dev, "Number of entries either 0 or not
> even\n");
> @@ -423,21 +427,33 @@ static struct ltc2983_custom_sensor
> *__ltc2983_custom_sensor_new(
>  	}
> 
>  	/* allocate the table */
> -	new_custom->table = devm_kzalloc(dev, new_custom->size,
> GFP_KERNEL);
> +	if (is_steinhart)
> +		new_custom->table = devm_kcalloc(dev, n_entries,
> sizeof(u32), GFP_KERNEL);
> +	else
> +		new_custom->table = devm_kcalloc(dev, n_entries,
> sizeof(u64), GFP_KERNEL);
>  	if (!new_custom->table)
>  		return ERR_PTR(-ENOMEM);
> 
> -	for (index = 0; index < n_entries; index++) {
> -		u64 temp = 0, j;
> -		/*
> -		 * Steinhart sensors are configured with raw values in
> the
> -		 * devicetree. For the other sensors we must convert
> the
> -		 * value to raw. The odd index's correspond to
> temperarures
> -		 * and always have 1/1024 of resolution. Temperatures
> also
> -		 * come in kelvin, so signed values is not possible
> -		 */
> -		if (!is_steinhart) {
> -			of_property_read_u64_index(np, propname,
> index, &temp);
> +	/*
> +	 * Steinhart sensors are configured with raw values in the
> firmware
> +	 * node. For the other sensors we must convert the value to
> raw.
> +	 * The odd index's correspond to temperatures and always
> have 1/1024
> +	 * of resolution. Temperatures also come in Kelvin, so signed
> values
> +	 * are not possible.
> +	 */
> +	if (is_steinhart) {
> +		ret = fwnode_property_read_u32_array(fn,
> propname, new_custom->table, n_entries);
> +		if (ret < 0)
> +			return ERR_PTR(ret);
> +
> +		cpu_to_be32_array(new_custom->table,
> new_custom->table, n_entries);
> +	} else {
> +		ret = fwnode_property_read_u64_array(fn,
> propname, new_custom->table, n_entries);
> +		if (ret < 0)
> +			return ERR_PTR(ret);
> +
> +		for (index = 0; index < n_entries; index++) {
> +			u64 temp = ((u64 *)new_custom-
> >table)[index];
> 
>  			if ((index % 2) != 0)
>  				temp = __convert_to_raw(temp, 1024);
> @@ -445,16 +461,9 @@ static struct ltc2983_custom_sensor
> *__ltc2983_custom_sensor_new(
>  				temp = __convert_to_raw_sign(temp,
> resolution);
>  			else
>  				temp = __convert_to_raw(temp,
> resolution);
> -		} else {
> -			u32 t32;
> 
> -			of_property_read_u32_index(np, propname,
> index, &t32);
> -			temp = t32;
> +			put_unaligned_be24(temp, new_custom-
> >table + index * 3);
>  		}
> -
> -		for (j = 0; j < n_size; j++)
> -			new_custom->table[tbl++] =
> -				temp >> (8 * (n_size - j - 1));
>  	}
> 
>  	new_custom->is_steinhart = is_steinhart;
> @@ -597,13 +606,12 @@ static int ltc2983_adc_assign_chan(struct
> ltc2983_data *st,
>  	return __ltc2983_chan_assign_common(st, sensor, chan_val);
>  }
> 
> -static struct ltc2983_sensor *ltc2983_thermocouple_new(
> -					const struct device_node *child,
> -					struct ltc2983_data *st,
> -					const struct ltc2983_sensor
> *sensor)
> +static struct ltc2983_sensor *
> +ltc2983_thermocouple_new(const struct fwnode_handle *child,
> struct ltc2983_data *st,
> +			 const struct ltc2983_sensor *sensor)
>  {
>  	struct ltc2983_thermocouple *thermo;
> -	struct device_node *phandle;
> +	struct fwnode_handle *ref;
>  	u32 oc_current;
>  	int ret;
> 
> @@ -611,11 +619,10 @@ static struct ltc2983_sensor
> *ltc2983_thermocouple_new(
>  	if (!thermo)
>  		return ERR_PTR(-ENOMEM);
> 
> -	if (of_property_read_bool(child, "adi,single-ended"))
> +	if (fwnode_property_read_bool(child, "adi,single-ended"))
>  		thermo->sensor_config =
> LTC2983_THERMOCOUPLE_SGL(1);
> 
> -	ret = of_property_read_u32(child, "adi,sensor-oc-current-
> microamp",
> -				   &oc_current);
> +	ret = fwnode_property_read_u32(child, "adi,sensor-oc-
> current-microamp", &oc_current);
>  	if (!ret) {
>  		switch (oc_current) {
>  		case 10:
> @@ -651,10 +658,9 @@ static struct ltc2983_sensor
> *ltc2983_thermocouple_new(
>  		return ERR_PTR(-EINVAL);
>  	}
> 
> -	phandle = of_parse_phandle(child, "adi,cold-junction-handle",
> 0);
> -	if (phandle) {
> -		ret = of_property_read_u32(phandle, "reg",
> -					   &thermo-
> >cold_junction_chan);
> +	ref = fwnode_find_reference(child, "adi,cold-junction-
> handle", 0);
> +	if (ref) {

This is nok. It needs to be 'if (IS_ERR(ref))'. We then should return
ERR_CAST() in case of errors inside the if block. As this reference
is also optional, we need to nullify ref in case we don't find the
it. Otherwise fwnode_handle_put() breaks.

We also need to use ptr error logic in the other places where
fwnode_find_reference() is used. Although, in the other cases
the ref is mandatory, so there's no need to care with breaking
fwnode_handle_put().

After these changes (I think the changes are straight enough;
but I can re-test if you or Jonathan ask for it):

Tested-by: Nuno Sá <nuno.sa@xxxxxxxxxx>