From: Tudor Ambarus <tudor.ambarus@linaro.org>
To: Mark Brown <broonie@kernel.org>
Cc: Sam Protsenko <semen.protsenko@linaro.org>,
andi.shyti@kernel.org, krzysztof.kozlowski@linaro.org,
alim.akhtar@samsung.com, jassi.brar@samsung.com,
linux-spi@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, andre.draszik@linaro.org,
peter.griffin@linaro.org, kernel-team@android.com,
willmcvicker@google.com
Subject: Re: [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros
Date: Mon, 29 Jan 2024 17:59:37 +0000 [thread overview]
Message-ID: <6877683f-6018-4b29-a378-3a04c7b7f838@linaro.org> (raw)
In-Reply-To: <56f81e10-86b7-4bd6-938c-e2e9acd6a755@sirena.org.uk>
On 1/29/24 16:42, Mark Brown wrote:
> On Sat, Jan 27, 2024 at 03:44:24AM +0000, Tudor Ambarus wrote:
>> On 1/27/24 03:38, Sam Protsenko wrote:
>
>>>>>> - val |= S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD;
>>>>>> - val |= S3C64XX_SPI_MODE_CH_TSZ_HALFWORD;
>>>>>> + val |= FIELD_PREP(S3C64XX_SPI_MODE_BUS_TSZ_MASK,
>>>>>> + S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD) |
>>>>>> + FIELD_PREP(S3C64XX_SPI_MODE_CH_TSZ_MASK,
>>>>>> + S3C64XX_SPI_MODE_CH_TSZ_HALFWORD);
>
>>>>> Two people complained it makes the code harder to read. Yet it's not
>>>>> addressed in v3. Please see my comments for your previous submission
>>>>> explaining what can be done, and also Andi's comment on that matter.
>
>>>> I kept these intentionally. Please read my reply on that matter or the
>>>> cover letter to this patch set.
>
>>> I read it. But still don't like it 🙂 I'm sure it's possible to do
>>> this modification, but at the same time keep the code clean an easy to
>>> read. The code above -- I don't like at all, sorry. It was much better
>>> before this patch, IMHO.
>
>> Yeah, I guess Mark will tip the scale.
>
> All other things being equal I tend to try not to get too involved with
> minor coding style stuff in drivers. People do seem to like
> FIELD_PREP() but I have a hard time getting *too* excited.
Ok, I'll remove FIELD_PREP. Would you please consider the other patches,
all are simple. There's another "controversy" on 6/17. You can ignore
that as well maybe, and I'll resend it where I refrain myself to just
removing the cast.
prev parent reply other threads:[~2024-01-29 17:59 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-26 17:15 [PATCH v3 00/17] spi: s3c64xx: straightforward cleanup Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 01/17] spi: s3c64xx: explicitly include <linux/io.h> Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 02/17] spi: s3c64xx: explicitly include <linux/bits.h> Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 03/17] spi: s3c64xx: avoid possible negative array index Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 04/17] spi: s3c64xx: fix typo, s/configuartion/configuration Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 05/17] spi: s3c64xx: sort headers alphabetically Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 06/17] spi: s3c64xx: remove unneeded (void *) casts in of_match_table Tudor Ambarus
2024-01-26 20:17 ` Sam Protsenko
2024-01-27 3:41 ` Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 07/17] spi: s3c64xx: remove else after return Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 08/17] spi: s3c64xx: move common code outside if else Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 09/17] spi: s3c64xx: check return code of dmaengine_slave_config() Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 10/17] spi: s3c64xx: propagate the dma_submit_error() error code Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 11/17] spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma() Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 12/17] spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout() Tudor Ambarus
2024-01-26 20:13 ` Sam Protsenko
2024-01-27 3:26 ` Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 13/17] spi: s3c64xx: drop blank line between declarations Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 14/17] spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props Tudor Ambarus
2024-01-26 17:15 ` [PATCH v3 15/17] spi: s3c64xx: remove duplicated definition Tudor Ambarus
2024-01-26 20:06 ` Sam Protsenko
2024-01-26 17:15 ` [PATCH v3 16/17] spi: s3c64xx: drop a superfluous bitwise NOT operation Tudor Ambarus
2024-01-26 20:09 ` Sam Protsenko
2024-01-26 17:15 ` [PATCH v3 17/17] spi: s3c64xx: use bitfield access macros Tudor Ambarus
2024-01-26 20:12 ` Sam Protsenko
2024-01-27 3:23 ` Tudor Ambarus
2024-01-27 3:38 ` Sam Protsenko
2024-01-27 3:44 ` Tudor Ambarus
2024-01-29 16:42 ` Mark Brown
2024-01-29 17:59 ` Tudor Ambarus [this message]
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=6877683f-6018-4b29-a378-3a04c7b7f838@linaro.org \
--to=tudor.ambarus@linaro.org \
--cc=alim.akhtar@samsung.com \
--cc=andi.shyti@kernel.org \
--cc=andre.draszik@linaro.org \
--cc=broonie@kernel.org \
--cc=jassi.brar@samsung.com \
--cc=kernel-team@android.com \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=peter.griffin@linaro.org \
--cc=semen.protsenko@linaro.org \
--cc=willmcvicker@google.com \
/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).