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
