Hello,
thank you for the code review.
On Wed, Nov 29, 2023 at 12:36:46PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 29, 2023 at 09:04:49AM +0200, Petre Rodan wrote:
> ...
>
> > + for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
> > + if (strncmp(hsc_range_config[index].triplet,
> > + triplet,
> > + HSC_PRESSURE_TRIPLET_LEN - 1) == 0) {
> > + hsc->pmin = hsc_range_config[index].pmin;
> > + hsc->pmax = hsc_range_config[index].pmax;
> > + found = 1;
> > + break;
> > + }
> > + }
> > + if (hsc->pmin == hsc->pmax || !found)
> > + return dev_err_probe(dev, -EINVAL,
> > + "honeywell,pressure-triplet is invalid\n");
>
> This one is important. I think I told already twice that this is NIH
> device_property_match_property_string(). Please, use this API directly.
let me see if we are on the same frequency.
you told me to use match_string/sysfs_match_string.
using any of those assume having the strings in an array, which means I have to
clutter the code with 242 extra lines in order to split up hsc_range_config:
enum hsc_variants {
HSC001BA = 0,
HSC1_6BA = 1,
[ .. 115 lines skipped ]
HSC150PG = 117,
}
static const char * const hsc_triplet_variants[] = {
[HSC001BA] = "001BA",
[HSC1_6BA] = "1.6BA",
[ .. 115 lines skipped ]
[HSC150PG] = "150PG",
};
and change my structure to
static const struct hsc_range_config hsc_range_config[] = {
[HSC001BA] = { .pmin = 0, .pmax = 100000 },
[ .. 117 lines skipped ]
};
just to have the privilege of using match_string() on hsc_triplet_variants.
now, which is worse for maintenance? a handful of lines of code that do a
loop of strncmp or the clutter depicted above?
I can go either way, but I want to make sure you see where this leads.
> > +#ifndef _HSC030PA_H
> > +#define _HSC030PA_H
>
> > +#include <linux/mutex.h>
>
> > +#include <linux/property.h>
>
> Is not used here.
ok.
since we are here, can you please tell me how is mutex.h used outside of hsc030pa.c where it was included previously?
cheers,
peter
--
petre rodan
