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>
