Re: [PATCH 1/1] Add ST STPDDC60 pmbus driver

[Erik Rosen]

Hi Guenter


On Fri, Jan 29, 2021 at 4:50 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> Hi Erik,
>
> On 1/29/21 5:07 AM, Erik Rosen wrote:
> [ ... ]
> >>
> >>> +             break;
> >>> +     case PMBUS_VOUT_OV_FAULT_LIMIT:
> >>> +     case PMBUS_VOUT_UV_FAULT_LIMIT:
> >>> +             ret = pmbus_read_word_data(client, page, phase, reg);
> >>> +             if (ret < 0)
> >>> +                     return ret;
> >>> +             ret &= 0x07ff;
> >>
> >> This needs explanation. The BMR481 datasheet does not suggest that only
> >> 11 bits are valid.
> >
> > Ok, I will add a clarification. These registers use LINEAR11 whereas
> > LINEAR16 is expected for vout-related registers.
> >
> Is that the correct fix, then ? LINEAR11 means that the exponent is flexible,
> while it is fixed for LINEAR16. Just assuming that the exponents always match
> seems risky. Also, bit 11 in LINEAR11 is the sign bit, meaning negative limits
> (which seem at least theoretically be possible) would be reported as large
> positive values.

The chip actually uses fixed exponents for representing all linear values and
the specification explicitly states the values of the LSB for all registers.
It also states that the limits can be handled as linear format when
writing  _if_
the exponent is the fixed value used for that limit. This means I'll have to
convert all limit writes to use the expected exponent.
Given this, I think it's safe to assume that the exponents are
constant, but I'll
add a check to handle potential negative values.

>
> >>
> >>> +             break;
> >>> +     default:
> >>> +             ret = -ENODATA;
> >>> +             break;
> >>> +     }
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +/*
> >>> + * The vout under- and over-voltage limits are readonly for this chip.
> >>> + */
> >>
> >> Not really. The BMR481 datasheet suggests that the value can be changed
> >> by writing to MFR_OV_LIMIT_OFFSET and MFR_UV_LIMIT_OFFSET.
> >> I am not saying that you should implement that if you don't want to,
> >> but I would suggest a better (more accurate) explanation.
> >
> > I have looked into this a bit more and it's a bit more to it than I expected.
> > The limits are actually dynamic values that will follow both commanded
> > output voltage via the PMBus or SVID/AVSBus as well as current
> > load (droop). To account for this I propose a mechanism to set the
>
> Yes, I noticed.
>
> > 'update' flag on selected sensors after auto-detection of limit attributes.
> >
> > Maybe add a function like this to pmbus_core that can be called
> > after the probing is done?
> >
> > int pmbus_set_update(struct i2c_client *client, u8 reg)
> > {
> >         struct pmbus_data *data = i2c_get_clientdata(client);
> >         struct pmbus_sensor *sensor;
> >
> >         for (sensor = data->sensors; sensor; sensor = sensor->next) {
> >                 if (sensor->reg == reg) {
> >                         sensor->update = true;
> >                         return 0;
> >                 }
> >         }
> >         return -ENXIO;
> > }
> >
>
> Add a boolean 'update' parameter (to be able to disable updates),
> and make the function void. Also, remember that 'reg' may be repeated
> on a chip with multiple pages, so you can't return after the first match.
> Otherwise looks ok.

Ok

>
> > I did also implemented writes to the limit registers via the offset
> > registers but it might be
> > a bit confusing to the user since the limits are set in relation to
> > the current commanded
> > output voltage and will change together with this. In the worst case,
> > the voltage might
> > change at the same time as the limit is written to, which will give
> > unexpected results.
>
> Agreed, that can be tricky.
>
> > The alternative would be to just set these limits read-only. What is
> > your opinion?
> >
>
> Your call. Just add a note explaining why it is made read-only to explain
> the reasoning for future readers.

Ok, I'll keep the functionality and add a note in the docs.


>
> > Also, I found a problem in how pmbus_set_sensor() works together with
> > pmbus_clear_cache()
> > as used in the zl6100 and lm25066 drivers. The new value is written to
> > the sensor struct _after_
> > the _pmbus_write_word_data() has returned and thus after
> > pmbus_clear_cache() is called.
> > The effect is that all values are cleared, except the one just written
> > to, which I believe defeats
> > the purpose of clearing the cache in the first place.> One solution would be to write the new value to sensor->data before
> > the _pmbus_write_word_data
> > is called and to restore the old value in case of error.
> > I can make a separate patch for this if it seems like a reasonable solution?
> >
>
> Good catch.
>
> An alternate and more generic solution might be to set sensor->data to
> -ENODATA after updates. After all, it seems risky to assume that the chip
> returns the value that was written (I have seen it often enough in other
> drivers that this is not necessarily the case). That would also mean that
> it would no longer be necessary to call pmbus_clear_cache() in the zl6100
> and lm25066 drivers. Impact would be that a limit read after a write would
> always be sent to the chip for all drivers, but that seems minimal compared
> to the gain (and presumably limit registers are not frequently updated).

I agree that this is a more robust solution. I'll send a separate patch for this
update. The zl6100 in fact still needs to clear the cache since a write to the
fault limit also changes the value of the warning limit and vice versa. As far
as I can see the usage in lm25066 can be removed however.

>
> Thanks,
> Guenter