Re: [PATCH v3 2/3] hwmon: tmp513: Drop enum tmp51x_ids and variable id from struct tmp51x_data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux