diff mbox series

[2/4] i3c: dw: Add platform operations

Message ID eb90bc9ee9f72efc2012abce3e4e50186552e194.1676532146.git.jk@codeconstruct.com.au (mailing list archive)
State Changes Requested
Headers show
Series i3c: Add support for ast2600 i3c controller | expand

Commit Message

Jeremy Kerr Feb. 16, 2023, 7:41 a.m. UTC
The dw i3c core can be integrated into various SoC devices. Platforms
that use this core may need a little configuration that is specific to
that platform.

Add a little infrastructure to allow platform-specific behaviour: a bit
of data on struct dw_i3c_master, and two hooks to the probe and init
calls to enable this.

A future change will add new platform support that uses these hooks.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/i3c/master/dw-i3c-master.c | 42 +++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 6 deletions(-)

Comments

Ben Dooks Feb. 16, 2023, 3:04 p.m. UTC | #1
On 16/02/2023 07:41, Jeremy Kerr wrote:
> The dw i3c core can be integrated into various SoC devices. Platforms
> that use this core may need a little configuration that is specific to
> that platform.
> 
> Add a little infrastructure to allow platform-specific behaviour: a bit
> of data on struct dw_i3c_master, and two hooks to the probe and init
> calls to enable this.
> 
> A future change will add new platform support that uses these hooks.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
>   drivers/i3c/master/dw-i3c-master.c | 42 +++++++++++++++++++++++++-----
>   1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index d73d57362b3b..49b891449222 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -241,6 +241,17 @@ struct dw_i3c_master {
>   	char version[5];
>   	char type[5];
>   	u8 addrs[MAX_DEVS];
> +
> +	/* platform-specific data */
> +	const struct dw_i3c_platform_ops *platform_ops;
> +	union {
> +	} pdata;
> +
> +};
> +
> +struct dw_i3c_platform_ops {
> +	int (*probe)(struct dw_i3c_master *i3c, struct platform_device *pdev);
> +	int (*init)(struct dw_i3c_master *i3c);
>   };

Given the comment below having this and the main probe defined in a 
header so users can just call in and we don't have to change the
main code here every time someone comes up with their own
special way of handing this?

