Message ID | 20230908055146.18347-6-Linhua.xu@unisoc.com |
---|---|
State | New |
Headers | show |
Series | pinctrl: sprd: Modification of UNIOC Platform pinctrl Driver | expand |
On Fri, Sep 08, 2023 at 01:51:45PM +0800, Linhua Xu wrote: > From: Linhua Xu <Linhua.Xu@unisoc.com> > > As the UNISOC pin controller version iterates, more registers are required > to meet new functional requirements. Thus modify them. LGTM, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On 9/8/2023 1:51 PM, Linhua Xu wrote: > From: Linhua Xu <Linhua.Xu@unisoc.com> > > As the UNISOC pin controller version iterates, more registers are required > to meet new functional requirements. Thus modify them. > > Signed-off-by: Linhua Xu <Linhua.Xu@unisoc.com> > --- > drivers/pinctrl/sprd/pinctrl-sprd.h | 30 +++++++++++++++-------------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/drivers/pinctrl/sprd/pinctrl-sprd.h b/drivers/pinctrl/sprd/pinctrl-sprd.h > index a696f81ce663..5357874186fd 100644 > --- a/drivers/pinctrl/sprd/pinctrl-sprd.h > +++ b/drivers/pinctrl/sprd/pinctrl-sprd.h > @@ -7,30 +7,32 @@ > #ifndef __PINCTRL_SPRD_H__ > #define __PINCTRL_SPRD_H__ > > +#include <linux/bits.h> > + > struct platform_device; > > -#define NUM_OFFSET (20) > -#define TYPE_OFFSET (16) > -#define BIT_OFFSET (8) > -#define WIDTH_OFFSET (4) > +#define NUM_OFFSET 22 > +#define TYPE_OFFSET 18 > +#define BIT_OFFSET 10 > +#define WIDTH_OFFSET 6 > > #define SPRD_PIN_INFO(num, type, offset, width, reg) \ > - (((num) & 0xFFF) << NUM_OFFSET | \ > - ((type) & 0xF) << TYPE_OFFSET | \ > - ((offset) & 0xFF) << BIT_OFFSET | \ > - ((width) & 0xF) << WIDTH_OFFSET | \ > - ((reg) & 0xF)) > + (((num) & GENMASK(10, 0)) << NUM_OFFSET | \ > + ((type) & GENMASK(3, 0)) << TYPE_OFFSET | \ > + ((offset) & GENMASK(7, 0)) << BIT_OFFSET | \ > + ((width) & GENMASK(3, 0)) << WIDTH_OFFSET | \ > + ((reg) & GENMASK(5, 0))) Can you define some readable macro for the mask bits? > > #define SPRD_PINCTRL_PIN(pin) SPRD_PINCTRL_PIN_DATA(pin, #pin) > > #define SPRD_PINCTRL_PIN_DATA(a, b) \ > { \ > .name = b, \ > - .num = (((a) >> NUM_OFFSET) & 0xfff), \ > - .type = (((a) >> TYPE_OFFSET) & 0xf), \ > - .bit_offset = (((a) >> BIT_OFFSET) & 0xff), \ > - .bit_width = ((a) >> WIDTH_OFFSET & 0xf), \ > - .reg = ((a) & 0xf) \ > + .num = (((a) & GENMASK(31, 22)) >> NUM_OFFSET), \ > + .type = (((a) & GENMASK(21, 18)) >> TYPE_OFFSET), \ > + .bit_offset = (((a) & GENMASK(17, 10)) >> BIT_OFFSET), \ > + .bit_width = (((a) & GENMASK(9, 6)) >> WIDTH_OFFSET), \ > + .reg = ((a) & GENMASK(5, 0)) \ Please keep the same logic operation as before, and you can reuse the readable macros if you defined. > } > > enum pin_type {
On 10/25/2023 7:29 PM, xu lh wrote: > On Tue, Sep 12, 2023 at 4:37 PM Baolin Wang <baolin.wang@linux.alibaba.com> > wrote: > >> >> >> On 9/8/2023 1:51 PM, Linhua Xu wrote: >>> From: Linhua Xu <Linhua.Xu@unisoc.com> >>> >>> As the UNISOC pin controller version iterates, more registers are >> required >>> to meet new functional requirements. Thus modify them. >>> >>> Signed-off-by: Linhua Xu <Linhua.Xu@unisoc.com> >>> --- >>> drivers/pinctrl/sprd/pinctrl-sprd.h | 30 +++++++++++++++-------------- >>> 1 file changed, 16 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/pinctrl/sprd/pinctrl-sprd.h >> b/drivers/pinctrl/sprd/pinctrl-sprd.h >>> index a696f81ce663..5357874186fd 100644 >>> --- a/drivers/pinctrl/sprd/pinctrl-sprd.h >>> +++ b/drivers/pinctrl/sprd/pinctrl-sprd.h >>> @@ -7,30 +7,32 @@ >>> #ifndef __PINCTRL_SPRD_H__ >>> #define __PINCTRL_SPRD_H__ >>> >>> +#include <linux/bits.h> >>> + >>> struct platform_device; >>> >>> -#define NUM_OFFSET (20) >>> -#define TYPE_OFFSET (16) >>> -#define BIT_OFFSET (8) >>> -#define WIDTH_OFFSET (4) >>> +#define NUM_OFFSET 22 >>> +#define TYPE_OFFSET 18 >>> +#define BIT_OFFSET 10 >>> +#define WIDTH_OFFSET 6 >>> >>> #define SPRD_PIN_INFO(num, type, offset, width, reg) \ >>> - (((num) & 0xFFF) << NUM_OFFSET | \ >>> - ((type) & 0xF) << TYPE_OFFSET | \ >>> - ((offset) & 0xFF) << BIT_OFFSET | \ >>> - ((width) & 0xF) << WIDTH_OFFSET | \ >>> - ((reg) & 0xF)) >>> + (((num) & GENMASK(10, 0)) << NUM_OFFSET | \ >>> + ((type) & GENMASK(3, 0)) << TYPE_OFFSET | \ >>> + ((offset) & GENMASK(7, 0)) << BIT_OFFSET | \ >>> + ((width) & GENMASK(3, 0)) << WIDTH_OFFSET | \ >>> + ((reg) & GENMASK(5, 0))) >> >> Can you define some readable macro for the mask bits? > > >>> okay. Do you think the following modification is okay? > +#define PIN_ID GENMASK(10, 0) > +#define PIN_TYPE GENMASK(3, 0) > +#define FIELD_OFFSET GENMASK(7, 0) > +#define FIELD_WIDTH GENMASK(3, 0) > +#define PIN_REG GENMASK(5, 0) Looks better. To keep consistent, I perfer to something as below: #define NUM_MASK xxx #define TYPE_MASK xxx #define BIT_MASK xxx #define WIDTH_MASK xxx #define REG_MASK xxx
diff --git a/drivers/pinctrl/sprd/pinctrl-sprd.h b/drivers/pinctrl/sprd/pinctrl-sprd.h index a696f81ce663..5357874186fd 100644 --- a/drivers/pinctrl/sprd/pinctrl-sprd.h +++ b/drivers/pinctrl/sprd/pinctrl-sprd.h @@ -7,30 +7,32 @@ #ifndef __PINCTRL_SPRD_H__ #define __PINCTRL_SPRD_H__ +#include <linux/bits.h> + struct platform_device; -#define NUM_OFFSET (20) -#define TYPE_OFFSET (16) -#define BIT_OFFSET (8) -#define WIDTH_OFFSET (4) +#define NUM_OFFSET 22 +#define TYPE_OFFSET 18 +#define BIT_OFFSET 10 +#define WIDTH_OFFSET 6 #define SPRD_PIN_INFO(num, type, offset, width, reg) \ - (((num) & 0xFFF) << NUM_OFFSET | \ - ((type) & 0xF) << TYPE_OFFSET | \ - ((offset) & 0xFF) << BIT_OFFSET | \ - ((width) & 0xF) << WIDTH_OFFSET | \ - ((reg) & 0xF)) + (((num) & GENMASK(10, 0)) << NUM_OFFSET | \ + ((type) & GENMASK(3, 0)) << TYPE_OFFSET | \ + ((offset) & GENMASK(7, 0)) << BIT_OFFSET | \ + ((width) & GENMASK(3, 0)) << WIDTH_OFFSET | \ + ((reg) & GENMASK(5, 0))) #define SPRD_PINCTRL_PIN(pin) SPRD_PINCTRL_PIN_DATA(pin, #pin) #define SPRD_PINCTRL_PIN_DATA(a, b) \ { \ .name = b, \ - .num = (((a) >> NUM_OFFSET) & 0xfff), \ - .type = (((a) >> TYPE_OFFSET) & 0xf), \ - .bit_offset = (((a) >> BIT_OFFSET) & 0xff), \ - .bit_width = ((a) >> WIDTH_OFFSET & 0xf), \ - .reg = ((a) & 0xf) \ + .num = (((a) & GENMASK(31, 22)) >> NUM_OFFSET), \ + .type = (((a) & GENMASK(21, 18)) >> TYPE_OFFSET), \ + .bit_offset = (((a) & GENMASK(17, 10)) >> BIT_OFFSET), \ + .bit_width = (((a) & GENMASK(9, 6)) >> WIDTH_OFFSET), \ + .reg = ((a) & GENMASK(5, 0)) \ } enum pin_type {