Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 16 Dec 2018 11:59:17 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Jeremy Fertic <jeremyfertic@gmail.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Michael Hennerich <Michael.Hennerich@analog.com>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-iio@vger.kernel.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/11] staging: iio: adt7316: change interpretation of
 write to dac update mode
Message-ID: <20181216115917.0c9739eb@archlinux>
In-Reply-To: <20181212005503.28054-11-jeremyfertic@gmail.com>
References: <20181212005503.28054-1-jeremyfertic@gmail.com>
        <20181212005503.28054-11-jeremyfertic@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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)