Date:   Thu, 3 Mar 2022 13:31:56 +0000
From:   "Sa, Nuno" <Nuno.Sa@...log.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>
Subject: RE: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device
 properties

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@...ux.intel.com>
> Sent: Thursday, February 10, 2022 2:55 PM
> To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>; linux-
> iio@...r.kernel.org; linux-kernel@...r.kernel.org
> Cc: Sa, Nuno <Nuno.Sa@...log.com>; Jonathan Cameron
> <jic23@...nel.org>; Lars-Peter Clausen <lars@...afoo.de>
> 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@...ux.intel.com>
> ---
> 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@...log.com>

Powered by blists - more mailing lists