Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2,5/5] drivers: uio: new driver for fsl_85xx_cache_sram
To:     Wang Wenhu <wenhu.wang@vivo.com>
Cc:     gregkh@linuxfoundation.org, kernel@vivo.com,
        linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        mpe@ellerman.id.au, oss@buserror.net
References: <37b6b890-e537-7424-6b26-04565681f40a@c-s.fr>
 <20200416052247.112887-1-wenhu.wang@vivo.com>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <99ae46c0-1160-6ad2-e426-6dcc9191ab41@c-s.fr>
Date:   Thu, 16 Apr 2020 08:02:19 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200416052247.112887-1-wenhu.wang@vivo.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


Le 16/04/2020 à 07:22, Wang Wenhu a écrit :
> Yes, kzalloc() would clean the allocated areas and the init of remaining array
> elements are redundant. I will remove the block in v3.
> 
>>>> +		dev_err(&pdev->dev, "error no valid uio-map configured\n");
>>>> +		ret = -EINVAL;
>>>> +		goto err_info_free_internel;
>>>> +	}
>>>> +
>>>> +	info->version = "0.1.0";
>>>
>>> Could you define some DRIVER_VERSION in the top of the file next to
>>> DRIVER_NAME instead of hard coding in the middle on a function ?
>>
>> That's what v1 had, and Greg KH said to remove it.  I'm guessing that he
>> thought it was the common-but-pointless practice of having the driver print a
>> version number that never gets updated, rather than something the UIO API
>> (unfortunately, compared to a feature query interface) expects.  That said,
>> I'm not sure what the value is of making it a macro since it should only be
>> used once, that use is self documenting, it isn't tunable, etc.  Though if
>> this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
>> again, it should be UIO_VERSION, not DRIVER_VERSION).
>>
>> Does this really need a three-part version scheme?  What's wrong with a
>> version of "1", to be changed to "2" in the hopefully-unlikely event that the
>> userspace API changes?  Assuming UIO is used for this at all, which doesn't
>> seem like a great fit to me.
>>
>> -Scott
>>
> 
> As Scott mentioned, the version define as necessity by uio core but actually
> useless for us here(and for many other type of devices I guess). So maybe the better
> way is to set it optionally, but this belong first to uio core.
> 
> For the cache-sram uio driver, I will define an UIO_VERSION micro as a compromise
> fit all wonders, no confusing as Greg first mentioned.

Yes I like it.

> 
>>> +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>>> +	{	.compatible = "uio,fsl,p2020-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p2010-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1020-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1011-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1013-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1022-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,mpc8548-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,mpc8544-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,mpc8572-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,mpc8536-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1021-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1012-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1025-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1016-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1024-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1015-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,p1010-l2-cache-controller",	},
>>> +	{	.compatible = "uio,fsl,bsc9131-l2-cache-controller",	},
>>> +	{},
>>> +};
>>
>> NACK
>>
>> The device tree describes the hardware, not what driver you want to bind the
>> hardware to, or how you want to allocate the resources.  And even if defining
>> nodes for sram allocation were the right way to go, why do you have a separate
>> compatible for each chip when you're just describing software configuration?
>>
>> Instead, have module parameters that take the sizes and alignments you'd like
>> to allocate and expose to userspace.  Better still would be some sort of
>> dynamic allocation (e.g. open a fd, ioctl to set the requested size/alignment,
>> if it succeeds you can mmap it, and when the fd is closed the region is
>> freed).
>>
>> -Scott
>>
> 
> Can not agree more. But what if I want to define more than one cache-sram uio devices?
> How about use the device tree for pseudo uio cache-sram driver?
> 
> static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> 	{	.compatible = "uio,cache-sram",	},
> 	{},
> };
> 

You can still give it a name in line with your driver, ie: 
"uio,mpc85xx-cache-sram"

After, it you have different behaviours depending on the compatible, 
then you have to add a .data field which will tell the driver which 
behaviour to implement.

Christophe