On Thu, 3 Feb 2022 13:45:06 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
This description needs expansion as it's not a straight forward
conversion.
Also, complex enough that I definitely want more eyes and preferably
some testing.
Thanks,
Jonathan
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/iio/temperature/ltc2983.c | 211 +++++++++++++++---------------
> 1 file changed, 105 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> index d272c303c1b0..a2fcea7d9d90 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>
> +
This may well be a valid addition but it's not called out in the patch
description.
> /* 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");
> @@ -415,29 +419,39 @@ static struct ltc2983_custom_sensor *__ltc2983_custom_sensor_new(
> }
> /* Check space on the table. */
> if (st->custom_table_size + new_custom->size >
> - (LTC2983_CUST_SENS_TBL_END_REG -
> - LTC2983_CUST_SENS_TBL_START_REG) + 1) {
> + (LTC2983_CUST_SENS_TBL_END_REG - LTC2983_CUST_SENS_TBL_START_REG) + 1) {
Shouldn't really be in this patch. Or at very least call out that there is
whitespace cleanup in the patch description.
> dev_err(dev, "No space left(%d) for new custom sensor(%zu)",
> st->custom_table_size, new_custom->size);
> return ERR_PTR(-EINVAL);
> }
>
> /* 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);
> + if (is_steinhart)
> + ret = fwnode_property_read_u32_array(fn, propname, 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);
> +
> + /*
> + * Steinhart sensors are configured with raw values in the device tree.
> + * 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 is
> + * not possible.
> + */
> + if (is_steinhart) {
Perhaps would be cleaner to combine this if else with the one above at the cost
of duplicating the if (ret < 0) check.
> + cpu_to_be32_array(new_custom->table, new_custom->table, n_entries);
I completely failed to register the hand coded big endian conversion. Nice
tidy up. However, definitely something to call out in the patch description.
> + } else {
> + 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 +459,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 +604,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_rtd_new(const struct device_node *child,
> - struct ltc2983_data *st,
> - const struct ltc2983_sensor *sensor)
> +static struct ltc2983_sensor *
> +ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
> + const struct ltc2983_sensor *sensor)
> {
> struct ltc2983_rtd *rtd;
> int ret = 0;
> struct device *dev = &st->spi->dev;
> - struct device_node *phandle;
> + struct fwnode_handle *ref;
> u32 excitation_current = 0, n_wires = 0;
>
> rtd = devm_kzalloc(dev, sizeof(*rtd), GFP_KERNEL);
> if (!rtd)
> return ERR_PTR(-ENOMEM);
>
> - phandle = of_parse_phandle(child, "adi,rsense-handle", 0);
> - if (!phandle) {
> + ref = fwnode_find_reference(child, "adi,rsense-handle", 0);
> + if (!ref) {
> dev_err(dev, "Property adi,rsense-handle missing or invalid");
> return ERR_PTR(-EINVAL);
> }
>
> - ret = of_property_read_u32(phandle, "reg", &rtd->r_sense_chan);
> + ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
> if (ret) {
> dev_err(dev, "Property reg must be given\n");
> goto fail;
> }
>
> - ret = of_property_read_u32(child, "adi,number-of-wires", &n_wires);
> + ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
> if (!ret) {
> switch (n_wires) {
> case 2:
> @@ -738,9 +742,9 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
> }
> }
>
> - if (of_property_read_bool(child, "adi,rsense-share")) {
> + if (fwnode_property_read_bool(child, "adi,rsense-share")) {
> /* Current rotation is only available with rsense sharing */
> - if (of_property_read_bool(child, "adi,current-rotate")) {
> + if (fwnode_property_read_bool(child, "adi,current-rotate")) {
> if (n_wires == 2 || n_wires == 3) {
> dev_err(dev,
> "Rotation not allowed for 2/3 Wire RTDs");
> @@ -803,7 +807,7 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
> "adi,custom-rtd",
> false, 2048, false);
> if (IS_ERR(rtd->custom)) {
> - of_node_put(phandle);
> + fwnode_handle_put(ref);
I guess there was a bunch of cut and paste in this driver ;) Same question as below
on whether we can just use a goto here to share the put in the fail path.
> return ERR_CAST(rtd->custom);
> }
> }
> @@ -812,8 +816,8 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
> rtd->sensor.fault_handler = ltc2983_common_fault_handler;
> rtd->sensor.assign_chan = ltc2983_rtd_assign_chan;
>
> - ret = of_property_read_u32(child, "adi,excitation-current-microamp",
> - &excitation_current);
> + ret = fwnode_property_read_u32(child, "adi,excitation-current-microamp",
> + &excitation_current);
> if (ret) {
> /* default to 5uA */
> rtd->excitation_current = 1;
> @@ -852,23 +856,22 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
> }
> }
>
> - of_property_read_u32(child, "adi,rtd-curve", &rtd->rtd_curve);
> + fwnode_property_read_u32(child, "adi,rtd-curve", &rtd->rtd_curve);
>
> - of_node_put(phandle);
> + fwnode_handle_put(ref);
> return &rtd->sensor;
> fail:
> - of_node_put(phandle);
> + fwnode_handle_put(ref);
> return ERR_PTR(ret);
> }
>
> -static struct ltc2983_sensor *ltc2983_thermistor_new(
> - const struct device_node *child,
> - struct ltc2983_data *st,
> - const struct ltc2983_sensor *sensor)
> +static struct ltc2983_sensor *
> +ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *st,
> + const struct ltc2983_sensor *sensor)
> {
> struct ltc2983_thermistor *thermistor;
> struct device *dev = &st->spi->dev;
> - struct device_node *phandle;
> + struct fwnode_handle *ref;
> u32 excitation_current = 0;
> int ret = 0;
>
> @@ -876,23 +879,23 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
> if (!thermistor)
> return ERR_PTR(-ENOMEM);
>
> - phandle = of_parse_phandle(child, "adi,rsense-handle", 0);
> - if (!phandle) {
> + ref = fwnode_find_reference(child, "adi,rsense-handle", 0);
> + if (!ref) {
> dev_err(dev, "Property adi,rsense-handle missing or invalid");
> return ERR_PTR(-EINVAL);
> }
>
> - ret = of_property_read_u32(phandle, "reg", &thermistor->r_sense_chan);
> + ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
> if (ret) {
> dev_err(dev, "rsense channel must be configured...\n");
> goto fail;
> }
>
> - if (of_property_read_bool(child, "adi,single-ended")) {
> + if (fwnode_property_read_bool(child, "adi,single-ended")) {
> thermistor->sensor_config = LTC2983_THERMISTOR_SGL(1);
> - } else if (of_property_read_bool(child, "adi,rsense-share")) {
> + } else if (fwnode_property_read_bool(child, "adi,rsense-share")) {
> /* rotation is only possible if sharing rsense */
> - if (of_property_read_bool(child, "adi,current-rotate"))
> + if (fwnode_property_read_bool(child, "adi,current-rotate"))
> thermistor->sensor_config =
> LTC2983_THERMISTOR_C_ROTATE(1);
> else
> @@ -926,7 +929,7 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
> steinhart,
> 64, false);
> if (IS_ERR(thermistor->custom)) {
> - of_node_put(phandle);
> + fwnode_handle_put(ref);
> return ERR_CAST(thermistor->custom);
Obviously not due to this patch, but this is odd. Why have one error path
that doesn't use the goto faill;?
If you could tidy that up and add a note on it to the patch description
that would be great.
> }
> }
> @@ -934,8 +937,8 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
> thermistor->sensor.fault_handler = ltc2983_common_fault_handler;
> thermistor->sensor.assign_chan = ltc2983_thermistor_assign_chan;
>
> - ret = of_property_read_u32(child, "adi,excitation-current-nanoamp",
> - &excitation_current);
> + ret = fwnode_property_read_u32(child, "adi,excitation-current-nanoamp",
> + &excitation_current);
> if (ret) {
> /* Auto range is not allowed for custom sensors */
> if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART)
> @@ -999,17 +1002,16 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
> }
> }
>
> - of_node_put(phandle);
> + fwnode_handle_put(ref);
> return &thermistor->sensor;
> fail:
> - of_node_put(phandle);
> + fwnode_handle_put(ref);
> return ERR_PTR(ret);
> }
>
