With 16 channel pwm support, we're registering two instances of pwm_chip
with 8 channels each. We need to update PM functions to use both instances
of pwm_chip during power state transitions.
Introduce struct dwc_pwm_drvdata and use it as driver_data, which will
maintain both instances of pwm_chip along with dwc_pwm_info and allow us
to use them inside suspend/resume handles.
Fixes: ebf2c89eb95e ("pwm: dwc: Add 16 channel support for Intel Elkhart Lake")
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/pwm/pwm-dwc-core.c | 1 -
drivers/pwm/pwm-dwc.c | 82 +++++++++++++++++++++++++-------------
drivers/pwm/pwm-dwc.h | 6 +++
3 files changed, 60 insertions(+), 29 deletions(-)
diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
index 043736972cb9..c8425493b95d 100644
--- a/drivers/pwm/pwm-dwc-core.c
+++ b/drivers/pwm/pwm-dwc-core.c
@@ -172,7 +172,6 @@ struct pwm_chip *dwc_pwm_alloc(struct device *dev)
dwc->clk_ns = 10;
chip->ops = &dwc_pwm_ops;
- dev_set_drvdata(dev, chip);
return chip;
}
EXPORT_SYMBOL_GPL(dwc_pwm_alloc);
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 676eaf8d7a53..1bf0ee4d3229 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -31,26 +31,34 @@ static const struct dwc_pwm_info ehl_pwm_info = {
.size = 0x1000,
};
-static int dwc_pwm_init_one(struct device *dev, void __iomem *base, unsigned int offset)
+static int dwc_pwm_init_one(struct device *dev, struct dwc_pwm_drvdata *data, unsigned int idx)
{
struct pwm_chip *chip;
struct dwc_pwm *dwc;
+ int ret;
chip = dwc_pwm_alloc(dev);
if (IS_ERR(chip))
return PTR_ERR(chip);
dwc = to_dwc_pwm(chip);
- dwc->base = base + offset;
+ dwc->base = data->io_base + (data->info->size * idx);
- return devm_pwmchip_add(dev, chip);
+ ret = devm_pwmchip_add(dev, chip);
+ if (ret)
+ return ret;
+
+ data->chips[idx] = chip;
+ return 0;
}
static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
{
const struct dwc_pwm_info *info;
struct device *dev = &pci->dev;
- int i, ret;
+ struct dwc_pwm_drvdata *data;
+ unsigned int idx;
+ int ret;
ret = pcim_enable_device(pci);
if (ret)
@@ -63,17 +71,25 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
return dev_err_probe(dev, ret, "Failed to iomap PCI BAR\n");
info = (const struct dwc_pwm_info *)id->driver_data;
+ data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL);
+ if (!data)
+ return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n");
- for (i = 0; i < info->nr; i++) {
- /*
- * No need to check for pcim_iomap_table() failure,
- * pcim_iomap_regions() already does it for us.
- */
- ret = dwc_pwm_init_one(dev, pcim_iomap_table(pci)[0], i * info->size);
+ /*
+ * No need to check for pcim_iomap_table() failure,
+ * pcim_iomap_regions() already does it for us.
+ */
+ data->io_base = pcim_iomap_table(pci)[0];
+ data->info = info;
+
+ for (idx = 0; idx < data->info->nr; idx++) {
+ ret = dwc_pwm_init_one(dev, data, idx);
if (ret)
return ret;
}
+ dev_set_drvdata(dev, data);
+
pm_runtime_put(dev);
pm_runtime_allow(dev);
@@ -88,19 +104,24 @@ static void dwc_pwm_remove(struct pci_dev *pci)
static int dwc_pwm_suspend(struct device *dev)
{
- struct pwm_chip *chip = dev_get_drvdata(dev);
- struct dwc_pwm *dwc = to_dwc_pwm(chip);
- int i;
+ struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
+ unsigned int idx;
- for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
- if (chip->pwms[i].state.enabled) {
- dev_err(dev, "PWM %u in use by consumer (%s)\n",
- i, chip->pwms[i].label);
- return -EBUSY;
+ for (idx = 0; idx < data->info->nr; idx++) {
+ struct pwm_chip *chip = data->chips[idx];
+ struct dwc_pwm *dwc = to_dwc_pwm(chip);
+ unsigned int i;
+
+ for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
+ if (chip->pwms[i].state.enabled) {
+ dev_err(dev, "PWM %u in use by consumer (%s)\n",
+ i, chip->pwms[i].label);
+ return -EBUSY;
+ }
+ dwc->ctx[i].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(i));
+ dwc->ctx[i].cnt2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(i));
+ dwc->ctx[i].ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i));
}
- dwc->ctx[i].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(i));
- dwc->ctx[i].cnt2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(i));
- dwc->ctx[i].ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i));
}
return 0;
@@ -108,14 +129,19 @@ static int dwc_pwm_suspend(struct device *dev)
static int dwc_pwm_resume(struct device *dev)
{
- struct pwm_chip *chip = dev_get_drvdata(dev);
- struct dwc_pwm *dwc = to_dwc_pwm(chip);
- int i;
+ struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
+ unsigned int idx;
- for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
- dwc_pwm_writel(dwc, dwc->ctx[i].cnt, DWC_TIM_LD_CNT(i));
- dwc_pwm_writel(dwc, dwc->ctx[i].cnt2, DWC_TIM_LD_CNT2(i));
- dwc_pwm_writel(dwc, dwc->ctx[i].ctrl, DWC_TIM_CTRL(i));
+ for (idx = 0; idx < data->info->nr; idx++) {
+ struct pwm_chip *chip = data->chips[idx];
+ struct dwc_pwm *dwc = to_dwc_pwm(chip);
+ unsigned int i;
+
+ for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
+ dwc_pwm_writel(dwc, dwc->ctx[i].cnt, DWC_TIM_LD_CNT(i));
+ dwc_pwm_writel(dwc, dwc->ctx[i].cnt2, DWC_TIM_LD_CNT2(i));
+ dwc_pwm_writel(dwc, dwc->ctx[i].ctrl, DWC_TIM_CTRL(i));
+ }
}
return 0;
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index a8b074841ae8..c6e2df5a6122 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -38,6 +38,12 @@ struct dwc_pwm_info {
unsigned int size;
};
+struct dwc_pwm_drvdata {
+ const struct dwc_pwm_info *info;
+ void __iomem *io_base;
+ struct pwm_chip *chips[];
+};
+
struct dwc_pwm_ctx {
u32 cnt;
u32 cnt2;
--
2.35.3
Hello,
On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
> diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
> index a8b074841ae8..c6e2df5a6122 100644
> --- a/drivers/pwm/pwm-dwc.h
> +++ b/drivers/pwm/pwm-dwc.h
> @@ -38,6 +38,12 @@ struct dwc_pwm_info {
> unsigned int size;
> };
>
> +struct dwc_pwm_drvdata {
> + const struct dwc_pwm_info *info;
> + void __iomem *io_base;
.io_base is only used during init time and so doesn't need to be tracked
in driver data.
Otherwise I (only slightly) dislike
> + struct dwc_pwm_drvdata *data;
because "data" is very generic. I'd call it ddata. But I don't feel
strong here. I'm happy if you change to "ddata" in v2, I will silently
apply anyhow if you prefer "data".
> + struct pwm_chip *chips[];
> +};
> +
> struct dwc_pwm_ctx {
> u32 cnt;
> u32 cnt2;
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
> With 16 channel pwm support, we're registering two instances of pwm_chip
> with 8 channels each. We need to update PM functions to use both instances
> of pwm_chip during power state transitions.
>
> Introduce struct dwc_pwm_drvdata and use it as driver_data, which will
> maintain both instances of pwm_chip along with dwc_pwm_info and allow us
> to use them inside suspend/resume handles.
..
> + data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL);
> + if (!data)
> + return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n");
Haven't noticed before, but we do not print messages for -ENOMEM. Just return
the code.
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-K?nig wrote:
> On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
..
> > +struct dwc_pwm_drvdata {
> > + const struct dwc_pwm_info *info;
> > + void __iomem *io_base;
>
> .io_base is only used during init time and so doesn't need to be tracked
> in driver data.
It's me who asked for this specific change and I think it's still beneficial
as it allows to simplify the code.
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 12, 2024 at 06:38:24PM +0300, Raag Jadav wrote:
> On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
..
> > Otherwise I (only slightly) dislike
> > > + struct dwc_pwm_drvdata *data;
> > because "data" is very generic. I'd call it ddata. But I don't feel
> > strong here. I'm happy if you change to "ddata" in v2, I will silently
> > apply anyhow if you prefer "data".
>
> I think "data" is more readable, something like "ddata" would make me
> re-adjust my glasses ;)
ddata is _kinda_ idiomatic, there at least several drivers use this name
(as of my knowledge). I am bending towards Uwe's suggestion here.
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
> > diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
> > index a8b074841ae8..c6e2df5a6122 100644
> > --- a/drivers/pwm/pwm-dwc.h
> > +++ b/drivers/pwm/pwm-dwc.h
> > @@ -38,6 +38,12 @@ struct dwc_pwm_info {
> > unsigned int size;
> > };
> >
> > +struct dwc_pwm_drvdata {
> > + const struct dwc_pwm_info *info;
> > + void __iomem *io_base;
>
> .io_base is only used during init time and so doesn't need to be tracked
> in driver data.
It will be useful for other features which are WIP.
> Otherwise I (only slightly) dislike
> > + struct dwc_pwm_drvdata *data;
> because "data" is very generic. I'd call it ddata. But I don't feel
> strong here. I'm happy if you change to "ddata" in v2, I will silently
> apply anyhow if you prefer "data".
I think "data" is more readable, something like "ddata" would make me
re-adjust my glasses ;)
Raag
On Fri, Apr 12, 2024 at 06:45:48PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 12, 2024 at 06:38:24PM +0300, Raag Jadav wrote:
> > On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-K?nig wrote:
> > > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
>
> ...
>
> > > Otherwise I (only slightly) dislike
> > > > + struct dwc_pwm_drvdata *data;
> > > because "data" is very generic. I'd call it ddata. But I don't feel
> > > strong here. I'm happy if you change to "ddata" in v2, I will silently
> > > apply anyhow if you prefer "data".
> >
> > I think "data" is more readable, something like "ddata" would make me
> > re-adjust my glasses ;)
>
> ddata is _kinda_ idiomatic, there at least several drivers use this name
> (as of my knowledge). I am bending towards Uwe's suggestion here.
Will update in v2.
Raag
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Fri, Apr 12, 2024 at 06:35:51PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
> > With 16 channel pwm support, we're registering two instances of pwm_chip
> > with 8 channels each. We need to update PM functions to use both instances
> > of pwm_chip during power state transitions.
> >
> > Introduce struct dwc_pwm_drvdata and use it as driver_data, which will
> > maintain both instances of pwm_chip along with dwc_pwm_info and allow us
> > to use them inside suspend/resume handles.
>
> ...
>
> > + data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL);
> > + if (!data)
> > + return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n");
>
> Haven't noticed before, but we do not print messages for -ENOMEM. Just return
> the code.
Any special case for -ENOMEM?
I found a few drivers which still do.
Raag
On Fri, Apr 12, 2024 at 07:15:11PM +0300, Raag Jadav wrote:
> On Fri, Apr 12, 2024 at 06:35:51PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
..
> > > + data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL);
> > > + if (!data)
> > > + return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n");
> >
> > Haven't noticed before, but we do not print messages for -ENOMEM. Just return
> > the code.
>
> Any special case for -ENOMEM?
Yes. But I have no link to the discussion at hand. Just believe me, or look into
Git history for changes that drop that messaging. Perhaps one or two will give a
hint where it starts from...
> I found a few drivers which still do.
They should be fixed.
--
With Best Regards,
Andy Shevchenko
Hello,
On Fri, Apr 12, 2024 at 06:32:37PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote:
>
> ...
>
> > > +struct dwc_pwm_drvdata {
> > > + const struct dwc_pwm_info *info;
> > > + void __iomem *io_base;
> >
> > .io_base is only used during init time and so doesn't need to be tracked
> > in driver data.
>
> It's me who asked for this specific change and I think it's still beneficial
> as it allows to simplify the code.
Ok then, feel free to keep it for v2 then.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |