Re: [RFC PATCH] slimbus: Linux driver framework for SLIMbus.

[Mark Brown]

On Wed, Aug 10, 2011 at 05:31:28PM -0600, Kenneth Heitke wrote:

> SLIMbus (Serial Low Power Interchip Media Bus) is a specification
> developed by MIPI (Mobile Industry Processor Interface) alliance.

Please CC me on any future spins of this patch.

> Signed-off-by: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>

You ought to sign off the patch as well as you're part of the chain for
it getting into the kernel.

> +#ifdef CONFIG_PM_SLEEP
> +static int slim_legacy_suspend(struct device *dev, pm_message_t mesg)
> +{
> +	struct slim_device *slim_dev = NULL;

Why have legacy stuff in a newly implemented subsystem?

> +static int slim_drv_probe(struct device *dev)
> +{
> +	const struct slim_driver *sdrv = to_slim_driver(dev->driver);
> +
> +	if (sdrv->probe)
> +		return sdrv->probe(to_slim_device(dev));
> +	return -ENODEV;
> +}

Why all the -ENODEVs if there's no function?  I'd expect that if there's
nothing to do it should be possible to omit functions from drivers.

> +int slim_driver_register(struct slim_driver *drv)
> +{
> +	drv->driver.bus = &slimbus_type;
> +	if (drv->probe)
> +		drv->driver.probe = slim_drv_probe;

Modifying the driver structure seems icky, why is this needed?  Other
bus types appear to manage without.

> +static u16 slim_slicecodefromsize(u32 req)
> +{
> +	u8 codetosize[8] = {1, 2, 3, 4, 6, 8, 12, 16};
> +	if (req >= 8)
> +		return 0;

ARRAY_SIZE()?  I guess the table should be static too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/