[PATCH v5 14/15] mfd: pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Andy Shevchenko]

On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad at pensando.io> wrote:
>
> From: Brad Larson <blarson at amd.com>
>
> Add support for the AMD Pensando Elba SoC System Resource chip
> using the SPI interface.  The Elba SR is a Multi-function Device
> supporting device register access using CS0, smbus interface for
> FRU and board peripherals using CS1, dual Lattice I2C masters for
> transceiver management using CS2, and CS3 for flash access.

...

> +#include <linux/mfd/pensando-elbasr.h>
> +#include <linux/mfd/core.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/ioctl.h>
> +#include <linux/fs.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/compat.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spidev.h>
> +#include <linux/delay.h>

Keep it sorted?
It would easily tell that types.h is missed, but maybe other headers
are superfluous.

...

> +/* The main reason to have this class is to make mdev/udev create the
> + * /dev/pensrB.C character device nodes exposing our userspace API.
> + * It also simplifies memory management.  The device nodes
> + * /dev/pensrB.C are common across Pensando boards.
> + */

/*
 * The above style of multi-line
 * comments is for networking,
 * the rest uses a slightly different one.
 */

...

> +static DECLARE_BITMAP(minors, ELBASR_MAX_DEVS);
> +static unsigned int bufsiz = 4096;
> +
> +static LIST_HEAD(device_list);
> +static DEFINE_MUTEX(device_list_lock);

Is it all to reinvent IDA?

...

> +static ssize_t
> +elbasr_spi_sync(struct elbasr_data *elbasr_spi, struct spi_message *message)
> +{
> +       int status;
> +       struct spi_device *spi;
> +
> +       spin_lock_irq(&elbasr_spi->spi_lock);
> +       spi = elbasr_spi->spi;
> +       spin_unlock_irq(&elbasr_spi->spi_lock);

> +

Drop this blank line and see below.

> +       if (spi == NULL)
> +               status = -ESHUTDOWN;

if (!spi)
  return ...

> +       else

> +               status = spi_sync(spi, message);
> +
> +       if (status == 0)
> +               status = message->actual_length;
> +
> +       return status;

status = spi_sync(...);
if (status)
  return status;

return message->actual_length;

> +}

...

> +       if (status) {

> +               pr_debug("elbasr_spi: nothing for minor %d\n", iminor(inode));

We have a device pointer, don't we?

> +               goto err_find_dev;
> +       }

...

> +static const struct file_operations elbasr_spi_fops = {
> +       .owner =        THIS_MODULE,
> +       .write =        elbasr_spi_write,
> +       .read =         elbasr_spi_read,
> +       .unlocked_ioctl = elbasr_spi_ioctl,
> +       .compat_ioctl = elbasr_spi_compat_ioctl,
> +       .open =         elbasr_spi_open,
> +       .release =      elbasr_spi_release,
> +       .llseek =       no_llseek,
> +};


As far as I can see the code looks like a proxy for SPI via SPI. Is
that the correct interpretation? If so, why this code repeating _a
lot_ from SPI framework, including character device IOCTL? This is a
big question here and since there is missed documentation for ABI and
no points to userspace tools which are going to use this ABI (red
flag!) the code is no go.

...

> +static bool
> +elbasr_reg_readable(struct device *dev, unsigned int reg)

It's pretty much one line, can you reduce the number of LoCs by
reindenting your code a bit?

...

> +static bool
> +elbasr_reg_writeable(struct device *dev, unsigned int reg)

Ditto and so on.

...

> +       struct spi_transfer t[2] = { { 0 } };

Isn't  `{ }` enough?

...

> +       spi_message_add_tail(&t[0], &m);
> +       spi_message_add_tail(&t[1], &m);

spi_message_init_with_transfers() ?
Here and elsewhere.

...

> +       struct spi_transfer t[1] = { { 0 } };

Why does `struct spi_transfer t = { };` not work?!

...

> +static const struct regmap_config pensando_elbasr_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .cache_type = REGCACHE_NONE,
> +       .readable_reg = elbasr_reg_readable,
> +       .writeable_reg = elbasr_reg_writeable,
> +       .reg_read = elbasr_regs_read,
> +       .reg_write = elbasr_regs_write,
> +       .max_register = ELBASR_MAX_REG

Leave a comma here.

> +};

...

> +       elbasr->elbasr_regs = devm_regmap_init(&spi->dev, NULL, spi,
> +                                              &pensando_elbasr_regmap_config);
> +       if (IS_ERR(elbasr->elbasr_regs)) {
> +               ret = PTR_ERR(elbasr->elbasr_regs);
> +               dev_err(&spi->dev, "Failed to allocate register map: %d\n", ret);
> +               return ret;

return dev_err_probe(...);

> +       }
> +
> +       ret = devm_mfd_add_devices(&spi->dev, PLATFORM_DEVID_NONE,
> +                                  pensando_elbasr_subdev_info,
> +                                  ARRAY_SIZE(pensando_elbasr_subdev_info),
> +                                  NULL, 0, NULL);
> +       if (ret)
> +               dev_err(&spi->dev, "Failed to register sub-devices: %d\n", ret);
> +
> +       return ret;

Ditto.

...

> +       /* Add Elba reset driver sub-device */
> +       if (spi->chip_select == 0)
> +               elbasr_regs_setup(spi, elbasr);

You have an awful mixture of devm_ vs. non-devm_ calls. Either move
from devm_ completely, or switch to devm_ in the rest of the ->probe()
code.

...

> +static const struct of_device_id elbasr_spi_of_match[] = {
> +       { .compatible = "amd,pensando-elbasr" },
> +       { /* sentinel */ },

Comma is not needed in terminator entry.

> +};

...

> +static struct spi_driver elbasr_spi_driver = {
> +       .probe = elbasr_spi_probe,
> +       .driver = {
> +               .name = "elbasr",

> +               .of_match_table = of_match_ptr(elbasr_spi_of_match),

of_match_ptr() is useless here (look at your Kconfig) and in some
cases is harmful. No need to use this.

> +       },
> +};

...

> +#include <linux/cdev.h>
> +#include <linux/regmap.h>

mutex.h and types.h are missed, for example.
You need to use headers for direct use of. And in some cases forward
declarations can be used instead of including a header.

--
With Best Regards,
Andy Shevchenko