Date:   Wed, 22 Dec 2021 09:07:45 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Cosmin Tanislav <demonsingur@...il.com>
Cc:     cosmin.tanislav@...log.com, Jean Delvare <jdelvare@...e.com>,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/7] hwmon: (adt7x10) Use
 devm_hwmon_device_register_with_info

On 12/21/21 10:35 PM, Cosmin Tanislav wrote:
> 
> 
> On 12/22/21 05:08, Guenter Roeck wrote:
>> On 12/21/21 1:58 PM, Cosmin Tanislav wrote:
>>> From: Cosmin Tanislav <cosmin.tanislav@...log.com>
>>>
>> [ ... ]
>>
>>> +
>>> +static int adt7x10_read(struct device *dev, enum hwmon_sensor_types type,
>>> +            u32 attr, int channel, long *val)
>>> +{
>>> +    struct adt7x10_data *data = dev_get_drvdata(dev);
>>> +
>>> +    switch (attr) {
>>> +    case hwmon_temp_input:
>>> +        return adt7x10_temp_read(data, 0, val);
>>> +    case hwmon_temp_max:
>>> +        return adt7x10_temp_read(data, 1, val);
>>> +    case hwmon_temp_min:
>>> +        return adt7x10_temp_read(data, 2, val);
>>> +    case hwmon_temp_crit:
>>> +        return adt7x10_temp_read(data, 3, val);
>>
>> Ok, so you want to keep using the internal "index" to indicate the
>> array position in the register cache. I _did_ specifically ask
>> to use defines for index values in this case. You did not explain
>> why you ignored this. So now you'll have to explain 1) why you
>> ignored my request and 2) why you want to keep the code as is.
>>
>> And, _please_, add a To: recipient to your patches. I am getting
>> tired having to handle the fallout.
>>
>> Guenter
> 
> 
> First of all, maybe you should compare the time these patches were
> sent to the time that your reply that "I ignored" was sent.
> I sent the patches before you "specifically asked me to use defines for
> index values in this case".
> 

Maybe it would be beneficial to wait a few minutes to give me time
to review all patches of a series before sending a new series.

> Second of all, this specific place is a 1:1 conversion from the original code. If I change to using defines here, I'll have to change to using
> defines everywhere else in the code, which doesn't seem to be the scope
> of this exact patch. Of course it looks bad, but it looked equally bad

As suggested, you could have dropped the use of 'index' entirely.
Its use was a side effect of the old hwmon API, where extra fields
in sensor attributes were used to separate attributes. Using the
with_info API, that is no longer necessary. You chose to keep that
part of the code, which is fine, but then I am also asking for
changing the hard coded constants to make it easier to understand
the code going forward. This is especially necessary because "index"
now has two meanings: First, it is the sensor channel, as passed
from the hwmon subsystem and, second, it is the index into
the register cache. Those different use cases now need to be
clearly separated, and the best way to do that is to use an enum
or define to indicate the index into the register cache.

... and I specifically asked for that because I got confused
about it while reviewing the code. People looking at the code
at a later time should not have that problem.

> before. I can introduce more following patches that fix some obvious
> non-functional problems with the driver.
> 
> Third of all, why are you so tense? You're making both of our lives
> harder. I understand being an upstream maintainer is hard work, but
> everyone's job is hard work. It's not like I wanted to work on
> refactoring ADT7x10, I just had to so I can later introduce a
> patch that implements debugfs reg access...

I tend to get more tense if people give me extra work, and do so
repeatedly (like sending follow-up patch series tagged as v1 and
with no change log). I am human, after all, Sorry for that.

Guenter

Powered by blists - more mailing lists