Re: [PATCH v5 01/16] spi: add basic support for SPI offloading

[Jonathan Cameron]

On Sun, 24 Nov 2024 12:01:23 -0600
David Lechner <dlechner@xxxxxxxxxxxx> wrote:

> On 11/24/24 10:32 AM, Jonathan Cameron wrote:
> > On Fri, 15 Nov 2024 14:18:40 -0600
> > David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> >   
> >> Add the basic infrastructure to support SPI offload providers and
> >> consumers.
> >>  
> 
> ...
> 
> >> +	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> >> +	if (!resource)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	resource->controller = spi->controller;
> >> +	resource->offload = spi->controller->get_offload(spi, config);
> >> +	ret = PTR_ERR_OR_ZERO(resource->offload);
> >> +	if (ret) {  
> > Why not simply
> > 	if (IS_ERR(resource->offload) {
> > 		kfree(resource);
> > 		return resource->offload;
> > 	}  
> >> +		kfree(resource);
> >> +		return ERR_PTR(ret);
> >> +	}  
> 
> Hmm... maybe somewhere along the way ret was being checked again
> after this, but doesn't to be the case anymore.
> 
> >> +
> >> +	ret = devm_add_action_or_reset(dev, spi_offload_put, resource);
> >> +	if (ret)
> >> +		return ERR_PTR(ret);
> >> +
> >> +	return resource->offload;
> >> +}
> >> +EXPORT_SYMBOL_GPL(devm_spi_offload_get);  
> >   
> >> diff --git a/include/linux/spi/spi-offload.h b/include/linux/spi/spi-offload.h
> >> new file mode 100644
> >> index 000000000000..81b115fc89bf
> >> --- /dev/null
> >> +++ b/include/linux/spi/spi-offload.h  
> >   
> >> +
> >> +MODULE_IMPORT_NS(SPI_OFFLOAD);  
> > 
> > This is rarely done in headers. (only pwm.h does it I think)
> > I'd push it down into code that uses this.  
> 
> Yes, it was Uwe that suggested that I put it in the header. :-)
> 
> Are there any unwanted side effects of having it in the header?
Reviewer surprise?
:)

Up to Mark as he gets to enjoy this code for ever.

> 
> > 
> > It might be worth splitting the header into a spi-offload-provider.h
> > and spi-offload-consumer.h with a common spi-offload-types.h included
> > by both.
> >