| Message ID | 1369681926-22185-13-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote: > From: Pantelis Antoniou <panto@antoniou-consulting.com> > Some confusing splitting between this and the previous patch that definitely wants to be cleaned up. > Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/iio/adc/ti_am335x_adc.c | 26 +++++++++++++++++--------- > drivers/input/touchscreen/ti_am335x_tsc.c | 13 ++++++++++--- > drivers/mfd/ti_am335x_tscadc.c | 1 + > 3 files changed, 28 insertions(+), 12 deletions(-) > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c > index cceff09..72ffe89 100644 > --- a/drivers/iio/adc/ti_am335x_adc.c > +++ b/drivers/iio/adc/ti_am335x_adc.c > @@ -25,7 +25,9 @@ > #include <linux/of_device.h> > #include <linux/iio/machine.h> > #include <linux/iio/driver.h> > +#include <linux/regmap.h> > > +#include <linux/io.h> That only went away in the previous patch... > #include <linux/mfd/ti_am335x_tscadc.h> > > struct tiadc_device { > @@ -37,13 +39,17 @@ struct tiadc_device { > > static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) > { > - return readl(adc->mfd_tscadc->tscadc_base + reg); > + unsigned int val; > + > + val = (unsigned int)-1; > + regmap_read(adc->mfd_tscadc->regmap_tscadc, reg, &val); > + return val; > } > > static void tiadc_writel(struct tiadc_device *adc, unsigned int reg, > unsigned int val) > { > - writel(val, adc->mfd_tscadc->tscadc_base + reg); > + regmap_write(adc->mfd_tscadc->regmap_tscadc, reg, val); > } > > static void tiadc_step_config(struct tiadc_device *adc_dev) > @@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) > tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); > } > > -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) > +static int tiadc_channel_init(struct iio_dev *indio_dev, > + struct tiadc_device *adc_dev) > { > struct iio_chan_spec *chan_array; > struct iio_chan_spec *chan; > char *s; > int i, len, size, ret; > + int channels = adc_dev->channels; This is an unconnected bit of cleanup. If you want it put it in the previous patch which only just introduced what you are changing. > > - size = indio_dev->num_channels * (sizeof(struct iio_chan_spec) + 6); > + size = channels * (sizeof(struct iio_chan_spec) + 6); > chan_array = kzalloc(size, GFP_KERNEL); > if (chan_array == NULL) > return -ENOMEM; > > /* buffer space is after the array */ > - s = (char *)(chan_array + indio_dev->num_channels); > + s = (char *)(chan_array + channels); > chan = chan_array; > - for (i = 0; i < indio_dev->num_channels; i++, chan++, s += len + 1) { > + for (i = 0; i < channels; i++, chan++, s += len + 1) { > > len = sprintf(s, "AIN%d", i); > > @@ -107,8 +115,9 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) > } > > indio_dev->channels = chan_array; > + indio_dev->num_channels = channels; > > - size = (indio_dev->num_channels + 1) * sizeof(struct iio_map); > + size = (channels + 1) * sizeof(struct iio_map); > adc_dev->map = kzalloc(size, GFP_KERNEL); > if (adc_dev->map == NULL) { > kfree(chan_array); > @@ -214,7 +223,7 @@ static int tiadc_probe(struct platform_device *pdev) > > tiadc_step_config(adc_dev); > > - err = tiadc_channel_init(indio_dev, adc_dev->channels); > + err = tiadc_channel_init(indio_dev, adc_dev); > if (err < 0) > goto err_free_device; > > @@ -223,7 +232,6 @@ static int tiadc_probe(struct platform_device *pdev) > goto err_free_channels; > > platform_set_drvdata(pdev, indio_dev); > - > return 0; > > err_free_channels: > diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c > index b467196..0dbf3df 100644 > --- a/drivers/input/touchscreen/ti_am335x_tsc.c > +++ b/drivers/input/touchscreen/ti_am335x_tsc.c > @@ -27,6 +27,7 @@ > #include <linux/delay.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/regmap.h> > > #include <linux/mfd/ti_am335x_tscadc.h> > > @@ -65,13 +66,17 @@ struct titsc { > > static unsigned int titsc_readl(struct titsc *ts, unsigned int reg) > { > - return readl(ts->mfd_tscadc->tscadc_base + reg); > + unsigned int val; > + > + val = (unsigned int)-1; > + regmap_read(ts->mfd_tscadc->regmap_tscadc, reg, &val); > + return val; > } > > static void titsc_writel(struct titsc *tsc, unsigned int reg, > unsigned int val) > { > - writel(val, tsc->mfd_tscadc->tscadc_base + reg); > + regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val); > } > > /* > @@ -484,8 +489,10 @@ static int titsc_probe(struct platform_device *pdev) > > /* register to the input system */ > err = input_register_device(input_dev); > - if (err) > + if (err) { > + dev_err(&pdev->dev, "Failed to register input device\n"); > goto err_free_irq; > + } > > platform_set_drvdata(pdev, ts_dev); > return 0; > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c > index bd127bd..a27401a 100644 > --- a/drivers/mfd/ti_am335x_tscadc.c > +++ b/drivers/mfd/ti_am335x_tscadc.c > @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg) > { > unsigned int val; > ???? What is this doing here? It's not doing the move to regmap but rather setting a default value. > + val = (unsigned int)-1; > regmap_read(tsadc->regmap_tscadc, reg, &val); > return val; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Jonathan Cameron | 2013-06-02 18:46:29 [+0100]: >On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote: >> From: Pantelis Antoniou <panto@antoniou-consulting.com> >> >Some confusing splitting between this and the previous patch that definitely wants >to be cleaned up. > >> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c >> index cceff09..72ffe89 100644 >> --- a/drivers/iio/adc/ti_am335x_adc.c >> +++ b/drivers/iio/adc/ti_am335x_adc.c >> @@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) >> tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); >> } >> >> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) >> +static int tiadc_channel_init(struct iio_dev *indio_dev, >> + struct tiadc_device *adc_dev) >> { >> struct iio_chan_spec *chan_array; >> struct iio_chan_spec *chan; >> char *s; >> int i, len, size, ret; >> + int channels = adc_dev->channels; >This is an unconnected bit of cleanup. If you want it put it in the previous patch >which only just introduced what you are changing. It will be probably best. >> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c >> index bd127bd..a27401a 100644 >> --- a/drivers/mfd/ti_am335x_tscadc.c >> +++ b/drivers/mfd/ti_am335x_tscadc.c >> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg) >> { >> unsigned int val; >> >???? What is this doing here? It's not doing the move to regmap but rather setting a default value. No lo sé :) I saw that and was wondering myself a little and planned the romval of this default for later. Now I think I do this earlier. >> + val = (unsigned int)-1; >> regmap_read(tsadc->regmap_tscadc, reg, &val); >> return val; >> } Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi there, On Jun 4, 2013, at 1:23 PM, Sebastian Andrzej Siewior wrote: > * Jonathan Cameron | 2013-06-02 18:46:29 [+0100]: > >> On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote: >>> From: Pantelis Antoniou <panto@antoniou-consulting.com> >>> >> Some confusing splitting between this and the previous patch that definitely wants >> to be cleaned up. >> >>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c >>> index cceff09..72ffe89 100644 >>> --- a/drivers/iio/adc/ti_am335x_adc.c >>> +++ b/drivers/iio/adc/ti_am335x_adc.c >>> @@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) >>> tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); >>> } >>> >>> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) >>> +static int tiadc_channel_init(struct iio_dev *indio_dev, >>> + struct tiadc_device *adc_dev) >>> { >>> struct iio_chan_spec *chan_array; >>> struct iio_chan_spec *chan; >>> char *s; >>> int i, len, size, ret; >>> + int channels = adc_dev->channels; >> This is an unconnected bit of cleanup. If you want it put it in the previous patch >> which only just introduced what you are changing. > > It will be probably best. > >>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c >>> index bd127bd..a27401a 100644 >>> --- a/drivers/mfd/ti_am335x_tscadc.c >>> +++ b/drivers/mfd/ti_am335x_tscadc.c >>> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg) >>> { >>> unsigned int val; >>> >> ???? What is this doing here? It's not doing the move to regmap but rather setting a default value. > > No lo sé :) I saw that and was wondering myself a little and planned the > romval of this default for later. Now I think I do this earlier. > The tscadc_read function doesn't have a failure mode; doesn't return an error in case of a regmap_read fail. Rather that change all the callers, we make sure we don't ever return an uninitialized value in case of an error (when val should be unchanged). >>> + val = (unsigned int)-1; >>> regmap_read(tsadc->regmap_tscadc, reg, &val); >>> return val; >>> } > > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Pantelis Antoniou | 2013-06-04 13:25:28 [+0300]: >>>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c >>>> index bd127bd..a27401a 100644 >>>> --- a/drivers/mfd/ti_am335x_tscadc.c >>>> +++ b/drivers/mfd/ti_am335x_tscadc.c >>>> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg) >>>> { >>>> unsigned int val; >>>> >>> ???? What is this doing here? It's not doing the move to regmap but rather setting a default value. >> >> No lo sé :) I saw that and was wondering myself a little and planned the >> romval of this default for later. Now I think I do this earlier. >> > >The tscadc_read function doesn't have a failure mode; doesn't return an error in case of a regmap_read >fail. Rather that change all the callers, we make sure we don't ever return an uninitialized value >in case of an error (when val should be unchanged). One stupid question: Why did you start using regmap in the first place? >>>> + val = (unsigned int)-1; >>>> regmap_read(tsadc->regmap_tscadc, reg, &val); >>>> return val; >>>> } Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What make you think I am the original author? I just fixed whatever I had to fix to get it working. Namely that the multi function device was using regmap in one place and not in another, which had the nice effect of the regmap cache getting out of sync. Regards -- Pantelis On Jun 4, 2013, at 1:52 PM, Sebastian Andrzej Siewior wrote: > * Pantelis Antoniou | 2013-06-04 13:25:28 [+0300]: > >>>>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c >>>>> index bd127bd..a27401a 100644 >>>>> --- a/drivers/mfd/ti_am335x_tscadc.c >>>>> +++ b/drivers/mfd/ti_am335x_tscadc.c >>>>> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg) >>>>> { >>>>> unsigned int val; >>>>> >>>> ???? What is this doing here? It's not doing the move to regmap but rather setting a default value. >>> >>> No lo sé :) I saw that and was wondering myself a little and planned the >>> romval of this default for later. Now I think I do this earlier. >>> >> >> The tscadc_read function doesn't have a failure mode; doesn't return an error in case of a regmap_read >> fail. Rather that change all the callers, we make sure we don't ever return an uninitialized value >> in case of an error (when val should be unchanged). > > One stupid question: Why did you start using regmap in the first place? > >>>>> + val = (unsigned int)-1; >>>>> regmap_read(tsadc->regmap_tscadc, reg, &val); >>>>> return val; >>>>> } > > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Pantelis Antoniou | 2013-06-04 13:54:48 [+0300]: >What make you think I am the original author? Nothing, you just made _this_ patch. >I just fixed whatever I had to fix to get it working. Fair enough. >Namely that the multi function device was using regmap in one place and >not in another, which had the nice effect of the regmap cache getting out of sync. Yeah, that is a pain. Currently I think why we do need regmap at all here and I think about getting rid of it. Most of the values read here need no caching at all. >Regards > >-- Pantelis Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jun 4, 2013, at 2:05 PM, Sebastian Andrzej Siewior wrote: > * Pantelis Antoniou | 2013-06-04 13:54:48 [+0300]: > >> What make you think I am the original author? > Nothing, you just made _this_ patch. > I'm the author of that patch that fixes it, I'm not the author of the original driver. >> I just fixed whatever I had to fix to get it working. > Fair enough. > >> Namely that the multi function device was using regmap in one place and >> not in another, which had the nice effect of the regmap cache getting out of sync. > > Yeah, that is a pain. Currently I think why we do need regmap at all > here and I think about getting rid of it. Most of the values read here > need no caching at all. > You need to talk to the original driver author then. >> Regards >> >> -- Pantelis > > Sebastian Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c index cceff09..72ffe89 100644 --- a/drivers/iio/adc/ti_am335x_adc.c +++ b/drivers/iio/adc/ti_am335x_adc.c @@ -25,7 +25,9 @@ #include <linux/of_device.h> #include <linux/iio/machine.h> #include <linux/iio/driver.h> +#include <linux/regmap.h> +#include <linux/io.h> #include <linux/mfd/ti_am335x_tscadc.h> struct tiadc_device { @@ -37,13 +39,17 @@ struct tiadc_device { static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg) { - return readl(adc->mfd_tscadc->tscadc_base + reg); + unsigned int val; + + val = (unsigned int)-1; + regmap_read(adc->mfd_tscadc->regmap_tscadc, reg, &val); + return val; } static void tiadc_writel(struct tiadc_device *adc, unsigned int reg, unsigned int val) { - writel(val, adc->mfd_tscadc->tscadc_base + reg); + regmap_write(adc->mfd_tscadc->regmap_tscadc, reg, val); } static void tiadc_step_config(struct tiadc_device *adc_dev) @@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB); } -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) +static int tiadc_channel_init(struct iio_dev *indio_dev, + struct tiadc_device *adc_dev) { struct iio_chan_spec *chan_array; struct iio_chan_spec *chan; char *s; int i, len, size, ret; + int channels = adc_dev->channels; - size = indio_dev->num_channels * (sizeof(struct iio_chan_spec) + 6); + size = channels * (sizeof(struct iio_chan_spec) + 6); chan_array = kzalloc(size, GFP_KERNEL); if (chan_array == NULL) return -ENOMEM; /* buffer space is after the array */ - s = (char *)(chan_array + indio_dev->num_channels); + s = (char *)(chan_array + channels); chan = chan_array; - for (i = 0; i < indio_dev->num_channels; i++, chan++, s += len + 1) { + for (i = 0; i < channels; i++, chan++, s += len + 1) { len = sprintf(s, "AIN%d", i); @@ -107,8 +115,9 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels) } indio_dev->channels = chan_array; + indio_dev->num_channels = channels; - size = (indio_dev->num_channels + 1) * sizeof(struct iio_map); + size = (channels + 1) * sizeof(struct iio_map); adc_dev->map = kzalloc(size, GFP_KERNEL); if (adc_dev->map == NULL) { kfree(chan_array); @@ -214,7 +223,7 @@ static int tiadc_probe(struct platform_device *pdev) tiadc_step_config(adc_dev); - err = tiadc_channel_init(indio_dev, adc_dev->channels); + err = tiadc_channel_init(indio_dev, adc_dev); if (err < 0) goto err_free_device; @@ -223,7 +232,6 @@ static int tiadc_probe(struct platform_device *pdev) goto err_free_channels; platform_set_drvdata(pdev, indio_dev); - return 0; err_free_channels: diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c index b467196..0dbf3df 100644 --- a/drivers/input/touchscreen/ti_am335x_tsc.c +++ b/drivers/input/touchscreen/ti_am335x_tsc.c @@ -27,6 +27,7 @@ #include <linux/delay.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/regmap.h> #include <linux/mfd/ti_am335x_tscadc.h> @@ -65,13 +66,17 @@ struct titsc { static unsigned int titsc_readl(struct titsc *ts, unsigned int reg) { - return readl(ts->mfd_tscadc->tscadc_base + reg); + unsigned int val; + + val = (unsigned int)-1; + regmap_read(ts->mfd_tscadc->regmap_tscadc, reg, &val); + return val; } static void titsc_writel(struct titsc *tsc, unsigned int reg, unsigned int val) { - writel(val, tsc->mfd_tscadc->tscadc_base + reg); + regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val); } /* @@ -484,8 +489,10 @@ static int titsc_probe(struct platform_device *pdev) /* register to the input system */ err = input_register_device(input_dev); - if (err) + if (err) { + dev_err(&pdev->dev, "Failed to register input device\n"); goto err_free_irq; + } platform_set_drvdata(pdev, ts_dev); return 0; diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c index bd127bd..a27401a 100644 --- a/drivers/mfd/ti_am335x_tscadc.c +++ b/drivers/mfd/ti_am335x_tscadc.c @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg) { unsigned int val; + val = (unsigned int)-1; regmap_read(tsadc->regmap_tscadc, reg, &val); return val; }