Messages in this thread | | | Date | Fri, 22 Sep 2023 09:07:42 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCH v2 2/3] iio: pressure: Support ROHM BU1390 |
| |
On 9/19/23 17:32, Jonathan Cameron wrote: > >>>> +static int bm1390_read_raw(struct iio_dev *idev, >>>> + struct iio_chan_spec const *chan, >>>> + int *val, int *val2, long mask) >>>> +{ >>>> + struct bm1390_data *data = iio_priv(idev); >>>> + int ret; >>>> + >>>> + switch (mask) { >>>> + case IIO_CHAN_INFO_SCALE: >>>> + if (chan->type == IIO_TEMP) { >>>> + *val = 31; >>>> + *val2 = 250000; >>>> + >>>> + return IIO_VAL_INT_PLUS_MICRO; >>>> + } else if (chan->type == IIO_PRESSURE) { >>>> + *val = 0; >>>> + /* >>>> + * pressure in hPa is register value divided by 2048. >>>> + * This means kPa is 1/20480 times the register value, >>>> + * which equals to 48828.125 * 10 ^ -9 >>>> + * This is 48828.125 nano kPa. >>>> + * >>>> + * When we scale this using IIO_VAL_INT_PLUS_NANO we >>>> + * get 48828 - which means we lose some accuracy. Well, >>>> + * let's try to live with that. >>>> + */ >>>> + *val2 = 48828; >>>> + >>>> + return IIO_VAL_INT_PLUS_NANO; >>>> + } >>>> + >>>> + return -EINVAL; >>>> + case IIO_CHAN_INFO_RAW: >>>> + ret = iio_device_claim_direct_mode(idev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = bm1390_read_data(data, chan, val, val2); >>>> + iio_device_release_direct_mode(idev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + return IIO_VAL_INT; >>>> + default: >>>> + return -EINVAL; >>> >>> Certainly useless, but should we break and return -EINVAL after the >>> switch, so that it is more explicit that bm1390_read_raw() always >>> returns a value? >> >> I think there is also opposite opinions on this. For my eyes the return >> at the end of the function would also be clearer - but I think I have >> been asked to drop the useless return when I've been working with other >> sensors in IIO domain :) My personal preference would definitely be: >> >> int ret; >> >> switch (foo) >> { >> case BAR: >> ret = func1(); >> if (ret) >> break; >> >> ret = func2(); >> if (ret) >> break; >> >> ... >> break; >> >> case BAZ: >> ret = -EINVAL; >> break; >> } >> >> return ret; >> >> - but I've learned to think this is not the IIO preference. > > Some static analyzers get confused (probably when there is a little > bit more going on after the function) by that and moan that some > cases are not considered in the switch. I got annoyed enough with the > noise they were generating to advocate always having explicit defaults.
Oh, yes. I see I omitted the default from the example - but this was not what I tried to highlight ;) With a bit more thought I would've added:
default: ret = -EINVAL; break;
As you probably guess, what I was after is that for a simple (not deeply nested) cases like this, I would rather use a variable for return value and a single point of exit at the end of the function - instead of having returns in the switch-case. That'd suit better _my_ taste.
Yours, -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |