| Message ID | 20230402164247.3089146-2-risca@dalakolonin.se (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: adc: palmas_gpadc: add iio events | expand |
Hi Patrik, > Am 02.04.2023 um 18:42 schrieb Patrik Dahlström <risca@dalakolonin.se>: > > It does not seem to be used by anyone and later patches in this series > are made simpler by first removing this. There is now a lot of dead code > that cannot be reached, until later patches revive it. Arguably, this is > preferred over removing the code only to add it again. > > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> > --- > drivers/iio/adc/palmas_gpadc.c | 50 ++++------------------------------ > include/linux/mfd/palmas.h | 8 ------ > 2 files changed, 6 insertions(+), 52 deletions(-) > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > index 24d7c096e4b8..943ac579eb1f 100644 > --- a/drivers/iio/adc/palmas_gpadc.c > +++ b/drivers/iio/adc/palmas_gpadc.c > @@ -76,6 +76,12 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = { > PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true), > }; > > +struct palmas_adc_wakeup_property { > + int adc_channel_number; > + int adc_high_threshold; > + int adc_low_threshold; > +}; > + > /* > * struct palmas_gpadc - the palmas_gpadc structure > * @ch0_current: channel 0 current source setting > @@ -492,11 +498,6 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev, > return 0; > } > > -static void palmas_disable_wakeup(void *dev) something seems to be mixed up here. There is no palmas_disable_wakeup() upstream that can be removed. So this patch can not be applied as 1/7 to any upstream kernel. Please rebase your series on either linus/master or linux-next/master. BR, Nikolaus > -{ > - device_wakeup_disable(dev); > -} > - > static int palmas_gpadc_probe(struct platform_device *pdev) > { > struct palmas_gpadc *adc; > @@ -547,36 +548,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > return dev_err_probe(adc->dev, ret, > "request irq %d failed\n", adc->irq); > > - if (gpadc_pdata->adc_wakeup1_data) { > - memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data, > - sizeof(adc->wakeup1_data)); > - adc->wakeup1_enable = true; > - adc->irq_auto_0 = platform_get_irq(pdev, 1); > - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, > - NULL, palmas_gpadc_irq_auto, > - IRQF_ONESHOT, > - "palmas-adc-auto-0", adc); > - if (ret < 0) > - return dev_err_probe(adc->dev, ret, > - "request auto0 irq %d failed\n", > - adc->irq_auto_0); > - } > - > - if (gpadc_pdata->adc_wakeup2_data) { > - memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data, > - sizeof(adc->wakeup2_data)); > - adc->wakeup2_enable = true; > - adc->irq_auto_1 = platform_get_irq(pdev, 2); > - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, > - NULL, palmas_gpadc_irq_auto, > - IRQF_ONESHOT, > - "palmas-adc-auto-1", adc); > - if (ret < 0) > - return dev_err_probe(adc->dev, ret, > - "request auto1 irq %d failed\n", > - adc->irq_auto_1); > - } > - > /* set the current source 0 (value 0/5/15/20 uA => 0..3) */ > if (gpadc_pdata->ch0_current <= 1) > adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0; > @@ -616,15 +587,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > palmas_gpadc_calibrate(adc, i); > } > > - if (adc->wakeup1_enable || adc->wakeup2_enable) { > - device_wakeup_enable(&pdev->dev); > - ret = devm_add_action_or_reset(&pdev->dev, > - palmas_disable_wakeup, > - &pdev->dev); > - if (ret) > - return ret; > - } > - > return 0; > } > > diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h > index 1e61c7e9f50d..55f22adb1a9e 100644 > --- a/include/linux/mfd/palmas.h > +++ b/include/linux/mfd/palmas.h > @@ -129,12 +129,6 @@ struct palmas_pmic_driver_data { > struct regulator_config config); > }; > > -struct palmas_adc_wakeup_property { > - int adc_channel_number; > - int adc_high_threshold; > - int adc_low_threshold; > -}; > - > struct palmas_gpadc_platform_data { > /* Channel 3 current source is only enabled during conversion */ > int ch3_current; /* 0: off; 1: 10uA; 2: 400uA; 3: 800 uA */ > @@ -153,8 +147,6 @@ struct palmas_gpadc_platform_data { > int start_polarity; > > int auto_conversion_period_ms; > - struct palmas_adc_wakeup_property *adc_wakeup1_data; > - struct palmas_adc_wakeup_property *adc_wakeup2_data; > }; > > struct palmas_reg_init { > -- > 2.25.1 >
On Mon, Apr 03, 2023 at 06:49:36PM +0200, H. Nikolaus Schaller wrote: > Hi Patrik, > > > Am 02.04.2023 um 18:42 schrieb Patrik Dahlström <risca@dalakolonin.se>: > > > > It does not seem to be used by anyone and later patches in this series > > are made simpler by first removing this. There is now a lot of dead code > > that cannot be reached, until later patches revive it. Arguably, this is > > preferred over removing the code only to add it again. > > > > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> > > --- > > drivers/iio/adc/palmas_gpadc.c | 50 ++++------------------------------ > > include/linux/mfd/palmas.h | 8 ------ > > 2 files changed, 6 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > > index 24d7c096e4b8..943ac579eb1f 100644 > > --- a/drivers/iio/adc/palmas_gpadc.c > > +++ b/drivers/iio/adc/palmas_gpadc.c > > @@ -76,6 +76,12 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = { > > PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true), > > }; > > > > +struct palmas_adc_wakeup_property { > > + int adc_channel_number; > > + int adc_high_threshold; > > + int adc_low_threshold; > > +}; > > + > > /* > > * struct palmas_gpadc - the palmas_gpadc structure > > * @ch0_current: channel 0 current source setting > > @@ -492,11 +498,6 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev, > > return 0; > > } > > > > -static void palmas_disable_wakeup(void *dev) > > something seems to be mixed up here. > > There is no palmas_disable_wakeup() upstream that can be removed. So this patch > can not be applied as 1/7 to any upstream kernel. > > Please rebase your series on either linus/master or linux-next/master. I'm sorry for the confusion. I should have been more clear in the cover letter. This series is based on Jonathan Cameron's iio tree[1], plus the patches at [2] and [3]. The first patch is already part of linux-next and I was under the impression that [3] would be soon too. Would it be best to rebase this series on linux-next instead? [1] git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git [2] https://patchwork.kernel.org/project/linux-iio/patch/20230313205029.1881745-1-risca@dalakolonin.se/ [3] https://patchwork.kernel.org/project/linux-iio/patch/20230318163039.56115-1-jic23@kernel.org/ > > BR, > Nikolaus > > > -{ > > - device_wakeup_disable(dev); > > -} > > - > > static int palmas_gpadc_probe(struct platform_device *pdev) > > { > > struct palmas_gpadc *adc; > > @@ -547,36 +548,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > return dev_err_probe(adc->dev, ret, > > "request irq %d failed\n", adc->irq); > > > > - if (gpadc_pdata->adc_wakeup1_data) { > > - memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data, > > - sizeof(adc->wakeup1_data)); > > - adc->wakeup1_enable = true; > > - adc->irq_auto_0 = platform_get_irq(pdev, 1); > > - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, > > - NULL, palmas_gpadc_irq_auto, > > - IRQF_ONESHOT, > > - "palmas-adc-auto-0", adc); > > - if (ret < 0) > > - return dev_err_probe(adc->dev, ret, > > - "request auto0 irq %d failed\n", > > - adc->irq_auto_0); > > - } > > - > > - if (gpadc_pdata->adc_wakeup2_data) { > > - memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data, > > - sizeof(adc->wakeup2_data)); > > - adc->wakeup2_enable = true; > > - adc->irq_auto_1 = platform_get_irq(pdev, 2); > > - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, > > - NULL, palmas_gpadc_irq_auto, > > - IRQF_ONESHOT, > > - "palmas-adc-auto-1", adc); > > - if (ret < 0) > > - return dev_err_probe(adc->dev, ret, > > - "request auto1 irq %d failed\n", > > - adc->irq_auto_1); > > - } > > - > > /* set the current source 0 (value 0/5/15/20 uA => 0..3) */ > > if (gpadc_pdata->ch0_current <= 1) > > adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0; > > @@ -616,15 +587,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > palmas_gpadc_calibrate(adc, i); > > } > > > > - if (adc->wakeup1_enable || adc->wakeup2_enable) { > > - device_wakeup_enable(&pdev->dev); > > - ret = devm_add_action_or_reset(&pdev->dev, > > - palmas_disable_wakeup, > > - &pdev->dev); > > - if (ret) > > - return ret; > > - } > > - > > return 0; > > } > > > > diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h > > index 1e61c7e9f50d..55f22adb1a9e 100644 > > --- a/include/linux/mfd/palmas.h > > +++ b/include/linux/mfd/palmas.h > > @@ -129,12 +129,6 @@ struct palmas_pmic_driver_data { > > struct regulator_config config); > > }; > > > > -struct palmas_adc_wakeup_property { > > - int adc_channel_number; > > - int adc_high_threshold; > > - int adc_low_threshold; > > -}; > > - > > struct palmas_gpadc_platform_data { > > /* Channel 3 current source is only enabled during conversion */ > > int ch3_current; /* 0: off; 1: 10uA; 2: 400uA; 3: 800 uA */ > > @@ -153,8 +147,6 @@ struct palmas_gpadc_platform_data { > > int start_polarity; > > > > int auto_conversion_period_ms; > > - struct palmas_adc_wakeup_property *adc_wakeup1_data; > > - struct palmas_adc_wakeup_property *adc_wakeup2_data; > > }; > > > > struct palmas_reg_init { > > -- > > 2.25.1 > > >
On Tue, 4 Apr 2023 12:33:28 +0200 Patrik Dahlström <risca@dalakolonin.se> wrote: > On Mon, Apr 03, 2023 at 06:49:36PM +0200, H. Nikolaus Schaller wrote: > > Hi Patrik, > > > > > Am 02.04.2023 um 18:42 schrieb Patrik Dahlström <risca@dalakolonin.se>: > > > > > > It does not seem to be used by anyone and later patches in this series > > > are made simpler by first removing this. There is now a lot of dead code > > > that cannot be reached, until later patches revive it. Arguably, this is > > > preferred over removing the code only to add it again. > > > > > > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> > > > --- > > > drivers/iio/adc/palmas_gpadc.c | 50 ++++------------------------------ > > > include/linux/mfd/palmas.h | 8 ------ > > > 2 files changed, 6 insertions(+), 52 deletions(-) > > > > > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > > > index 24d7c096e4b8..943ac579eb1f 100644 > > > --- a/drivers/iio/adc/palmas_gpadc.c > > > +++ b/drivers/iio/adc/palmas_gpadc.c > > > @@ -76,6 +76,12 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = { > > > PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true), > > > }; > > > > > > +struct palmas_adc_wakeup_property { > > > + int adc_channel_number; > > > + int adc_high_threshold; > > > + int adc_low_threshold; > > > +}; > > > + > > > /* > > > * struct palmas_gpadc - the palmas_gpadc structure > > > * @ch0_current: channel 0 current source setting > > > @@ -492,11 +498,6 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev, > > > return 0; > > > } > > > > > > -static void palmas_disable_wakeup(void *dev) > > > > something seems to be mixed up here. > > > > There is no palmas_disable_wakeup() upstream that can be removed. So this patch > > can not be applied as 1/7 to any upstream kernel. > > > > Please rebase your series on either linus/master or linux-next/master. > > I'm sorry for the confusion. I should have been more clear in the cover > letter. > > This series is based on Jonathan Cameron's iio tree[1], plus the patches at > [2] and [3]. The first patch is already part of linux-next and I was under > the impression that [3] would be soon too. > > Would it be best to rebase this series on linux-next instead? > > [1] git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git > [2] https://patchwork.kernel.org/project/linux-iio/patch/20230313205029.1881745-1-risca@dalakolonin.se/ > [3] https://patchwork.kernel.org/project/linux-iio/patch/20230318163039.56115-1-jic23@kernel.org/ > This gets a bit complex because we have a mixture of fixes and updates and the desire to end up with a fairly simple history in git. Before [3] can be applied I need to move the togreg tree base to incorporate a newer version of char-misc/char-misc-next (which is the upstream for IIO pull requests) and the fix to have made the round trip to get there (which hasn't happened yet). At that point I'll pick up the above and your series will be on top of that. So you were fine to base this patch as you did, but you should make sure the cover letter calls out the required patches that haven't yet made it upstream. I generally don't mind simple cases of needing to rebase if things are based on rc1 (don't every use linux-next as it is not a stable base) whilst applying but in this case the devm change is sufficiently fiddly that you have two obvious choices. 1) Just add it to the start of your patch set (I'm fine with people doing that with anything I post). 2) Listing the patches that should be applied first in the cover letter. Jonathan > > > > BR, > > Nikolaus > > > > > -{ > > > - device_wakeup_disable(dev); > > > -} > > > - > > > static int palmas_gpadc_probe(struct platform_device *pdev) > > > { > > > struct palmas_gpadc *adc; > > > @@ -547,36 +548,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > return dev_err_probe(adc->dev, ret, > > > "request irq %d failed\n", adc->irq); > > > > > > - if (gpadc_pdata->adc_wakeup1_data) { > > > - memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data, > > > - sizeof(adc->wakeup1_data)); > > > - adc->wakeup1_enable = true; > > > - adc->irq_auto_0 = platform_get_irq(pdev, 1); > > > - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, > > > - NULL, palmas_gpadc_irq_auto, > > > - IRQF_ONESHOT, > > > - "palmas-adc-auto-0", adc); > > > - if (ret < 0) > > > - return dev_err_probe(adc->dev, ret, > > > - "request auto0 irq %d failed\n", > > > - adc->irq_auto_0); > > > - } > > > - > > > - if (gpadc_pdata->adc_wakeup2_data) { > > > - memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data, > > > - sizeof(adc->wakeup2_data)); > > > - adc->wakeup2_enable = true; > > > - adc->irq_auto_1 = platform_get_irq(pdev, 2); > > > - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, > > > - NULL, palmas_gpadc_irq_auto, > > > - IRQF_ONESHOT, > > > - "palmas-adc-auto-1", adc); > > > - if (ret < 0) > > > - return dev_err_probe(adc->dev, ret, > > > - "request auto1 irq %d failed\n", > > > - adc->irq_auto_1); > > > - } > > > - > > > /* set the current source 0 (value 0/5/15/20 uA => 0..3) */ > > > if (gpadc_pdata->ch0_current <= 1) > > > adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0; > > > @@ -616,15 +587,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > palmas_gpadc_calibrate(adc, i); > > > } > > > > > > - if (adc->wakeup1_enable || adc->wakeup2_enable) { > > > - device_wakeup_enable(&pdev->dev); > > > - ret = devm_add_action_or_reset(&pdev->dev, > > > - palmas_disable_wakeup, > > > - &pdev->dev); > > > - if (ret) > > > - return ret; > > > - } > > > - > > > return 0; > > > } > > > > > > diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h > > > index 1e61c7e9f50d..55f22adb1a9e 100644 > > > --- a/include/linux/mfd/palmas.h > > > +++ b/include/linux/mfd/palmas.h > > > @@ -129,12 +129,6 @@ struct palmas_pmic_driver_data { > > > struct regulator_config config); > > > }; > > > > > > -struct palmas_adc_wakeup_property { > > > - int adc_channel_number; > > > - int adc_high_threshold; > > > - int adc_low_threshold; > > > -}; > > > - > > > struct palmas_gpadc_platform_data { > > > /* Channel 3 current source is only enabled during conversion */ > > > int ch3_current; /* 0: off; 1: 10uA; 2: 400uA; 3: 800 uA */ > > > @@ -153,8 +147,6 @@ struct palmas_gpadc_platform_data { > > > int start_polarity; > > > > > > int auto_conversion_period_ms; > > > - struct palmas_adc_wakeup_property *adc_wakeup1_data; > > > - struct palmas_adc_wakeup_property *adc_wakeup2_data; > > > }; > > > > > > struct palmas_reg_init { > > > -- > > > 2.25.1 > > > > >
diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c index 24d7c096e4b8..943ac579eb1f 100644 --- a/drivers/iio/adc/palmas_gpadc.c +++ b/drivers/iio/adc/palmas_gpadc.c @@ -76,6 +76,12 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = { PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true), }; +struct palmas_adc_wakeup_property { + int adc_channel_number; + int adc_high_threshold; + int adc_low_threshold; +}; + /* * struct palmas_gpadc - the palmas_gpadc structure * @ch0_current: channel 0 current source setting @@ -492,11 +498,6 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev, return 0; } -static void palmas_disable_wakeup(void *dev) -{ - device_wakeup_disable(dev); -} - static int palmas_gpadc_probe(struct platform_device *pdev) { struct palmas_gpadc *adc; @@ -547,36 +548,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) return dev_err_probe(adc->dev, ret, "request irq %d failed\n", adc->irq); - if (gpadc_pdata->adc_wakeup1_data) { - memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data, - sizeof(adc->wakeup1_data)); - adc->wakeup1_enable = true; - adc->irq_auto_0 = platform_get_irq(pdev, 1); - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, - NULL, palmas_gpadc_irq_auto, - IRQF_ONESHOT, - "palmas-adc-auto-0", adc); - if (ret < 0) - return dev_err_probe(adc->dev, ret, - "request auto0 irq %d failed\n", - adc->irq_auto_0); - } - - if (gpadc_pdata->adc_wakeup2_data) { - memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data, - sizeof(adc->wakeup2_data)); - adc->wakeup2_enable = true; - adc->irq_auto_1 = platform_get_irq(pdev, 2); - ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, - NULL, palmas_gpadc_irq_auto, - IRQF_ONESHOT, - "palmas-adc-auto-1", adc); - if (ret < 0) - return dev_err_probe(adc->dev, ret, - "request auto1 irq %d failed\n", - adc->irq_auto_1); - } - /* set the current source 0 (value 0/5/15/20 uA => 0..3) */ if (gpadc_pdata->ch0_current <= 1) adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0; @@ -616,15 +587,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev) palmas_gpadc_calibrate(adc, i); } - if (adc->wakeup1_enable || adc->wakeup2_enable) { - device_wakeup_enable(&pdev->dev); - ret = devm_add_action_or_reset(&pdev->dev, - palmas_disable_wakeup, - &pdev->dev); - if (ret) - return ret; - } - return 0; } diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h index 1e61c7e9f50d..55f22adb1a9e 100644 --- a/include/linux/mfd/palmas.h +++ b/include/linux/mfd/palmas.h @@ -129,12 +129,6 @@ struct palmas_pmic_driver_data { struct regulator_config config); }; -struct palmas_adc_wakeup_property { - int adc_channel_number; - int adc_high_threshold; - int adc_low_threshold; -}; - struct palmas_gpadc_platform_data { /* Channel 3 current source is only enabled during conversion */ int ch3_current; /* 0: off; 1: 10uA; 2: 400uA; 3: 800 uA */ @@ -153,8 +147,6 @@ struct palmas_gpadc_platform_data { int start_polarity; int auto_conversion_period_ms; - struct palmas_adc_wakeup_property *adc_wakeup1_data; - struct palmas_adc_wakeup_property *adc_wakeup2_data; }; struct palmas_reg_init {
It does not seem to be used by anyone and later patches in this series are made simpler by first removing this. There is now a lot of dead code that cannot be reached, until later patches revive it. Arguably, this is preferred over removing the code only to add it again. Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> --- drivers/iio/adc/palmas_gpadc.c | 50 ++++------------------------------ include/linux/mfd/palmas.h | 8 ------ 2 files changed, 6 insertions(+), 52 deletions(-)