Re: [PATCH v6 1/4] ipmi: ssif_bmc: Add SSIF BMC driver

[Quan Nguyen]

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@xxxxxxxxxxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Chuong Tran <chuong@xxxxxxxxxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Linux device driver of the BMC IPMI SSIF 
> > interface.");
> > +MODULE_LICENSE("GPL");
>
> [...]