Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965022AbbLOKFN (ORCPT ); Tue, 15 Dec 2015 05:05:13 -0500 Received: from mail-wm0-f49.google.com ([74.125.82.49]:35982 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964873AbbLOKEt (ORCPT ); Tue, 15 Dec 2015 05:04:49 -0500 Message-ID: <566FE5BD.4040101@linaro.org> Date: Tue, 15 Dec 2015 10:04:45 +0000 From: Srinivas Kandagatla User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Andrew Lunn , GregKH , maxime.ripard@free-electrons.com, wsa@the-dreams.de, broonie@kernel.org, vz@mleia.com CC: afd@ti.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/6] nvmem: Add backwards compatibility support for older EEPROM drivers. References: <1449583511-22521-1-git-send-email-andrew@lunn.ch> <1449583511-22521-3-git-send-email-andrew@lunn.ch> In-Reply-To: <1449583511-22521-3-git-send-email-andrew@lunn.ch> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6342 Lines: 192 Below are few comments. On 08/12/15 14:05, Andrew Lunn wrote: > Older drivers made an 'eeprom' file available in the /sys device > directory. Have the NVMEM core provide this to retain backwards > compatibility. > > Signed-off-by: Andrew Lunn > --- > drivers/nvmem/Kconfig | 7 ++++ > drivers/nvmem/core.c | 75 +++++++++++++++++++++++++++++++++++++++--- > include/linux/nvmem-provider.h | 10 ++++++ > 3 files changed, 88 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig > index bc4ea585b42e..b4e79ba7d502 100644 > --- a/drivers/nvmem/Kconfig > +++ b/drivers/nvmem/Kconfig > @@ -13,6 +13,13 @@ menuconfig NVMEM > If unsure, say no. > > if NVMEM > +config NVMEM_COMPAT > + bool "Enable /sys compatibility with old eeprom drivers" > + help > + Older EEPROM drivers, such as AT24, AT25, provide access to > + the eeprom via a file called "eeprom" in /sys under the > + device node. Enabling this option makes the NVMEM core > + provide this file to retain backwards compatibility > Lets get rid of this Kconfig as Wolfram suggested. We are already adding NVMEM_COMPAT in the nvmem_device structrure lets move the flags into the struct nvmem_config and use the nvmem_register api as it is. nvmem_register() can decide what do do with that from there. I would also prefer a warning if this flag is set, this is to deter any new users. Let me know your thoughts? > config NVMEM_IMX_OCOTP > tristate "i.MX6 On-Chip OTP Controller support" > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index 4ccf03da6467..75a498f5e139 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -38,8 +38,13 @@ struct nvmem_device { > int users; > size_t size; > bool read_only; > + int flags; > + struct bin_attribute eeprom; > + struct device *base_dev; > }; > > +#define FLAG_COMPAT BIT(0) > + > struct nvmem_cell { > const char *name; > int offset; > @@ -62,10 +67,16 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj, > struct bin_attribute *attr, > char *buf, loff_t pos, size_t count) > { > - struct device *dev = container_of(kobj, struct device, kobj); > - struct nvmem_device *nvmem = to_nvmem_device(dev); > + struct device *dev; > + struct nvmem_device *nvmem; > int rc; > > + if (attr->private) > + dev = attr->private; > + else > + dev = container_of(kobj, struct device, kobj); > + nvmem = to_nvmem_device(dev); > + > /* Stop the user from reading */ > if (pos >= nvmem->size) > return 0; > @@ -87,10 +98,16 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj, > struct bin_attribute *attr, > char *buf, loff_t pos, size_t count) > { > - struct device *dev = container_of(kobj, struct device, kobj); > - struct nvmem_device *nvmem = to_nvmem_device(dev); > + struct device *dev; > + struct nvmem_device *nvmem; > int rc; > > + if (attr->private) > + dev = attr->private; > + else > + dev = container_of(kobj, struct device, kobj); > + nvmem = to_nvmem_device(dev); > + > /* Stop the user from writing */ > if (pos >= nvmem->size) > return 0; > @@ -421,6 +438,53 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > } > EXPORT_SYMBOL_GPL(nvmem_register); > > +#if IS_ENABLED(CONFIG_NVMEM_COMPAT) > +/** > + * nvmem_register_compat() - Register a nvmem device for given nvmem_config. > + * Also creates an binary entry in /sys/bus/nvmem/devices/dev-name/nvmem and > + * an eeprom file in the drivers sys directory. > + * > + * @config: nvmem device configuration with which nvmem device is created. > + * @dev: device structure of underlying device > + * > + * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device > + * on success. > + */ > + > +struct nvmem_device *nvmem_register_compat(const struct nvmem_config *config, > + struct device *base_dev) > +{ Possibly move most of it or some of it a local static function which will be called from nvmem_register depending on the NVMEM_FLAG_COMPAT. > + struct nvmem_device *nvmem; > + int rval; > + > + nvmem = nvmem_register(config); > + if (IS_ERR(nvmem)) > + return nvmem; > + > + if (nvmem->read_only) > + nvmem->eeprom = bin_attr_ro_root_nvmem; > + else > + nvmem->eeprom = bin_attr_rw_root_nvmem; > + nvmem->eeprom.attr.name = "eeprom"; > + nvmem->eeprom.size = nvmem->size; > + nvmem->eeprom.private = &nvmem->dev; > + nvmem->base_dev = base_dev; > + > + rval = device_create_bin_file(nvmem->base_dev, &nvmem->eeprom); > + if (rval) { > + dev_err(&nvmem->dev, > + "Failed to create eeprom binary file %d\n", rval); > + nvmem_unregister(nvmem); > + return ERR_PTR(rval); > + } > + > + nvmem->flags |= FLAG_COMPAT; > + > + return nvmem; > +} > +EXPORT_SYMBOL_GPL(nvmem_register_compat); > +#endif /* CONFIG_NVMEM_COMPAT */ > + > /** > * nvmem_unregister() - Unregister previously registered nvmem device > * > @@ -437,6 +501,9 @@ int nvmem_unregister(struct nvmem_device *nvmem) > } > mutex_unlock(&nvmem_mutex); > > + if (nvmem->flags & FLAG_COMPAT) > + device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom); > + > nvmem_device_remove_all_cells(nvmem); > device_del(&nvmem->dev); > > diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h > index d24fefa0c11d..012030bd4495 100644 > --- a/include/linux/nvmem-provider.h > +++ b/include/linux/nvmem-provider.h > @@ -45,4 +45,14 @@ static inline int nvmem_unregister(struct nvmem_device *nvmem) > > #endif /* CONFIG_NVMEM */ > > +#if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_NVMEM_COMPAT) > +struct nvmem_device *nvmem_register_compat(const struct nvmem_config *config, > + struct device *base_dev); > +#else > +static inline struct nvmem_device * > +nvmem_register_compat(const struct nvmem_config *c, struct device *base_dev) > +{ > + return ERR_PTR(-ENOSYS); > +} > +#endif /* CONFIG_NVMEM && CONFIG_NVMEM_COMPAT */ > #endif /* ifndef _LINUX_NVMEM_PROVIDER_H */ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/