| Message ID | 20181212005503.28054-11-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:55:02PM -0700, Jeremy Fertic wrote: > Based on the output of adt7316_show_all_DAC_update_modes() and > adt7316_show_DAC_update_mode(), adt7316_store_DAC_update_mode() should > expect the user to enter an integer input from 0 to 3. The user input is > currently expected to account for the actual bit positions in the register. > For example, choosing option 3 would require a write of 0x30 (actually 48 > since it expects base 10). To address this inconsistency, create a shift > macro to be used in the valid input check as well as the calculation for > the register write. > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > --- > I'm not sure if this patch is appropriate since it's making a user visible > change. I've included it since the driver is still in staging. We don't want to break user space, but I agree with you that applying this patch is probably the right thing. regards, dan carpenter
On 12 December 2018 08:31:32 GMT, Dan Carpenter <dan.carpenter@oracle.com> wrote: >On Tue, Dec 11, 2018 at 05:55:02PM -0700, Jeremy Fertic wrote: >> Based on the output of adt7316_show_all_DAC_update_modes() and >> adt7316_show_DAC_update_mode(), adt7316_store_DAC_update_mode() >should >> expect the user to enter an integer input from 0 to 3. The user input >is >> currently expected to account for the actual bit positions in the >register. >> For example, choosing option 3 would require a write of 0x30 >(actually 48 >> since it expects base 10). To address this inconsistency, create a >shift >> macro to be used in the valid input check as well as the calculation >for >> the register write. >> >> Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> >> --- >> I'm not sure if this patch is appropriate since it's making a user >visible >> change. I've included it since the driver is still in staging. > >We don't want to break user space, but I agree with you that applying >this patch is probably the right thing. > >regards, >dan carpenter This driver breaks the standard abi in loads of ways. It is going to change userspace interface a lot before it is ready to move out of staging. That includes this particular interface almost certainly being completely replaced. Hence good to move towards something sensible. Don't worry at all about userapace ABI breaks in this one! Jonathan
On Tue, 11 Dec 2018 17:55:02 -0700 Jeremy Fertic <jeremyfertic@gmail.com> wrote: > Based on the output of adt7316_show_all_DAC_update_modes() and > adt7316_show_DAC_update_mode(), adt7316_store_DAC_update_mode() should > expect the user to enter an integer input from 0 to 3. The user input is > currently expected to account for the actual bit positions in the register. > For example, choosing option 3 would require a write of 0x30 (actually 48 > since it expects base 10). To address this inconsistency, create a shift > macro to be used in the valid input check as well as the calculation for > the register write. > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> As I mentioned, long term this interface is going to need to be replaced with something more generic. Probably something like the power down modes where we use a string to describe what is going on on. Still this is a step in the right direction even if we may go further shortly! Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it, Thanks, Jonathan > --- > I'm not sure if this patch is appropriate since it's making a user visible > change. I've included it since the driver is still in staging. > > drivers/staging/iio/addac/adt7316.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c > index bca599d8c51c..58b462ad0c83 100644 > --- a/drivers/staging/iio/addac/adt7316.c > +++ b/drivers/staging/iio/addac/adt7316.c > @@ -129,6 +129,7 @@ > */ > #define ADT7316_DA_2VREF_CH_MASK 0xF > #define ADT7316_DA_EN_MODE_MASK 0x30 > +#define ADT7316_DA_EN_MODE_SHIFT 4 > #define ADT7316_DA_EN_MODE_SINGLE 0x00 > #define ADT7316_DA_EN_MODE_AB_CD 0x10 > #define ADT7316_DA_EN_MODE_ABCD 0x20 > @@ -879,11 +880,11 @@ static ssize_t adt7316_store_DAC_update_mode(struct device *dev, > return -EPERM; > > ret = kstrtou8(buf, 10, &data); > - if (ret || data > ADT7316_DA_EN_MODE_MASK) > + if (ret || data > (ADT7316_DA_EN_MODE_MASK >> ADT7316_DA_EN_MODE_SHIFT)) > return -EINVAL; > > dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK); > - dac_config |= data; > + dac_config |= data << ADT7316_DA_EN_MODE_SHIFT; > > ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config); > if (ret)
diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c index bca599d8c51c..58b462ad0c83 100644 --- a/drivers/staging/iio/addac/adt7316.c +++ b/drivers/staging/iio/addac/adt7316.c @@ -129,6 +129,7 @@ */ #define ADT7316_DA_2VREF_CH_MASK 0xF #define ADT7316_DA_EN_MODE_MASK 0x30 +#define ADT7316_DA_EN_MODE_SHIFT 4 #define ADT7316_DA_EN_MODE_SINGLE 0x00 #define ADT7316_DA_EN_MODE_AB_CD 0x10 #define ADT7316_DA_EN_MODE_ABCD 0x20 @@ -879,11 +880,11 @@ static ssize_t adt7316_store_DAC_update_mode(struct device *dev, return -EPERM; ret = kstrtou8(buf, 10, &data); - if (ret || data > ADT7316_DA_EN_MODE_MASK) + if (ret || data > (ADT7316_DA_EN_MODE_MASK >> ADT7316_DA_EN_MODE_SHIFT)) return -EINVAL; dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK); - dac_config |= data; + dac_config |= data << ADT7316_DA_EN_MODE_SHIFT; ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config); if (ret)
Based on the output of adt7316_show_all_DAC_update_modes() and adt7316_show_DAC_update_mode(), adt7316_store_DAC_update_mode() should expect the user to enter an integer input from 0 to 3. The user input is currently expected to account for the actual bit positions in the register. For example, choosing option 3 would require a write of 0x30 (actually 48 since it expects base 10). To address this inconsistency, create a shift macro to be used in the valid input check as well as the calculation for the register write. Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> --- I'm not sure if this patch is appropriate since it's making a user visible change. I've included it since the driver is still in staging. drivers/staging/iio/addac/adt7316.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)