On Sat, 5 Feb 2022 20:23:43 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> 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).
I figured that out whilst it was sending but didn't hit cancel fast enough! :)
>
> 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.
Good description to put in the patch description.
>
> ...
>
> > > 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.
>
> See above.
>
> > > + } 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));
> > > }
>
> ...
>
> > > 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.
>
> Probably as separate (preparatory) patch?
Perfect
>
> > > return ERR_CAST(rtd->custom);
> > > }
>
> ...
>
> > > 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.
>
> Same answer as above.
>
> > > }
>
