Re: [RFD] voltage/current regulator consumer interface

[Mark Brown <broonie@sirena.org.uk>]

On Tue, Apr 21, 2009 at 03:00:41PM +0300, Mike Rapoport wrote:

> I've managed to create some preliminary "line-consumer" driver. I don't really
> like the name, but I couldn't think of something better and "virtual" is
> already taken :)

"userspace" or "user"?  The goal here is to let userspace applications
be consumers.

> +static ssize_t set_state(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	struct line_consumer_data *data = dev_get_drvdata(dev);
> +	bool enabled;
> +	int ret;
> +
> +	/*
> +	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
> +	 * will be shared with show_state(), above.
> +	 */
> +	if (sysfs_streq(buf, "enabled\n") == 0)
> +		enabled = true;
> +	else if (sysfs_streq(buf, "disabled\n") == 0)
> +		enabled = false;

I'd be inclined to also accept 1 and 0 here.

> +static DEVICE_ATTR(name, 0444, show_name, NULL);
> +static DEVICE_ATTR(state, 0666, show_state, set_state);

Permissions for set_state() should probably be tighter?

> +	for (i = 0; i < ARRAY_SIZE(attributes); i++) {
> +		ret = device_create_file(&pdev->dev, attributes[i]);
> +		if (ret != 0)
> +			goto err_create_attrs;
> +	}

device_add_attributes()?

> +	if (pdata->init_on)
> +		ret = regulator_bulk_enable(drvdata->num_supplies,
> +					    drvdata->supplies);
> +	else
> +		ret = regulator_bulk_disable(drvdata->num_supplies,
> +					     drvdata->supplies);

The disable case will lead to unbalanced enables and disables, loosing
a reference to the supply.  Just don't do anything if the supplies
should not be enabled.

> +static int regulator_line_consumer_remove(struct platform_device *pdev)
> +{
> +	struct line_consumer_data *drvdata = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(attributes); i++)
> +		device_remove_file(&pdev->dev, attributes[i]);
> +
> +	regulator_bulk_free(drvdata->num_supplies, drvdata->supplies);
> +	kfree(drvdata->supplies);
> +	kfree(drvdata);
> +
> +	return 0;
> +}

Hrm.  You probably want to disable the supplies if you weren't using
them here.