| Message ID | 5688E268.9090600@users.sourceforge.net |
|---|---|
| State | Rejected |
| Headers | show |
On Sun, 3 Jan 2016, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sun, 3 Jan 2016 09:19:32 +0100 > > Pass a value directly in a call of the function "regmap_update_bits" > instead of an extra initialisation for one local variable at the beginning. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/rtc/rtc-ab-b5ze-s3.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c > index 88f1d0b..fed52d0 100644 > --- a/drivers/rtc/rtc-ab-b5ze-s3.c > +++ b/drivers/rtc/rtc-ab-b5ze-s3.c > @@ -561,7 +561,6 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm, > { > struct abb5zes3_rtc_data *data = dev_get_drvdata(dev); > u8 regs[ABB5ZES3_TIMA_SEC_LEN]; > - u8 mask = ABB5ZES3_REG_TIM_CLK_TAC0 | ABB5ZES3_REG_TIM_CLK_TAC1; > int ret; > > /* Program given number of seconds to Timer A registers */ > @@ -575,7 +574,9 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm, > > /* Configure Timer A as a watchdog timer */ > ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK, > - mask, ABB5ZES3_REG_TIM_CLK_TAC1); > + ABB5ZES3_REG_TIM_CLK_TAC0 > + | ABB5ZES3_REG_TIM_CLK_TAC1, > + ABB5ZES3_REG_TIM_CLK_TAC1); This doesn't seem like an improvement. The concept (mask) has disappeared, the binary operation is strangely broken, and the function call has one more line of arguments, which all look sort of the same and thus are hard to understand. Don't underestimate the value of naming things. julia > if (ret) > dev_err(dev, "%s: failed to update timer\n", __func__); > > -- > 2.6.3 > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
>> ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK, >> - mask, ABB5ZES3_REG_TIM_CLK_TAC1); >> + ABB5ZES3_REG_TIM_CLK_TAC0 >> + | ABB5ZES3_REG_TIM_CLK_TAC1, >> + ABB5ZES3_REG_TIM_CLK_TAC1); > > This doesn't seem like an improvement. Interesting … > The concept (mask) has disappeared, I suggest to drop another local variable. Can the operator "Bitwise OR" be sufficient to indicate the concept "mask"? > the binary operation is strangely broken, Do you prefer an other source code formatting within the usual line length range? > and the function call has one more line of arguments, How should several long preprocessor symbols be combined together with indentation so that they will fit into the limit of 80 characters? > which all look sort of the same and thus are hard to understand. Is this an usual consequence from an ordinary name pattern? Regards, Markus
On Sun, 3 Jan 2016, SF Markus Elfring wrote: > >> ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK, > >> - mask, ABB5ZES3_REG_TIM_CLK_TAC1); > >> + ABB5ZES3_REG_TIM_CLK_TAC0 > >> + | ABB5ZES3_REG_TIM_CLK_TAC1, > >> + ABB5ZES3_REG_TIM_CLK_TAC1); > > > > This doesn't seem like an improvement. > > Interesting … > > > > The concept (mask) has disappeared, > > I suggest to drop another local variable. > Can the operator "Bitwise OR" be sufficient to indicate the concept "mask"? > > > > the binary operation is strangely broken, > > Do you prefer an other source code formatting within the usual line length range? > > > > and the function call has one more line of arguments, > > How should several long preprocessor symbols be combined together with indentation > so that they will fit into the limit of 80 characters? > > > > which all look sort of the same and thus are hard to understand. > > Is this an usual consequence from an ordinary name pattern? The original code was better. No 80 character problem, easy to distinguish one argument from another, moderately meaningful variable name, etc. julia
diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c index 88f1d0b..fed52d0 100644 --- a/drivers/rtc/rtc-ab-b5ze-s3.c +++ b/drivers/rtc/rtc-ab-b5ze-s3.c @@ -561,7 +561,6 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm, { struct abb5zes3_rtc_data *data = dev_get_drvdata(dev); u8 regs[ABB5ZES3_TIMA_SEC_LEN]; - u8 mask = ABB5ZES3_REG_TIM_CLK_TAC0 | ABB5ZES3_REG_TIM_CLK_TAC1; int ret; /* Program given number of seconds to Timer A registers */ @@ -575,7 +574,9 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm, /* Configure Timer A as a watchdog timer */ ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK, - mask, ABB5ZES3_REG_TIM_CLK_TAC1); + ABB5ZES3_REG_TIM_CLK_TAC0 + | ABB5ZES3_REG_TIM_CLK_TAC1, + ABB5ZES3_REG_TIM_CLK_TAC1); if (ret) dev_err(dev, "%s: failed to update timer\n", __func__);