2020-09-15 12:54:02

by Vadym Kochan

[permalink] [raw]
Subject: [PATCH 0/3] nvmem: add ONIE NVMEM cells provider

This series adds cells parser 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.

Because ONIE stores info in TLV format it should be parsed first and
then register the cells. Current NVMEM API allows to register cell
table with known cell's offset which is not guaranteed in case of TLV.

To make it properly handled the NVMEM parser object is introduced. The
parser needs to be registered before target NVMEM device is registered.
During the registration of NVMEM device the parser is called to parse
the device's cells and reister the cell table.

Vadym Kochan (3):
nvmem: core: introduce cells parser
nvmem: add ONIE nvmem cells parser
dt-bindings: nvmem: add description for ONIE cells parser

.../bindings/nvmem/onie,nvmem-cells.txt | 11 +
drivers/nvmem/Kconfig | 9 +
drivers/nvmem/Makefile | 3 +
drivers/nvmem/core.c | 80 ++++
drivers/nvmem/onie-cells.c | 370 ++++++++++++++++++
include/linux/nvmem-provider.h | 30 ++
6 files changed, 503 insertions(+)
create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
create mode 100644 drivers/nvmem/onie-cells.c

--
2.17.1


2020-09-16 00:49:03

by Vadym Kochan

[permalink] [raw]
Subject: [PATCH 3/3] dt-bindings: nvmem: add description for ONIE cells parser

Add device-tree binding description for the ONIE cells parser.

Signed-off-by: Vadym Kochan <[email protected]>
---
.../devicetree/bindings/nvmem/onie-nvmem-cells.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
create mode 100644 Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt

diff --git a/Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt b/Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt
new file mode 100644
index 000000000000..db06d8b297b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/onie-nvmem-cells.txt
@@ -0,0 +1,11 @@
+= Device tree bindings for ONIE cells parser =
+
+Required properties:
+- compatible: should be "onie-nvmem-cells"
+- nvmem: phandle to nvmem device node
+
+Example:
+ onie_cells {
+ compatible = "onie-nvmem-cells"
+ nvmem = <&at24>;
+ };
--
2.17.1

2020-09-22 00:59:45

by Vadym Kochan

[permalink] [raw]
Subject: Re: [PATCH 0/3] nvmem: add ONIE NVMEM cells provider

Hi Srinivas,

On Tue, Sep 15, 2020 at 03:41:13PM +0300, Vadym Kochan wrote:
> This series adds cells parser 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.
>
> Because ONIE stores info in TLV format it should be parsed first and
> then register the cells. Current NVMEM API allows to register cell
> table with known cell's offset which is not guaranteed in case of TLV.
>
> To make it properly handled the NVMEM parser object is introduced. The
> parser needs to be registered before target NVMEM device is registered.
> During the registration of NVMEM device the parser is called to parse
> the device's cells and reister the cell table.
>
> Vadym Kochan (3):
> nvmem: core: introduce cells parser
> nvmem: add ONIE nvmem cells parser
> dt-bindings: nvmem: add description for ONIE cells parser
>
> .../bindings/nvmem/onie,nvmem-cells.txt | 11 +
> drivers/nvmem/Kconfig | 9 +
> drivers/nvmem/Makefile | 3 +
> drivers/nvmem/core.c | 80 ++++
> drivers/nvmem/onie-cells.c | 370 ++++++++++++++++++
> include/linux/nvmem-provider.h | 30 ++
> 6 files changed, 503 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
> create mode 100644 drivers/nvmem/onie-cells.c
>
> --
> 2.17.1
>

I sent a newer version than this one which actually registers nvmem provider
and does not require changes in the core.c

Thanks,
Vadym Kochan

2020-09-22 09:52:20

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH 0/3] nvmem: add ONIE NVMEM cells provider



