Re: [PATCH v5 2/3] nvmem: core: deprecate reg_write callback with reg_write_const

[Srinivas Kandagatla <srini@kernel.org>]

On 8/4/26 2:59 PM, Link Mauve wrote:
> This callback used to take a mutable void * for no reason, which causes
> the compiler to be unaware that the val buffer should never be modified
> by the callback.
> 
> This was found while drafting the nvmem-provider Rust abstraction.
> 
> Signed-off-by: Link Mauve <linkmauve@linkmauve.fr>
> ---
>  drivers/nvmem/core.c           | 28 ++++++++++++++++------------
>  drivers/nvmem/internals.h      |  1 +
>  include/linux/nvmem-provider.h |  6 +++++-
>  3 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 0556d140170a..08f235874ce0 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -66,19 +66,22 @@ static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
>  }
>  
>  static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
> -			     void *val, size_t bytes)
> +			     const void *val, size_t bytes)
>  {
>  	struct nvmem_operations *ops = nvmem->ops;
>  	int ret, wr_ok;
>  
> -	if (!ops->reg_write)
> +	if (!ops->reg_write && !ops->reg_write_const)
>  		return -EOPNOTSUPP;
>  
>  	ret = gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
>  	if (ret)
>  		return ret;
>  
> -	wr_ok = ops->reg_write(nvmem->priv, offset, val, bytes);
> +	if (ops->reg_write_const)
> +		wr_ok = ops->reg_write_const(nvmem->priv, offset, val, bytes);
> +	else
> +		wr_ok = ops->reg_write(nvmem->priv, offset, (void *)val, bytes);
>  
>  	ret = gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
>  	if (ret)
> @@ -111,7 +114,7 @@ static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
>  			kend = min(end, keepout->start);
>  			ksize = kend - offset;
>  			if (write)
> -				rc = __nvmem_reg_write(nvmem, offset, val, ksize);
> +				rc = __nvmem_reg_write(nvmem, offset, (const void *)val, ksize);

these casts are redundant all such instances should be fixed.

>  			else
>  				rc = __nvmem_reg_read(nvmem, offset, val, ksize);
>  
> @@ -143,7 +146,7 @@ static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
>  	if (offset < end) {
>  		ksize = end - offset;
>  		if (write)
> -			return __nvmem_reg_write(nvmem, offset, val, ksize);
> +			return __nvmem_reg_write(nvmem, offset, (const void *)val, ksize);
>  		else
>  			return __nvmem_reg_read(nvmem, offset, val, ksize);
>  	}
> @@ -164,7 +167,7 @@ static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
>  			   void *val, size_t bytes)
>  {
>  	if (!nvmem->nkeepout)
> -		return __nvmem_reg_write(nvmem, offset, val, bytes);
> +		return __nvmem_reg_write(nvmem, offset, (const void *)val, bytes);
>  
>  	return nvmem_access_with_keepouts(nvmem, offset, val, bytes, true);
>  }
> @@ -299,7 +302,7 @@ static umode_t nvmem_bin_attr_get_umode(struct nvmem_device *nvmem)
>  	if (!nvmem->read_only)
>  		mode |= 0200;
>  
> -	if (!ops->reg_write)
> +	if (!ops->reg_write && !ops->reg_write_const)
>  		mode &= ~0200;
>  
>  	if (!ops->reg_read)
> @@ -336,13 +339,13 @@ static umode_t nvmem_attr_is_visible(struct kobject *kobj,
>  	struct nvmem_operations *ops = nvmem->ops;
>  
>  	/*
> -	 * If the device has no .reg_write operation, do not allow
> -	 * configuration as read-write.
> +	 * If the device has no .reg_write or .reg_write_const operation, do
> +	 * not allow configuration as read-write.
>  	 * If the device is set as read-only by configuration, it
>  	 * can be forced into read-write mode using the 'force_ro'
>  	 * attribute.
>  	 */
> -	if (attr == &dev_attr_force_ro.attr && !ops->reg_write)
> +	if (attr == &dev_attr_force_ro.attr && !ops->reg_write && !ops->reg_write_const)
>  		return 0;	/* Attribute not visible */
>  
>  	return attr->mode;
> @@ -893,7 +896,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  	if (!config->dev)
>  		return ERR_PTR(-EINVAL);
>  
> -	if (!config->reg_read && !config->reg_write)
> +	if (!config->reg_read && !config->reg_write && !config->reg_write_const)
>  		return ERR_PTR(-EINVAL);
>  
>  	nvmem = kzalloc_obj(*nvmem);
> @@ -937,6 +940,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  
>  	ops->reg_read = config->reg_read;
>  	ops->reg_write = config->reg_write;
> +	ops->reg_write_const = config->reg_write_const;
>  
>  	nvmem->owner = config->owner;
>  	if (!nvmem->owner && config->dev->driver)
> @@ -972,7 +976,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  		goto err_put_device;
>  
>  	nvmem->read_only = device_property_present(config->dev, "read-only") ||
> -			   config->read_only || !ops->reg_write;
> +			   config->read_only || (!ops->reg_write && !ops->reg_write_const);
>  
>  #ifdef CONFIG_NVMEM_SYSFS
>  	nvmem->dev.groups = nvmem_dev_groups;
> diff --git a/drivers/nvmem/internals.h b/drivers/nvmem/internals.h
> index 4e610deeaa7b..c1766f8fa482 100644
> --- a/drivers/nvmem/internals.h
> +++ b/drivers/nvmem/internals.h
> @@ -11,6 +11,7 @@
>  struct nvmem_operations {
>  	nvmem_reg_read_t	reg_read;
>  	nvmem_reg_write_t	reg_write;
> +	nvmem_reg_write_const_t	reg_write_const;
>  };
>  
>  struct nvmem_device {
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 6063fe5b7784..e944530999b4 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -20,6 +20,8 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
>  				void *val, size_t bytes);
>  typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
>  				 void *val, size_t bytes);
> +typedef int (*nvmem_reg_write_const_t)(void *priv, unsigned int offset,
> +				       const void *val, size_t bytes);
>  /* used for vendor specific post processing of cell data */
>  typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, int index,
>  					 unsigned int offset, void *buf,
> @@ -93,7 +95,8 @@ struct nvmem_cell_info {
>   * @root_only:	Device is accessibly to root only.
>   * @of_node:	If given, this will be used instead of the parent's of_node.
>   * @reg_read:	Callback to read data; return zero if successful.
> - * @reg_write:	Callback to write data; return zero if successful.
> + * @reg_write:	**DEPRECATED** - please use reg_write_const instead.

Marking here as deprecated is total waste as no one will read it, May be
move this message to the core while registering.


> + * @reg_write_const:	Callback to write data; return zero if successful.
>   * @size:	Device size.
>   * @word_size:	Minimum read/write access granularity.
>   * @stride:	Minimum read/write access stride.
> @@ -128,6 +131,7 @@ struct nvmem_config {
>  	struct device_node	*of_node;
>  	nvmem_reg_read_t	reg_read;
>  	nvmem_reg_write_t	reg_write;

flag this with compile time __deprecated flag.

> +	nvmem_reg_write_const_t	reg_write_const;
>  	int	size;
>  	int	word_size;
>  	int	stride;