On Fri, Aug 25, 2023 at 09:53:44PM +0100, Biju Das wrote:
> Drop variable id from struct tmp51x_data and enum tmp51x_ids as all the
> hw differences can be handled by max_channels.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v2->v3:
> * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit definitions.
> * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT.
> ---
[ ... ]
I had another look at those. First of all, using MASK and FIELD_PREP
for single-bit fields doesn't add value. Just drop the _MASK from
all those and just use BIT().
> +#define TMP51X_TEMP_CONFIG_GPM_MASK BIT(2)
GPM is bit 0 and 1, so this is wrong. This should probably
be TMP51X_TEMP_CONFIG_GP which is bit 2. It is also a read-only
value, so setting it is never warranted.
> +#define TMP51X_TEMP_CONFIG_RC_MASK BIT(10)
> +#define TMP51X_TEMP_CONFIG_CONT_MASK BIT(15)
Drop _MASK.
> +
> +#define TMP51X_TEMP_CONFIG_GPM FIELD_PREP(GENMASK(1, 0), 0)
Technically, using a 2-bit field here is misleading, since bit 1 defines
if the gpio bit is an input or output, and bit 0 defines the state of
the pin if it is an output. This would have to change if and when gpio
support is added to the driver, so it is best to not define anything GP
related in the first place.
> +#define TMP51X_TEMP_CONFIG_GP FIELD_PREP(TMP51X_TEMP_CONFIG_GPM_MASK, 0)
> +#define TMP51X_TEMP_CONFIG_CONV_RATE FIELD_PREP(GENMASK(9, 7), 0x7)
> +#define TMP51X_TEMP_CONFIG_RC FIELD_PREP(TMP51X_TEMP_CONFIG_RC_MASK, 1)
Those are really the values to be put into the configuration register,
which I find is just confusing. But define the register bits, set the bit
if needed, and otherwise keep it alone.
> +#define TMP51X_TEMP_CHANNEL_MASK(n) FIELD_PREP(GENMASK(14, 11), GENMASK(n, 0) > 1)
This is wrong. Either s/>/>>/ or GENMASK((n) - 1, 0). I personally prefer
the latter since I find it easier to understand.
> +#define TMP51X_TEMP_CONFIG_CONT FIELD_PREP(TMP51X_TEMP_CONFIG_CONT_MASK, 1)
> +
> +#define TMP51X_TEMP_CONFIG_DEFAULT(n) \
> + (TMP51X_TEMP_CONFIG_GPM | TMP51X_TEMP_CONFIG_GP | \
> + TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC | \
> + TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT)
> +
I would very much prefer something like
#define TMP51X_TEMP_CONFIG_DEFAULT(n) (TMP51X_TEMP_CONFIG_CONT | \
TMP51X_TEMP_CHANNEL_MASK(n) \
TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
and drop the unnecessary complexity of defining single bit values with
FIELD_PREP().
Guenter
