Re: [PATCH 2/2] hwmon: (pmbus/lx1308) Add support for LX1308

[Brian Chiang]

> > +
> > +static struct pmbus_driver_info lx1308_info = {
> > +	.pages = 1,
> > +	.format[PSC_VOLTAGE_IN] = linear,
> > +	.format[PSC_VOLTAGE_OUT] = linear,
> > +	.format[PSC_CURRENT_IN] = linear,
> > +	.format[PSC_CURRENT_OUT] = linear,
> > +	.format[PSC_POWER] = linear,
> > +	.format[PSC_TEMPERATURE] = linear,
> > +
> > +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> > +		| PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
> > +		| PMBUS_HAVE_PIN | PMBUS_HAVE_POUT
> > +		| PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
> > +		| PMBUS_HAVE_STATUS_INPUT,
> > +
> > +	.read_word_data  = lx1308_read_word_data,
> > +	.write_word_data = lx1308_write_word_data,
> > +};
> > +
> > +static const struct i2c_device_id lx1308_id[] = {
> > +	{ "lx1308" },
> > +	{ }
> > +};
>
> All ID tables should be next to each other, usually just before the
> struct with driver.

Will reorganize in v2 - i2c_device_id and of_device_id tables grouped
together immediately before struct i2c_driver.

> > +
> > +MODULE_DEVICE_TABLE(i2c, lx1308_id);
> > +
>
> ...
>
> > +	if (ret != 12 || buf[0] != 'V')
> > +		return dev_err_probe(&client->dev, -ENODEV,
> > +				     "Unsupported Manufacturer Revision '%s'\n", buf);
> > +	return pmbus_do_probe(client, &lx1308_info);
> > +}
> > +
> > +static const struct of_device_id lx1308_of_match[] = {
> > +	{ .compatible = "luxshare,lx1308" },
>
> Where is the rest of your compatibles? Do not document ABI which is
> unused. submitting-patches in DT forbids that.

Addressed in the patch 1/2 reply: the variant compatibles encode only
mechanical differences (pin length, pin-14 assignment, output cap) that
are invisible to software. In v2 I will drop them from the binding and
move to trivial-devices.yaml with a single "luxshare,lx1308" compatible.

> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, lx1308_of_match);
>
> Best regards,
> Krzysztof

Best regards,
Brian