| Message ID | 20200831015539.26811-1-vadym.kochan@plvision.eu |
|---|---|
| Headers | show |
| Series | nvmem: add ONIE NVMEM cells provider | expand |
Hi Bartosz, On Mon, Aug 31, 2020 at 07:21:39PM +0200, Bartosz Golaszewski wrote: > On Mon, Aug 31, 2020 at 3:56 AM Vadym Kochan <vadym.kochan@plvision.eu> wrote: > > > > During nvmem_register() the nvmem core sends notifications when: > > > > - cell added > > - nvmem added > > > > and during these notifications some callback func may access the nvmem > > device, which will fail in case of at24 eeprom because regulator and pm > > are enabled after nvmem_register(). > > > > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> > > --- > > v3: > > 1) at24 driver enables regulator and pm state machine after nvmem > > registration which does not allow to use it on handing NVMEM_PRE_ADD event. > > > > drivers/misc/eeprom/at24.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > > index 2591c21b2b5d..26a23abc053d 100644 > > --- a/drivers/misc/eeprom/at24.c > > +++ b/drivers/misc/eeprom/at24.c > > @@ -692,10 +692,6 @@ static int at24_probe(struct i2c_client *client) > > nvmem_config.word_size = 1; > > nvmem_config.size = byte_len; > > > > - at24->nvmem = devm_nvmem_register(dev, &nvmem_config); > > - if (IS_ERR(at24->nvmem)) > > - return PTR_ERR(at24->nvmem); > > - > > i2c_set_clientdata(client, at24); > > > > err = regulator_enable(at24->vcc_reg); > > @@ -708,6 +704,13 @@ static int at24_probe(struct i2c_client *client) > > pm_runtime_set_active(dev); > > pm_runtime_enable(dev); > > > > + at24->nvmem = devm_nvmem_register(dev, &nvmem_config); > > + if (IS_ERR(at24->nvmem)) { > > + pm_runtime_disable(dev); > > + regulator_disable(at24->vcc_reg); > > + return PTR_ERR(at24->nvmem); > > + } > > + > > /* > > * Perform a one-byte test read to verify that the > > * chip is functional. > > -- > > 2.17.1 > > > > Good catch! Thanks for the patch. Can I queue it for fixes > independently from the earlier patches in this series? > > Bart Sure. Regards, Vadym Kochan
On Mon, Aug 31, 2020 at 3:56 AM Vadym Kochan <vadym.kochan@plvision.eu> wrote: > > During nvmem_register() the nvmem core sends notifications when: > > - cell added > - nvmem added > > and during these notifications some callback func may access the nvmem > device, which will fail in case of at24 eeprom because regulator and pm > are enabled after nvmem_register(). > > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> > --- > v3: > 1) at24 driver enables regulator and pm state machine after nvmem > registration which does not allow to use it on handing NVMEM_PRE_ADD event. > > drivers/misc/eeprom/at24.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > index 2591c21b2b5d..26a23abc053d 100644 > --- a/drivers/misc/eeprom/at24.c > +++ b/drivers/misc/eeprom/at24.c > @@ -692,10 +692,6 @@ static int at24_probe(struct i2c_client *client) > nvmem_config.word_size = 1; > nvmem_config.size = byte_len; > > - at24->nvmem = devm_nvmem_register(dev, &nvmem_config); > - if (IS_ERR(at24->nvmem)) > - return PTR_ERR(at24->nvmem); > - > i2c_set_clientdata(client, at24); > > err = regulator_enable(at24->vcc_reg); > @@ -708,6 +704,13 @@ static int at24_probe(struct i2c_client *client) > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > + at24->nvmem = devm_nvmem_register(dev, &nvmem_config); > + if (IS_ERR(at24->nvmem)) { > + pm_runtime_disable(dev); > + regulator_disable(at24->vcc_reg); > + return PTR_ERR(at24->nvmem); > + } > + > /* > * Perform a one-byte test read to verify that the > * chip is functional. > -- > 2.17.1 > Queued for fixes, Cc'ed stable and added Fixes: tag. Thanks! Bart
Hi Srinivas, On Fri, Sep 04, 2020 at 12:02:40PM +0100, Srinivas Kandagatla wrote: > Hi Vadym, > > Thanks for the patch, > On 31/08/2020 02:55, Vadym Kochan wrote: > > Add NVMEM_PRE_ADD notification step which is called before any cells > > binding - from lookup table or config, this allows to register cells > > in some specific layout (tlv) which should be parsed first and then > > registered. So there might be a cell parser driver which can register > > lookup table during this notification step. > > > This is going in right direction but totally not correct way to do it. > > 1> this is not scalable as any consumer that will register for this even > will have no idea of which what kind of parsing that provider needs. > It can work in your case but not really useful. > > 2> this is a consumer API, not the provider api. > > How about adding a "parse_cells" callback in struct nvmem_config along with > encoding type. > > > thanks, > srini > Looks like I missed main point here that this cells parser should be registered as nvmem provider. I will think on it. Thanks, > > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu> > > --- > > v3: > > 1) Update core.c changes by extending notification mechanism > > by adding new NVMEM_PRE_ADD event id which is called before lookup > > table cells binding, this allows for notification handler to > > register cells which require nvmem parsing. > > > > drivers/nvmem/core.c | 2 ++ > > include/linux/nvmem-consumer.h | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > > index 6cd3edb2eaf6..c48a69e0ebbe 100644 > > --- a/drivers/nvmem/core.c > > +++ b/drivers/nvmem/core.c > > @@ -668,6 +668,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > > goto err_device_del; > > } > > + blocking_notifier_call_chain(&nvmem_notifier, NVMEM_PRE_ADD, nvmem); > > + > > if (config->cells) { > > rval = nvmem_add_cells(nvmem, config->cells, config->ncells); > > if (rval) > > diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h > > index 052293f4cbdb..0f7107276756 100644 > > --- a/include/linux/nvmem-consumer.h > > +++ b/include/linux/nvmem-consumer.h > > @@ -50,6 +50,7 @@ enum { > > NVMEM_REMOVE, > > NVMEM_CELL_ADD, > > NVMEM_CELL_REMOVE, > > + NVMEM_PRE_ADD, > > }; > > #if IS_ENABLED(CONFIG_NVMEM) > >
Hi Srinivas, On Fri, Sep 04, 2020 at 02:23:10PM +0300, Vadym Kochan wrote: > Hi Srinivas, > > On Fri, Sep 04, 2020 at 12:02:40PM +0100, Srinivas Kandagatla wrote: > > Hi Vadym, > > > > Thanks for the patch, > > On 31/08/2020 02:55, Vadym Kochan wrote: > > > Add NVMEM_PRE_ADD notification step which is called before any cells > > > binding - from lookup table or config, this allows to register cells > > > in some specific layout (tlv) which should be parsed first and then > > > registered. So there might be a cell parser driver which can register > > > lookup table during this notification step. > > > > > This is going in right direction but totally not correct way to do it. > > > > 1> this is not scalable as any consumer that will register for this even > > will have no idea of which what kind of parsing that provider needs. > > It can work in your case but not really useful. > > > > 2> this is a consumer API, not the provider api. > > > > How about adding a "parse_cells" callback in struct nvmem_config along with > > encoding type. > > > > > > thanks, > > srini > > > > Looks like I missed main point here that this cells parser should be > registered as nvmem provider. I will think on it. > > Thanks, > I am trying to re-work this approach, but still I need to clarify something. It looks strange that this cells parser should be a nvmem provider (or I missed something) but I remember that you suggested about introducing something like nvmem parser. And adding nvmem parser looks more clear for me, because what it should do is just access the nvmem device during its registration and provide list of cells, that's all: struct nvmem_device *nvmem_register(const struct nvmem_config *config) { struct nvmem_cell_table table = { }; ... parser = find_nvmem_parser(); /* I think that cell lookups may be added on the parser's probe statically */ parser->parse_cells(parser->priv, nvmem, &table); ... } /* here I used struct nvmem_config, not sure it is a right way to mix nvmem's and parser's struct fields, so may be something like struct nvmem_parser_config might be introduced or fill the struct nvmem_parser directly by the driver and pass it to the registration func */ struct nvmem_parser *nvmem_parser_register(const struct nvmem_config *config) { ... } void nvmem_parser_unregister(struct nvmem_parser *parser) { ... } Regards, Vadym Kochan
This series adds cells provider for the ONIE TLV attributes which are stored on NVMEM device. It adds possibility to read the mac address (and other info) by other drivers. There is a special case for this driver that before registering cells the nvmem device should be first parsed to read the TLV layout which holds the cells data. v3: 1) Update core.c changes by extending notification mechanism by adding new NVMEM_PRE_ADD event id which is called before lookup table cells binding, this allows for notification handler to register cells which require nvmem parsing. 2) Update onie-cells.c to use nvmem notification to parse and register cells before device and cells are finally registered. Use subsys_init() macro which allows to probe before nvmem drivers. 3) at24 driver enables regulator and pm state machine after nvmem registration which does not allow to use it on handing NVMEM_PRE_ADD event. v2: 1) Fixed wrong memcmp comparison Vadym Kochan (3): nvmem: core: allow to register cells during nvmem registration nvmem: add ONIE NVMEM cells support misc: eeprom: at24: register nvmem only after eeprom is ready to use drivers/misc/eeprom/at24.c | 11 +- drivers/nvmem/Kconfig | 9 + drivers/nvmem/Makefile | 3 + drivers/nvmem/core.c | 2 + drivers/nvmem/onie-cells.c | 410 +++++++++++++++++++++++++++++++++ include/linux/nvmem-consumer.h | 1 + 6 files changed, 432 insertions(+), 4 deletions(-) create mode 100644 drivers/nvmem/onie-cells.c