diff for duplicates of <20130828130124.GC14111@linutronix.de> diff --git a/a/1.txt b/N1/1.txt index 69601e0..fe1ee30 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -3,22 +3,24 @@ I am mostly happy with it. There are just two things I pointed out. Besides that, it looks good from my side. ->diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c +>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_a= +dc.c >index a952538..ae2202b 100644 >--- a/drivers/iio/adc/ti_am335x_adc.c >+++ b/drivers/iio/adc/ti_am335x_adc.c ->@@ -85,7 +96,175 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) -> adc_dev->channel_step[i] = steps; +>@@ -85,7 +96,175 @@ static void tiadc_step_config(struct tiadc_device *adc= +_dev) +> adc_dev->channel_step[i] =3D steps; > steps++; > } >+} >+ >+static irqreturn_t tiadc_irq(int irq, void *private) >+{ ->+ struct iio_dev *indio_dev = private; ->+ struct tiadc_device *adc_dev = iio_priv(indio_dev); +>+ struct iio_dev *indio_dev =3D private; +>+ struct tiadc_device *adc_dev =3D iio_priv(indio_dev); >+ unsigned int status, config; ->+ status = tiadc_readl(adc_dev, REG_IRQSTATUS); +>+ status =3D tiadc_readl(adc_dev, REG_IRQSTATUS); >+ >+ /* >+ * ADC and touchscreen share the IRQ line. @@ -26,8 +28,8 @@ Besides that, it looks good from my side. >+ */ >+ if (status & IRQENB_FIFO1OVRRUN) { >+ /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */ ->+ config = tiadc_readl(adc_dev, REG_CTRL); ->+ config &= ~(CNTRLREG_TSCSSENB); +>+ config =3D tiadc_readl(adc_dev, REG_CTRL); +>+ config &=3D ~(CNTRLREG_TSCSSENB); >+ tiadc_writel(adc_dev, REG_CTRL, config); >+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN >+ | IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES); @@ -40,8 +42,8 @@ Besides that, it looks good from my side. >+ return IRQ_NONE; >+ >+ /* If any IRQ flags left, return none. So TSC can handle its IRQs */ ->+ status = tiadc_readl(adc_dev, REG_IRQSTATUS); ->+ if (status == false) +>+ status =3D tiadc_readl(adc_dev, REG_IRQSTATUS); +>+ if (status =3D=3D false) >+ return IRQ_HANDLED; >+ else >+ return IRQ_NONE; @@ -49,23 +51,23 @@ Besides that, it looks good from my side. As I said in 1/2 of this series, you shouldn't do this. Both handlers of a shared line are invoked once an interrupt is detected. -… +=E2=80=A6 >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev) >+{ ->+ struct tiadc_device *adc_dev = iio_priv(indio_dev); ->+ struct iio_buffer *buffer = indio_dev->buffer; ->+ unsigned int enb = 0, stepnum; +>+ struct tiadc_device *adc_dev =3D iio_priv(indio_dev); +>+ struct iio_buffer *buffer =3D indio_dev->buffer; +>+ unsigned int enb =3D 0, stepnum; >+ u8 bit; >+ ->+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); ->+ if (adc_dev->data == NULL) +>+ adc_dev->data =3D kmalloc(indio_dev->scan_bytes, GFP_KERNEL); +>+ if (adc_dev->data =3D=3D NULL) >+ return -ENOMEM; >+ >+ tiadc_step_config(indio_dev); >+ for_each_set_bit(bit, buffer->scan_mask, >+ adc_dev->channels) { ->+ struct iio_chan_spec const *chan = indio_dev->channels + bit; +>+ struct iio_chan_spec const *chan =3D indio_dev->channels + bit; >+ /* >+ * There are a total of 16 steps available >+ * that are shared between ADC and touchscreen. @@ -73,16 +75,16 @@ shared line are invoked once an interrupt is detected. >+ * ADC. Hence the relation between input channel >+ * and step for ADC would be as below. >+ */ ->+ stepnum = chan->channel + 9; ->+ enb |= (1 << stepnum); +>+ stepnum =3D chan->channel + 9; +>+ enb |=3D (1 << stepnum); Hmm. This looks odd. - +=20 In tiadc_step_config() we do: -| steps = TOTAL_STEPS - adc_dev->channels; -|… -| for (i = 0; i < adc_dev->channels; i++) { -| chan = adc_dev->channel_line[i]; +| steps =3D TOTAL_STEPS - adc_dev->channels; +|=E2=80=A6 +| for (i =3D 0; i < adc_dev->channels; i++) { +| chan =3D adc_dev->channel_line[i]; | tiadc_writel(adc_dev, | REG_STEPCONFIG(steps), | stepconfig @@ -92,14 +94,14 @@ In tiadc_step_config() we do: | REG_STEPDELAY(steps), | STEPCONFIG_OPENDLY); | adc_dev->channel_step[i] -| = +| =3D | steps; | steps++; | } That means if we have only one channel we enable the last step bit / topmost that is bit 16. For the "default" four channels we get 0x1e000 -as mask which means bit 13 to 16. +as mask which means bit 13 to 16.=20 If you do have only one channel with the number 4 you then get_adc_step_mask() will compute the correct step mask but here enable step 14 instead of step 16. @@ -109,10 +111,11 @@ What about the following as replacement? index 08681d3..3bfcf1b 100644 --- a/drivers/iio/adc/ti_am335x_adc.c +++ b/drivers/iio/adc/ti_am335x_adc.c -@@ -65,6 +65,11 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev) +@@ -65,6 +65,11 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_de= +v) return step_en; } - +=20 +static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan) +{ + return 1 << adc_dev->channel_step[chan]; @@ -120,23 +123,25 @@ index 08681d3..3bfcf1b 100644 + static void tiadc_step_config(struct iio_dev *indio_dev) { - struct tiadc_device *adc_dev = iio_priv(indio_dev); -@@ -179,7 +187,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev) + struct tiadc_device *adc_dev =3D iio_priv(indio_dev); +@@ -179,7 +187,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indi= +o_dev) { - struct tiadc_device *adc_dev = iio_priv(indio_dev); - struct iio_buffer *buffer = indio_dev->buffer; -- unsigned int enb = 0, stepnum; -+ unsigned int enb = 0; + struct tiadc_device *adc_dev =3D iio_priv(indio_dev); + struct iio_buffer *buffer =3D indio_dev->buffer; +- unsigned int enb =3D 0, stepnum; ++ unsigned int enb =3D 0; u8 bit; - - adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); -@@ -187,19 +195,8 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev) +=20 + adc_dev->data =3D kmalloc(indio_dev->scan_bytes, GFP_KERNEL); +@@ -187,19 +195,8 @@ static int tiadc_buffer_postenable(struct iio_dev *ind= +io_dev) return -ENOMEM; - +=20 tiadc_step_config(indio_dev); - for_each_set_bit(bit, buffer->scan_mask, - adc_dev->channels) { -- struct iio_chan_spec const *chan = indio_dev->channels + bit; +- struct iio_chan_spec const *chan =3D indio_dev->channels + bit; - /* - * There are a total of 16 steps available - * that are shared between ADC and touchscreen. @@ -144,13 +149,13 @@ index 08681d3..3bfcf1b 100644 - * ADC. Hence the relation between input channel - * and step for ADC would be as below. - */ -- stepnum = chan->channel + 9; -- enb |= (1 << stepnum); +- stepnum =3D chan->channel + 9; +- enb |=3D (1 << stepnum); - } + for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels) -+ enb |= get_adc_step_bit(adc_dev, bit); - - adc_dev->buffer_en_ch_steps = enb; ++ enb |=3D get_adc_step_bit(adc_dev, bit); +=20 + adc_dev->buffer_en_ch_steps =3D enb; am335x_tsc_se_set(adc_dev->mfd_tscadc, enb); diff --git a/a/content_digest b/N1/content_digest index 9ec68c2..a3a61dc 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -17,22 +17,24 @@ "I am mostly happy with it. There are just two things I pointed out.\n" "Besides that, it looks good from my side.\n" "\n" - ">diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c\n" + ">diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_a=\n" + "dc.c\n" ">index a952538..ae2202b 100644\n" ">--- a/drivers/iio/adc/ti_am335x_adc.c\n" ">+++ b/drivers/iio/adc/ti_am335x_adc.c\n" - ">@@ -85,7 +96,175 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)\n" - "> \t\tadc_dev->channel_step[i] = steps;\n" + ">@@ -85,7 +96,175 @@ static void tiadc_step_config(struct tiadc_device *adc=\n" + "_dev)\n" + "> \t\tadc_dev->channel_step[i] =3D steps;\n" "> \t\tsteps++;\n" "> \t}\n" ">+}\n" ">+\n" ">+static irqreturn_t tiadc_irq(int irq, void *private)\n" ">+{\n" - ">+\tstruct iio_dev *indio_dev = private;\n" - ">+\tstruct tiadc_device *adc_dev = iio_priv(indio_dev);\n" + ">+\tstruct iio_dev *indio_dev =3D private;\n" + ">+\tstruct tiadc_device *adc_dev =3D iio_priv(indio_dev);\n" ">+\tunsigned int status, config;\n" - ">+\tstatus = tiadc_readl(adc_dev, REG_IRQSTATUS);\n" + ">+\tstatus =3D tiadc_readl(adc_dev, REG_IRQSTATUS);\n" ">+\n" ">+\t/*\n" ">+\t * ADC and touchscreen share the IRQ line.\n" @@ -40,8 +42,8 @@ ">+\t */\n" ">+\tif (status & IRQENB_FIFO1OVRRUN) {\n" ">+\t\t/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */\n" - ">+\t\tconfig = tiadc_readl(adc_dev, REG_CTRL);\n" - ">+\t\tconfig &= ~(CNTRLREG_TSCSSENB);\n" + ">+\t\tconfig =3D tiadc_readl(adc_dev, REG_CTRL);\n" + ">+\t\tconfig &=3D ~(CNTRLREG_TSCSSENB);\n" ">+\t\ttiadc_writel(adc_dev, REG_CTRL, config);\n" ">+\t\ttiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN\n" ">+\t\t\t\t| IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);\n" @@ -54,8 +56,8 @@ ">+\t\treturn IRQ_NONE;\n" ">+\n" ">+\t/* If any IRQ flags left, return none. So TSC can handle its IRQs */\n" - ">+\tstatus = tiadc_readl(adc_dev, REG_IRQSTATUS);\n" - ">+\tif (status == false)\n" + ">+\tstatus =3D tiadc_readl(adc_dev, REG_IRQSTATUS);\n" + ">+\tif (status =3D=3D false)\n" ">+\t\treturn IRQ_HANDLED;\n" ">+\telse\n" ">+\t\treturn IRQ_NONE;\n" @@ -63,23 +65,23 @@ "As I said in 1/2 of this series, you shouldn't do this. Both handlers of a\n" "shared line are invoked once an interrupt is detected.\n" "\n" - "\342\200\246\n" + "=E2=80=A6\n" "\n" ">+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)\n" ">+{\n" - ">+\tstruct tiadc_device *adc_dev = iio_priv(indio_dev);\n" - ">+\tstruct iio_buffer *buffer = indio_dev->buffer;\n" - ">+\tunsigned int enb = 0, stepnum;\n" + ">+\tstruct tiadc_device *adc_dev =3D iio_priv(indio_dev);\n" + ">+\tstruct iio_buffer *buffer =3D indio_dev->buffer;\n" + ">+\tunsigned int enb =3D 0, stepnum;\n" ">+\tu8 bit;\n" ">+\n" - ">+\tadc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);\n" - ">+\tif (adc_dev->data == NULL)\n" + ">+\tadc_dev->data =3D kmalloc(indio_dev->scan_bytes, GFP_KERNEL);\n" + ">+\tif (adc_dev->data =3D=3D NULL)\n" ">+\t\treturn -ENOMEM;\n" ">+\n" ">+\ttiadc_step_config(indio_dev);\n" ">+\tfor_each_set_bit(bit, buffer->scan_mask,\n" ">+\t\t\tadc_dev->channels) {\n" - ">+\t\tstruct iio_chan_spec const *chan = indio_dev->channels + bit;\n" + ">+\t\tstruct iio_chan_spec const *chan =3D indio_dev->channels + bit;\n" ">+\t\t/*\n" ">+\t\t * There are a total of 16 steps available\n" ">+\t\t * that are shared between ADC and touchscreen.\n" @@ -87,16 +89,16 @@ ">+\t\t * ADC. Hence the relation between input channel\n" ">+\t\t * and step for ADC would be as below.\n" ">+\t\t */\n" - ">+\t\tstepnum = chan->channel + 9;\n" - ">+\t\tenb |= (1 << stepnum);\n" + ">+\t\tstepnum =3D chan->channel + 9;\n" + ">+\t\tenb |=3D (1 << stepnum);\n" "\n" "Hmm. This looks odd.\n" - " \n" + "=20\n" "In tiadc_step_config() we do:\n" - "| steps = TOTAL_STEPS - adc_dev->channels;\n" - "|\342\200\246\n" - "| for (i = 0; i < adc_dev->channels; i++) {\n" - "| chan = adc_dev->channel_line[i];\n" + "| steps =3D TOTAL_STEPS - adc_dev->channels;\n" + "|=E2=80=A6\n" + "| for (i =3D 0; i < adc_dev->channels; i++) {\n" + "| chan =3D adc_dev->channel_line[i];\n" "| tiadc_writel(adc_dev,\n" "| REG_STEPCONFIG(steps),\n" "| stepconfig\n" @@ -106,14 +108,14 @@ "| REG_STEPDELAY(steps),\n" "| STEPCONFIG_OPENDLY);\n" "| adc_dev->channel_step[i]\n" - "| =\n" + "| =3D\n" "| steps;\n" "| steps++;\n" "| }\n" "\n" "That means if we have only one channel we enable the last step bit /\n" "topmost that is bit 16. For the \"default\" four channels we get 0x1e000\n" - "as mask which means bit 13 to 16. \n" + "as mask which means bit 13 to 16.=20\n" "If you do have only one channel with the number 4 you then\n" "get_adc_step_mask() will compute the correct step mask but here enable\n" "step 14 instead of step 16.\n" @@ -123,10 +125,11 @@ "index 08681d3..3bfcf1b 100644\n" "--- a/drivers/iio/adc/ti_am335x_adc.c\n" "+++ b/drivers/iio/adc/ti_am335x_adc.c\n" - "@@ -65,6 +65,11 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)\n" + "@@ -65,6 +65,11 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_de=\n" + "v)\n" " \treturn step_en;\n" " }\n" - " \n" + "=20\n" "+static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)\n" "+{\n" "+\treturn 1 << adc_dev->channel_step[chan];\n" @@ -134,23 +137,25 @@ "+\n" " static void tiadc_step_config(struct iio_dev *indio_dev)\n" " {\n" - " \tstruct tiadc_device *adc_dev = iio_priv(indio_dev);\n" - "@@ -179,7 +187,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)\n" + " \tstruct tiadc_device *adc_dev =3D iio_priv(indio_dev);\n" + "@@ -179,7 +187,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indi=\n" + "o_dev)\n" " {\n" - " \tstruct tiadc_device *adc_dev = iio_priv(indio_dev);\n" - " \tstruct iio_buffer *buffer = indio_dev->buffer;\n" - "-\tunsigned int enb = 0, stepnum;\n" - "+\tunsigned int enb = 0;\n" + " \tstruct tiadc_device *adc_dev =3D iio_priv(indio_dev);\n" + " \tstruct iio_buffer *buffer =3D indio_dev->buffer;\n" + "-\tunsigned int enb =3D 0, stepnum;\n" + "+\tunsigned int enb =3D 0;\n" " \tu8 bit;\n" - " \n" - " \tadc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);\n" - "@@ -187,19 +195,8 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)\n" + "=20\n" + " \tadc_dev->data =3D kmalloc(indio_dev->scan_bytes, GFP_KERNEL);\n" + "@@ -187,19 +195,8 @@ static int tiadc_buffer_postenable(struct iio_dev *ind=\n" + "io_dev)\n" " \t\treturn -ENOMEM;\n" - " \n" + "=20\n" " \ttiadc_step_config(indio_dev);\n" "-\tfor_each_set_bit(bit, buffer->scan_mask,\n" "-\t\t\tadc_dev->channels) {\n" - "-\t\tstruct iio_chan_spec const *chan = indio_dev->channels + bit;\n" + "-\t\tstruct iio_chan_spec const *chan =3D indio_dev->channels + bit;\n" "-\t\t/*\n" "-\t\t * There are a total of 16 steps available\n" "-\t\t * that are shared between ADC and touchscreen.\n" @@ -158,13 +163,13 @@ "-\t\t * ADC. Hence the relation between input channel\n" "-\t\t * and step for ADC would be as below.\n" "-\t\t */\n" - "-\t\tstepnum = chan->channel + 9;\n" - "-\t\tenb |= (1 << stepnum);\n" + "-\t\tstepnum =3D chan->channel + 9;\n" + "-\t\tenb |=3D (1 << stepnum);\n" "-\t}\n" "+\tfor_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)\n" - "+\t\tenb |= get_adc_step_bit(adc_dev, bit);\n" - " \n" - " \tadc_dev->buffer_en_ch_steps = enb;\n" + "+\t\tenb |=3D get_adc_step_bit(adc_dev, bit);\n" + "=20\n" + " \tadc_dev->buffer_en_ch_steps =3D enb;\n" " \tam335x_tsc_se_set(adc_dev->mfd_tscadc, enb);\n" "\n" "\n" @@ -172,4 +177,4 @@ "\n" Sebastian -fa1cebc6c9425df3e635d6f9c96e495ca7d0bcaadbd44cbf5a57e040fc85c50a +68c1f911f7236eaa5e0cf5daa941dce9d2f428193e3c87fade9f9a4314a048f5
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.