RE: [PATCH v1 2/2] iio: temperature: ltc2983: Make use of device properties

[Sa, Nuno]

Hi all,

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Saturday, February 5, 2022 7:24 PM
> To: Jonathan Cameron <jic23@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sa, Nuno
> <Nuno.Sa@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>
> Subject: Re: [PATCH v1 2/2] iio: temperature: ltc2983: Make use of
> device properties
> 
> [External]
> 
> On Sat, Feb 05, 2022 at 05:14:54PM +0000, Jonathan Cameron wrote:
> > 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.
> 
> That's fair. I also spent most of the time on this change in comparison
> to the
> whole bundle.
> 
> ...
> 
> > > +#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.
> 
> This is a side effect of the change. Below I will try to explain, tell me if
> that is what you want to be added to the commit message (feel free to
> correct
> my English).
> 
> 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 allocated the amount of memory needed to retrieve all values at
> once from
> the property and then convert them as required.

Yeah, I understood what you are doing... We allocate some extra bytes so
that we can simplify a lot the logic to get, convert and put back the properties
in the table... And since we are only writing the __real__ table size into the device
I'm fairly positive this works. However, I do agree with Jonathan and I would
be more confident to give this a test before being applied...

That said, I'm off till the end of the month (just doing minor stuff like replying
to emails :)), so if we can hold this one, I will test it first thing I'm back in the
office.


> ...
> 
> > >  	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.
> 
> Good catch! It's a leftover, one case became a patch 1 in this series.
> 
> ...
> 
> > > +	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.
> 
> OK, I'm fine with either approach.
> 
> > > +		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.

The hand coded big endian was done to not have to make a distinction between
3 and 4 bytes properties. With these changes it does not make sense anymore
to have it...

- Nuno Sá