by Dinh Nguyen

[permalink] [raw]

Subject: Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms

On 4/19/2023 6:46 AM, Andy Shevchenko wrote:
> On Tue, Apr 18, 2023 at 12:29:40PM -0500, Dinh Nguyen wrote:
>> On 4/17/2023 4:51 PM, Guenter Roeck wrote:
>>> On 4/17/23 13:55, Dinh Nguyen wrote:
> ...
>
>>> ... and this contradict each other. If bit 31 indicates an error,
>>> this can not be a signed 32-bit value.
>>>
>> You're right! I've re-read the spec and should have the the code look for
>> the specific error values:
>>
>> 0x80000000 - inactive
>> 0x80000001 - old value
>> 0x80000002 - invalid channel
>> 0x80000003 - corrupted.
> No, they are not hex. Probably you need to define an error space with it, but
> at least just use signed _decimal_ values.
>
> Instead of BIT(31) this should go as
>
> #define ..._ERR_BASE INT_MIN // or equivalent if the type is not int
> #define ..._ERR_MAX ... // or whatever name is better
>
> Then in your code
>
> if (value >= _ERR_MAX)
> return 0;
>
> err = _ERR_MAX - value;
> switch (err) {
> ...
> }
>
> P.S. I asked during internal review if the values are bit fielded when errors.
> AFAIU that time they are, now it seems different.

Can I ask what's wrong with this simple implementation?

static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
{
        int value = priv->temperature.value;

        switch (value) {
        case ETEMP_NOT_PRESENT:
                return -ENOENT;
        case ETEMP_CORRUPT:
        case ETEMP_NOT_INITIALIZED:
                return -ENODATA;
        case ETEMP_BUSY:
                return -EBUSY;
        case ETEMP_INACTIVE:
        case ETEMP_TIMEOUT:
        case ETEMP_TOO_OLD:
                return -EAGAIN;
        default:
                /* No error */
                return 0;
        }
}

Dinh

2023-04-21 09:43:53

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms

On Thu, Apr 20, 2023 at 09:46:20AM -0500, Dinh Nguyen wrote:
> On 4/19/2023 6:46 AM, Andy Shevchenko wrote:
> > On Tue, Apr 18, 2023 at 12:29:40PM -0500, Dinh Nguyen wrote:
> > > On 4/17/2023 4:51 PM, Guenter Roeck wrote:
> > > > On 4/17/23 13:55, Dinh Nguyen wrote:

...

> > > > ... and this contradict each other. If bit 31 indicates an error,
> > > > this can not be a signed 32-bit value.
> > > >
> > > You're right! I've re-read the spec and should have the the code look for
> > > the specific error values:
> > >
> > > 0x80000000 - inactive
> > > 0x80000001 - old value
> > > 0x80000002 - invalid channel
> > > 0x80000003 -? corrupted.
> > No, they are not hex. Probably you need to define an error space with it, but
> > at least just use signed _decimal_ values.
> >
> > Instead of BIT(31) this should go as
> >
> > #define ..._ERR_BASE INT_MIN // or equivalent if the type is not int
> > #define ..._ERR_MAX ... // or whatever name is better
> >
> > Then in your code
> >
> > if (value >= _ERR_MAX)
> > return 0;
> >
> > err = _ERR_MAX - value;
> > switch (err) {
> > ...
> > }
> >
> > P.S. I asked during internal review if the values are bit fielded when errors.
> > AFAIU that time they are, now it seems different.
>
> Can I ask what's wrong with this simple implementation?

Technically, nothing, but from understanding point of view it would be better
to have explicit ranges of error number space vs. actual value space.

The idea in the firmware of that device seems to me similar to what we have in
the Linux kernel. Note, it may be not _so_ explicitly, but the error number
space is limited by a PAGE_SIZE. All the same may be applied here.

> static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv *priv)
> {
> ??????? int value = priv->temperature.value;
>
> ??????? switch (value) {
> ??????? case ETEMP_NOT_PRESENT:
> ??????????????? return -ENOENT;
> ??????? case ETEMP_CORRUPT:
> ??????? case ETEMP_NOT_INITIALIZED:
> ??????????????? return -ENODATA;
> ??????? case ETEMP_BUSY:
> ??????????????? return -EBUSY;
> ??????? case ETEMP_INACTIVE:
> ??????? case ETEMP_TIMEOUT:
> ??????? case ETEMP_TOO_OLD:
> ??????????????? return -EAGAIN;
> ??????? default:
> ??????????????? /* No error */
> ??????????????? return 0;
> ??????? }
> }

--
With Best Regards,
Andy Shevchenko