Re: [PATCH v1 3/4] platform/x86: i2c-multi-instantiate: Make number of clients unsigned

From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mark Gross <mgross@linux.intel.com>, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v1 3/4] platform/x86: i2c-multi-instantiate: Make number of clients unsigned
Date: Mon, 9 Nov 2020 13:43:50 +0100	[thread overview]
Message-ID: <1abf0d37-9689-cd37-9983-1bec6f7081d0@redhat.com> (raw)
In-Reply-To: <20201109115316.GA4077@smile.fi.intel.com>

Hi,

On 11/9/20 12:53 PM, Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 12:39:45PM +0100, Hans de Goede wrote:
>> On 11/5/20 12:05 PM, Andy Shevchenko wrote:
>>> There is no need to use signed type for number of clients. Moreover,
>>> it's cleaner to show that we never go negative there.
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> I'm not a big fan of this change, it feels like needless churn to me.
> 
> Feel free to not apply it. I think I don't need to resend w/o it (IIRC the rest
> pretty much independent of this change). But if you need a v2, tell me.

No need for a v2.

>> Integers are signed by default and just because a value cannot become
>> negative is not really a reason to make it unsigned. E.g. your typical
>> "int i" is often used as an array index so it cannot go negative, still
>> it almost always is an "int i" not an "unsigned int i".
>>
>> IMHO good reasons for deviating from the default signedness and
>> making a value unsigned are:
>>
>> 1. Because the value cannot go negative and we need more range.
>> 2. To avoid sign-extension when upcasting it to a larger integer type.
>>
>> Neither is the case here.
> 
> I consider one more, i.e. if we know that value may not be negative the
> unsigned type gives a hint. I always stumbled over signed integers used for
> loop counters since they confuse me (Q in mind: "should I read code carefully
> and check if it may or may not be signed? Why it's signed?").
> 
> That's why I like the idea of be a bit stricter about types.
> 
> Hope this explains my motivation.

It does and I understand your point of view here.

>> I do like the other 3 patches, thank you for those. I'm going to wait
>> a bit with applying them though, to see where things go with the
>> "[RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients"
>>
>> Merging them now may get in the way with merging that series if
>> Wolfram wants to pick up the entire series (since it also involves
>> an i2c-core change).
> 
> Usually I expect that RFC has less priority than normal series> and I wouldn't
> expect any maintainer (with some rare exceptions) to take series marked as RFC.

Right, but you suggested resending it as a non RFC, and then there is some
cross tree coordination which needs to happen to merge it; and since
this series is just cleanups with no functional changes I would prefer to
delay this one a bit to make the cross tree coordination simpler
(iow keeps i2c-multi-instatiate.c unmodified from rc1 for now).

I hope this explains why I'm delaying your cleanups a bit.

> And TBH I was wondering why you marked them as such, to me that was fine to
> send as normal one.

As I tried to explain in my reaction to the RFC cover-letter I'm not sure we
should apply those patches right now as there is no immediate need for
passing the fwnode and a non 0 chance of regressions. But lets discuss that
in that thread. If we decide to not apply that series for now then I'll apply
this series (minus patch 3) right away.

Regards,

Hans