Re: [PATCH v3 3/5] hwmon: ltc2945: Handle error case in ltc2945_value_store

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Alright, I'll take another pass at it.

On Wed, Jan 11, 2023 at 7:44 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On Tue, Jan 10, 2023 at 02:25:37PM -0500, Jon Cormier wrote:
> > On Tue, Jan 10, 2023 at 1:22 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > >
> > > On 1/10/23 10:19, Jon Cormier wrote:
> > > > On Mon, Jan 9, 2023 at 7:04 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > > >>
> > > >> On 1/9/23 15:35, Jonathan Cormier wrote:
> > > >>> ltc2945_val_to_reg errors were not being handled
> > > >>> which would have resulted in register being set to
> > > >>> 0 (clamped) instead of being left alone.
> > > >>>
> > > >>> Change reg_to_val and val_to_reg to return values
> > > >>> via parameters to make it more obvious when an
> > > >>> error case isn't handled. Also to allow
> > > >>> the regval type to be the correct sign in prep for
> > > >>> next commits.
> > > >>>
> > > >>
> > > >> Sorry, I don't see that as reason or argument for such invasive changes.
> > > >> As far as I can see, a two-liner to check the return value of val_to_reg()
> > > >> should have been sufficient. Most of the rest, such as splitting
> > > >> the return value into two elements, is POV and just adds additional code
> > > >> and complexity for zero gain.
> > > > I can do that. However, you had also mentioned changing the return
> > > > type to match what the calling function was expecting, an unsigned
> > > > long. But I can't do that since error codes are negative so it would
> > > > be a signed long which would lose precision and seemingly defeat the
> > > > point of matching the variable type the caller wants.  I could make it
> > > > a signed long long but that still doesn't match.  So it seemed saner
> > > > to just return the error and the value separately, that way the
> > > > function declaration was explicit about the types it wanted/returned,
> > > > and less room for error.  Would love to know your preferred solution.
> > > >
> > >
> > > That is only true if the upper bit is actually ever set in that signed long.
> > > Which means I'll have to verify if "would lose precision" is actually
> > > a correct statement.
> > I'd like to argue that is another reason to go with this change
> > instead of working out the math of just how many bits are needed in
> > the worst case and having to document it. And potentially getting that
> > calculation wrong.  But I can if you'd like me to.
>
> You are turning things on its head. We don't make changes like that
> because of maybe. It is you who has to show that the change is
> necessary, and that there is indeed a loss of precision otherwise.
>
> Guenter



-- 
Jonathan Cormier
Software Engineer

Voice:  315.425.4045 x222



http://www.CriticalLink.com
6712 Brooklawn Parkway, Syracuse, NY 13211



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux