| Message ID | 20181212005503.28054-4-jeremyfertic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: adt7316: dac fixes | expand |
On Tue, 11 Dec 2018 17:54:55 -0700 Jeremy Fertic <jeremyfertic@gmail.com> wrote: > The only assignment to dac_bits is in adt7316_store_da_high_resolution(). > This function 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 these assignments since they're unnecessary for the > dac high resolution functionality. > > Instead, assign a value to dac_bits in adt7316_probe() since the number > of dac bits might be needed as soon as the device is registered and > available to userspace. > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> I'm a little confused as it seems to me that in the orignal code if we were setting high resolution 'off' we would fall back to 8 bits for either type of device. Now you just have a check on the device type? The result will be that we read more bytes than we want to in show_DAC. I'd need a pretty strong argument to persuade me it is worth supporting the 8 bit mode at all btw on devices that will go higher. It adds interface complexity and the number of usecases where the saving in bus traffic is worthwhile are probably fairly few! Jonathan > --- > drivers/staging/iio/addac/adt7316.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c > index e5e1f9d6357f..a9990e7f2a4d 100644 > --- a/drivers/staging/iio/addac/adt7316.c > +++ b/drivers/staging/iio/addac/adt7316.c > @@ -651,17 +651,10 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, > u8 config3; > int ret; > > - chip->dac_bits = 8; > - > - if (buf[0] == '1') { > + if (buf[0] == '1') > config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION; > - if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) > - chip->dac_bits = 12; > - else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) > - chip->dac_bits = 10; > - } else { > + else > config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION); > - } > > ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3); > if (ret) > @@ -2123,6 +2116,13 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus, > else > return -ENODEV; > > + if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) > + chip->dac_bits = 12; > + else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) > + chip->dac_bits = 10; > + else > + chip->dac_bits = 8; > + > chip->ldac_pin = devm_gpiod_get_optional(dev, "adi,ldac", GPIOD_OUT_LOW); > if (IS_ERR(chip->ldac_pin)) { > ret = PTR_ERR(chip->ldac_pin);
On Sun, Dec 16, 2018 at 11:37:56AM +0000, Jonathan Cameron wrote: > On Tue, 11 Dec 2018 17:54:55 -0700 > Jeremy Fertic <jeremyfertic@gmail.com> wrote: > > > The only assignment to dac_bits is in adt7316_store_da_high_resolution(). > > This function 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 these assignments since they're unnecessary for the > > dac high resolution functionality. > > > > Instead, assign a value to dac_bits in adt7316_probe() since the number > > of dac bits might be needed as soon as the device is registered and > > available to userspace. > > > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > > I'm a little confused as it seems to me that in the orignal code > if we were setting high resolution 'off' we would fall back to 8 > bits for either type of device. Now you just have a check on the > device type? > > The result will be that we read more bytes than we want to > in show_DAC. > > I'd need a pretty strong argument to persuade me it is worth > supporting the 8 bit mode at all btw on devices that will go > higher. It adds interface complexity and the number of usecases > where the saving in bus traffic is worthwhile are probably fairly > few! > > Jonathan Thanks for the feedback Jonathan, and sorry for the confusion on this one. My commit message should have been clearer. This is not a general purpose option to change the dac resolution. It is specific to an optional feature where one or two of the four dacs can be set to output voltage proportional to temperature. If the user chooses to set dac a and/or dac b to ouput voltage proportional to temperature, this da_high_resolution attribute can optionally be enabled to use 10 bit resolution rather than the default 8 bits. It is only available on the 10 and 12 bit dac devices. If the user attempts to read or write dacs a or b under these settings, the driver's current behaviour is to return an error. The dacs c and d continue to operate normally under these conditions. With the above in mind, dac_bits should have never been assigned to in this da_high_resolution attribute. Before this patch, if the user didn't write to this optional attribute, dac_bits would be 0 since it was never assigned to anywhere else. This would result in incorrect calculations in show_DAC and store_DAC. Jeremy > > --- > > drivers/staging/iio/addac/adt7316.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c > > index e5e1f9d6357f..a9990e7f2a4d 100644 > > --- a/drivers/staging/iio/addac/adt7316.c > > +++ b/drivers/staging/iio/addac/adt7316.c > > @@ -651,17 +651,10 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, > > u8 config3; > > int ret; > > > > - chip->dac_bits = 8; > > - > > - if (buf[0] == '1') { > > + if (buf[0] == '1') > > config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION; > > - if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) > > - chip->dac_bits = 12; > > - else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) > > - chip->dac_bits = 10; > > - } else { > > + else > > config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION); > > - } > > > > ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3); > > if (ret) > > @@ -2123,6 +2116,13 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus, > > else > > return -ENODEV; > > > > + if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) > > + chip->dac_bits = 12; > > + else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) > > + chip->dac_bits = 10; > > + else > > + chip->dac_bits = 8; > > + > > chip->ldac_pin = devm_gpiod_get_optional(dev, "adi,ldac", GPIOD_OUT_LOW); > > if (IS_ERR(chip->ldac_pin)) { > > ret = PTR_ERR(chip->ldac_pin); >
On Mon, 17 Dec 2018 14:30:59 -0700 Jeremy Fertic <jeremyfertic@gmail.com> wrote: > On Sun, Dec 16, 2018 at 11:37:56AM +0000, Jonathan Cameron wrote: > > On Tue, 11 Dec 2018 17:54:55 -0700 > > Jeremy Fertic <jeremyfertic@gmail.com> wrote: > > > > > The only assignment to dac_bits is in adt7316_store_da_high_resolution(). > > > This function 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 these assignments since they're unnecessary for the > > > dac high resolution functionality. > > > > > > Instead, assign a value to dac_bits in adt7316_probe() since the number > > > of dac bits might be needed as soon as the device is registered and > > > available to userspace. > > > > > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> > > > > I'm a little confused as it seems to me that in the orignal code > > if we were setting high resolution 'off' we would fall back to 8 > > bits for either type of device. Now you just have a check on the > > device type? > > > > The result will be that we read more bytes than we want to > > in show_DAC. > > > > I'd need a pretty strong argument to persuade me it is worth > > supporting the 8 bit mode at all btw on devices that will go > > higher. It adds interface complexity and the number of usecases > > where the saving in bus traffic is worthwhile are probably fairly > > few! > > > > Jonathan > > Thanks for the feedback Jonathan, and sorry for the confusion on this one. > My commit message should have been clearer. This is not a general purpose > option to change the dac resolution. It is specific to an optional feature > where one or two of the four dacs can be set to output voltage proportional > to temperature. If the user chooses to set dac a and/or dac b to ouput > voltage proportional to temperature, this da_high_resolution attribute can > optionally be enabled to use 10 bit resolution rather than the default 8 > bits. It is only available on the 10 and 12 bit dac devices. If the user > attempts to read or write dacs a or b under these settings, the driver's > current behaviour is to return an error. The dacs c and d continue to > operate normally under these conditions. > > With the above in mind, dac_bits should have never been assigned to in this > da_high_resolution attribute. Before this patch, if the user didn't write > to this optional attribute, dac_bits would be 0 since it was never assigned > to anywhere else. This would result in incorrect calculations in show_DAC > and store_DAC. > Good explanation. Send me a v2 with that in the description. Thanks, Jonathan > Jeremy > > > > --- > > > drivers/staging/iio/addac/adt7316.c | 18 +++++++++--------- > > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c > > > index e5e1f9d6357f..a9990e7f2a4d 100644 > > > --- a/drivers/staging/iio/addac/adt7316.c > > > +++ b/drivers/staging/iio/addac/adt7316.c > > > @@ -651,17 +651,10 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, > > > u8 config3; > > > int ret; > > > > > > - chip->dac_bits = 8; > > > - > > > - if (buf[0] == '1') { > > > + if (buf[0] == '1') > > > config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION; > > > - if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) > > > - chip->dac_bits = 12; > > > - else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) > > > - chip->dac_bits = 10; > > > - } else { > > > + else > > > config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION); > > > - } > > > > > > ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3); > > > if (ret) > > > @@ -2123,6 +2116,13 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus, > > > else > > > return -ENODEV; > > > > > > + if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) > > > + chip->dac_bits = 12; > > > + else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) > > > + chip->dac_bits = 10; > > > + else > > > + chip->dac_bits = 8; > > > + > > > chip->ldac_pin = devm_gpiod_get_optional(dev, "adi,ldac", GPIOD_OUT_LOW); > > > if (IS_ERR(chip->ldac_pin)) { > > > ret = PTR_ERR(chip->ldac_pin); > >
diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c index e5e1f9d6357f..a9990e7f2a4d 100644 --- a/drivers/staging/iio/addac/adt7316.c +++ b/drivers/staging/iio/addac/adt7316.c @@ -651,17 +651,10 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev, u8 config3; int ret; - chip->dac_bits = 8; - - if (buf[0] == '1') { + if (buf[0] == '1') config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION; - if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) - chip->dac_bits = 12; - else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) - chip->dac_bits = 10; - } else { + else config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION); - } ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3); if (ret) @@ -2123,6 +2116,13 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus, else return -ENODEV; + if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516) + chip->dac_bits = 12; + else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517) + chip->dac_bits = 10; + else + chip->dac_bits = 8; + chip->ldac_pin = devm_gpiod_get_optional(dev, "adi,ldac", GPIOD_OUT_LOW); if (IS_ERR(chip->ldac_pin)) { ret = PTR_ERR(chip->ldac_pin);
The only assignment to dac_bits is in adt7316_store_da_high_resolution(). This function 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 these assignments since they're unnecessary for the dac high resolution functionality. Instead, assign a value to dac_bits in adt7316_probe() since the number of dac bits might be needed as soon as the device is registered and available to userspace. Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> --- drivers/staging/iio/addac/adt7316.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)