On 22/09/2020 00:56, Vadym Kochan wrote:
> Hi Srinivas,
>
> On Tue, Sep 15, 2020 at 03:41:13PM +0300, Vadym Kochan wrote:
>> This series adds cells parser 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.
>>
>> Because ONIE stores info in TLV format it should be parsed first and
>> then register the cells. Current NVMEM API allows to register cell
>> table with known cell's offset which is not guaranteed in case of TLV.
>>
>> To make it properly handled the NVMEM parser object is introduced. The
>> parser needs to be registered before target NVMEM device is registered.
>> During the registration of NVMEM device the parser is called to parse
>> the device's cells and reister the cell table.
>>
>> Vadym Kochan (3):
>> nvmem: core: introduce cells parser
>> nvmem: add ONIE nvmem cells parser
>> dt-bindings: nvmem: add description for ONIE cells parser
>>
>> .../bindings/nvmem/onie,nvmem-cells.txt | 11 +
>> drivers/nvmem/Kconfig | 9 +
>> drivers/nvmem/Makefile | 3 +
>> drivers/nvmem/core.c | 80 ++++
>> drivers/nvmem/onie-cells.c | 370 ++++++++++++++++++
>> include/linux/nvmem-provider.h | 30 ++
>> 6 files changed, 503 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
>> create mode 100644 drivers/nvmem/onie-cells.c
>>
>> --
>> 2.17.1
>>

Hi Vdaym,

Am totally confused with this patchset, There is no versioning in any of
your patches, you always send it with PATCH, please add version so that
I know which one should I review!

This makes my mailbox totally confused with all the patches with same
subject prefix!

Please note that maintenance is not my full time job, so please be
patient and I can try shift gears as an when possible!

>
> I sent a newer version than this one which actually registers nvmem provider
> and does not require changes in the core.c
This is a NO-NO as onie is not a real provider here, at24 is the actual
nvmem provider in your case.

Why do you keep changing the total approach here! what is the reasoning
to do so!
As I said in my last review we were okay with this parser approach!

I don't mind having changes in core as long as it done properly!


thanks,
srini




>
> Thanks,
> Vadym Kochan
>

2020-09-22 10:05:59

by Vadym Kochan

[permalink] [raw]
Subject: Re: [PATCH 0/3] nvmem: add ONIE NVMEM cells provider

Hi Srinivas,

On Tue, Sep 22, 2020 at 10:48:23AM +0100, Srinivas Kandagatla wrote:
>
>
> On 22/09/2020 00:56, Vadym Kochan wrote:
> > Hi Srinivas,
> >
> > On Tue, Sep 15, 2020 at 03:41:13PM +0300, Vadym Kochan wrote:
> > > This series adds cells parser 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.
> > >
> > > Because ONIE stores info in TLV format it should be parsed first and
> > > then register the cells. Current NVMEM API allows to register cell
> > > table with known cell's offset which is not guaranteed in case of TLV.
> > >
> > > To make it properly handled the NVMEM parser object is introduced. The
> > > parser needs to be registered before target NVMEM device is registered.
> > > During the registration of NVMEM device the parser is called to parse
> > > the device's cells and reister the cell table.
> > >
> > > Vadym Kochan (3):
> > > nvmem: core: introduce cells parser
> > > nvmem: add ONIE nvmem cells parser
> > > dt-bindings: nvmem: add description for ONIE cells parser
> > >
> > > .../bindings/nvmem/onie,nvmem-cells.txt | 11 +
> > > drivers/nvmem/Kconfig | 9 +
> > > drivers/nvmem/Makefile | 3 +
> > > drivers/nvmem/core.c | 80 ++++
> > > drivers/nvmem/onie-cells.c | 370 ++++++++++++++++++
> > > include/linux/nvmem-provider.h | 30 ++
> > > 6 files changed, 503 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/nvmem/onie,nvmem-cells.txt
> > > create mode 100644 drivers/nvmem/onie-cells.c
> > >
> > > --
> > > 2.17.1
> > >
>
> Hi Vdaym,
>
> Am totally confused with this patchset, There is no versioning in any of
> your patches, you always send it with PATCH, please add version so that I
> know which one should I review!
>
> This makes my mailbox totally confused with all the patches with same
> subject prefix!
>
> Please note that maintenance is not my full time job, so please be patient
> and I can try shift gears as an when possible!

Thank you!

>
> >
> > I sent a newer version than this one which actually registers nvmem provider
> > and does not require changes in the core.c
> This is a NO-NO as onie is not a real provider here, at24 is the actual
> nvmem provider in your case.
>
> Why do you keep changing the total approach here! what is the reasoning to
> do so!

Well, I though that maybe anyway it also better to show the code, and
try different approaches.

> As I said in my last review we were okay with this parser approach!
>
> I don't mind having changes in core as long as it done properly!
>
>
> thanks,
> srini

I am really sorry for this! I was really conceptually confused about how
to make it right by design. I will use "parser" word in next series
subject update with a versioning (v2) so you can identify that this is
related to parser approach with which you are conceptually fine.

Again sorry for this!

>
>
>
>
> >
> > Thanks,
> > Vadym Kochan
> >