>
> 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.
This is how it is required by Linux spdx checker,
>
> > +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.
Good point, it's a new C feature.
>
> > + size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
> > + spi = kzalloc(size, GFP_KERNEL);
>
> Use at least array_size().
Regions is not fixed size array, it will not work.
>
> > + 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.
What are you suggesting
>
> > + 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().
As I understand kasprintf allocates memory, this is not required here.
>
> > +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...
It makes sure the data is set to NULL.
