RE: [PATCH v6 01/12] spi: add driver for intel graphics on-die spi device

["Winkler, Tomas" <tomas.winkler@xxxxxxxxx>]

> 
> 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.