Messages in this thread | | | Date | Mon, 26 Jun 2023 15:20:35 +0200 | From | Neil Armstrong <> | Subject | Re: [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC |
| |
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
| |