Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Martin Kaiser <martin@kaiser.cx>
Cc: Hartmut Knaack <knaack.h@gmx.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Lars-Peter Clausen <lars@metafoo.de>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: iio: Is storing output values to non volatile registers something we should do automatically or leave to userspace action.  [was Re: [PATCH] iio: potentiometer: max5432: update the non-volatile position]
Date: Sun, 11 Aug 2019 10:11:37 +0100
Message-ID: <20190811101137.5bd495e9@archlinux> (raw)
In-Reply-To: <20190809160617.21035-1-martin@kaiser.cx>

On Fri,  9 Aug 2019 18:06:17 +0200
Martin Kaiser <martin@kaiser.cx> wrote:

> Keep track of the wiper position that was set by user space. Store the
> current wiper position in the chip's non-volatile register when the
> system is rebooted. This will be the initial position next time the
> max5432 chip is powered.
> 
> Update the register in the i2c client's shutdown function. Unlike the
> remove function, shutdown is called when the system is rebooted. It's
> safe to send an i2c message in the shutdown function.
> 
> Skip the update if user space never changed the wiper position.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>

Hi Martin,

The patch is fine, but I'm wondering about whether we need some element
of policy control on this restore to current value.

A few things to take into account.

* Some devices don't have a non volatile store.  So userspace will be
  responsible for doing the restore on reboot.
* This may be one of several related devices, and it may make no sense
  to restore this one if the others aren't going to be in the same
  state as before the reboot.
* Some devices only have non volatile registers for this sort of value
  (or save to non volatile on removal of power). Hence any policy to
  not store the value can't apply to this class of device.

My initial thought is that these probably don't matter that much and
we should apply this, but I would like to seek input from others!

I thought there were some other drivers doing similar store to no
volatile but I can't find one.

Thanks,

Jonathan

> ---
> The patch is against the testing branch in
> git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> It seems that this branch is not merged into linux-next.
> 
>  drivers/iio/potentiometer/max5432.c | 43 +++++++++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/potentiometer/max5432.c b/drivers/iio/potentiometer/max5432.c
> index 641b1821fdf6..73449078c89b 100644
> --- a/drivers/iio/potentiometer/max5432.c
> +++ b/drivers/iio/potentiometer/max5432.c
> @@ -16,16 +16,33 @@
>  
>  /* All chip variants have 32 wiper positions. */
>  #define MAX5432_MAX_POS 31
> +/*
> + * Initial value when the wiper position is unknown.
> + * (The chip does not support reading the current position.)
> + */
> +#define MAX5432_INVALID_POS (MAX5432_MAX_POS + 1)
>  
>  #define MAX5432_OHM_50K   (50  * 1000)
>  #define MAX5432_OHM_100K  (100 * 1000)
>  
>  /* Update the volatile (currently active) setting. */
>  #define MAX5432_CMD_VREG  0x11
> +/*
> + * Update the non-volatile setting that's used to initialize the wiper
> + * at startup.
> + */
> +#define MAX5432_CMD_NVREG 0x21
> +
> +/*
> + * Wiper position is in bits D7-D3 of the data byte.
> + * (D2-D0 are don't care bits.)
> + */
> +#define WIPER_POS_DATA(pos) ((pos) << 3)
>  
>  struct max5432_data {
>  	struct i2c_client *client;
>  	unsigned long ohm;
> +	u8 wiper_pos;
>  };
>  
>  static const struct iio_chan_spec max5432_channels[] = {
> @@ -63,7 +80,6 @@ static int max5432_write_raw(struct iio_dev *indio_dev,
>  			int val, int val2, long mask)
>  {
>  	struct max5432_data *data = iio_priv(indio_dev);
> -	u8 data_byte;
>  
>  	if (mask != IIO_CHAN_INFO_RAW)
>  		return -EINVAL;
> @@ -74,10 +90,10 @@ static int max5432_write_raw(struct iio_dev *indio_dev,
>  	if (val2 != 0)
>  		return -EINVAL;
>  
> -	/* Wiper position is in bits D7-D3. (D2-D0 are don't care bits.) */
> -	data_byte = val << 3;
> +	data->wiper_pos = val;
> +
>  	return i2c_smbus_write_byte_data(data->client, chan->address,
> -			data_byte);
> +			WIPER_POS_DATA(data->wiper_pos));
>  }
>  
>  static const struct iio_info max5432_info = {
> @@ -101,6 +117,7 @@ static int max5432_probe(struct i2c_client *client,
>  	data = iio_priv(indio_dev);
>  	data->client = client;
>  	data->ohm = (unsigned long)of_device_get_match_data(dev);
> +	data->wiper_pos = MAX5432_INVALID_POS;
>  
>  	indio_dev->dev.parent = dev;
>  	indio_dev->info = &max5432_info;
> @@ -111,6 +128,23 @@ static int max5432_probe(struct i2c_client *client,
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> +static void max5432_shutdown(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max5432_data *data;
> +
> +	indio_dev = i2c_get_clientdata(client);
> +	if (!indio_dev)
> +		return;
> +
> +	data = iio_priv(indio_dev);
> +	if (data->wiper_pos == MAX5432_INVALID_POS)
> +		return;
> +
> +	i2c_smbus_write_byte_data(client, MAX5432_CMD_NVREG,
> +			WIPER_POS_DATA(data->wiper_pos));
> +}
> +
>  static const struct of_device_id max5432_dt_ids[] = {
>  	{ .compatible = "maxim,max5432", .data = (void *)MAX5432_OHM_50K  },
>  	{ .compatible = "maxim,max5433", .data = (void *)MAX5432_OHM_100K },
> @@ -126,6 +160,7 @@ static struct i2c_driver max5432_driver = {
>  		.of_match_table = of_match_ptr(max5432_dt_ids),
>  	},
>  	.probe = max5432_probe,
> +	.shutdown = max5432_shutdown,
>  };
>  
>  module_i2c_driver(max5432_driver);


  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-09 16:06 [PATCH] iio: potentiometer: max5432: update the non-volatile position Martin Kaiser
2019-08-11  9:11 ` Jonathan Cameron [this message]
2019-08-12 10:37   ` iio: Is storing output values to non volatile registers something we should do automatically or leave to userspace action. [was Re: [PATCH] iio: potentiometer: max5432: update the non-volatile position] Martin Kaiser
2019-08-12 11:08     ` Phil Reid
2019-08-18 19:32       ` Jonathan Cameron
2019-08-22  8:36         ` Phil Reid

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190811101137.5bd495e9@archlinux \
    --to=jic23@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin@kaiser.cx \
    --cc=pmeerw@pmeerw.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git