>
> On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote:
> > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:
>
> > > > @@ -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,
>
> There is no incompatibility between SPDX and what I'm asking for...
>
> > > > + 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.
>
> Yes, that's the wrong helper - there is a relevent one though which I'm not
> remembering right now.
I don't think there is one, you can allocate arrays but this is not the case here.
>
> > > > + 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
>
> Just do normal open coded allocations, the reference counting is just
> obscure.
The kref here is for reason so we don't need to hunt the close open, I frankly don't understand
what is wrong with it,
> > > > + 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.
>
> There is a kzalloc() in the above code block?
Sorry you are right.
>
> > > > +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...
> o
> > It makes sure the data is set to NULL.
>
> Which is needed because...?
This is a boilerplate part, the content is consequent patches.
