Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iio: potentiometer: max5432: update the non-volatile position
@ 2019-08-09 16:06 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
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Kaiser @ 2019-08-09 16:06 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Lars-Peter Clausen
  Cc: linux-iio, linux-kernel, Martin Kaiser

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>
---
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);
-- 
2.11.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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]
  2019-08-09 16:06 [PATCH] iio: potentiometer: max5432: update the non-volatile position Martin Kaiser
@ 2019-08-11  9:11 ` Jonathan Cameron
  2019-08-12 10:37   ` Martin Kaiser
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2019-08-11  9:11 UTC (permalink / raw)
  To: Martin Kaiser
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Lars-Peter Clausen,
	linux-iio, linux-kernel

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);


^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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]
  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
  2019-08-12 11:08     ` Phil Reid
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Kaiser @ 2019-08-12 10:37 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Lars-Peter Clausen,
	linux-iio, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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]
  2019-08-12 10:37   ` Martin Kaiser
@ 2019-08-12 11:08     ` Phil Reid
  2019-08-18 19:32       ` Jonathan Cameron
  0 siblings, 1 reply; 6+ messages in thread
From: Phil Reid @ 2019-08-12 11:08 UTC (permalink / raw)
  To: Martin Kaiser, Jonathan Cameron
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Lars-Peter Clausen,
	linux-iio, linux-kernel

G'day Martin / Jonathan,

On 12/08/2019 18:37, Martin Kaiser wrote:
> 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.
> 
The AD5272/AD5274 Digital Rheostat has a 50-times limit for programming the NV register.
So you want to be real sure that you want to set it.

I'd rather my system default to a known "safe" value for next boot than
set to whatever the last write was. So some kind of policy on setting this would
be nice. I personally think it's something that userspace should initiate via an explicit
command.

Writing the NV for the AD5272 is something I planned to add at some stage.
But so far the default factory values have worked ok.
It'd be nice for cross device consistency for any interface for this.


-- 
Regards
Phil Reid

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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]
  2019-08-12 11:08     ` Phil Reid
@ 2019-08-18 19:32       ` Jonathan Cameron
  2019-08-22  8:36         ` Phil Reid
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2019-08-18 19:32 UTC (permalink / raw)
  To: Phil Reid
  Cc: Martin Kaiser, Hartmut Knaack, Peter Meerwald-Stadler,
	Lars-Peter Clausen, linux-iio, linux-kernel

On Mon, 12 Aug 2019 19:08:12 +0800
Phil Reid <preid@electromag.com.au> wrote:

> G'day Martin / Jonathan,
> 
> On 12/08/2019 18:37, Martin Kaiser wrote:
> > 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...

I'm not keen on multiple channels as that is a fairly non obvious interface.
Definitely want to avoid writing all the time.

> > 
> > Whatever we come up with, we should at least make the max* chips behave
> > the same way.
> >   
> The AD5272/AD5274 Digital Rheostat has a 50-times limit for programming the NV register.
> So you want to be real sure that you want to set it.

Ouch, I new some were limited to a few thousand cycles, but 50 is rather nasty!

> 
> I'd rather my system default to a known "safe" value for next boot than
> set to whatever the last write was. So some kind of policy on setting this would
> be nice. I personally think it's something that userspace should initiate via an explicit
> command.
Agreed. I think we should look at an explicit write.

Perhaps an extra attribute on the channels?  Hence a shared_by_all version
could be used when there is only one write command.

> 
> Writing the NV for the AD5272 is something I planned to add at some stage.
> But so far the default factory values have worked ok.
> It'd be nice for cross device consistency for any interface for this.
> 
Agreed. This is an area that crept up on me, so we haven't enforced any
consistency on it yet.  However, we definitely should!

Thanks,

Jonathan


^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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]
  2019-08-18 19:32       ` Jonathan Cameron
@ 2019-08-22  8:36         ` Phil Reid
  0 siblings, 0 replies; 6+ messages in thread
From: Phil Reid @ 2019-08-22  8:36 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Martin Kaiser, Hartmut Knaack, Peter Meerwald-Stadler,
	Lars-Peter Clausen, linux-iio, linux-kernel

On 19/08/2019 03:32, Jonathan Cameron wrote:
> On Mon, 12 Aug 2019 19:08:12 +0800
> Phil Reid <preid@electromag.com.au> wrote:
> 
>> G'day Martin / Jonathan,
>>
>> On 12/08/2019 18:37, Martin Kaiser wrote:
>>> 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...
> 
> I'm not keen on multiple channels as that is a fairly non obvious interface.
> Definitely want to avoid writing all the time.
> 
>>>
>>> Whatever we come up with, we should at least make the max* chips behave
>>> the same way.
>>>    
>> The AD5272/AD5274 Digital Rheostat has a 50-times limit for programming the NV register.
>> So you want to be real sure that you want to set it.
> 
> Ouch, I new some were limited to a few thousand cycles, but 50 is rather nasty!
> 
>>
>> I'd rather my system default to a known "safe" value for next boot than
>> set to whatever the last write was. So some kind of policy on setting this would
>> be nice. I personally think it's something that userspace should initiate via an explicit
>> command.
> Agreed. I think we should look at an explicit write.
> 
> Perhaps an extra attribute on the channels?  Hence a shared_by_all version
> could be used when there is only one write command.

Yes, now the only question is what should it be called.

> 
>>
>> Writing the NV for the AD5272 is something I planned to add at some stage.
>> But so far the default factory values have worked ok.
>> It'd be nice for cross device consistency for any interface for this.
>>
> Agreed. This is an area that crept up on me, so we haven't enforced any
> consistency on it yet.  However, we definitely should!
> 
> Thanks,
> 
> Jonathan
> 
> 
> 


-- 
Regards
Phil Reid


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2019-08-12 11:08     ` Phil Reid
2019-08-18 19:32       ` Jonathan Cameron
2019-08-22  8:36         ` Phil Reid

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