On Fri, 22 Mar 2024 01:32:11 +0100 Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote: > On Wed, Mar 20, 2024 at 10:37 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > > > On 19/03/2024 22:27, Lothar Rubusch wrote: > > > Adds the spi-3wire feature and adds general refactoring to the > > > > Add > > > > > iio driver. > > > > > > The patch moves driver wide constants and fields into the > > > > Please do not use "This commit/patch/change", but imperative mood. See > > longer explanation here: > > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > > > > header. Thereby reduces redundant info struct definitions. > > > Allows to pass a function pointer from SPI/I2C specific probe, > > > and smaller refactorings. A regmap_update_bits() in the core > > > file replaces the regmap_write() to format_data. > > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx> > > > --- > > Agree > > > > static int adxl345_spi_probe(struct spi_device *spi) > > > { > > > + const struct adxl345_chip_info *chip_data; > > > struct regmap *regmap; > > > > > > + /* Retrieve device name to pass it as driver specific data */ > > > + chip_data = device_get_match_data(&spi->dev); > > > + if (!chip_data) > > > + chip_data = (const struct adxl345_chip_info *) spi_get_device_id(spi)->driver_data; > > > > Are you sure you need the cast? > > > > And why aren't you using spi_get_device_match_data()? > > Agree > > > > + > > > /* Bail out if max_speed_hz exceeds 5 MHz */ > > > if (spi->max_speed_hz > ADXL345_MAX_SPI_FREQ_HZ) > > > return dev_err_probe(&spi->dev, -EINVAL, "SPI CLK, %d Hz exceeds 5 MHz\n", > > > spi->max_speed_hz); > > > > > > regmap = devm_regmap_init_spi(spi, &adxl345_spi_regmap_config); > > > - if (IS_ERR(regmap)) > > > - return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n"); > > > + if (IS_ERR(regmap)) { > > > + dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing spi regmap: %ld\n", > > > + PTR_ERR(regmap)); > > > + return PTR_ERR(regmap); > > > > Why are you changing correct code into incorrect? > > I'll adjust that. It looks odd, I agree, but is this incorrect code? I found > similar code in the neighbor adxl313 accel driver. I may change/update > that then, too. Thank you for the hints. > > adxl313_i2c.c > 72 if (IS_ERR(regmap)) { > 73 dev_err(&client->dev, "Error initializing i2c regmap: %ld\n", > 74 PTR_ERR(regmap)); > 75 return PTR_ERR(regmap); > 76 } dev_err_probe() already pretty prints the error so no point in repeating it. It also returns the error value so you can do it on one line. dev_err() does neither of these things. Likely that axl313_i2c.c could be converted to use dev_err_probe() for all calls in the probe() function and anything only called as part of that. Patches welcome. Jonathan > > > > + } > > > > > > - return adxl345_core_probe(&spi->dev, regmap); > > > + return adxl345_core_probe(&spi->dev, regmap, chip_data, &adxl345_spi_setup); > > > } > > > > > > Best regards, > > Krzysztof > >
