On Wed, Dec 25, 2024 at 05:54:12PM +0800, ChiangBrian 江泳緻 TAO wrote:
> From: Brian Chiang<chiang.brian@xxxxxxxxxxxx>
>
> As the driver is not supported, TPS53685 reading is added based on the
> datasheet.
>
Neither subject nor description are appropriate. This patch
doesn't add support for a driver, it adds support for a chip
to an existing driver.
> Signed-off-by: Brian Chiang<chiang.brian@xxxxxxxxxxxx>
> ---
> drivers/hwmon/pmbus/tps53679.c | 58 ++++++++++++++++++++++++++++++++--
> 1 file changed, 56 insertions(+), 2 deletions(-)
>
> * Since those chips have special configuration registers, we want to have
> @@ -132,12 +157,33 @@ static int tps53679_identify_multiphase(struct
> i2c_client *client,
> return tps53679_identify_phases(client, info);
> }
>
> +static int tps53685_identify_multiphase(struct i2c_client *client,
> + struct pmbus_driver_info *info,
> + int pmbus_rev)
> +{
> + int ret;
> + ret = tps53685_identify_chip(client, pmbus_rev);
> + if (ret < 0)
> + return ret;
> +
> + info->format[PSC_VOLTAGE_OUT] = linear;
> +
> + return 0;
> +}
> +
> static int tps53679_identify(struct i2c_client *client,
> struct pmbus_driver_info *info)
> {
> return tps53679_identify_mode(client, info);
> }
>
> +static int tps53685_identify(struct i2c_client *client,
> + struct pmbus_driver_info *info)
> +{
> + return tps53685_identify_multiphase(client, info,
> + TPS53681_PMBUS_REVISION);
> +}
> +
The whole point of the existing identify functions was to use them for
multiple chips. Please ajust the code to only use the existing functions.
That should be possible by changing the 'id' parameter of
tps53679_identify_chip() and tps53679_identify_multiphase() to be a char *
instead of an u16, and by passing the expected return length.
> static int tps53681_identify(struct i2c_client *client,
> struct pmbus_driver_info *info)
> {
> @@ -215,7 +261,9 @@ static struct pmbus_driver_info tps53679_info = {
> PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
> PMBUS_HAVE_POUT,
> - .func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> + .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
> + PMBUS_HAVE_STATUS_INPUT |
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
> PMBUS_HAVE_POUT,
> @@ -263,6 +311,10 @@ static int tps53679_probe(struct i2c_client *client)
> info->identify = tps53681_identify;
> info->read_word_data = tps53681_read_word_data;
> break;
> + case tps53685:
> + info->pages = 2;
> + info->identify = tps53685_identify;
> + break;
> default:
> return -ENODEV;
> }
> @@ -278,6 +330,7 @@ static const struct i2c_device_id tps53679_id[] = {
> {"tps53679", tps53679},
> {"tps53681", tps53681},
> {"tps53688", tps53688},
> + {"tps53685", tps53685},
Numeric order, please.
> {}
> };
>
> @@ -290,6 +343,7 @@ static const struct of_device_id __maybe_unused
> tps53679_of_match[] = {
> {.compatible = "ti,tps53679", .data = (void *)tps53679},
> {.compatible = "ti,tps53681", .data = (void *)tps53681},
> {.compatible = "ti,tps53688", .data = (void *)tps53688},
> + {.compatible = "ti,tps53685", .data = (void *)tps53685},
Numeric order, please.
> {}
> };
> MODULE_DEVICE_TABLE(of, tps53679_of_match);
>
> diff --git a/drivers/hwmon/pmbus/tps53679.c b/drivers/hwmon/pmbus/tps53679.c
> index 81b9d813655a..89753f004edb 100644
> --- a/drivers/hwmon/pmbus/tps53679.c
> +++ b/drivers/hwmon/pmbus/tps53679.c
> @@ -16,7 +16,7 @@
> #include "pmbus.h"
>
> enum chips {
> - tps53647, tps53667, tps53676, tps53679, tps53681, tps53688
> + tps53647, tps53667, tps53676, tps53679, tps53681, tps53688, tps53685
Please retain numeric order.
> };
>
> #define TPS53647_PAGE_NUM 1
> @@ -109,6 +109,31 @@ static int tps53679_identify_chip(struct
> i2c_client *client,
> return 0;
> }
>
> +static int tps53685_identify_chip(struct i2c_client *client,
> + u8 revision)
> +{
> + u8 buf[I2C_SMBUS_BLOCK_MAX];
> + int ret;
> +
> + ret = pmbus_read_byte_data(client, 0, PMBUS_REVISION);
> + if (ret < 0)
> + return ret;
> + if (ret != revision) {
> + dev_err(&client->dev, "Unexpected PMBus revision 0x%x\n", ret);
> + return -ENODEV;
> + }
> +
> + ret = i2c_smbus_read_block_data(client, PMBUS_IC_DEVICE_ID, buf);
> + if (ret < 0)
> + return ret;
> +
> + if (strncmp("\x54\x49\x53\x68\x50\x00", buf, 6)) {
That is "TIShP" with a trailing '\0'. Please provide as text.
Also, the return length should be checked.
> + dev_err(&client->dev, "Unexpected device ID: %s\n", buf);
The length of 'buf' is 32 bytes, and 32 bytes may be returned by the chip,
leaving the text unterminated. On top of that, there is no guarantee that
the chip 0-terminates its returned data, or that it is in text form
in the first place. This may result in unexpected output, meaning you can
not use %s here. I would suggest to use "%*ph" or similar.
> + return -ENODEV;
> + }
> + return 0;
> +}
> +
> /*
> * Common identification function for chips with multi-phase support.
