Received-SPF: pass (google.com: domain of linux-kernel+bounces-118867-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <c0647197-5c87-4154-8a50-92e7dd2297ec@opensynergy.com>
Date: Tue, 26 Mar 2024 12:19:06 +0100
Subject: Re: [PATCH v2 3/3] virtio-spi: Add virtio SPI driver.
To: Haixu Cui <quic_haixcui@quicinc.com>,
 Viresh Kumar <viresh.kumar@linaro.org>
Cc: Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
 linux-kernel@vger.kernel.org, quic_ztu@quicinc.com,
 Matti Moell <Matti.Moell@opensynergy.com>,
 Mikhail Golubev <Mikhail.Golubev@opensynergy.com>
References: <20240304154342.44021-1-Harald.Mommer@opensynergy.com>
 <20240304154342.44021-4-Harald.Mommer@opensynergy.com>
 <99afc631-c02b-42da-a8d4-87c65087f390@quicinc.com>
 <5dedcd26-ed59-415f-b978-87a546a0816d@opensynergy.com>
 <20240320031253.eutoon7l7nkjehft@vireshk-i7>
 <8fbb904c-8d24-4d00-bff8-65f1e4bad5bb@quicinc.com>
Content-Language: en-US
From: Harald Mommer <harald.mommer@opensynergy.com>
In-Reply-To: <8fbb904c-8d24-4d00-bff8-65f1e4bad5bb@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?V1d2WnFWVXVYeTdoMHRFQlJlTFZqL3czbVkxaWxKZDVEaFlDc0NiakRBMFQ1?=
 =?utf-8?B?eHBOUHJxSEFaK2VsM1VMTUNzK2tGci9ZSlFkc2hFSkpzMW5qdXhldzFTV2pW?=
 =?utf-8?B?WEg5QU5yQzhtZUQ4MTkvVXV1a3JnRWdlbE5aSlErVzh1VW81SHRnYmhrOXlk?=
 =?utf-8?B?MU9YR2wzZG9zYXhmaEZWZG4rSi9RWHcrTmZXbjBVVWhBN2xHZW1nclVrZkZr?=
 =?utf-8?B?VmRNNUkxc0ljRjh6aDlEUlk3dWU3eTZuSGkzcTZBTDI3dVpwY01WYjQ5SXB3?=
 =?utf-8?B?L2ZXN0plTFdGVkpTQzNOQXVPUzBQQ3dzRmZ0elFWVk9YdGZTS1dzMnRYb2NH?=
 =?utf-8?B?MTFoWjA4dm5RbU5JRnROOUExTWsvTE9QSzRMZjM0S2RBMVNiMUpCaUpLV1FG?=
 =?utf-8?B?VXlWQmJzVGYrcnRwTzU4VVlxaFc5UExiZEdtanA4TlNCWENxeVFLbnVTODdv?=
 =?utf-8?B?V1U0d0drN1Q2RFM3bGZMWlg1ZFVlTFoxR0xLS3plV1NGMVJXbWt4OFNqcGRP?=
 =?utf-8?B?MEJaNWpJd3BvViswV0ErMTI4NUFuZTlCeFJSYjJjaXo2bTNiWndlQmZjS0VD?=
 =?utf-8?B?YnNKN2J2aVZvWkIyTHFmQy9TL1lIY1R6MHVCeUYxODNGd3d4RGFlbmdKS2ZZ?=
 =?utf-8?B?Wnora2FoeG5KQitINExJRlVTWlAyeWVUaE5hRmRGNDliTkF6N084MmJXYmx1?=
 =?utf-8?B?OUVGeGZEZFpBRWtFUHcrbTRhZkxKTDQxTkZNdlRNWHFIMnorMkFOdklpdTFo?=
 =?utf-8?B?aVdrUEJqaC84NGYxdFRLeVNqK0Zjd0RtZ0pBdmMvcENRelozZWtuaVk1bHU4?=
 =?utf-8?B?b1k3WWZSeFZ5WHlnYmFNcUtORTZiM2FIOVFVRGRsWEV6cXB6WUMwU1lRc3ZN?=
 =?utf-8?B?QnlBZFBIakxWanJaZzFOcFhSQ1ZtSktuOXp2c0E2MWFOQjBYUXNmSm4yYTd1?=
 =?utf-8?B?cENEay9DSnlDZ1h6aW9JOENBVTBXdm9rcjVDK3pEQ3VWY2dOaXFoWEI4L1FQ?=
 =?utf-8?B?bkc0UStBeHBLQkw0cVUxM2hJdG1Sb0tMTG5OSjBGaDNjVkowcHlhczhFeGV4?=
 =?utf-8?B?anhFbEZOeUhMc3RncUsvaFB4SFdQZHN3WFFwdmlZYkNIcW0vOXM2aytlbXFT?=
 =?utf-8?B?OTRhdGNyUS9ZazlMYlBRUHhXcXFSblRld2ZIcmwyWGhDNWNnSFIzamNzVXNx?=
 =?utf-8?B?ZUhRZC9PMXFyekZFc3htdGRrSFZBOHBQTC9pVmVSelhNN0ZqSEFBbTVMcjJh?=
 =?utf-8?B?SFVZMUVBa2JNUTNlMi9Da3dHNk4zbU9ON3ViM1Bqckhla1NOcDQycWVYVDdk?=
 =?utf-8?B?QWdlSUNZb0VuRENOOWxRL0p3RUh4NzBsWUFWemJWWC9kSFpuVkNieUJqdlgy?=
 =?utf-8?B?cE42aDZBTFYwUWR1N3dlWFBmaWpJSUNmQ0F3QkQzYnBtck1vUHFtY29rTjl4?=
 =?utf-8?B?N3BlbjBaWHVoRVQzYy9OVGtxamJuM01lM2EwU0tPVGZIdDNqemxwUEJNMTB4?=
 =?utf-8?B?VmtPejVwek5vbmJ4Ukl1TnU5MzdMb2VIUmY2cE1CNnlrVk9rNlZZOC9yYkRn?=
 =?utf-8?B?bFRSN0huTythQldoY0gzdC9GbWxWSWE1cXBiZVpUVllzQnQ5N2FHU3gvQnU3?=
 =?utf-8?B?bElzcnhsUWhoMm9ldklTSEFIQ0V0dmVvWDBlZjBuVHNwWUxUeDNrSkIxSE1l?=
 =?utf-8?B?dGZFSDJucGZEK0VFNU9EYmthdlJDbGV5VmRtYXBaQWpYUWhMUmdYWlBveXJP?=
 =?utf-8?B?ckVMMXJHdUhSdGx0Y09rU2VDTGxxZlFEYzQwWjJuRk5SVHB5TGowa3JybXdE?=
 =?utf-8?B?S1M5TUVJZlU1WmVxbi9YM1htR0l0Nm5oSC9kSmh1ZlV6MUx1eU5lSTVWZUxn?=
 =?utf-8?B?Z293Yk5CVFZ4QkxYdWJ3VksvQXVTQ0RWRkU2SU14ampsVk04c0pPbngyTW5z?=
 =?utf-8?B?ZnMxQllJNlZSeU0xN0JBbEJhRHlDYUV4ajVONlNrT1lTVCtlZ1lqV0NyaW1s?=
 =?utf-8?B?Rm8zWTJxSk40S3FTRHVEOC9DQjMwTjg0NGJ3YjBtVVRKcnZOdlBQUTllUEJT?=
 =?utf-8?B?KzRYSGh4V1Y2L2dwZVRqSitsdHJ2bkJyQkxYQ3BGVTcyVVc3VmhTM2IrMjRG?=
 =?utf-8?Q?sL8lAxyV2JUHiz3AyeA9M7EyV?=
X-OriginatorOrg: opensynergy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a71c7be9-8f95-4cc6-28d0-08dc4d868db6
X-MS-Exchange-CrossTenant-AuthSource: BEZP281MB2581.DEUP281.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Mar 2024 11:19:07.9888
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 800fae25-9b1b-4edc-993d-c939c4e84a64
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9pnsJ0GjeUV2go/oaNt7HJkyfYpNln3puW95rSO33XZLI4N/PVgwTACMc2DJYTW0HVigNO1he5EB4sBmlcvJ7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: FR6P281MB4063
X-TM-AS-ERS: 104.47.7.169-0.0.0.0
X-TMASE-Version: StarCloud-1.3-9.1.1015-28274.007
X-TMASE-Result: 10--14.223200-4.000000
X-TMASE-MatchedRID: fE0JoqABJp35ETspAEX/ngw4DIWv1jSVbU+XbFYs1xLnjJ+nnTOINl1H
	PAVML+u5FMKWPMuVgO3X0jf+J2tD8HZPiJta99mXbAQSvnY/AMVJnvfeG6RJLBD2yE25b9QtO80
	PnvMDYjDf69IM6MToGRTgrdLYcNhD3GKpf8GMgGRIhbHCNJtZqDsagavr4+KJBKO0biDL/NBoVZ
	galxlsblwBC+ObXMKeq/Jl1DTp3XzbBiuX+mNviHhmp8N0bz0uN0X64jGy2dZryOt9YYJsiAia1
	smLyWvDWNP1tVsAO7cX+nZswyazH+aCQmqvrVWusZIjJc0QK/bVLrJfyVWkTZAzrN62v5yEaGsP
	Gx9OkcRNnzYACfvJEAIX3FyA1B7Ja8QS26OstHdCPQBD3xA/3aryV/2jRq/XEgg3cwDHl/1O85J
	coT0w/W9dYCMo3/8Lv9rSd36EgUIj80Za3RRg8DSTRzEor+hcsV9ZIhTtxnAXkvxd87HT8a8qV5
	p41vn6dPa+kOXl+GY=
X-TMASE-XGENCLOUD: 2ebaf597-46dd-4b2b-9b92-d3d752227300-0-0-200-0
X-TM-Deliver-Signature: A4D90A08B5A0029B8968E70B5B65C827
X-TM-Addin-Auth: XY90kivpwnFA4KqlNN41cBzp6yjPiHIOdS5wRWf4GGkodMm63S6YDLKBIks
	sz41vp6kZNBx8mvcdbA+icHff/G2oEcypdEMD9lYjwrHSkFxYSRfAWldQOQ6WZU4iEQ/dDO8+yu
	f4BDCBKYz0xzXya8FUxDhuWaFGFXnZ52u8JQJVWN1yqlWEhZs7ZYRl/Gnqo5roDdRgIK9uai4o2
	W9qNWXuk3B74c5DNPHNj3zRPkrxHMA6tVsqPGO86BOoeZ8CpZrmVvG2Z/wIGWC/qBMPRMq0wmSX
	Cp/l4pihf3752i4=.sX7zeYq/gmG2DthBSwb4VeUETcDqJDNCOidFT+4mKPnB/ur/8PjqXFLG6h
	A+pcMBD/w16hWa7iBND1pIarL+7YzHcnMSX1jZmzedKsGBGRSvjZL5A1XyuFSgR8eHJhugvTM+Z
	EKozJVuMD82VGrd/udw3lI8w5NV6IqPhtH1fSJslSJHfLWX3SdbYMD+VsiJsckLsj0p2X5fQigy
	uCgEYM0ybH74UN07eTWFoI+ZDOn7SGKqoVnPgZjDWdpQITJGTevVmVHBzP4Qz4Xpf+8c/sckl9P
	Jo2RIy0tlbCdpLKZL4uTYVzILz82xueXJXifYN535FB/UtyHbNG5214YYJQ==
X-TM-Addin-ProductCode: EMS
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=opensynergy.com;
	s=TM-DKIM-20210503141657; t=1711451950;
	bh=unvlpPNbGLpXmsCJNtgGIa9l1RRZQsC1875UeDDl4+s=; l=3400;
	h=Date:To:From;
	b=U/UfK66iPcqeoCSL0D07926LsBs89P4OwqtGxvb8ziVtUC2bgxK0fjXGJXKHGZJue
	 /hqrOw8OxF9NmOpKBwZ9xNcAYmSZeNpOjcCT1nhBOeMKA61xa88vkvBRelAR2jYHPb
	 VT4OZHnSPmRQGXm/3xcoMTMx6mV8yLu+xcqG/wfNxou6rDLSjQzlDXOjWK3aQ5TYug
	 nWv6v5FBzzzmNFeIVoQ6A1y7GBM1gX8ElgjddLrqrS1W5hwqOM4KQPEyrzMn4+wM5y
	 q8bg5E40mN6Z8MelaEuh4ozvEgYj29TqC23Dq4Epo6sXArQsWxag+lVKNm6ONvfj/F
	 CyTAfG4ztZ1mw==

Hello,

On 26.03.24 10:26, Haixu Cui wrote:
>
>> Looking at how i2c-virtio does it, it should be tied to the device 
>> itself
>> instead of its parent:
>
> Yes, it is
>         ctrl->dev.of_node = vdev->dev.of_node;


got it when looking into Viresh's code changes.


>>
>> Right, some work was done in the past to standardize these compatibles:
>>
>> $ git log -p --stat --reverse 0d8c9e7d4b40..694a1116b405
>>
>     Here I would like the inner layer "spidev", to match then probe 
> the spidev driver, the "reg" is the chip select index. 
> "spi-max-frequency" is not necessary, while It doesn't matter.
>
>     You can also customize the inner layer to match your own driver.
>
>     In my test, I set the cs max number as 1, and in device tree node 
> inner layer, reg as 0. So certainly spidev driver probed and spidev0.0 
> is added successfully.
>
>     But then the driver proceed to the following code, chip select 
> index 0 device is created again, the driver fail with log: "chipselect 
> 0 already in use".


The following lines

     board_info.max_speed_hz = priv->max_freq_hz;
     board_info.bus_num = (u16)ctrl->bus_num;

     if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
         board_info.mode = SPI_MODE_0;
     else
         board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;

are moved before the sequence

     err = spi_register_controller(ctrl);
     if (err) {
         dev_err(&vdev->dev, "Cannot register controller\n");
         goto err_return;
     }

     if (vdev->dev.of_node) { // <=== This block is new
         dev_dbg(&vdev->dev, "Final setup triggered by DT child node\n");
         return 0;
     }

>
>         for (csi = 0; csi < ctrl->num_chipselect; csi++) {
>             dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
>             board_info.chip_select = csi;
>
>             if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH)) // 
> <=== This if else is moved up. It is csi invariant anyway.
>                 board_info.mode = SPI_MODE_0;
>             else
>                 board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
>
>             if (!spi_new_device(ctrl, &board_info)) {
>                 dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
>                 spi_unregister_controller(ctrl);
>                 err = -ENODEV;
>                 goto err_return;
>             }
>         }
>
>

This means when there is no device tree entry everything behaves as 
before (for loop executed) and if there is a device tree entry the chip 
select will be setup already by spi_register_controller() and the for 
loop need not and will not be executed not annoying anyone with 
"chipselect 0 already in use".

I was undecided whether to keep Viresh Kumar's "reviewed by" in the 
commit message because I'm unsure whether this is to be considered as a 
substantial change. Changes are small but program logic is changed (vs. 
change of comment, removal of trace, change of trace wording) so I think 
it may be considered as a substantial change and therefore I'll remove 
the "reviewed-by" now.