From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755649AbcETRR4 (ORCPT ); Fri, 20 May 2016 13:17:56 -0400 Received: from smtp81.iad3a.emailsrvr.com ([173.203.187.81]:45797 "EHLO smtp81.iad3a.emailsrvr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751331AbcETRRz (ORCPT ); Fri, 20 May 2016 13:17:55 -0400 X-Auth-ID: abbotti@mev.co.uk X-Sender-Id: abbotti@mev.co.uk Subject: Re: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values To: Hartley Sweeten , "devel@driverdev.osuosl.org" References: <1463752162-15181-1-git-send-email-abbotti@mev.co.uk> <1463752162-15181-5-git-send-email-abbotti@mev.co.uk> Cc: Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" From: Ian Abbott Message-ID: <573F46C0.4050808@mev.co.uk> Date: Fri, 20 May 2016 18:17:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20/05/16 17:37, Hartley Sweeten wrote: > On Friday, May 20, 2016 6:49 AM, Ian Abbott wrote: >> Rename the macros for the PLX PCI 9080 LAS0RR and LAS1RR registers in >> "plx9080.h", using the prefix `PLX_LASRR_`. Make use of the `BIT(x)` >> and `GENMASK(h,l)` macros to define the values. >> >> Define a macro `PLX_LASRR_PREFETCH` for the "prefetchable memory" bit in >> this register, and define a macro `PLX_LASRR_MLOC_MASK` to mask the PCI >> memory location control bits. >> >> Signed-off-by: Ian Abbott >> --- > > [snip] > >> diff --git a/drivers/staging/comedi/drivers/plx9080.h b/drivers/staging/comedi/drivers/plx9080.h >> index 92d2480..8788117 100644 >> --- a/drivers/staging/comedi/drivers/plx9080.h >> +++ b/drivers/staging/comedi/drivers/plx9080.h >> @@ -54,14 +54,16 @@ struct plx_dma_desc { >> /* Local Address Space 1 Range Register */ >> #define PLX_REG_LAS1RR 0x00f0 >> >> -#define LRNG_IO 0x00000001 /* Map to: 1=I/O, 0=Mem */ >> -#define LRNG_ANY32 0x00000000 /* Locate anywhere in 32 bit */ >> -#define LRNG_LT1MB 0x00000002 /* Locate in 1st meg */ >> -#define LRNG_ANY64 0x00000004 /* Locate anywhere in 64 bit */ >> -/* bits that specify range for memory io */ >> -#define LRNG_MEM_MASK 0xfffffff0 >> -/* bits that specify range for normal io */ >> -#define LRNG_IO_MASK 0xfffffffc >> +#define PLX_LASRR_IO BIT(0) /* Map to: 1=I/O, 0=Mem */ >> +#define PLX_LASRR_ANY32 (BIT(1) * 0) /* Locate anywhere in 32 bit */ >> +#define PLX_LASRR_LT1MB (BIT(1) * 1) /* Locate in 1st meg */ >> +#define PLX_LASRR_ANY64 (BIT(1) * 2) /* Locate anywhere in 64 bit */ > > The (BIT(n) * x) looks ugly. You won't like the remaining patches then! FWIW, all the constants end up with the same type (unsigned long) this way. I have been looking for a solution to the problem where random people change something like this: #define MY_COOLREG_VAL_FOO (0 << 5) #define MY_COOLREG_VAL_BAR (1 << 5) #define MY_COOLREG_VAL_BAZ (2 << 5) to: #define MY_COOLREG_VAL_FOO (0 << 5) #define MY_COOLREG_VAL_BAR BIT(5) #define MY_COOLREG_VAL_BAZ (2 << 5) and this seemed like one way to do it. > These bit define the memory space encoding. I would prefer something > like this: > > #define PLX_LASSR_MLOC(x) (((x) & 0x3) << 1) > #define PLX_LASSR_MLOC_ANY32 PLX_LASSR_MLOC(0) > #define PLX_LASSR_MLOC_LT1MB PLX_LASSR_MLOC(1) > #define PLX_LASSR_MLOC_ANY64 PLX_LASSR_MLOC(2) > >> +#define PLX_LASRR_MLOC_MASK GENMASK(2, 1) /* Memory location bits */ > > I guess the GENMASK() macro is common but it's currently > not used by any of the comedi code. It is handy when matching it up with the data sheet though. I have a field that occupies bits 2 and 1. It also doesn't expose a fairly useless PLX_LASRR_MLOC() macro to the user of the header file. > > Using the macro above, the 'mask' would be: > > #define PLX_LASSR_MLOC_MASK PLX_LASSR_MLOC(3) > >> +#define PLX_LASRR_PREFETCH BIT(3) /* Memory is prefetchable */ >> +/* bits that specify range for memory space decode bits */ >> +#define PLX_LASRR_MEM_MASK GENMASK(31, 4) >> +/* bits that specify range for i/o space decode bits */ >> +#define PLX_LASRR_IO_MASK GENMASK(31, 2) > > I suppose the GENMASK() use makes sense for these. > > Regards, > Hartley > -- -=( Ian Abbott @ MEV Ltd. E-mail: )=- -=( Web: http://www.mev.co.uk/ )=-