Re: [PATCH v2 1/3] nvmem: core: introduce cells parser

[Vadym Kochan]

Hi Srini,

Sorry for such delay in replies, I am still confused how to
implement it properly, let me please explain the issues
which I faced with:

Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> writes:

> On 16/06/2021 13:33, Vadym Kochan wrote:
>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>> index bca671ff4e54..648373ced6d4 100644
>>>> --- a/drivers/nvmem/core.c
>>>> +++ b/drivers/nvmem/core.c
>>>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>>>    	nvmem_reg_read_t	reg_read;
>>>>    	nvmem_reg_write_t	reg_write;
>>>>    	struct gpio_desc	*wp_gpio;
>>>> +	struct nvmem_parser_data *parser_data;
>>> This should be renamed to nvmem_cell_info_parser or something on those lines
>>> to avoid any misunderstanding on what exactly this parser is about.
>>>
>>> May be can totally avoid this by using parser name only during register.
>>>
>> I added this to keep parsed cells particulary for this nvmem in case
>> same parser is used for several nvmem's and mostly because of using also
>> cell lookup info. I will try to also answer your below question why do I need
>> lookups ?
>> 
>> I use of_get_mac_address() func to fetch mac-address from nvmem cell.
>> Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
>> correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
>> info of platform_device. I use the 2nd option with the following sample
>> solution:
>> 
>> 	## DT ##
>> 	eeprom_at24: at24@56 {
>> 		compatible = "atmel,24c32";
>> 		nvmem-cell-parser-name = "onie-tlv-cells";
>> 		reg = <0x56>;
>> 	};
>> 
>> 	onie_tlv_parser: onie-tlv-cells {
>> 		compatible = "nvmem-cell-parser";
>> 		status = "okay";
>> 
>> ---> add ability here to map cell con_id to cell_name ?
>> 
>> 	};
>> 
>> 	some_dev_node {
>> 		compatible = "xxx";
>> 		base-mac-provider = <&onie_tlv_parser>;
>
> Real nvmem provider is eeprom_at24, why do you use onie_tlv_parse as 
> your mac provider?
> If you use eeprom_at24 then of_get_mac_address() should get mac-address 
> directly from cell info.

1) This DT node is a trick to register it as a platform_device because of:

static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
{
        struct platform_device *pdev = of_find_device_by_node(np);
        struct nvmem_cell *cell;
        const void *mac;
        size_t len;
        int ret;
 
        /* Try lookup by device first, there might be a nvmem_cell_lookup
         * associated with a given device.
         */
        if (pdev) {
                ret = nvmem_get_mac_address(&pdev->dev, addr);
                put_device(&pdev->dev);
                return ret;
        }
 
        cell = of_nvmem_cell_get(np, "mac-address");
        if (IS_ERR(cell))
                return PTR_ERR(cell);
 
        ...
}

I tried to use at24_eeprom as ref in DTS file, but this device is not a
platform device but a nvmem bus device, so it fails on:

        ...

        struct platform_device *pdev = of_find_device_by_node(np);

        ...

        /* Try lookup by device first, there might be a nvmem_cell_lookup
         * associated with a given device.
         */
        if (pdev) {
                ret = nvmem_get_mac_address(&pdev->dev, addr);
                put_device(&pdev->dev);
                return ret;
        }

        ...

Probably this might be fixed by lookup nvmem device too ?

2) Regarding cell lookups registration, I had to use it because
of_nvmem_cell_get() will not find parser cells via OF.

>
>
>> 		status = "okay";
>> 	};
>> 	########
>> 
>> 	== CODE ==
>> 	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
>> 	ret = of_get_mac_address(base_mac_np, base_mac);
>> 	==========
>> 
>> 
>> And it works with this implementation because onie-tlv-cells is
>> registered as platform_device which name is the same as parser's name.
>> So the really tricky part for me is to make this cells lookup work.
>
> cell lookups are more of intended for board files, adding them in this 
> case is really not correct.  The whole purpose of this driver is to 
> parse the tlv cell infos into nvmem cell info.
>
>
> --srini
>
>
>> 
>> Of course would be great if you can point a way/idea to get rid the need of
>> lookups.
>>