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
