Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
From:   Brad Larson <blarson@amd.com>
To:     <andy.shevchenko@gmail.com>
CC:     <adrian.hunter@intel.com>, <alcooperx@gmail.com>, <arnd@arndb.de>,
        <blarson@amd.com>, <brendan.higgins@linux.dev>,
        <briannorris@chromium.org>, <broonie@kernel.org>,
        <catalin.marinas@arm.com>, <conor+dt@kernel.org>,
        <davidgow@google.com>, <devicetree@vger.kernel.org>,
        <fancer.lancer@gmail.com>, <gerg@linux-m68k.org>,
        <gsomlo@gmail.com>, <hal.feng@starfivetech.com>,
        <hasegawa-hitomi@fujitsu.com>, <j.neuschaefer@gmx.net>,
        <joel@jms.id.au>, <kernel@esmil.dk>, <krzk@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <lee.jones@linaro.org>,
        <lee@kernel.org>, <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-mmc@vger.kernel.org>,
        <linux-spi@vger.kernel.org>, <p.zabel@pengutronix.de>,
        <rdunlap@infradead.org>, <robh+dt@kernel.org>,
        <samuel@sholland.org>, <skhan@linuxfoundation.org>,
        <suravee.suthikulpanit@amd.com>, <thomas.lendacky@amd.com>,
        <tonyhuang.sunplus@gmail.com>, <ulf.hansson@linaro.org>,
        <vaishnav.a@ti.com>, <walker.chen@starfivetech.com>,
        <will@kernel.org>, <zhuyinbo@loongson.cn>
Subject: Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller
Date:   Tue, 26 Sep 2023 13:05:41 -0700
Message-ID: <20230926200541.35787-1-blarson@amd.com>
In-Reply-To: <CAHp75VfRLv1=3M+a9pr=ZJgNwtBOrT9xi0UjDJMuY8uM9+ffSw@mail.gmail.com>
References: <CAHp75VfRLv1=3M+a9pr=ZJgNwtBOrT9xi0UjDJMuY8uM9+ffSw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2023 20:05:51.8733
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b969c263-7d64-46f5-a98f-08dbbecbfc0e
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: BL6PEPF0001AB75.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5033
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Tue, 26 Sep 2023 13:06:01 -0700 (PDT)

Hi Andy,

On Thu, Sep 21, 2023 at 18:19:57 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Sep 14, 2023 at 12:52 AM Brad Larson <blarson@amd.com> wrote:
>>
>> The Pensando SoC controller is a SPI connected companion device
>> that is present in all Pensando SoC board designs.  The essential
>> board management registers are accessed on chip select 0 with
>> board mgmt IO support accessed using additional chip selects.

...

>> +#include <linux/cdev.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/fs.h>
>> +#include <linux/init.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/spi/spi.h>
>
> types.h ?

I'll add types.h

>> +#include <linux/uaccess.h>

...

>> +       struct penctrl_device *penctrl;
>
>> +       u8 tx_buf[PENCTRL_MAX_MSG_LEN];
>> +       u8 rx_buf[PENCTRL_MAX_MSG_LEN];
>
> These are not DMA-safe, is this a problem?

It's not a problem, the peripheral is PIO FIFO driven only.

>> +       struct spi_transfer t[2] = {};
>> +       struct penctrl_spi_xfer *msg;
>> +       struct spi_device *spi;
>> +       unsigned int num_msgs;
>> +       struct spi_message m;
>> +       u32 size;
>> +       int ret;

...

>> +       /* Verify and prepare SPI message */
>> +       size = _IOC_SIZE(cmd);
>> +       num_msgs = size / sizeof(struct penctrl_spi_xfer);
>
> sizeof (*msg) ?

Yes, more compact for here and below.

>
>> +       if (num_msgs > 2 || size == 0 || size % sizeof(struct penctrl_spi_xfer)) {
>
> Dito.
>
>> +               ret = -EINVAL;
>> +               goto out_unlock;
>> +       }

...

>> +       msg = memdup_user((struct penctrl_spi_xfer *)arg, size);
>> +       if (IS_ERR(msg)) {
>> +               ret = PTR_ERR(msg);
>> +               goto out_unlock;
>> +       }
>
> Wondering if you can start using cleanup.h.

Perhaps if recommended, I don't see DEFINE_(FREE,UNLOCK,...) being used.

...

>> +       /* Perform the transfer */
>> +       mutex_lock(&spi_lock);
>> +       ret = spi_sync(spi, &m);
>> +       mutex_unlock(&spi_lock);

>> +       if (ret || (num_msgs == 1))
>> +               goto out_unlock;
>
> Second conditional will return 0. Is it by design?
> Since it's not so obvious I would split these conditionals.

I'll split this to be clear, yes return 0 for success.

