Re: [PATCH v3 11/19] hwmon: (mr75203) add VM pre-scaler support

["Farber, Eliav" <farbere@amazon.com>]

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