Re: [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 26/06/2023 15:01, Jeff LaBundy wrote:
Hi Neil,

On Mon, Jun 26, 2023 at 09:02:16AM +0200, Neil Armstrong wrote:

[...]

+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+	struct regmap_config *regmap_config;
+	struct regmap *regmap;
+	size_t max_size;
+	int error = 0;
+
+	regmap_config = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
+				     sizeof(*regmap_config), GFP_KERNEL);
+	if (!regmap_config)
+		return -ENOMEM;

Is there any reason we cannot simply pass goodix_berlin_spi_regmap_conf to
devm_regmap_init() below? Why to duplicate and pass the copy?

For reference, BMP280 in IIO is a similar example of a device with regmap
sitting atop a bespoke SPI protocol; it does not seem to take this extra
step.

The goodix_berlin_spi_regmap_conf copy is modified after with the correct
max raw read/write size, and I'm not a fan of modifying a global structure
that could be use for multiple probes, I can make a copy in a stack variable
if it feels simpler.

Ah, that makes sense; in that case, the existing implementation seems fine
to me. No changes necessary.

Correct me if I'm wrong, but the stack variable method wouldn't work since
that memory is gone after goodix_berlin_spi_probe() returns.

The config is only needed for the devm_regmap_init() duration, so keeping
the memory allocated for the whole lifetime of the device seems useless.

Neil


Kind regards,
Jeff LaBundy




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux