diff for duplicates of <20111108143047.25f8fdd6@skate> diff --git a/a/1.txt b/N1/1.txt index 1ac901f..db38902 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,5 +1,5 @@ Le Mon, 7 Nov 2011 17:08:32 +0100, -Maxime Ripard <maxime.ripard@free-electrons.com> a =C3=A9crit : +Maxime Ripard <maxime.ripard@free-electrons.com> a ?crit : > +static inline u32 at91adc_reg_read(void *base, u8 reg) > +{ @@ -19,8 +19,7 @@ static inline u32 at91adc_reg_read(struct at91adc_state *st, u8 reg) return readl_relaxed(st->reg_base + reg); } -static inline void at91adc_reg_write(struct at91adc_state *st, u8 reg, = -u32 val) +static inline void at91adc_reg_write(struct at91adc_state *st, u8 reg, u32 val) { writel_relaxed(val, st->reg_base + reg); } @@ -29,58 +28,55 @@ u32 val) > +static irqreturn_t at91adc_eoc_trigger(int irq, void *private) > +{ > + int chan; -> + struct iio_dev *idev =3D private; -> + struct at91adc_state *st =3D iio_priv(idev); -> + unsigned int status =3D at91adc_reg_read(st->reg_base, AT91_ADC_SR)= -; +> + struct iio_dev *idev = private; +> + struct at91adc_state *st = iio_priv(idev); +> + unsigned int status = at91adc_reg_read(st->reg_base, AT91_ADC_SR); would become -+ unsigned int status =3D at91adc_reg_read(st, AT91_ADC_SR); ++ unsigned int status = at91adc_reg_read(st, AT91_ADC_SR); > + if (!(status & AT91_ADC_DRDY)) > + return IRQ_HANDLED; > + -> + for (chan =3D 0; chan < idev->num_channels; chan++) +> + for (chan = 0; chan < idev->num_channels; chan++) > + if (status & AT91_ADC_EOC(chan)) { -> + st->done =3D true; -> + st->last_value =3D at91adc_reg_read(st->reg_base, +> + st->done = true; +> + st->last_value = at91adc_reg_read(st->reg_base, > + AT91_ADC_CHR(chan)); would become -+ st->last_value =3D at91adc_reg_read(st, AT91_ADC_CHR(chan)); ++ st->last_value = at91adc_reg_read(st, AT91_ADC_CHR(chan)); > +static int at91adc_channel_init(struct iio_dev *idev, > + struct at91_adc_data *pdata) > +{ > + struct iio_chan_spec *chan_array; -> + int bit, idx =3D 0; +> + int bit, idx = 0; > + -> + idev->num_channels =3D bitmap_weight(&(pdata->channels_used), +> + idev->num_channels = bitmap_weight(&(pdata->channels_used), > + pdata->num_channels); -> + chan_array =3D kcalloc(idev->num_channels, sizeof(struct iio_chan_s= -pec), +> + chan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec), > + GFP_KERNEL); > + -> + if (chan_array =3D=3D NULL) +> + if (chan_array == NULL) > + return -ENOMEM; > + -> + for_each_set_bit(bit, &(pdata->channels_used), pdata->num_channels)= - { -> + struct iio_chan_spec *chan =3D chan_array + idx; -> + chan->type =3D IIO_VOLTAGE; -> + chan->indexed =3D 1; -> + chan->channel =3D bit; -> + chan->scan_type.sign =3D 's'; +> + for_each_set_bit(bit, &(pdata->channels_used), pdata->num_channels) { +> + struct iio_chan_spec *chan = chan_array + idx; +> + chan->type = IIO_VOLTAGE; +> + chan->indexed = 1; +> + chan->channel = bit; +> + chan->scan_type.sign = 's'; It's an unsigned value that you're reading from the ADC, so maybe this -should be =3D 'u'. +should be = 'u'. > +static void at91adc_channel_remove(struct iio_dev *idev) > +{ -> + if (idev->channels =3D=3D NULL) +> + if (idev->channels == NULL) > + return; > + > + kfree(idev->channels); @@ -92,7 +88,7 @@ useless here. See http://lxr.free-electrons.com/source/mm/slab.c#L3863. > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ -> + struct at91adc_state *st =3D iio_priv(idev); +> + struct at91adc_state *st = iio_priv(idev); > + unsigned int scale_uv; > + > + switch (mask) { @@ -119,31 +115,31 @@ successful. > + */ > + at91adc_reg_write(st->reg_base, AT91_ADC_CR, AT91_ADC_SWRST); > + at91adc_reg_write(st->reg_base, AT91_ADC_IDR, 0xFFFFFFFF); -> + ret =3D request_irq(st->irq, +> + ret = request_irq(st->irq, > + at91adc_eoc_trigger, 0, pdev->dev.driver->name, idev); > + if (ret) { > + dev_err(&pdev->dev, "Failed to allocate IRQ.\n"); > + goto error_unmap_reg; > + } > + -> + st->clk =3D clk_get(&pdev->dev, "adc_clk"); +> + st->clk = clk_get(&pdev->dev, "adc_clk"); > + if (IS_ERR(st->clk)) { > + dev_err(&pdev->dev, "Failed to get the clock.\n"); -> + ret =3D PTR_ERR(st->clk); +> + ret = PTR_ERR(st->clk); > + goto error_free_irq; > + } > + > + clk_enable(st->clk); -> + mstrclk =3D clk_get_rate(st->clk); +> + mstrclk = clk_get_rate(st->clk); You do a clk_get()/clk_enable() here but nowhere in the ->remove() function you do a clk_disable()/clk_put(). -> + prsc =3D (mstrclk / (2 * pdata->adc_clock)) - 1; +> + prsc = (mstrclk / (2 * pdata->adc_clock)) - 1; [...] -> + ticks =3D round_up((pdata->startup_time * pdata->adc_clock / +> + ticks = round_up((pdata->startup_time * pdata->adc_clock / > + 1000000) - 1, 8) / 8; Both of those computations would be better with a small comment @@ -151,10 +147,9 @@ explaining what's happening. > +static int __devexit at91adc_remove(struct platform_device *pdev) > +{ -> + struct iio_dev *idev =3D platform_get_drvdata(pdev); -> + struct resource *res =3D platform_get_resource(pdev, IORESOURCE_MEM= -, 0); -> + struct at91adc_state *st =3D iio_priv(idev); +> + struct iio_dev *idev = platform_get_drvdata(pdev); +> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); +> + struct at91adc_state *st = iio_priv(idev); > + > + at91adc_channel_remove(idev); > + iio_device_unregister(idev); @@ -165,11 +160,11 @@ explaining what's happening. As said above, clk_disable()/clk_put() is missing. -> +static struct platform_driver at91adc_driver =3D { -> + .probe =3D at91adc_probe, -> + .remove =3D __devexit_p(at91adc_remove), -> + .driver =3D { -> + .name =3D "at91adc", +> +static struct platform_driver at91adc_driver = { +> + .probe = at91adc_probe, +> + .remove = __devexit_p(at91adc_remove), +> + .driver = { +> + .name = "at91adc", > + }, Nitpick: wrongly placed parenthesis. @@ -177,7 +172,7 @@ Nitpick: wrongly placed parenthesis. Regards, Thomas ---=20 +-- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. diff --git a/a/content_digest b/N1/content_digest index 62530bf..56e4e63 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,19 +1,14 @@ "ref\01319041134-19712-1-git-send-email-maxime.ripard@free-electrons.com\0" "ref\01320682113-23655-1-git-send-email-maxime.ripard@free-electrons.com\0" "ref\01320682113-23655-3-git-send-email-maxime.ripard@free-electrons.com\0" - "From\0Thomas Petazzoni <thomas.petazzoni@free-electrons.com>\0" - "Subject\0Re: [PATCH 2/3] ARM: AT91: IIO: Add AT91 ADC driver.\0" + "From\0thomas.petazzoni@free-electrons.com (Thomas Petazzoni)\0" + "Subject\0[PATCH 2/3] ARM: AT91: IIO: Add AT91 ADC driver.\0" "Date\0Tue, 8 Nov 2011 14:30:47 +0100\0" - "To\0Maxime Ripard <maxime.ripard@free-electrons.com>\0" - "Cc\0linux-arm-kernel@lists.infradead.org" - linux-iio@vger.kernel.org - Patrice Vilchez <patrice.vilchez@atmel.com> - Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com> - " Nicolas Ferre <nicolas.ferre@atmel.com>\0" + "To\0linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "Le Mon, 7 Nov 2011 17:08:32 +0100,\n" - "Maxime Ripard <maxime.ripard@free-electrons.com> a =C3=A9crit :\n" + "Maxime Ripard <maxime.ripard@free-electrons.com> a ?crit :\n" "\n" "> +static inline u32 at91adc_reg_read(void *base, u8 reg)\n" "> +{\n" @@ -33,8 +28,7 @@ "\treturn readl_relaxed(st->reg_base + reg);\n" "}\n" "\n" - "static inline void at91adc_reg_write(struct at91adc_state *st, u8 reg, =\n" - "u32 val)\n" + "static inline void at91adc_reg_write(struct at91adc_state *st, u8 reg, u32 val)\n" "{\n" "\twritel_relaxed(val, st->reg_base + reg);\n" "}\n" @@ -43,58 +37,55 @@ "> +static irqreturn_t at91adc_eoc_trigger(int irq, void *private)\n" "> +{\n" "> +\tint chan;\n" - "> +\tstruct iio_dev *idev =3D private;\n" - "> +\tstruct at91adc_state *st =3D iio_priv(idev);\n" - "> +\tunsigned int status =3D at91adc_reg_read(st->reg_base, AT91_ADC_SR)=\n" - ";\n" + "> +\tstruct iio_dev *idev = private;\n" + "> +\tstruct at91adc_state *st = iio_priv(idev);\n" + "> +\tunsigned int status = at91adc_reg_read(st->reg_base, AT91_ADC_SR);\n" "\n" "would become\n" "\n" - "+\tunsigned int status =3D at91adc_reg_read(st, AT91_ADC_SR);\n" + "+\tunsigned int status = at91adc_reg_read(st, AT91_ADC_SR);\n" "\n" "> +\tif (!(status & AT91_ADC_DRDY))\n" "> +\t\treturn IRQ_HANDLED;\n" "> +\n" - "> +\tfor (chan =3D 0; chan < idev->num_channels; chan++)\n" + "> +\tfor (chan = 0; chan < idev->num_channels; chan++)\n" "> +\t\tif (status & AT91_ADC_EOC(chan)) {\n" - "> +\t\t\tst->done =3D true;\n" - "> +\t\t\tst->last_value =3D at91adc_reg_read(st->reg_base,\n" + "> +\t\t\tst->done = true;\n" + "> +\t\t\tst->last_value = at91adc_reg_read(st->reg_base,\n" "> +\t\t\t\t\t\t\t AT91_ADC_CHR(chan));\n" "\n" "would become\n" "\n" - "+\t\t\tst->last_value =3D at91adc_reg_read(st, AT91_ADC_CHR(chan));\n" + "+\t\t\tst->last_value = at91adc_reg_read(st, AT91_ADC_CHR(chan));\n" "\n" "\n" "> +static int at91adc_channel_init(struct iio_dev *idev,\n" "> +\t\t\t\tstruct at91_adc_data *pdata)\n" "> +{\n" "> +\tstruct iio_chan_spec *chan_array;\n" - "> +\tint bit, idx =3D 0;\n" + "> +\tint bit, idx = 0;\n" "> +\n" - "> +\tidev->num_channels =3D bitmap_weight(&(pdata->channels_used),\n" + "> +\tidev->num_channels = bitmap_weight(&(pdata->channels_used),\n" "> +\t\t\t\t\t pdata->num_channels);\n" - "> +\tchan_array =3D kcalloc(idev->num_channels, sizeof(struct iio_chan_s=\n" - "pec),\n" + "> +\tchan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec),\n" "> +\t\t\t GFP_KERNEL);\n" "> +\n" - "> +\tif (chan_array =3D=3D NULL)\n" + "> +\tif (chan_array == NULL)\n" "> +\t\treturn -ENOMEM;\n" "> +\n" - "> +\tfor_each_set_bit(bit, &(pdata->channels_used), pdata->num_channels)=\n" - " {\n" - "> +\t\tstruct iio_chan_spec *chan =3D chan_array + idx;\n" - "> +\t\tchan->type =3D IIO_VOLTAGE;\n" - "> +\t\tchan->indexed =3D 1;\n" - "> +\t\tchan->channel =3D bit;\n" - "> +\t\tchan->scan_type.sign =3D 's';\n" + "> +\tfor_each_set_bit(bit, &(pdata->channels_used), pdata->num_channels) {\n" + "> +\t\tstruct iio_chan_spec *chan = chan_array + idx;\n" + "> +\t\tchan->type = IIO_VOLTAGE;\n" + "> +\t\tchan->indexed = 1;\n" + "> +\t\tchan->channel = bit;\n" + "> +\t\tchan->scan_type.sign = 's';\n" "\n" "It's an unsigned value that you're reading from the ADC, so maybe this\n" - "should be =3D 'u'.\n" + "should be = 'u'.\n" "\n" "> +static void at91adc_channel_remove(struct iio_dev *idev)\n" "> +{\n" - "> +\tif (idev->channels =3D=3D NULL)\n" + "> +\tif (idev->channels == NULL)\n" "> +\t\treturn;\n" "> +\n" "> +\tkfree(idev->channels);\n" @@ -106,7 +97,7 @@ "> +\t\t\t struct iio_chan_spec const *chan,\n" "> +\t\t\t int *val, int *val2, long mask)\n" "> +{\n" - "> +\tstruct at91adc_state *st =3D iio_priv(idev);\n" + "> +\tstruct at91adc_state *st = iio_priv(idev);\n" "> +\tunsigned int scale_uv;\n" "> +\n" "> +\tswitch (mask) {\n" @@ -133,31 +124,31 @@ "> +\t */\n" "> +\tat91adc_reg_write(st->reg_base, AT91_ADC_CR, AT91_ADC_SWRST);\n" "> +\tat91adc_reg_write(st->reg_base, AT91_ADC_IDR, 0xFFFFFFFF);\n" - "> +\tret =3D request_irq(st->irq,\n" + "> +\tret = request_irq(st->irq,\n" "> +\t\t\t at91adc_eoc_trigger, 0, pdev->dev.driver->name, idev);\n" "> +\tif (ret) {\n" "> +\t\tdev_err(&pdev->dev, \"Failed to allocate IRQ.\\n\");\n" "> +\t\tgoto error_unmap_reg;\n" "> +\t}\n" "> +\n" - "> +\tst->clk =3D clk_get(&pdev->dev, \"adc_clk\");\n" + "> +\tst->clk = clk_get(&pdev->dev, \"adc_clk\");\n" "> +\tif (IS_ERR(st->clk)) {\n" "> +\t\tdev_err(&pdev->dev, \"Failed to get the clock.\\n\");\n" - "> +\t\tret =3D PTR_ERR(st->clk);\n" + "> +\t\tret = PTR_ERR(st->clk);\n" "> +\t\tgoto error_free_irq;\n" "> +\t}\n" "> +\n" "> +\tclk_enable(st->clk);\n" - "> +\tmstrclk =3D clk_get_rate(st->clk);\n" + "> +\tmstrclk = clk_get_rate(st->clk);\n" "\n" "You do a clk_get()/clk_enable() here but nowhere in the ->remove()\n" "function you do a clk_disable()/clk_put().\n" "\n" - "> +\tprsc =3D (mstrclk / (2 * pdata->adc_clock)) - 1;\n" + "> +\tprsc = (mstrclk / (2 * pdata->adc_clock)) - 1;\n" "\n" "[...]\n" "\n" - "> +\tticks =3D round_up((pdata->startup_time * pdata->adc_clock /\n" + "> +\tticks = round_up((pdata->startup_time * pdata->adc_clock /\n" "> +\t\t\t 1000000) - 1, 8) / 8;\n" "\n" "Both of those computations would be better with a small comment\n" @@ -165,10 +156,9 @@ "\n" "> +static int __devexit at91adc_remove(struct platform_device *pdev)\n" "> +{\n" - "> +\tstruct iio_dev *idev =3D platform_get_drvdata(pdev);\n" - "> +\tstruct resource *res =3D platform_get_resource(pdev, IORESOURCE_MEM=\n" - ", 0);\n" - "> +\tstruct at91adc_state *st =3D iio_priv(idev);\n" + "> +\tstruct iio_dev *idev = platform_get_drvdata(pdev);\n" + "> +\tstruct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n" + "> +\tstruct at91adc_state *st = iio_priv(idev);\n" "> +\n" "> +\tat91adc_channel_remove(idev);\n" "> +\tiio_device_unregister(idev);\n" @@ -179,11 +169,11 @@ "\n" "As said above, clk_disable()/clk_put() is missing.\n" "\n" - "> +static struct platform_driver at91adc_driver =3D {\n" - "> +\t.probe =3D at91adc_probe,\n" - "> +\t.remove =3D __devexit_p(at91adc_remove),\n" - "> +\t.driver =3D {\n" - "> +\t\t .name =3D \"at91adc\",\n" + "> +static struct platform_driver at91adc_driver = {\n" + "> +\t.probe = at91adc_probe,\n" + "> +\t.remove = __devexit_p(at91adc_remove),\n" + "> +\t.driver = {\n" + "> +\t\t .name = \"at91adc\",\n" "> +\t\t },\n" "\n" "Nitpick: wrongly placed parenthesis.\n" @@ -191,10 +181,10 @@ "Regards,\n" "\n" "Thomas\n" - "--=20\n" + "-- \n" "Thomas Petazzoni, Free Electrons\n" "Kernel, drivers, real-time and embedded Linux\n" "development, consulting, training and support.\n" http://free-electrons.com -e6711cd15671218d739ec4b7b0e04c26caacb52c062a3874b2ed3e40de151577 +e6d1e56cf4e6566a781fff7945fe4a63da662217cf6b770b8d3c526966079519
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.