Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v3 11/19] hwmon: (mr75203) add VM pre-scaler support
Message-ID: <917126b6-3acc-48df-77d2-0a725e7475d6@amazon.com>
Date:   Thu, 1 Sep 2022 17:17:40 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.1
Content-Language: en-US
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
CC:     <jdelvare@suse.com>, <linux@roeck-us.net>, <robh+dt@kernel.org>,
        <p.zabel@pengutronix.de>, <rtanwar@maxlinear.com>,
        <linux-hwmon@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <talel@amazon.com>,
        <hhhawa@amazon.com>, <jonnyc@amazon.com>, <hanochu@amazon.com>,
        <ronenk@amazon.com>, <itamark@amazon.com>, <shellykz@amazon.com>,
        <shorer@amazon.com>, <amitlavi@amazon.com>, <almogbs@amazon.com>,
        <dkl@amazon.com>, "Farber, Eliav" <farbere@amazon.com>
References: <20220830192212.28570-1-farbere@amazon.com>
 <20220830192212.28570-12-farbere@amazon.com>
 <Yw9N6sr+k/4lcmT7@smile.fi.intel.com>
From:   "Farber, Eliav" <farbere@amazon.com>
In-Reply-To: <Yw9N6sr+k/4lcmT7@smile.fi.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/31/2022 3:02 PM, Andy Shevchenko wrote:
> On Tue, Aug 30, 2022 at 07:22:04PM +0000, Eliav Farber wrote:
>> +static int pvt_get_pre_scaler(struct device *dev, struct pvt_device 
>> *pvt)
>> +{
>> +     const struct device_node *np = dev->of_node;
>> +     u32 total_channels = pvt->vm_channels.total;
>> +     u32 channel;
>> +     u8 *pre_scaler_ch_list;
>> +     int i, ret, num_ch;
>> +
>> +     /* Set default pre-scaler value to be 1. */
>> +     for (i = 0; i < total_channels; i++)
>> +             pvt->vd[i].pre_scaler = PRE_SCALER_X1;
>> +
>> +     /* Get number of channels configured in 
>> "moortec,vm-pre-scaler". */
>> +     num_ch = of_property_count_u8_elems(np, "moortec,vm-pre-scaler");
>
> of_ ?!
>
Replaced of_property_count_u8_elems() with
device_property_count_u8().

>> +     if (num_ch <= 0)
>> +             return 0;
>> +
>> +     pre_scaler_ch_list = kcalloc(total_channels,
>> +                                  sizeof(*pre_scaler_ch_list), 
>> GFP_KERNEL);
>> +     if (!pre_scaler_ch_list)
>> +             return -ENOMEM;
>> +
>> +     /* Get list of all channels that have pre-scaler of 2. */
>> +     ret = device_property_read_u8_array(dev, "moortec,vm-pre-scaler",
>> +                                         pre_scaler_ch_list, num_ch);
>> +     if (ret)
>> +             goto out;
>> +
>> +     for (i = 0; i < num_ch; i++) {
>> +             channel = pre_scaler_ch_list[i];
>
>> +
>
> Unnecessary blank line.

Blank line removed.


>> +             if (channel >= total_channels) {
>> +                     dev_err(dev,
>> +                             "invalid channel (%u) in pre-scaler 
>> list\n",
>> +                             channel);
>> +                     ret = -EINVAL;
>
>> +                     goto out;
>
> Wouldn't
>
>                        break;
>
> suffice? (I understand the point, up to you)
I prefer to exit the moment I detect a problem.
For now I can use a break but in the future someone else can add new code
in between that will set ret to 0 and instead of failing driver flow will
continue with incomplete pre-scaler value.
So I prefer keeping it as it.

--
Best regards, Eliav