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

[Farber, Eliav]

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