All of lore.kernel.org
 help / color / mirror / Atom feed
From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
To: Dmitry Osipenko <digetx@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Mikko Perttunen <mperttunen@nvidia.com>
Cc: linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-clk@vger.kernel.org
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms
Date: Mon, 17 May 2021 10:04:21 -0400	[thread overview]
Message-ID: <1cde1d43-f139-cb90-395e-8f8fceb41bce@canonical.com> (raw)
In-Reply-To: <c120ecf5-7202-9f1d-6e70-a99db2f5335f@gmail.com>

On 17/05/2021 09:47, Dmitry Osipenko wrote:
> 17.05.2021 16:39, Krzysztof Kozlowski пишет:
>>>>>  #define DRAM_DEV_SEL_ALL			0
>>>>> -#define DRAM_DEV_SEL_0				(2 << 30)
>>>>> -#define DRAM_DEV_SEL_1				(1 << 30)
>>>>> +#define DRAM_DEV_SEL_0				(2u << 30)
>>>>> +#define DRAM_DEV_SEL_1				(1u << 30)
>>>>
>>>> Why not using BIT()? This would make even this 2<<30 less awkard...
>>>
>>> The bitfield 31:30 is a enum, 3 is a wrong value. Formally it's
>>> incorrect to use the BIT() macro here.
>>
>> Why "3"? BIT(31) is the same as 2<<30.
> 
> By 3 I meant BIT(31)|BIT(30). This bitfield is explicitly designated as
> a enum in the hardware documentation.

I understand it and using BIT() here does not mean someone has to set
both of them. BIT() is a helper pointing out that you want to toggle one
bit. It does not mean that it is allowed to do so always!

> 
>> It's common to use BIT for
>> register fields which do not accept all possible values. Now you
>> basically reimplement BIT() which is error-prone.
> 
> Could you please show couple examples? The common practice today is to
> use FIELD_PREP helpers, but this driver was written before these helpers
> existed.


There are plenty of such examples so I guess it would be easier to ask
you to provide counter ones. Few IT for enum-like registers found within 2 minutes:

https://elixir.bootlin.com/linux/latest/C/ident/MAX77620_CNFG_GPIO_INT_MASK
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/max77650-regulator.c#L18
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/tps6524x-regulator.c#L62
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/tps80031-regulator.c#L39
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/da9121-regulator.h#L200
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/da9121-regulator.h#L231

Best regards,
Krzysztof

  reply	other threads:[~2021-05-17 14:05 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-16 16:12 [PATCH v2 0/4] Enable compile-testing of Tegra memory drivers Dmitry Osipenko
2021-05-16 16:12 ` [PATCH v2 1/4] soc/tegra: fuse: Add missing stubs Dmitry Osipenko
2021-05-16 16:12 ` [PATCH v2 2/4] clk: tegra: Add stubs needed for compile-testing Dmitry Osipenko
2021-05-16 16:12 ` [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms Dmitry Osipenko
2021-05-17  6:26   ` Nathan Chancellor
2021-05-17 11:28   ` Krzysztof Kozlowski
2021-05-17 13:35     ` Dmitry Osipenko
2021-05-17 13:39       ` Krzysztof Kozlowski
2021-05-17 13:47         ` Dmitry Osipenko
2021-05-17 14:04           ` Krzysztof Kozlowski [this message]
2021-05-17 14:23             ` Dmitry Osipenko
2021-05-17 14:24   ` Krzysztof Kozlowski
2021-05-17 14:53     ` Dmitry Osipenko
2021-05-16 16:12 ` [PATCH v2 4/4] memory: tegra: Enable compile testing for all drivers Dmitry Osipenko
2021-05-17 11:33   ` Krzysztof Kozlowski
2021-05-17 11:32 ` [PATCH v2 0/4] Enable compile-testing of Tegra memory drivers 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=1cde1d43-f139-cb90-395e-8f8fceb41bce@canonical.com \
    --to=krzysztof.kozlowski@canonical.com \
    --cc=digetx@gmail.com \
    --cc=jonathanh@nvidia.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=mperttunen@nvidia.com \
    --cc=nathan@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=thierry.reding@gmail.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.