Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Martin Kaiser <martin@kaiser.cx>
To: Jonathan Cameron <jic23@kernel.org>
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: Re: 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: Mon, 12 Aug 2019 12:37:51 +0200
Message-ID: <20190812103751.gumfzgazlytq5zqm@viti.kaiser.cx> (raw)
In-Reply-To: <20190811101137.5bd495e9@archlinux>

Hi Jonathan,

Thus wrote Jonathan Cameron (jic23@kernel.org):

> 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.

I see your point. You'd like a consistent bahaviour across all
potentiometer drivers. Or at least a way for user space to figure out
whether a chip uses non-volatile storage or not.
This property doesn't quite fit into the channel info that are defined
in the arrays in drivers/iio/industrialio-core.c. Is there any other way
to set such a property?

> 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.

drivers/iio/potentiometer/max5481.c and max5487.c do something similar.

They use a command to transfer the setting from volatile to non-volatile
register in the spi remove function. I guess that the intention is to
save the current setting when the system is rebooted. However, my
understanding is that the remove function is called only when a module
is unloaded or when user space does explicitly unbind the device from
the bus via sysfs. That's why I tried using the shutdown function
instead.

Still, this approach has some disadvantages. For many systems, there's a
soft reboot (shutdown -r) where the driver's shutdown function is called
and a "hard reboot" where the power from the CPU and the potentiometer
chip is removed and reapplied. In this case, the current value would not
be transfered to the non-volatile register. 

At least for the max5432 family, there's no way to read the current
setting. The only option for user space to have a well-defined setting
is to set the wiper position explicitly at startup.

I guess the only sensible way to use a non-volatile register would be a
write operation where user space gets a response about successful
completion.

We could have two channels to write to the volatile or to non-volatile
register. Or we stick to one channel and update both volatile and
non-volatile registers when user space changes the value. This assumes
that the setting does not change frequently, which is prabably not true
for all applications...

Whatever we come up with, we should at least make the max* chips behave
the same way.

Best regards,
Martin

  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 ` 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] Jonathan Cameron
2019-08-12 10:37   ` Martin Kaiser [this message]
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=20190812103751.gumfzgazlytq5zqm@viti.kaiser.cx \
    --to=martin@kaiser.cx \
    --cc=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=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 linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


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