Date:   Fri, 27 Jan 2023 11:09:45 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Markuss Broks <markuss.broks@gmail.com>
Cc:     linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor
Message-ID: <Y9OU2e6YMIUTtto0@smile.fi.intel.com>
References: <20230123231028.26073-1-markuss.broks@gmail.com>
 <20230123231028.26073-3-markuss.broks@gmail.com>
 <Y8+xamtH/U4vK75e@smile.fi.intel.com>
 <a782a0d9-c47e-4328-774f-6bef78161d81@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a782a0d9-c47e-4328-774f-6bef78161d81@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Jan 26, 2023 at 05:54:56PM +0200, Markuss Broks wrote:
> On 1/24/23 12:22, Andy Shevchenko wrote:
> > On Tue, Jan 24, 2023 at 01:10:25AM +0200, Markuss Broks wrote:

First of all, I assume you agree on the comments you left unanswered,
so we will expect them all being addressed in the next version. Is it
correct perception?

...

> > > +static const struct regmap_config tcs3490_regmap_config = {
> > > +	.reg_bits	= 8,
> > > +	.val_bits	= 8,
> > Seems you are using regmap internal serialization, but does it guarantee the
> > serialization on the transaction level? Or why is it not a problem?
> Well, other drivers seem to have it this way too.

They all may be buggy, unclear, or hardware there doesn't require transaction
level locks. It means we have to avoid cargo cult.

> I don't really understand
> why it should be a problem, could you please clarify?

Because one should distinguish IO with register vs. IO transaction.
Imaging two threads which do an IO:

	CPU0					CPU1
						read REG2
						update value
	read REG1
						write REG2
	update value
	write REG1

If in our hypothetical example the writing to REG2 has a side effect on
the values in REG1, we are doomed.

You have to check all possible scenarios and tell if it's a problem or not with
the certain hardware. According to the result, add a corresponding comment to
the code and, if required, change the locking scheme.

> > > +};

-- 
With Best Regards,
Andy Shevchenko