Re: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support

[Mark Brown]

On Wed, Sep 21, 2016 at 01:44:39PM +0900, Eric Jeong wrote:

> +#ifdef CONFIG_OF
> +static const struct of_device_id pv88080_dt_ids[] = {
> +	{ .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> +	{ .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pv88080_dt_ids);
> +#endif

This doesn't list the old compatible string and therefore ends up
breaking since the code defaults to assuming something it hasn't heard
of is BB when it seems more likely that old DTs using the old compatible
string will be for boards that also use the old silicon.  It would be
much better practice to explicitly list the old compatible string and
how we handle it, that won't break in future and avoids this kind of
error.

Is it really not possible to read the chip revision from the device
using ID registers?  That'd be even better.

> +	if (i2c->dev.of_node) {
> +		match = of_match_node(pv88080_dt_ids, i2c->dev.of_node);
> +		if (!match) {
> +			dev_err(chip->dev, "Failed to get of_match_node\n");
> +			return -EINVAL;
> +		}
> +		chip->type = (unsigned long)match->data;

This will generate the warning for all existing DTs as a result of the
above which isn't great.

> +	if (chip->type == TYPE_PV88080_AA)
> +		chip->regmap_config = &pv88080_aa_regs;
> +	else
> +		chip->regmap_config = &pv88080_ba_regs;

Use a switch statement here, it is more extensible if we get future chip
versions and will improve error handling since we won't just silently
treat unrecognized values as BB.

>  static const struct i2c_device_id pv88080_i2c_id[] = {
> -	{"pv88080", 0},
> +	{ "pv88080-aa", TYPE_PV88080_AA },
> +	{ "pv88080-ba", TYPE_PV88080_BA },

Same thing as with the compatible string here.

Attachment: signature.asc
Description: PGP signature