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:
signature.asc
Description: Digital signature
