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

[Sebastian Reichel <sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4841 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]