Re: [PATCH bluetooth-next 1/4] ieee802154/atusb: Add function for partial register writes

[Varka Bhadram <varkabhadram@gmail.com>]

Hi Stefan,

I found two checkpatch issues on this patch.. See inline

On 05/29/2015 03:16 AM, Stefan Schmidt wrote:

> From: Stefan Schmidt <s.schmidt@samsung.com>
>
> With this function we can set individual bits in the registers if needed.
> With the old SR_VALUE macro we could only set one bit in the register
> which was ok for some scenarios but not for all. With this subreg write
> function we can now set more bits defined by the mask while not touching
> the rest.
>
> We start using it for the current SR_VALUE use case and will use it more
> in upcoming patches.
>
> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
> ---
>   drivers/net/ieee802154/atusb.c | 44 +++++++++++++++++++++++++-----------------
>   1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c
> index 5b6bb9a..93431a5 100644
> --- a/drivers/net/ieee802154/atusb.c
> +++ b/drivers/net/ieee802154/atusb.c
> @@ -58,17 +58,6 @@ struct atusb {
>   	uint8_t tx_ack_seq;		/* current TX ACK sequence number */
>   };
>   
> -/* at86rf230.h defines values as <reg, mask, shift> tuples. We use the more
> - * traditional style of having registers and or-able values. SR_REG extracts
> - * the register number. SR_VALUE uses the shift to prepare a value accordingly.
> - */
> -
> -#define __SR_REG(reg, mask, shift)	(reg)
> -#define SR_REG(sr)			__SR_REG(sr)
> -
> -#define __SR_VALUE(reg, mask, shift, val)	((val) << (shift))
> -#define SR_VALUE(sr, val)			__SR_VALUE(sr, (val))
> -
>   /* ----- USB commands without data ----------------------------------------- */
>   
>   /* To reduce the number of error checks in the code, we record the first error
> @@ -130,6 +119,29 @@ static int atusb_read_reg(struct atusb *atusb, uint8_t reg)
>   	return ret >= 0 ? value : ret;
>   }
>   
> +static int atusb_write_subreg(struct atusb *atusb, uint8_t reg, uint8_t mask,
> +			      uint8_t shift, uint8_t value)
> +{
> +	struct usb_device *usb_dev = atusb->usb_dev;
> +	uint8_t orig, tmp;
> +	int ret = 0;
> +
> +	dev_dbg(&usb_dev->dev, "atusb_write_subreg: 0x%02x <- 0x%02x\n",
> +	    reg, value);

CHECK: Alignment should match open parenthesis
#56: FILE: drivers/net/ieee802154/atusb.c:130:
+	dev_dbg(&usb_dev->dev, "atusb_write_subreg: 0x%02x <- 0x%02x\n",
+	    reg, value);

> +
> +	orig = atusb_read_reg(atusb, reg);
> +
> +	/* Write the value only into that part of the register which is allowed
> +	 * by the mask. All other bits stay as before. */

WARNING: networking block comments put the trailing */ on a separate line
#61: FILE: drivers/net/ieee802154/atusb.c:135:
+	 * by the mask. All other bits stay as before. */

regards,

-- 
Varka Bhadram