linux-kernel - Re: [PATCH v5 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZWdB-vU2MAptRk8d@sunspire>
Date:   Wed, 29 Nov 2023 15:51:54 +0200
From:   Petre Rodan <petre.rodan@...dimension.ro>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        Andreas Klinger <ak@...klinger.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH v5 2/2] iio: pressure: driver for Honeywell HSC/SSC
 series pressure sensors


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