From: Jonathan Cameron <jic23@kernel.org>
To: Lothar Rubusch <l.rubusch@gmail.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>,
lars@metafoo.de, Michael.Hennerich@analog.com,
linux-iio@vger.kernel.org, eraretuya@gmail.com
Subject: Re: [PATCH 1/2] iio: adxl345: add spi-3wire
Date: Sun, 24 Mar 2024 13:14:41 +0000 [thread overview]
Message-ID: <20240324131441.1d148869@jic23-huawei> (raw)
In-Reply-To: <CAFXKEHZr5S83sra6_eWPS+Hn03rFDPh5nmNHb9dfXYGjfpsx9g@mail.gmail.com>
On Fri, 22 Mar 2024 01:32:11 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Wed, Mar 20, 2024 at 10:37 AM Krzysztof Kozlowski <krzk@kernel.org> 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@gmail.com>
> > > ---
>
> 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
> >
next prev parent reply other threads:[~2024-03-24 13:14 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-19 21:27 [PATCH 0/2] iio: adxl345: add spi-3wire and refac Lothar Rubusch
2024-03-19 21:27 ` [PATCH 1/2] iio: adxl345: add spi-3wire Lothar Rubusch
2024-03-20 9:37 ` Krzysztof Kozlowski
2024-03-22 0:32 ` Lothar Rubusch
2024-03-22 5:37 ` Krzysztof Kozlowski
2024-03-22 6:58 ` Nuno Sá
2024-03-24 13:14 ` Jonathan Cameron [this message]
2024-03-20 10:34 ` Nuno Sá
2024-03-22 0:33 ` Lothar Rubusch
2024-03-19 21:27 ` [PATCH 2/2] iio: adxl345: update documentation for spi-3wire Lothar Rubusch
2024-03-20 9:34 ` Krzysztof Kozlowski
2024-03-22 0:27 ` Lothar Rubusch
2024-03-22 5:36 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240324131441.1d148869@jic23-huawei \
--to=jic23@kernel.org \
--cc=Michael.Hennerich@analog.com \
--cc=eraretuya@gmail.com \
--cc=krzk@kernel.org \
--cc=l.rubusch@gmail.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).