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(-)
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 <raag.jadav@intel.com>
---
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.3On 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 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 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 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: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 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 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
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/ |
© 2016 - 2026 Red Hat, Inc.