>   struct dw_i3c_i2c_dev_data {
> @@ -612,6 +623,12 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
>   	u32 thld_ctrl;
>   	int ret;
>   
> +	if (master->platform_ops && master->platform_ops->init) {
> +		ret = master->platform_ops->init(master);
> +		if (ret)
> +			return ret;
> +	}

I'd rather have a "default" set of ops than have all this checking for
NULL pointers all over the place.

> +
>   	switch (bus->mode) {
>   	case I3C_BUS_MODE_MIXED_FAST:
>   	case I3C_BUS_MODE_MIXED_LIMITED:
> @@ -1128,8 +1145,15 @@ static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
>   	.i2c_xfers = dw_i3c_master_i2c_xfers,
>   };
>   
> +static const struct of_device_id dw_i3c_master_of_match[] = {
> +	{ .compatible = "snps,dw-i3c-master-1.00a", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
> +
>   static int dw_i3c_probe(struct platform_device *pdev)
>   {
> +	const struct of_device_id *match;
>   	struct dw_i3c_master *master;
>   	int ret, irq;
>   
> @@ -1181,6 +1205,18 @@ static int dw_i3c_probe(struct platform_device *pdev)
>   	master->maxdevs = ret >> 16;
>   	master->free_pos = GENMASK(master->maxdevs - 1, 0);
>   
> +	/* match any platform-specific ops */
> +	match = of_match_node(dw_i3c_master_of_match, pdev->dev.of_node);
> +	if (match && match->data)
> +		master->platform_ops = match->data;

I'm sure there's a of_device_get_match_data() which would have
both removed hte need to move the match table around and the
call to of_match_node().

> +
> +	/* platform-specific probe */
> +	if (master->platform_ops && master->platform_ops->probe) {
> +		ret = master->platform_ops->probe(master, pdev);
> +		if (ret)
> +			goto err_assert_rst;
> +	}
> +
>   	ret = i3c_master_register(&master->base, &pdev->dev,
>   				  &dw_mipi_i3c_ops, false);
>   	if (ret)
> @@ -1213,12 +1249,6 @@ static int dw_i3c_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> -static const struct of_device_id dw_i3c_master_of_match[] = {
> -	{ .compatible = "snps,dw-i3c-master-1.00a", },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
> -
>   static struct platform_driver dw_i3c_driver = {
>   	.probe = dw_i3c_probe,
>   	.remove = dw_i3c_remove,
Jeremy Kerr Feb. 17, 2023, 9:43 a.m. UTC | #2
Hi Ben,

Thanks for taking a look at the patch. My responses inline (just
re-ordered, simple stuff first)

> >   struct dw_i3c_i2c_dev_data {
> > @@ -612,6 +623,12 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
> >         u32 thld_ctrl;
> >         int ret;
> >   
> > +       if (master->platform_ops && master->platform_ops->init) {
> > +               ret = master->platform_ops->init(master);
> > +               if (ret)
> > +                       return ret;
> > +       }
> 
> I'd rather have a "default" set of ops than have all this checking for
> NULL pointers all over the place.

Yep, that's a better structure, changed for v2.

> > @@ -1181,6 +1205,18 @@ static int dw_i3c_probe(struct platform_device *pdev)
> >         master->maxdevs = ret >> 16;
> >         master->free_pos = GENMASK(master->maxdevs - 1, 0);
> >   
> > +       /* match any platform-specific ops */
> > +       match = of_match_node(dw_i3c_master_of_match, pdev->dev.of_node);
> > +       if (match && match->data)
> > +               master->platform_ops = match->data;
> 
> I'm sure there's a of_device_get_match_data() which would have
> both removed hte need to move the match table around and the
> call to of_match_node().

That's the one I was looking for! Thanks for the pointer, I have updated
in v2.

> > @@ -241,6 +241,17 @@ struct dw_i3c_master {
> >         char version[5];
> >         char type[5];
> >         u8 addrs[MAX_DEVS];
> > +
> > +       /* platform-specific data */
> > +       const struct dw_i3c_platform_ops *platform_ops;
> > +       union {
> > +       } pdata;
> > +
> > +};
> > +
> > +struct dw_i3c_platform_ops {
> > +       int (*probe)(struct dw_i3c_master *i3c, struct platform_device *pdev);
> > +       int (*init)(struct dw_i3c_master *i3c);
> >   };
> 
> Given the comment below having this and the main probe defined in a 
> header so users can just call in and we don't have to change the
> main code here every time someone comes up with their own
> special way of handing this?

I'm not sure I 100% understand the intention here - is it that we'd
split the platform-specific code into entirely new drivers, and have
those call into dw_i3c_probe() (presumably doing a bit of custom init
either before or after that call)?

If so: I think the platform support should stay fairly minimal, so I'm
not sure that warrants a new driver for each instance. In the ast2600
case it's just a couple of extra reg writes in the i3c init path. I'd be
reluctant to split that out completely at this stage - but if this does
grow, we can certainly reconsider.

Also, I'd like to allow for the case where the platform-specific parts
may access the fields of struct dw_i3c_master; with this approach we
don't need to expose that struct outside of the single driver.

Cheers,


Jeremy
diff mbox series

Patch

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index d73d57362b3b..49b891449222 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -241,6 +241,17 @@  struct dw_i3c_master {
 	char version[5];
 	char type[5];
 	u8 addrs[MAX_DEVS];
+
+	/* platform-specific data */
+	const struct dw_i3c_platform_ops *platform_ops;
+	union {
+	} pdata;
+
+};
+
+struct dw_i3c_platform_ops {
+	int (*probe)(struct dw_i3c_master *i3c, struct platform_device *pdev);
+	int (*init)(struct dw_i3c_master *i3c);
 };
 
 struct dw_i3c_i2c_dev_data {
@@ -612,6 +623,12 @@  static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
 	u32 thld_ctrl;
 	int ret;
 
+	if (master->platform_ops && master->platform_ops->init) {
+		ret = master->platform_ops->init(master);
+		if (ret)
+			return ret;
+	}
+
 	switch (bus->mode) {
 	case I3C_BUS_MODE_MIXED_FAST:
 	case I3C_BUS_MODE_MIXED_LIMITED:
@@ -1128,8 +1145,15 @@  static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
 	.i2c_xfers = dw_i3c_master_i2c_xfers,
 };
 
+static const struct of_device_id dw_i3c_master_of_match[] = {
+	{ .compatible = "snps,dw-i3c-master-1.00a", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
+
 static int dw_i3c_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct dw_i3c_master *master;
 	int ret, irq;
 
@@ -1181,6 +1205,18 @@  static int dw_i3c_probe(struct platform_device *pdev)
 	master->maxdevs = ret >> 16;
 	master->free_pos = GENMASK(master->maxdevs - 1, 0);
 
+	/* match any platform-specific ops */
+	match = of_match_node(dw_i3c_master_of_match, pdev->dev.of_node);
+	if (match && match->data)
+		master->platform_ops = match->data;
+
+	/* platform-specific probe */
+	if (master->platform_ops && master->platform_ops->probe) {
+		ret = master->platform_ops->probe(master, pdev);
+		if (ret)
+			goto err_assert_rst;
+	}
+
 	ret = i3c_master_register(&master->base, &pdev->dev,
 				  &dw_mipi_i3c_ops, false);
 	if (ret)
@@ -1213,12 +1249,6 @@  static int dw_i3c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id dw_i3c_master_of_match[] = {
-	{ .compatible = "snps,dw-i3c-master-1.00a", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
-
 static struct platform_driver dw_i3c_driver = {
 	.probe = dw_i3c_probe,
 	.remove = dw_i3c_remove,