| Message ID | 20220901221857.2600340-14-michael@walle.cc |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | nvmem: core: introduce NVMEM layouts | expand |
On 01/09/2022 23:18, Michael Walle wrote: > It doesn't make any more sense to have a opaque pointer set up by the > nvmem device. Usually, the layout isn't associated with a particular > nvmem device. > This is really not a good idea to remove the context pointer, as this is the only way for callback to get context which it can make use of. I would prefer this to be left as it is. --srini > Signed-off-by: Michael Walle <michael@walle.cc> > --- > changes since v1: > - new patch > > drivers/nvmem/core.c | 4 ++-- > drivers/nvmem/imx-ocotp.c | 4 ++-- > include/linux/nvmem-provider.h | 5 +++-- > 3 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index d31d3f0ab517..6910796937f9 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -1523,8 +1523,8 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem, > nvmem_shift_read_buffer_in_place(cell, buf); > > if (cell->read_post_process) { > - rc = cell->read_post_process(nvmem->priv, id, index, > - cell->offset, buf, cell->bytes); > + rc = cell->read_post_process(id, index, cell->offset, buf, > + cell->bytes); > if (rc) > return rc; > } > diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c > index ac0edb6398f1..5e869d4a81c5 100644 > --- a/drivers/nvmem/imx-ocotp.c > +++ b/drivers/nvmem/imx-ocotp.c > @@ -222,8 +222,8 @@ static int imx_ocotp_read(void *context, unsigned int offset, > return ret; > } > > -static int imx_ocotp_cell_pp(void *context, const char *id, int index, > - unsigned int offset, void *data, size_t bytes) > +static int imx_ocotp_cell_pp(const char *id, int index, unsigned int offset, > + void *data, size_t bytes) > { > u8 *buf = data; > int i; > diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h > index 9d22dc5a3fa5..46067a6a0395 100644 > --- a/include/linux/nvmem-provider.h > +++ b/include/linux/nvmem-provider.h > @@ -19,8 +19,9 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset, > typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset, > 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, size_t bytes); > +typedef int (*nvmem_cell_post_process_t)(const char *id, int index, > + unsigned int offset, void *buf, > + size_t bytes); > > enum nvmem_type { > NVMEM_TYPE_UNKNOWN = 0,
Am 2022-09-09 10:52, schrieb Srinivas Kandagatla: > On 01/09/2022 23:18, Michael Walle wrote: >> It doesn't make any more sense to have a opaque pointer set up by the >> nvmem device. Usually, the layout isn't associated with a particular >> nvmem device. >> > This is really not a good idea to remove the context pointer, as this > is the only way for callback to get context which it can make use of. In which case? As I mentioned it's the priv to the nvmem driver and all the "normal" callbacks can do very little with it. If there will be a future need, then there should be a proper opaque pointer associated with the layout and not the nvmem driver. -michael > I would prefer this to be left as it is. > > --srini > >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> changes since v1: >> - new patch >> >> drivers/nvmem/core.c | 4 ++-- >> drivers/nvmem/imx-ocotp.c | 4 ++-- >> include/linux/nvmem-provider.h | 5 +++-- >> 3 files changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c >> index d31d3f0ab517..6910796937f9 100644 >> --- a/drivers/nvmem/core.c >> +++ b/drivers/nvmem/core.c >> @@ -1523,8 +1523,8 @@ static int __nvmem_cell_read(struct nvmem_device >> *nvmem, >> nvmem_shift_read_buffer_in_place(cell, buf); >> if (cell->read_post_process) { >> - rc = cell->read_post_process(nvmem->priv, id, index, >> - cell->offset, buf, cell->bytes); >> + rc = cell->read_post_process(id, index, cell->offset, buf, >> + cell->bytes); >> if (rc) >> return rc; >> } >> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c >> index ac0edb6398f1..5e869d4a81c5 100644 >> --- a/drivers/nvmem/imx-ocotp.c >> +++ b/drivers/nvmem/imx-ocotp.c >> @@ -222,8 +222,8 @@ static int imx_ocotp_read(void *context, unsigned >> int offset, >> return ret; >> } >> -static int imx_ocotp_cell_pp(void *context, const char *id, int >> index, >> - unsigned int offset, void *data, size_t bytes) >> +static int imx_ocotp_cell_pp(const char *id, int index, unsigned int >> offset, >> + void *data, size_t bytes) >> { >> u8 *buf = data; >> int i; >> diff --git a/include/linux/nvmem-provider.h >> b/include/linux/nvmem-provider.h >> index 9d22dc5a3fa5..46067a6a0395 100644 >> --- a/include/linux/nvmem-provider.h >> +++ b/include/linux/nvmem-provider.h >> @@ -19,8 +19,9 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned >> int offset, >> typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset, >> 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, size_t bytes); >> +typedef int (*nvmem_cell_post_process_t)(const char *id, int index, >> + unsigned int offset, void *buf, >> + size_t bytes); >> enum nvmem_type { >> NVMEM_TYPE_UNKNOWN = 0,
On 09/09/2022 09:58, Michael Walle wrote: > Am 2022-09-09 10:52, schrieb Srinivas Kandagatla: >> On 01/09/2022 23:18, Michael Walle wrote: >>> It doesn't make any more sense to have a opaque pointer set up by the >>> nvmem device. Usually, the layout isn't associated with a particular >>> nvmem device. >>> >> This is really not a good idea to remove the context pointer, as this >> is the only way for callback to get context which it can make use of. > > In which case? As I mentioned it's the priv to the nvmem driver and all > the "normal" callbacks can do very little with it. If there will be a > future need, then there should be a proper opaque pointer associated > with the layout and not the nvmem driver. Yes, the opaque object here is the layout priv which I agree with, but removing the context totally from the callback is not a good idea. We should have some context to callbacks to be able to allow them to deal with some private info. --srini > > -michael > >> I would prefer this to be left as it is. >> >> --srini >> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> changes since v1: >>> - new patch >>> >>> drivers/nvmem/core.c | 4 ++-- >>> drivers/nvmem/imx-ocotp.c | 4 ++-- >>> include/linux/nvmem-provider.h | 5 +++-- >>> 3 files changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c >>> index d31d3f0ab517..6910796937f9 100644 >>> --- a/drivers/nvmem/core.c >>> +++ b/drivers/nvmem/core.c >>> @@ -1523,8 +1523,8 @@ static int __nvmem_cell_read(struct >>> nvmem_device *nvmem, >>> nvmem_shift_read_buffer_in_place(cell, buf); >>> if (cell->read_post_process) { >>> - rc = cell->read_post_process(nvmem->priv, id, index, >>> - cell->offset, buf, cell->bytes); >>> + rc = cell->read_post_process(id, index, cell->offset, buf, >>> + cell->bytes); >>> if (rc) >>> return rc; >>> } >>> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c >>> index ac0edb6398f1..5e869d4a81c5 100644 >>> --- a/drivers/nvmem/imx-ocotp.c >>> +++ b/drivers/nvmem/imx-ocotp.c >>> @@ -222,8 +222,8 @@ static int imx_ocotp_read(void *context, unsigned >>> int offset, >>> return ret; >>> } >>> -static int imx_ocotp_cell_pp(void *context, const char *id, int >>> index, >>> - unsigned int offset, void *data, size_t bytes) >>> +static int imx_ocotp_cell_pp(const char *id, int index, unsigned int >>> offset, >>> + void *data, size_t bytes) >>> { >>> u8 *buf = data; >>> int i; >>> diff --git a/include/linux/nvmem-provider.h >>> b/include/linux/nvmem-provider.h >>> index 9d22dc5a3fa5..46067a6a0395 100644 >>> --- a/include/linux/nvmem-provider.h >>> +++ b/include/linux/nvmem-provider.h >>> @@ -19,8 +19,9 @@ typedef int (*nvmem_reg_read_t)(void *priv, >>> unsigned int offset, >>> typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset, >>> 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, size_t bytes); >>> +typedef int (*nvmem_cell_post_process_t)(const char *id, int index, >>> + unsigned int offset, void *buf, >>> + size_t bytes); >>> enum nvmem_type { >>> NVMEM_TYPE_UNKNOWN = 0,
Am 2022-09-09 11:08, schrieb Srinivas Kandagatla: > On 09/09/2022 09:58, Michael Walle wrote: >> Am 2022-09-09 10:52, schrieb Srinivas Kandagatla: >>> On 01/09/2022 23:18, Michael Walle wrote: >>>> It doesn't make any more sense to have a opaque pointer set up by >>>> the >>>> nvmem device. Usually, the layout isn't associated with a particular >>>> nvmem device. >>>> >>> This is really not a good idea to remove the context pointer, as this >>> is the only way for callback to get context which it can make use of. >> >> In which case? As I mentioned it's the priv to the nvmem driver and >> all >> the "normal" callbacks can do very little with it. If there will be a >> future need, then there should be a proper opaque pointer associated >> with the layout and not the nvmem driver. > > Yes, the opaque object here is the layout priv which I agree with, but > removing the context totally from the callback is not a good idea. > > We should have some context to callbacks to be able to allow them to > deal with some private info. I agree, but my thinking was this: as the old priv pointer doesn't make any sense and no one is using it at the moment for now, we can remove it it. If it's needed again it can easily added together with a user. That being said, I could leave the pointer argument and just pass NULL, so if someone (re)adds that, we don't have to modify all the callbacks. Because we don't have any user for now, I could only speculate who should or could set that pointer. -michael
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index d31d3f0ab517..6910796937f9 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -1523,8 +1523,8 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem, nvmem_shift_read_buffer_in_place(cell, buf); if (cell->read_post_process) { - rc = cell->read_post_process(nvmem->priv, id, index, - cell->offset, buf, cell->bytes); + rc = cell->read_post_process(id, index, cell->offset, buf, + cell->bytes); if (rc) return rc; } diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c index ac0edb6398f1..5e869d4a81c5 100644 --- a/drivers/nvmem/imx-ocotp.c +++ b/drivers/nvmem/imx-ocotp.c @@ -222,8 +222,8 @@ static int imx_ocotp_read(void *context, unsigned int offset, return ret; } -static int imx_ocotp_cell_pp(void *context, const char *id, int index, - unsigned int offset, void *data, size_t bytes) +static int imx_ocotp_cell_pp(const char *id, int index, unsigned int offset, + void *data, size_t bytes) { u8 *buf = data; int i; diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h index 9d22dc5a3fa5..46067a6a0395 100644 --- a/include/linux/nvmem-provider.h +++ b/include/linux/nvmem-provider.h @@ -19,8 +19,9 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset, typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset, 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, size_t bytes); +typedef int (*nvmem_cell_post_process_t)(const char *id, int index, + unsigned int offset, void *buf, + size_t bytes); enum nvmem_type { NVMEM_TYPE_UNKNOWN = 0,
It doesn't make any more sense to have a opaque pointer set up by the nvmem device. Usually, the layout isn't associated with a particular nvmem device. Signed-off-by: Michael Walle <michael@walle.cc> --- changes since v1: - new patch drivers/nvmem/core.c | 4 ++-- drivers/nvmem/imx-ocotp.c | 4 ++-- include/linux/nvmem-provider.h | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-)