Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ba91c809-ad81-1f69-cc02-3f5cf28f6350@os.amperecomputing.com>
Date:   Thu, 17 Mar 2022 16:00:08 +0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.0
Subject: Re: [PATCH v6 1/4] ipmi: ssif_bmc: Add SSIF BMC driver
Content-Language: en-US
To:     Jae Hyun Yoo <quic_jaehyoo@quicinc.com>,
        Corey Minyard <minyard@acm.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@aj.id.au>,
        Brendan Higgins <brendanhiggins@google.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Wolfram Sang <wsa@kernel.org>,
        openipmi-developer@lists.sourceforge.net,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        linux-i2c@vger.kernel.org, openbmc@lists.ozlabs.org
Cc:     Open Source Submission <patches@amperecomputing.com>,
        "Thang Q . Nguyen" <thang@os.amperecomputing.com>,
        Phong Vo <phong@os.amperecomputing.com>
References: <20220310114119.13736-1-quan@os.amperecomputing.com>
 <20220310114119.13736-2-quan@os.amperecomputing.com>
 <7cd9301f-f417-e555-6025-74acfc53598a@quicinc.com>
From:   Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <7cd9301f-f417-e555-6025-74acfc53598a@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dGJ4M29KK3J2YWFPWW9FWEJkL25uMiswSVlBZTNKalU1a3ZPdVZ2SE5ubmh4?=
 =?utf-8?B?SlU3WHRDeFV2L2FreG1uY3dXc2xxSHpsS3RFSm1MaHFUTTZMaThqRlpCc3BP?=
 =?utf-8?B?UGJBcFJJSVNublY1dlo5NzFUaXFmdDZUdXhvM1hqSS8xRjBIVFhjNmJBTGtx?=
 =?utf-8?B?dTRBZHlHNnAwaHlUb0lYY3NVQzVvOFhTNjZxTEFaT28xWEo2T2thQlBob0Jp?=
 =?utf-8?B?Q3lBanVtVzFVNHJVeDFXVVpTc0dnbVlsWis5NUtZdm9NT1JFMlpma050VDRE?=
 =?utf-8?B?VmwzdnRVSVA0Q0N5cW12OWlPeVVjbVljQlJWOHRRUGRGV2JySk1mMjJFclNB?=
 =?utf-8?B?K1gybzVkUWVzQ2hNaUxYRTV2NmdNVXBJU1k2dDJ3ekhZamVhUTRXZ0hCdDU2?=
 =?utf-8?B?N1ptV0dFSEVsRTM3U3pUdjNVcXRpZjFiRHpRbWEwVTN4WnYyRmlsTkkvaHA1?=
 =?utf-8?B?eW54bGJpNHUwNERzUkM2ZldoWUtSYkFTSG9JR3ZBU1pKR2psMFByR09ac1dR?=
 =?utf-8?B?VjFjRUxJNmc2VlBZZjlURzdRb0ovVFJYTm4zRkdEQVNscENVaVB1WGZzYjBr?=
 =?utf-8?B?WEthd3ZXS2tOOW1sK3NRQzJxaTlYaEFkU1B0azhQSkFKWXVDU2doOGRjVEhL?=
 =?utf-8?B?V3o0T05uN0tPY3JDREtZYWFzVmV0emVraStscnlPREU3UTBVNkhhN2VqNXV4?=
 =?utf-8?B?dFgzZ000UXRtbVB4NmpWbHhuWWtIY3RHREFiYTBxdjNtMklMYUN3Qit3VUFZ?=
 =?utf-8?B?bW45NHl5Tm45Z3B3dFlmdUY1WHh3dUVPSFlhRUJsc3BPVEcwNmlSNWoxQUQ3?=
 =?utf-8?B?RU9YNVUwcU5Ra3BWTkxnZXhWaTh2L0FEK2VIVzN6aW1scERhZFc0ZWJvMXJa?=
 =?utf-8?B?aUlvS0ZmQlpKOEYzQmxGeFhTV1MydlprMERLS3pBZWdkdkZmWGQyVDNVRmM2?=
 =?utf-8?B?T0U0akRSQnFtb3poOVJVN1pzSTRlWGRHWE90Mm9Jai9WeXpiLzlkdmppekJ0?=
 =?utf-8?B?amJMRWQwdWViK1dHbXhCb1lVYUNmTVRPVkFFTmN4OTk1WkhSOGpSRjlnbkIw?=
 =?utf-8?B?QzVaQ094czR5QVFkT3pueWt2RnU1T0tsRk9kVlhSTm1oYlo4T1ppS25XZWwx?=
 =?utf-8?B?QVV4WU5nSkdzUUJKMkJ0NVFsNTdmZDdWWnAveEp5RUU3RXJxa0szUXp1dlQ5?=
 =?utf-8?B?UVd2UE1oWVQ2MXpyMGErZjVOZzZNRzZLUWxUWmJUaXZuQ2xQYUtYbnhvL3Nl?=
 =?utf-8?B?K0V3dGJrOVVpakNVSXg5SGNtNGpiTmpRSXV6dkFVZVEvSFphVjF0aUFvVXZ4?=
 =?utf-8?B?OWNFMmpIeUZTVlRVY2k5MzRuRFJNSTNtTzdlc0VFUDVmTG8yK20yM2ZrYzE4?=
 =?utf-8?B?T21FRmxmQmhIYm5DeGM3b0JDU2oraUpnbVRwOW93U3hxaHBwSEVUT2JrRFNX?=
 =?utf-8?B?SmpDV0hWencrb0cxNHB0dkUzbTlib1RVWVZ4MTNRWGVGSTJZS3JISmJISUlT?=
 =?utf-8?B?NUoyMkFlVUFqQjBxMHlLYmtlVU9NQnl2aDU2SXdFOTdPOWZ0UmhJSlQxcy9W?=
 =?utf-8?B?SUFqdi9rZm9FekYrV25vZVNOajA4US9ReFdCSmo3VDVLQVNCUXJnYUpZczhp?=
 =?utf-8?B?R21jek9ucWlpNlhZQjIzZ2EycVRKNzNTODhpdzF1Qk14NHpEbFIwdDlaOVhU?=
 =?utf-8?B?NjhGSjU5WGpvRFplNXZ6ZVd5Q3JGZXdadXBTSTNmR1BXV01wT01CUkF4d2Ra?=
 =?utf-8?B?NTkxKzZ2azN3SjdUbVU0NC9QWE1kNmMvaDJyR1lrTjFWWWlUaVZRQW1pWUF4?=
 =?utf-8?B?Zk01UDBRY2UvM1hFRzgzV0xxejcyRG9Mc3kzUkVYQkpqOGRjZDU2MS9NVmRK?=
 =?utf-8?B?Y1FJOFkvS0xSb0g0M3ViSk9QMk85VzFYRnAwZHhZNll2S3N5Z002S3g1VE01?=
 =?utf-8?B?YkMxd2F4S250VithVTN6SklDNUhZWlBxVGJzbFV4aHRHdDZHVjZ3S0N2Z29L?=
 =?utf-8?Q?sT+b06ivwDh56Z3p/DX2IlkmCi9uwU=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 617915d1-2e4d-4542-85a2-08da07f48f02
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR01MB7282.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2022 09:00:18.6578
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: honCA59C7zFdfmNs04QZq+tGoKoXKU2E/G5hM0OKnUjVOrKSfaDs56I0HyO/hLtnOQEAnHNsM0bYpzN4S05MV199iTWbYvSZwTE3DxLKN+M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR01MB5236
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 15/03/2022 01:15, Jae Hyun Yoo wrote:
> Hi Quan,
> 
> [...]
> 
>> +static void on_read_requested_event(struct ssif_bmc_ctx *ssif_bmc, u8 
>> *val)
>> +{
>> +    if (ssif_bmc->state == SSIF_READY ||
>> +        ssif_bmc->state == SSIF_START ||
>> +        ssif_bmc->state == SSIF_REQ_RECVING ||
>> +        ssif_bmc->state == SSIF_RES_SENDING) {
>> +        ssif_bmc->state = SSIF_BAD_SMBUS;
>> +        dev_warn(&ssif_bmc->client->dev,
>> +             "Warn: %s unexpected READ REQUESTED in state=%s\n",
>> +             __func__, state_to_string(ssif_bmc->state));
> 
> It will print out that the case happens in SSIF_BAD_SMBUS state always
> because ssif_bmc->state is already assigned as SSIF_BAD_SMBUS above.
> Move the assignment to after the dev_warn printing.
> 
> [...]
> 

Thanks for pointing this out. Will fix this in next version.

>> +static const struct of_device_id ssif_bmc_match[] = {
>> +    { .compatible = "ampere,ssif-bmc" },
> 
> Does this driver have any Ampere specific handling? If not, it could be
> "ssif-bmc" as a generic SSIF driver.
> 
Will change as you suggested in next version.

>> +    { },
>> +};
> 
> Add 'MODULE_DEVICE_TABLE(of, ssif_bmc_match);'
> 
Will do in next version
>> +
>> +static const struct i2c_device_id ssif_bmc_id[] = {
>> +    { DEVICE_NAME, 0 },
>> +    { },
>> +};
>> +
> 
> Remove this blank line.
> 
Will update in next version

>> +MODULE_DEVICE_TABLE(i2c, ssif_bmc_id);
>> +
>> +static struct i2c_driver ssif_bmc_driver = {
>> +    .driver         = {
>> +        .name           = DEVICE_NAME,
>> +        .of_match_table = ssif_bmc_match,
>> +    },
>> +    .probe          = ssif_bmc_probe,
>> +    .remove         = ssif_bmc_remove,
>> +    .id_table       = ssif_bmc_id,
>> +};
>> +
> 
> Remove this blank line too.
> 
> -Jae
>

Thanks Jae for the review, will update in next version.
- Quan

>> +module_i2c_driver(ssif_bmc_driver);
>> +
>> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
>> +MODULE_AUTHOR("Chuong Tran <chuong@os.amperecomputing.com>");
>> +MODULE_DESCRIPTION("Linux device driver of the BMC IPMI SSIF 
>> interface.");
>> +MODULE_LICENSE("GPL");
> 
> [...]