...

>> +       spi->chip_select = current_cs;
>
> spi_set_chipselect()

Yes, I'll change to inline function spi_set_chipselect(spi, 0, current_cs).  The
second arg must be legacy as its unused.

...

>> +static int penctrl_regs_read(struct penctrl_device *penctrl, u32 reg, u32 *val)
>> +{
>> +       struct spi_device *spi = penctrl->spi;
>> +       struct spi_transfer t[2] = {};
>> +       struct spi_message m;
>
>> +       u8 txbuf[3];
>> +       u8 rxbuf[1];
>
> Not DMA-safe. Is it a problem?
>

Not a problem, the peripheral is PIO only using FIFOs.

>> +       int ret;
>
>> +       txbuf[0] = PENCTRL_SPI_CMD_REGRD;
>> +       txbuf[1] = reg;
>> +       txbuf[2] = 0;
>
> Can be assigned in the definition block
>
>       u8 txbuf[] = { ... };
>

I'll change that here and below.

>> +       t[0].tx_buf = txbuf;
>> +       t[0].len = sizeof(txbuf);
>
>> +       rxbuf[0] = 0;
>
> Ditto.
>
>    u8 rxbuf[] = { 0 };
>
>> +       t[1].rx_buf = rxbuf;
>> +       t[1].len = sizeof(rxbuf);
>> +
>> +       spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
>> +       ret = spi_sync(spi, &m);
>> +       if (ret)
>> +               return ret;
>> +
>> +       *val = rxbuf[0];
>> +       return 0;
>> +}

...

>> +static int penctrl_regs_write(struct penctrl_device *penctrl, u32 reg, u32 val)
>> +{
>> +       struct spi_device *spi = penctrl->spi;
>> +       struct spi_transfer t = {};
>> +       struct spi_message m;
>> +       u8 txbuf[4];

>> +       txbuf[0] = PENCTRL_SPI_CMD_REGWR;
>> +       txbuf[1] = reg;
>> +       txbuf[2] = val;
>> +       txbuf[3] = 0;

> Can be assigned in the definition block.

>> +       t.tx_buf = txbuf;
>> +       t.len = sizeof(txbuf);
>> +       spi_message_init_with_transfers(&m, &t, 1);
>> +       return spi_sync(spi, &m);
>> +}

...

>> +       struct penctrl_device *penctrl =
>> +               container_of(rcdev, struct penctrl_device, rcdev);
>
> One line?

I'll check/change.

>
>...
>
>> +       spi->chip_select = 0;
>
> spi_set_chipselect()

Yes, spi_set_chipselect(spi, 0, 0);

...

>> +       struct penctrl_device *penctrl =
>> +               container_of(rcdev, struct penctrl_device, rcdev);
>
> One line?

I'll check/change.

...

>> +       spi->chip_select = 0;
>
> spi_set_chipselect()

Yes, spi_set_chipselect(spi, 0, 0);

...

>> +static int penctrl_spi_probe(struct spi_device *spi)
>> +{
>> +       int i, ret;
>> +
>> +       /* Allocate driver data */
>> +       penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
>
> devm_kzalloc() ?

Yes will change to devm_kzalloc().

>> +       if (!penctrl)
>> +               return -ENOMEM;
>> +
>> +       penctrl->spi = spi;
>> +       mutex_init(&spi_lock);
>> +
>> +       for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) {
>> +               ret = misc_register(&penctrl_devices[i]);
>> +               if (ret) {
>> +                       dev_err(&spi->dev, "Failed to register device %s\n",
>> +                               penctrl_devices[i].name);
>> +                       goto cleanup;
>> +               }
>> +       }
>> +
>> +       /* Register reset controller */
>> +       penctrl->rcdev.dev = &spi->dev;
>> +       penctrl->rcdev.ops = &penctrl_reset_ops;
>> +       penctrl->rcdev.owner = THIS_MODULE;
>> +       penctrl->rcdev.of_node = spi->dev.of_node;
>> +       penctrl->rcdev.nr_resets = 1;
>> +       device_set_node(penctrl->rcdev.dev, dev_fwnode(&spi->dev));
>> +
>> +       ret = reset_controller_register(&penctrl->rcdev);
>> +       if (ret)
>> +               return dev_err_probe(&spi->dev, ret,
>> +                                    "failed to register reset controller\n");
>> +       return 0;
>
>> +cleanup:
>
> err_cleanup: ?

Will use err_cleanup:

>> +       for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) {
>
>  while (i--) {
>

Yes, can change to while(), order doesn't matter.

>> +               if (penctrl_devices[i].this_device)
>> +                       misc_deregister(&penctrl_devices[i]);
>> +       }
>> +       return ret;
>> +}

Regards,
Brad