Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Wed, 29 Nov 2023 16:14:58 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Petre Rodan <petre.rodan@subdimension.ro>
Cc:     linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Angel Iglesias <ang.iglesiasg@gmail.com>,
        Matti Vaittinen <mazziesaccount@gmail.com>,
        Andreas Klinger <ak@it-klinger.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Subject: Re: [PATCH v5 2/2] iio: pressure: driver for Honeywell HSC/SSC
 series pressure sensors
Message-ID: <ZWdHYsWN-RWr75T6@smile.fi.intel.com>
References: <20231129070452.1521-1-petre.rodan@subdimension.ro>
 <ZWcUPkzfGqxYsysp@smile.fi.intel.com>
 <ZWdB-vU2MAptRk8d@sunspire>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZWdB-vU2MAptRk8d@sunspire>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Wed, Nov 29, 2023 at 03:51:54PM +0200, Petre Rodan wrote:
> 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,

	HSC_VARIANTS_MAX

> }
> 
> static const char * const hsc_triplet_variants[] = {

static const char * const hsc_triplet_variants[HSC_VARIANTS_MAX] = {

> 	[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[] = {

static const struct hsc_range_config hsc_range_config[HSC_VARIANTS_MAX] = {

> 	[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.

That's the suggestion, yes.

...

> > > +#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?

I'm not sure I understand you. Maybe I was unclear, sorry for that.
I meant the property.h is not used in the _header_ file. Am I mistaken?

-- 
With Best Regards,
Andy Shevchenko