We are using RXINT bits definition when looking at RXSTA register.
These bits are equal but it's not really proper.
Introduce the RXSTA bits and use them to have coherency.
Signed-off-by: Clément Péron <[email protected]>
---
drivers/media/rc/sunxi-cir.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
index 0504ebfc831f..572bd2257d35 100644
--- a/drivers/media/rc/sunxi-cir.c
+++ b/drivers/media/rc/sunxi-cir.c
@@ -48,11 +48,11 @@
/* Rx Interrupt Enable */
#define SUNXI_IR_RXINT_REG 0x2C
-/* Rx FIFO Overflow */
+/* Rx FIFO Overflow Interrupt Enable */
#define REG_RXINT_ROI_EN BIT(0)
-/* Rx Packet End */
+/* Rx Packet End Interrupt Enable */
#define REG_RXINT_RPEI_EN BIT(1)
-/* Rx FIFO Data Available */
+/* Rx FIFO Data Available Interrupt Enable */
#define REG_RXINT_RAI_EN BIT(4)
/* Rx FIFO available byte level */
@@ -60,6 +60,12 @@
/* Rx Interrupt Status */
#define SUNXI_IR_RXSTA_REG 0x30
+/* Rx FIFO Overflow */
+#define REG_RXSTA_ROI BIT(0)
+/* Rx Packet End */
+#define REG_RXSTA_RPE BIT(1)
+/* Rx FIFO Data Available */
+#define REG_RXSTA_RA BIT(4)
/* RX FIFO Get Available Counter */
#define REG_RXSTA_GET_AC(val) (((val) >> 8) & (ir->fifo_size * 2 - 1))
/* Clear all interrupt status value */
@@ -119,7 +125,7 @@ static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
/* clean all pending statuses */
writel(status | REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
- if (status & (REG_RXINT_RAI_EN | REG_RXINT_RPEI_EN)) {
+ if (status & (REG_RXSTA_RA | REG_RXSTA_RPE)) {
/* How many messages in fifo */
rc = REG_RXSTA_GET_AC(status);
/* Sanity check */
@@ -135,9 +141,9 @@ static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
}
}
- if (status & REG_RXINT_ROI_EN) {
+ if (status & REG_RXSTA_ROI) {
ir_raw_event_reset(ir->rc);
- } else if (status & REG_RXINT_RPEI_EN) {
+ } else if (status & REG_RXSTA_RPE) {
ir_raw_event_set_idle(ir->rc, true);
ir_raw_event_handle(ir->rc);
}
--
2.20.1
On Tue, Jun 04, 2019 at 06:29:50PM +0200, Cl?ment P?ron wrote:
> We are using RXINT bits definition when looking at RXSTA register.
>
> These bits are equal but it's not really proper.
>
> Introduce the RXSTA bits and use them to have coherency.
>
> Signed-off-by: Cl?ment P?ron <[email protected]>
> ---
> drivers/media/rc/sunxi-cir.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index 0504ebfc831f..572bd2257d35 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c
> @@ -48,11 +48,11 @@
>
> /* Rx Interrupt Enable */
> #define SUNXI_IR_RXINT_REG 0x2C
> -/* Rx FIFO Overflow */
> +/* Rx FIFO Overflow Interrupt Enable */
> #define REG_RXINT_ROI_EN BIT(0)
> -/* Rx Packet End */
> +/* Rx Packet End Interrupt Enable */
> #define REG_RXINT_RPEI_EN BIT(1)
> -/* Rx FIFO Data Available */
> +/* Rx FIFO Data Available Interrupt Enable */
> #define REG_RXINT_RAI_EN BIT(4)
>
> /* Rx FIFO available byte level */
> @@ -60,6 +60,12 @@
>
> /* Rx Interrupt Status */
> #define SUNXI_IR_RXSTA_REG 0x30
> +/* Rx FIFO Overflow */
> +#define REG_RXSTA_ROI BIT(0)
> +/* Rx Packet End */
> +#define REG_RXSTA_RPE BIT(1)
> +/* Rx FIFO Data Available */
> +#define REG_RXSTA_RA BIT(4)
I'm fine with it on principle, but if the consistency needs to be
maintained then we could just reuse the above defines
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Maxime,
On Wed, 5 Jun 2019 at 11:51, Maxime Ripard <[email protected]> wrote:
>
> On Tue, Jun 04, 2019 at 06:29:50PM +0200, Clément Péron wrote:
> > We are using RXINT bits definition when looking at RXSTA register.
> >
> > These bits are equal but it's not really proper.
> >
> > Introduce the RXSTA bits and use them to have coherency.
> >
> > Signed-off-by: Clément Péron <[email protected]>
> > ---
> > drivers/media/rc/sunxi-cir.c | 18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> > index 0504ebfc831f..572bd2257d35 100644
> > --- a/drivers/media/rc/sunxi-cir.c
> > +++ b/drivers/media/rc/sunxi-cir.c
> > @@ -48,11 +48,11 @@
> >
> > /* Rx Interrupt Enable */
> > #define SUNXI_IR_RXINT_REG 0x2C
> > -/* Rx FIFO Overflow */
> > +/* Rx FIFO Overflow Interrupt Enable */
> > #define REG_RXINT_ROI_EN BIT(0)
> > -/* Rx Packet End */
> > +/* Rx Packet End Interrupt Enable */
> > #define REG_RXINT_RPEI_EN BIT(1)
> > -/* Rx FIFO Data Available */
> > +/* Rx FIFO Data Available Interrupt Enable */
> > #define REG_RXINT_RAI_EN BIT(4)
> >
> > /* Rx FIFO available byte level */
> > @@ -60,6 +60,12 @@
> >
> > /* Rx Interrupt Status */
> > #define SUNXI_IR_RXSTA_REG 0x30
> > +/* Rx FIFO Overflow */
> > +#define REG_RXSTA_ROI BIT(0)
> > +/* Rx Packet End */
> > +#define REG_RXSTA_RPE BIT(1)
> > +/* Rx FIFO Data Available */
> > +#define REG_RXSTA_RA BIT(4)
>
> I'm fine with it on principle, but if the consistency needs to be
> maintained then we could just reuse the above defines
There is no comment why we can reuse them, they can also be some wrong
case for example the RXINT_DRQ_EN bit is not present in RXSTA and same
for STAT bit present in RXSTA and not present in RXINT.
I have discover and read this code a month ago and this logic is
really not obvious nor explain.
Maybe this hack could be done when we will introduce a quirks, but for
the moment I really think that it's more proper and readable to
introduce them properly.
Regards,
Clément
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
On Wed, Jun 05, 2019 at 02:44:06PM +0200, Cl?ment P?ron wrote:
> Hi Maxime,
>
> On Wed, 5 Jun 2019 at 11:51, Maxime Ripard <[email protected]> wrote:
> >
> > On Tue, Jun 04, 2019 at 06:29:50PM +0200, Cl?ment P?ron wrote:
> > > We are using RXINT bits definition when looking at RXSTA register.
> > >
> > > These bits are equal but it's not really proper.
> > >
> > > Introduce the RXSTA bits and use them to have coherency.
> > >
> > > Signed-off-by: Cl?ment P?ron <[email protected]>
> > > ---
> > > drivers/media/rc/sunxi-cir.c | 18 ++++++++++++------
> > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> > > index 0504ebfc831f..572bd2257d35 100644
> > > --- a/drivers/media/rc/sunxi-cir.c
> > > +++ b/drivers/media/rc/sunxi-cir.c
> > > @@ -48,11 +48,11 @@
> > >
> > > /* Rx Interrupt Enable */
> > > #define SUNXI_IR_RXINT_REG 0x2C
> > > -/* Rx FIFO Overflow */
> > > +/* Rx FIFO Overflow Interrupt Enable */
> > > #define REG_RXINT_ROI_EN BIT(0)
> > > -/* Rx Packet End */
> > > +/* Rx Packet End Interrupt Enable */
> > > #define REG_RXINT_RPEI_EN BIT(1)
> > > -/* Rx FIFO Data Available */
> > > +/* Rx FIFO Data Available Interrupt Enable */
> > > #define REG_RXINT_RAI_EN BIT(4)
> > >
> > > /* Rx FIFO available byte level */
> > > @@ -60,6 +60,12 @@
> > >
> > > /* Rx Interrupt Status */
> > > #define SUNXI_IR_RXSTA_REG 0x30
> > > +/* Rx FIFO Overflow */
> > > +#define REG_RXSTA_ROI BIT(0)
> > > +/* Rx Packet End */
> > > +#define REG_RXSTA_RPE BIT(1)
> > > +/* Rx FIFO Data Available */
> > > +#define REG_RXSTA_RA BIT(4)
> >
> > I'm fine with it on principle, but if the consistency needs to be
> > maintained then we could just reuse the above defines
>
> There is no comment why we can reuse them, they can also be some wrong
> case for example the RXINT_DRQ_EN bit is not present in RXSTA and same
> for STAT bit present in RXSTA and not present in RXINT.
>
> I have discover and read this code a month ago and this logic is
> really not obvious nor explain.
>
> Maybe this hack could be done when we will introduce a quirks, but for
> the moment I really think that it's more proper and readable to
> introduce them properly.
I don't think we understood each other :)
I was talking about having something like
#define REG_RXSTA_ROI REG_RXINT_ROI_EN
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, 5 Jun 2019 at 22:00, Maxime Ripard <[email protected]> wrote:
>
> On Wed, Jun 05, 2019 at 02:44:06PM +0200, Clément Péron wrote:
> > Hi Maxime,
> >
> > On Wed, 5 Jun 2019 at 11:51, Maxime Ripard <[email protected]> wrote:
> > >
> > > On Tue, Jun 04, 2019 at 06:29:50PM +0200, Clément Péron wrote:
> > > > We are using RXINT bits definition when looking at RXSTA register.
> > > >
> > > > These bits are equal but it's not really proper.
> > > >
> > > > Introduce the RXSTA bits and use them to have coherency.
> > > >
> > > > Signed-off-by: Clément Péron <[email protected]>
> > > > ---
> > > > drivers/media/rc/sunxi-cir.c | 18 ++++++++++++------
> > > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> > > > index 0504ebfc831f..572bd2257d35 100644
> > > > --- a/drivers/media/rc/sunxi-cir.c
> > > > +++ b/drivers/media/rc/sunxi-cir.c
> > > > @@ -48,11 +48,11 @@
> > > >
> > > > /* Rx Interrupt Enable */
> > > > #define SUNXI_IR_RXINT_REG 0x2C
> > > > -/* Rx FIFO Overflow */
> > > > +/* Rx FIFO Overflow Interrupt Enable */
> > > > #define REG_RXINT_ROI_EN BIT(0)
> > > > -/* Rx Packet End */
> > > > +/* Rx Packet End Interrupt Enable */
> > > > #define REG_RXINT_RPEI_EN BIT(1)
> > > > -/* Rx FIFO Data Available */
> > > > +/* Rx FIFO Data Available Interrupt Enable */
> > > > #define REG_RXINT_RAI_EN BIT(4)
> > > >
> > > > /* Rx FIFO available byte level */
> > > > @@ -60,6 +60,12 @@
> > > >
> > > > /* Rx Interrupt Status */
> > > > #define SUNXI_IR_RXSTA_REG 0x30
> > > > +/* Rx FIFO Overflow */
> > > > +#define REG_RXSTA_ROI BIT(0)
> > > > +/* Rx Packet End */
> > > > +#define REG_RXSTA_RPE BIT(1)
> > > > +/* Rx FIFO Data Available */
> > > > +#define REG_RXSTA_RA BIT(4)
> > >
> > > I'm fine with it on principle, but if the consistency needs to be
> > > maintained then we could just reuse the above defines
> >
> > There is no comment why we can reuse them, they can also be some wrong
> > case for example the RXINT_DRQ_EN bit is not present in RXSTA and same
> > for STAT bit present in RXSTA and not present in RXINT.
> >
> > I have discover and read this code a month ago and this logic is
> > really not obvious nor explain.
> >
> > Maybe this hack could be done when we will introduce a quirks, but for
> > the moment I really think that it's more proper and readable to
> > introduce them properly.
>
> I don't think we understood each other :)
>
> I was talking about having something like
>
> #define REG_RXSTA_ROI REG_RXINT_ROI_EN
Ok, I will send an update.
Thanks for the review,
Clément
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com