The regmap is coming from the parent also in case of Xe
GPUs. Reusing the Wangxun quirk for that.
Originally-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 879719e91df2..a35e4c64a1d4 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -101,7 +101,7 @@ static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
}
#endif
-static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
+static int dw_i2c_get_parent_regmap(struct dw_i2c_dev *dev)
{
dev->map = dev_get_regmap(dev->dev->parent, NULL);
if (!dev->map)
@@ -123,12 +123,15 @@ static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev)
struct platform_device *pdev = to_platform_device(dev->dev);
int ret;
+ if (device_is_compatible(dev->dev, "intel,xe-i2c"))
+ return dw_i2c_get_parent_regmap(dev);
+
switch (dev->flags & MODEL_MASK) {
case MODEL_BAIKAL_BT1:
ret = bt1_i2c_request_regs(dev);
break;
case MODEL_WANGXUN_SP:
- ret = txgbe_i2c_request_regs(dev);
+ ret = dw_i2c_get_parent_regmap(dev);
break;
default:
dev->base = devm_platform_ioremap_resource(pdev, 0);
@@ -205,25 +208,28 @@ static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
+ u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
struct device *device = &pdev->dev;
struct i2c_adapter *adap;
struct dw_i2c_dev *dev;
int irq, ret;
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
+ irq = platform_get_irq_optional(pdev, 0);
+ if (irq == -ENXIO)
+ flags |= ACCESS_POLLING;
+ else if (irq < 0)
return irq;
dev = devm_kzalloc(device, sizeof(*dev), GFP_KERNEL);
if (!dev)
return -ENOMEM;
- dev->flags = (uintptr_t)device_get_match_data(device);
if (device_property_present(device, "wx,i2c-snps-model"))
- dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
+ flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
dev->dev = device;
dev->irq = irq;
+ dev->flags = flags;
platform_set_drvdata(pdev, dev);
ret = dw_i2c_plat_request_regs(dev);
--
2.47.2Hi Heikki, On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote: > The regmap is coming from the parent also in case of Xe > GPUs. Reusing the Wangxun quirk for that. What I don't like, though, is that there is no mention of the change in the probe in the commit log. Besides, are these changes related? Can we have them split in two different patches? I guess this would also make Andy's point about the extra churn. Andi > Originally-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Mon, Jun 30, 2025 at 08:28:12PM +0200, Andi Shyti wrote: > Hi Heikki, > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote: > > The regmap is coming from the parent also in case of Xe > > GPUs. Reusing the Wangxun quirk for that. > > What I don't like, though, is that there is no mention of the > change in the probe in the commit log. > > Besides, are these changes related? Can we have them split in two > different patches? I guess this would also make Andy's point > about the extra churn. I think you are right about splitting the patch. I'll do just that. Thanks for the review Andi. -- heikki
On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> The regmap is coming from the parent also in case of Xe
> GPUs. Reusing the Wangxun quirk for that.
...
> static int dw_i2c_plat_probe(struct platform_device *pdev)
> {
> + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> - dev->flags = (uintptr_t)device_get_match_data(device);
> if (device_property_present(device, "wx,i2c-snps-model"))
> - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
>
> dev->dev = device;
> dev->irq = irq;
> + dev->flags = flags;
Maybe I'm missing something, but why do we need these (above) changes?
--
With Best Regards,
Andy Shevchenko
On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> > The regmap is coming from the parent also in case of Xe
> > GPUs. Reusing the Wangxun quirk for that.
>
> ...
>
> > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > {
> > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
>
> > - dev->flags = (uintptr_t)device_get_match_data(device);
> > if (device_property_present(device, "wx,i2c-snps-model"))
> > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> >
> > dev->dev = device;
> > dev->irq = irq;
> > + dev->flags = flags;
>
> Maybe I'm missing something, but why do we need these (above) changes?
in between, it is introduced a new one:
flags |= ACCESS_POLLING;
So, the initialization moved up, before the ACCESS_POLLING, and
it let the assignment to the last, along with the group.
Looks good to me:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
...
> > > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > {
> > > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> >
> > > - dev->flags = (uintptr_t)device_get_match_data(device);
> > > if (device_property_present(device, "wx,i2c-snps-model"))
> > > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > >
> > > dev->dev = device;
> > > dev->irq = irq;
> > > + dev->flags = flags;
> >
> > Maybe I'm missing something, but why do we need these (above) changes?
>
> in between, it is introduced a new one:
> flags |= ACCESS_POLLING;
>
> So, the initialization moved up, before the ACCESS_POLLING, and
> it let the assignment to the last, along with the group.
I still don't get. The cited code is complete equivalent.
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
>
> ...
>
> > > > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > {
> > > > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > >
> > > > - dev->flags = (uintptr_t)device_get_match_data(device);
> > > > if (device_property_present(device, "wx,i2c-snps-model"))
> > > > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > >
> > > > dev->dev = device;
> > > > dev->irq = irq;
> > > > + dev->flags = flags;
> > >
> > > Maybe I'm missing something, but why do we need these (above) changes?
> >
> > in between, it is introduced a new one:
> > flags |= ACCESS_POLLING;
> >
> > So, the initialization moved up, before the ACCESS_POLLING, and
> > it let the assignment to the last, along with the group.
>
> I still don't get. The cited code is complete equivalent.
This was requested by Jarkko.
thanks,
--
heikki
On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
...
> > > > > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > {
> > > > > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > >
> > > > > - dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > >
> > > > > dev->dev = device;
> > > > > dev->irq = irq;
> > > > > + dev->flags = flags;
> > > >
> > > > Maybe I'm missing something, but why do we need these (above) changes?
> > >
> > > in between, it is introduced a new one:
> > > flags |= ACCESS_POLLING;
> > >
> > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > it let the assignment to the last, along with the group.
> >
> > I still don't get. The cited code is complete equivalent.
>
> This was requested by Jarkko.
Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
required?
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
>
> ...
>
> > > > > > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > {
> > > > > > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > >
> > > > > > - dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > >
> > > > > > dev->dev = device;
> > > > > > dev->irq = irq;
> > > > > > + dev->flags = flags;
> > > > >
> > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > >
> > > > in between, it is introduced a new one:
> > > > flags |= ACCESS_POLLING;
> > > >
> > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > it let the assignment to the last, along with the group.
> > >
> > > I still don't get. The cited code is complete equivalent.
> >
> > This was requested by Jarkko.
>
> Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> required?
You need to ask why from Jarkko - I did not really question him on
this one. Unfortunately his on vacation at the moment. I can drop
this, but then I'll have to drop also Jarkko's ACK.
I think we already agreed that this function, and probable the entire
file, need to be refactored a bit, so would you mind much if we just
went ahead with this as it is?
I'm asking that also because I don't have means or time to test this
anymore before I start my vacation.
thanks,
--
heikki
On Mon, Jun 30, 2025 at 02:59:21PM +0300, Heikki Krogerus wrote:
> On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
...
> > > > > > > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > > {
> > > > > > > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > >
> > > > > > > - dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > > if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > >
> > > > > > > dev->dev = device;
> > > > > > > dev->irq = irq;
> > > > > > > + dev->flags = flags;
> > > > > >
> > > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > >
> > > > > in between, it is introduced a new one:
> > > > > flags |= ACCESS_POLLING;
> > > > >
> > > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > > it let the assignment to the last, along with the group.
> > > >
> > > > I still don't get. The cited code is complete equivalent.
> > >
> > > This was requested by Jarkko.
> >
> > Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> > required?
>
> You need to ask why from Jarkko - I did not really question him on
> this one. Unfortunately his on vacation at the moment.
Yeah :-(
> I can drop this, but then I'll have to drop also Jarkko's ACK.
I can give mine if it helps. The code as far as I can see is 100% equivalent.
> I think we already agreed that this function, and probable the entire
> file, need to be refactored a bit, so would you mind much if we just
> went ahead with this as it is?
>
> I'm asking that also because I don't have means or time to test this
> anymore before I start my vacation.
I see, then we may ask Andi and Wolfram on this. I slightly prefer to have
no additional churn added without a good reason.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Mon, Jun 30, 2025 at 04:16:56PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 02:59:21PM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > > > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
>
> ...
>
> > > > > > > > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > > > {
> > > > > > > > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > > >
> > > > > > > > - dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > > > if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > > > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > >
> > > > > > > > dev->dev = device;
> > > > > > > > dev->irq = irq;
> > > > > > > > + dev->flags = flags;
> > > > > > >
> > > > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > > >
> > > > > > in between, it is introduced a new one:
> > > > > > flags |= ACCESS_POLLING;
> > > > > >
> > > > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > > > it let the assignment to the last, along with the group.
> > > > >
> > > > > I still don't get. The cited code is complete equivalent.
> > > >
> > > > This was requested by Jarkko.
> > >
> > > Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> > > required?
I don't think it makes sense to add the extra flag later as it
would be a non relevant change, so that I'm fine with having it
here.
> > You need to ask why from Jarkko - I did not really question him on
> > this one. Unfortunately his on vacation at the moment.
>
> Yeah :-(
>
> > I can drop this, but then I'll have to drop also Jarkko's ACK.
Just for reference, Ack is not Reviewed. If Jarkko Acks, I assume
he is OK with the general idea, not specifically with the code. I
think that if you change a bit the code without altering the
result you can keep the Ack. Up to you.
> I can give mine if it helps. The code as far as I can see is 100% equivalent.
>
> > I think we already agreed that this function, and probable the entire
> > file, need to be refactored a bit, so would you mind much if we just
> > went ahead with this as it is?
> >
> > I'm asking that also because I don't have means or time to test this
> > anymore before I start my vacation.
>
> I see, then we may ask Andi and Wolfram on this. I slightly prefer to have
> no additional churn added without a good reason.
To be honest I don't really mind. I think that if we add the
extra "flag" or we don't, the amount of code is the same.
If we decide not to add the extra "flag", then we need to move
the initialization of dev->flag above or shuffle something else
around.
In the meantime:
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Thanks,
Andi
On Mon, Jun 30, 2025 at 04:16:56PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 02:59:21PM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > > > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
>
> ...
>
> > > > > > > > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > > > {
> > > > > > > > + u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > > >
> > > > > > > > - dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > > > if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > > > - dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > > + flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > >
> > > > > > > > dev->dev = device;
> > > > > > > > dev->irq = irq;
> > > > > > > > + dev->flags = flags;
> > > > > > >
> > > > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > > >
> > > > > > in between, it is introduced a new one:
> > > > > > flags |= ACCESS_POLLING;
> > > > > >
> > > > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > > > it let the assignment to the last, along with the group.
> > > > >
> > > > > I still don't get. The cited code is complete equivalent.
> > > >
> > > > This was requested by Jarkko.
> > >
> > > Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> > > required?
> >
> > You need to ask why from Jarkko - I did not really question him on
> > this one. Unfortunately his on vacation at the moment.
>
> Yeah :-(
>
> > I can drop this, but then I'll have to drop also Jarkko's ACK.
>
> I can give mine if it helps. The code as far as I can see is 100% equivalent.
>
> > I think we already agreed that this function, and probable the entire
> > file, need to be refactored a bit, so would you mind much if we just
> > went ahead with this as it is?
> >
> > I'm asking that also because I don't have means or time to test this
> > anymore before I start my vacation.
>
> I see, then we may ask Andi and Wolfram on this. I slightly prefer to have
> no additional churn added without a good reason.
OK. I'll fix this.
thanks,
--
heikki
© 2016 - 2025 Red Hat, Inc.