Re: [PATCH 2/2] hwmon: (pmbus/adp1050): add support for adp1051, adp1055 and ltp8800

[Guenter Roeck]

On 11/20/24 05:52, Andy Shevchenko wrote:
> On Wed, Nov 20, 2024 at 11:58:26AM +0800, Cedric Encarnacion wrote:
>
> I would start the commit message with the plain English sentence that describes
> the list given below. E.g., "Introduce support for the following components:".
>
> >      ADP1051: 6 PWM for I/O Voltage, I/O Current, Temperature
> >      ADP1055: 6 PWM for I/O Voltage, I/O Current, Power, Temperature
> >      LTP8800-1A/-2/-4A: 150A/135A/200A DC/DC µModule Regulator
> >
> > The LTP8800 is a family of step-down μModule regulators that provides
> > microprocessor core voltage from 54V power distribution architecture.
> > LTP8800 features telemetry monitoring of input/output voltage, input
> > current, output power, and temperature over PMBus.
>
> ...
>
> >     - Radu Sabau <radu.sabau@xxxxxxxxxx>
> >   
> > -
> >   Description
> >   -----------
>
> Stray change.
>
> ...
>
> > -This driver supprts hardware monitoring for Analog Devices ADP1050 Digital
> > -Controller for Isolated Power Supply with PMBus interface.
> > +This driver supports hardware monitoring for Analog Devices ADP1050, ADP1051, and
> > +ADP1055 Digital Controller for Isolated Power Supply with PMBus interface.
> >   
> > -The ADP1050 is an advanced digital controller with a PMBus™
> > +The ADP105X is an advanced digital controller with a PMBus™
>
> Can we use small x to make it more visible that it's _not_ the part of the
> name, but a glob-like placeholder?

As mentioned in my other reply, I'd rather not have a placeholder in the first
place.

> >   interface targeting high density, high efficiency dc-to-dc power
> >   conversion used to monitor system temperatures, voltages and currents.
>
> ...
>
> > +#if IS_ENABLED(CONFIG_SENSORS_ADP1050_REGULATOR)
>
> Why? Is the data type undefined without this?

Look into other drivers. That is how it is implemented there,
and not really the point. One has to know about an alternative to use it.

> > +static const struct regulator_desc adp1050_reg_desc[] = {
> > +	PMBUS_REGULATOR_ONE("vout"),
> > +};
> > +#endif /* CONFIG_SENSORS_ADP1050_REGULATOR */
>
> Note, this can be dropped anyway in order to use PTR_IF() below, if required.

FWIW, PTR_IF() isn't widely used, and I for my part was not aware that
it exists.

> ...
>
> > +#if IS_ENABLED(CONFIG_SENSORS_ADP1050_REGULATOR)
> > +	.num_regulators = 1,
> > +	.reg_desc = adp1050_reg_desc,
> > +#endif
>
> Ditto, are the fields not defined without the symbol?

They are, but they must be 0/NULL. PTR_IF() would be an alternative.
It is a bit odd to use it for a non-pointer, but it is type-agnostic,
so using it should be ok to avoid the #ifdefs. We should maybe adopt
that mechanism for other PMBus drivers.

> ...
>
> >   static int adp1050_probe(struct i2c_client *client)
> >   {
> > -	return pmbus_do_probe(client, &adp1050_info);
> > +	const struct pmbus_driver_info *info;
> > +
> > +	info = device_get_match_data(&client->dev);
>
> Why not i2c_get_match_data()?
>
> > +	if (!info)
> > +		return -ENODEV;
> > +
> > +	return pmbus_do_probe(client, info);
> >   }
>
> ...
>
> >   static const struct i2c_device_id adp1050_id[] = {
> > -	{"adp1050"},
> > +	{ .name = "adp1050", .driver_data = (kernel_ulong_t)&adp1050_info},
>
> Please, split this patch to at least two:
> 1) Introduce chip_info;

That would really be "Use driver data to point to chip info".

> 2) add new devices.

I don't really care much about separating those two (after all, they are
related), but adding regulator support to the driver is a major change
and should be a separate patch. On top of that, it isn't even mentioned
in the patch description.

Thanks,
Guenter