by Nishanth Menon

[permalink] [raw]

Subject: Re: [PATCHv1 5/6] HSI: Introduce OMAP SSI driver

On 02/24/2014 09:51 AM, Mark Rutland wrote:
> On Sun, Feb 23, 2014 at 11:50:00PM +0000, Sebastian Reichel wrote:

[...]
>> +static int omap_ssi_port_runtime_suspend(struct device *dev)
>> +{
>> + struct hsi_port *port = dev_get_drvdata(dev);
>> + struct omap_ssi_port *omap_port = hsi_port_drvdata(port);
>> + struct hsi_controller *ssi = to_hsi_controller(port->device.parent);
>> + struct omap_ssi_controller *omap_ssi = hsi_controller_drvdata(ssi);
>> +
>> + dev_dbg(dev, "port runtime suspend!\n");
>> +
>> + ssi_set_port_mode(omap_port, SSI_MODE_SLEEP);
>> + if (omap_ssi->get_loss)
>> + omap_port->loss_count =
>> + (*omap_ssi->get_loss)(ssi->device.parent);
>
> You don't need to do (*struct->func)(args) when invoking a function
> pointer. You can jsut have struct->func(args) as we do elsewhere. This
> can be:
>
> omap_ssi->get_loss(ssi->device.parent)
>
> This should be fixed up in the other sites too.

in fact, we should stop expecting that service and drivers should
manage their own context_loss detection
Examples:
MMC:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0a82e06e6183a252608df48cc4793b83e2d73dfd

DSS: http://marc.info/?l=linux-omap&m=139272358619064&w=2

--
Regards,
Nishanth Menon

2014-02-24 19:43:24

Hi Mark,

On Mon, Feb 24, 2014 at 03:51:32PM +0000, Mark Rutland wrote:
> > + irq = platform_get_resource_byname(pd, IORESOURCE_IRQ, "gdd_mpu");
> > + if (!irq) {
> > + dev_err(&pd->dev, "GDD IRQ resource missing\n");
> > + err = -ENXIO;
> > + goto out_err;
> > + }
> > + omap_ssi->gdd_irq = irq->start;
>
> You can use platform_get_irq_byname here.

Right. Will be changed in PATCHv2.

> > +static inline int ssi_of_get_available_child_count(const struct device_node *np)
> > +{
> > + struct device_node *child;
> > + int num = 0;
> > +
> > + for_each_child_of_node(np, child)
> > + if (of_device_is_available(child))
> > + num++;
> > +
> > + return num;
> > +}
>
> You can find of_get_available_child_count in <linux/of.h>.

That did not exist when I started with the DT conversion of the
driver.

> That said, this seems to be trying to count the number of ports,
> which should all be compatible with "ti,omap3-ssi-port", no?
>
> So maybe you should count all available child nodes compatible with
> that.

I updated the function to check the compatible string
and use for_each_available_child_of_node(), which has
also been added after I wrote this function.

> > +static int __init ssi_probe(struct platform_device *pd)
> > +{
> > + struct device_node *np = pd->dev.of_node;
> > + struct hsi_controller *ssi;
> > + int err;
> > + int num_ports;
> > +
> > + if (!np) {
> > + dev_err(&pd->dev, "missing device tree data\n");
> > + return -EINVAL;
> > + }
> > +
> > + num_ports = ssi_of_get_available_child_count(np);
> > +
> > + ssi = hsi_alloc_controller(num_ports, GFP_KERNEL);
> > + if (!ssi) {
> > + dev_err(&pd->dev, "No memory for controller\n");
> > + return -ENOMEM;
> > + }
> > +
> > + platform_set_drvdata(pd, ssi);
> > +
> > + err = ssi_add_controller(ssi, pd);
> > + if (err < 0)
> > + goto out1;
> > +
> > + pm_runtime_irq_safe(&pd->dev);
> > + pm_runtime_enable(&pd->dev);
> > +
> > + err = ssi_hw_init(ssi);
> > + if (err < 0)
> > + goto out2;
> > +#ifdef CONFIG_DEBUG_FS
> > + err = ssi_debug_add_ctrl(ssi);
> > + if (err < 0)
> > + goto out2;
> > +#endif
> > +
> > + err = of_platform_populate(pd->dev.of_node, NULL, NULL, &pd->dev);
>
> I'm not keen on doing this because it allows arbitrary devices which are
> not ssi ports to be placed in the ssi host controller node that will be
> probed, which is nonsensical and something I'd like to avoid by
> construction.
>
> Is there any reason the ports have to be platform devices at all?

not strictly, but I get system resources via platform_get_resource_byname.

> If so, is there no way we can register them directly and skip any other
> devices?

I set the second parameter (matches) of the of_platform_populate()
call to a table containing the ssi-port compatible value.

> > +static int __exit ssi_remove(struct platform_device *pd)
> > +{
> > + struct hsi_controller *ssi = platform_get_drvdata(pd);
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > + ssi_debug_remove_ctrl(ssi);
> > +#endif
> > + ssi_remove_controller(ssi);
> > + platform_set_drvdata(pd, NULL);
> > +
> > + pm_runtime_disable(&pd->dev);
> > +
> > + /* cleanup of of_platform_populate() call */
> > + device_for_each_child(&pd->dev, NULL, ssi_remove_ports);
>
> This would certainly be broken for a non ssi port device.

I never intended to support other subdevices, but this only
unregisters each subdevice, so its probably safe...

> > +static int omap_ssi_port_runtime_suspend(struct device *dev)
> > +{
> > + struct hsi_port *port = dev_get_drvdata(dev);
> > + struct omap_ssi_port *omap_port = hsi_port_drvdata(port);
> > + struct hsi_controller *ssi = to_hsi_controller(port->device.parent);
> > + struct omap_ssi_controller *omap_ssi = hsi_controller_drvdata(ssi);
> > +
> > + dev_dbg(dev, "port runtime suspend!\n");
> > +
> > + ssi_set_port_mode(omap_port, SSI_MODE_SLEEP);
> > + if (omap_ssi->get_loss)
> > + omap_port->loss_count =
> > + (*omap_ssi->get_loss)(ssi->device.parent);
>
> You don't need to do (*struct->func)(args) when invoking a function
> pointer. You can jsut have struct->func(args) as we do elsewhere. This
> can be:
>
> omap_ssi->get_loss(ssi->device.parent)
>
> This should be fixed up in the other sites too.

Thanks, fixed everywhere.

-- Sebastian

Attachments:

(No filename) (4.59 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-02-27 00:24:28

by Sebastian Reichel

[permalink] [raw]

Subject: Re: [PATCHv1 3/6] HSI: hsi-char: add Device Tree support

On Mon, Feb 24, 2014 at 03:13:01PM +0000, Mark Rutland wrote:
> On Sun, Feb 23, 2014 at 11:49:58PM +0000, Sebastian Reichel wrote:
> > Add of_match_table to hsi_char driver, so that it can
> > be referenced from Device Tree.
> >
> > Signed-off-by: Sebastian Reichel <[email protected]>
> > ---
> > drivers/hsi/clients/hsi_char.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/hsi/clients/hsi_char.c b/drivers/hsi/clients/hsi_char.c
> > index e61e5f9..7f64bed 100644
> > --- a/drivers/hsi/clients/hsi_char.c
> > +++ b/drivers/hsi/clients/hsi_char.c
> > @@ -42,6 +42,7 @@
> > #include <linux/stat.h>
> > #include <linux/hsi/hsi.h>
> > #include <linux/hsi/hsi_char.h>
> > +#include <linux/of_device.h>
> >
> > #define HSC_DEVS 16 /* Num of channels */
> > #define HSC_MSGS 4
> > @@ -758,12 +759,22 @@ static int hsc_remove(struct device *dev)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id hsi_char_of_match[] = {
> > + { .compatible = "ssi-char", },
>
> This string is undocumented.
>
> > + { .compatible = "hsi-char", },
>
> I'm not sure either string makes sense though; this feels like a binding
> for the sake of the driver rather than describing the device and
> allowing the driver to pick it up if it makes sense to do so.
>
> What exactly is a "ssi-char" device or a "hsi-char" device?

mh. I guess I will update the hsi framework, so that it simply loads
the client driver for each registered port. This is indeed only a
binding to load the driver.

-- Sebastian

Attachments:

(No filename) (1.53 kB)
signature.asc (819.00 B)
Digital signature Download all attachments