| Message ID | 20181212005503.28054-5-jeremyfertic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: adt7316: dac fixes | expand |
On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote: > @@ -651,10 +649,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, > u8 config3; > int ret; > > + if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519) > + return -EPERM; return -EINVAL is more appropriate than -EPERM. regards, dan carpenter
On Wed, Dec 12, 2018 at 11:23:16AM +0300, Dan Carpenter wrote: > On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote: > > @@ -651,10 +649,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, > > u8 config3; > > int ret; > > > > + if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519) > > + return -EPERM; > > return -EINVAL is more appropriate than -EPERM. > > regards, > dan carpenter > I chose -EPERM because the driver uses it quite a few times in similar circumstances. At least with this driver, -EINVAL is used when the user attempts to write data that would never be valid. -EPERM is used when either the current device settings prevent some functionality from being used, or the device never supports that functionality. This patch is the latter, that these two chip ids never support this function. I'll change to -EINVAL in a v2 series, but I wonder if I should hold off on a separate patch for other instances in this driver since it will be undergoing a substantial refactoring. Is there any rule of thumb about when -EPERM is appropriate for a driver, or is -EINVAL almost always the better option?
On Thu, Dec 13, 2018 at 03:01:46PM -0700, Jeremy Fertic wrote: > On Wed, Dec 12, 2018 at 11:23:16AM +0300, Dan Carpenter wrote: > > On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote: > > > @@ -651,10 +649,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, > > > u8 config3; > > > int ret; > > > > > > + if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519) > > > + return -EPERM; > > > > return -EINVAL is more appropriate than -EPERM. > > > > regards, > > dan carpenter > > > > I chose -EPERM because the driver uses it quite a few times in similar > circumstances. Yeah. I saw that when I reviewed the later patches in this series. It's really not doing it right. -EPERM means permission checks like access_ok() failed so it's not appropriate. -EINVAL is sort of general purpose for invalid commands so it's probably the correct thing. > At least with this driver, -EINVAL is used when the user > attempts to write data that would never be valid. -EPERM is used when > either the current device settings prevent some functionality from being > used, or the device never supports that functionality. This patch is the > latter, that these two chip ids never support this function. > > I'll change to -EINVAL in a v2 series, but I wonder if I should hold off > on a separate patch for other instances in this driver since it will be > undergoing a substantial refactoring. Generally, you should prefer kernel standards over driver standards and especially for staging. But it doesn't matter. When I reviewed this patch, I hadn't seen that the driver was doing it like this but now I know so it's fine. We can clean it all at once later if you want. regards, dan carpenter
On Fri, Dec 14, 2018 at 09:26:18AM +0300, Dan Carpenter wrote: > On Thu, Dec 13, 2018 at 03:01:46PM -0700, Jeremy Fertic wrote: > > On Wed, Dec 12, 2018 at 11:23:16AM +0300, Dan Carpenter wrote: > > > On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote: > > > > @@ -651,10 +649,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, > > > > u8 config3; > > > > int ret; > > > > > > > > + if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519) > > > > + return -EPERM; > > > > > > return -EINVAL is more appropriate than -EPERM. > > > > > > regards, > > > dan carpenter > > > > > > > I chose -EPERM because the driver uses it quite a few times in similar > > circumstances. > > Yeah. I saw that when I reviewed the later patches in this series. > > It's really not doing it right. -EPERM means permission checks like > access_ok() failed so it's not appropriate. -EINVAL is sort of general > purpose for invalid commands so it's probably the correct thing. > > > At least with this driver, -EINVAL is used when the user > > attempts to write data that would never be valid. -EPERM is used when > > either the current device settings prevent some functionality from being > > used, or the device never supports that functionality. This patch is the > > latter, that these two chip ids never support this function. > > > > I'll change to -EINVAL in a v2 series, but I wonder if I should hold off > > on a separate patch for other instances in this driver since it will be > > undergoing a substantial refactoring. > > Generally, you should prefer kernel standards over driver standards and > especially for staging. But it doesn't matter. When I reviewed this > patch, I hadn't seen that the driver was doing it like this but now I > know so it's fine. We can clean it all at once later if you want. > > regards, > dan carpenter > I'll wait to deal with these error values since some of them might go away with all the changes necessary to get the driver out of staging. Thanks for clarifying things for me.
diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c index a9990e7f2a4d..eee7c04f93f4 100644 --- a/drivers/staging/iio/addac/adt7316.c +++ b/drivers/staging/iio/addac/adt7316.c @@ -632,9 +632,7 @@ static ssize_t adt7316_show_da_high_resolution(struct device *dev, struct adt7316_chip_info *chip = iio_priv(dev_info); if (chip->config3 & ADT7316_DA_HIGH_RESOLUTION) { - if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) - return sprintf(buf, "1 (12 bits)\n"); - if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) + if (chip->id != ID_ADT7318 && chip->id != ID_ADT7519) return sprintf(buf, "1 (10 bits)\n"); } @@ -651,10 +649,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, u8 config3; int ret; + if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519) + return -EPERM; + + config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION); if (buf[0] == '1') - config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION; - else - config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION); + config3 |= ADT7316_DA_HIGH_RESOLUTION; ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3); if (ret)
The dac high resolution option enables or disables 10 bit dac resolution for the adt7316/7 and adt7516/7 when they're set to output voltage proportional to temperature. Remove the "1 (12 bits)" output from the show function since that is not an option for this mode. Return "1 (10 bits)" if the device is one of the above mentioned chips and the dac high resolution mode is enabled. In the store function, return an error if the device does not support this mode. Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> --- drivers/staging/iio/addac/adt7316.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)