On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:
> Add auxiliary driver for intel discrete graphics
> on-die spi device.
This doesn't actually do anything AFAICT? It doesn't register with any
subsystem or anything. Please don't submit empty boilerplate like this,
submit a driver that is at least minimally useful - assuming some other
patches in the series add functionality squash at least a basic set of
functionality into this. This makes review and test easier.
> +++ b/drivers/spi/spi-intel-dg.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> + */
Please make the entire comment a C++ one so things look more
intentional.
> +struct intel_dg_spi {
> + struct kref refcnt;
> + void __iomem *base;
> + size_t size;
> + unsigned int nregions;
> + struct {
> + const char *name;
> + u8 id;
> + u64 offset;
> + u64 size;
> + } regions[];
Use __counted_by() for the benefit of the static checkers.
> + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
> + spi = kzalloc(size, GFP_KERNEL);
Use at least array_size().
> + kref_init(&spi->refcnt);
This refcount feels more complex than just freeing in the error and
release paths, it's not a common pattern for drivers.
> + spi->nregions = nregions;
> + for (n = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) {
> + if (ispi->regions[i].name) {
> + name_size = strlen(dev_name(&aux_dev->dev)) +
> + strlen(ispi->regions[i].name) + 2; /* for point */
> + name = kzalloc(name_size, GFP_KERNEL);
> + if (!name)
> + continue;
> + snprintf(name, name_size, "%s.%s",
> + dev_name(&aux_dev->dev), ispi->regions[i].name);
kasprintf().
> +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev)
> +{
> + struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);
> +
> + if (!spi)
> + return;
> +
> + dev_set_drvdata(&aux_dev->dev, NULL);
If the above is doing anything there's a problem...
Attachment:
signature.asc
Description: PGP signature
