| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230818065624.GM986605@google.com>
Date: Fri, 18 Aug 2023 07:56:24 +0100
From: Lee Jones <lee@...nel.org>
To: Andre Werner <andre.werner@...tec-electronic.com>
Cc: werneazc@...il.com, lgirdwood@...il.com, broonie@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mfd: (tps65086): Read DEVICE ID register 1 from
device
On Fri, 18 Aug 2023, Andre Werner wrote:
> On Thu, 17 Aug 2023, Lee Jones wrote:
>
> > On Wed, 09 Aug 2023, werneazc@...il.com wrote:
> >
> > > From: Andre Werner <andre.werner@...tec-electronic.com>
> > >
> > > This commit prepares a following commit for the regulator part of the MFD.
> > > The driver should support different device chips that differ in their
> > > register definitions, for instance to control LDOA1 and SWB2.
> > > So it is necessary to use a dedicated regulator description for a
> > > specific device variant. Thus, the content from DEVICEID Register 1 is
> > > used to choose a dedicated configuration between the different device
> > > variants.
> > >
> > > Signed-off-by: Andre Werner <andre.werner@...tec-electronic.com>
> > > ---
> > > drivers/mfd/tps65086.c | 37 ++++++++++++++++++++++++++++++------
> > > include/linux/mfd/tps65086.h | 27 ++++++++++++++++++++------
> > > 2 files changed, 52 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
> > > index 6a21000aad4a..38f8572c265e 100644
> > > --- a/drivers/mfd/tps65086.c
> > > +++ b/drivers/mfd/tps65086.c
> > > @@ -64,7 +64,7 @@ MODULE_DEVICE_TABLE(of, tps65086_of_match_table);
> > > static int tps65086_probe(struct i2c_client *client)
> > > {
> > > struct tps65086 *tps;
> > > - unsigned int version;
> > > + unsigned int version, id;
> > > int ret;
> > >
> > > tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> > > @@ -81,16 +81,41 @@ static int tps65086_probe(struct i2c_client *client)
> > > return PTR_ERR(tps->regmap);
> > > }
> > >
> > > - ret = regmap_read(tps->regmap, TPS65086_DEVICEID, &version);
> > > + ret = regmap_read(tps->regmap, TPS65086_DEVICEID1, &id);
> > > if (ret) {
> > > - dev_err(tps->dev, "Failed to read revision register\n");
> > > + dev_err(tps->dev, "Failed to read revision register 1\n");
> > > + return ret;
> > > + }
> > > +
> > > + /* Store device ID to load regulator configuration that fit to IC variant */
> > > + switch (id) {
> > > + case TPS6508640_ID:
> > > + tps->chip_id = TPS6508640;
> >
> > Why not use the meaningful TPS6508640_ID for the chip_id instead of an
> > arbitrary enum?
>
> In the regulator part for this MFD I use this enum ID to select the
> right configuration from an array. So the intention is using the enum as
> the index for this table. I can move this selection into the regulator
> part and store the meaningful TPS65086 IDs in the MFD data if you
> prefer?
That would make more sense for the reader I feel. Thanks.
> > > + break;
> > > + case TPS65086401_ID:
> > > + tps->chip_id = TPS65086401;
> > > + break;
> > > + case TPS6508641_ID:
> > > + tps->chip_id = TPS6508641;
> > > + break;
> > > + case TPS65086470_ID:
> > > + tps->chip_id = TPS65086470;
> > > + break;
> > > + default:
> > > + dev_err(tps->dev, "Unknown device ID. Cannot determine regulator config.\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + ret = regmap_read(tps->regmap, TPS65086_DEVICEID2, &version);
> > > + if (ret) {
> > > + dev_err(tps->dev, "Failed to read revision register 2\n");
> > > return ret;
> > > }
> > >
> > > dev_info(tps->dev, "Device: TPS65086%01lX, OTP: %c, Rev: %ld\n",
> > > - (version & TPS65086_DEVICEID_PART_MASK),
> > > - (char)((version & TPS65086_DEVICEID_OTP_MASK) >> 4) + 'A',
> > > - (version & TPS65086_DEVICEID_REV_MASK) >> 6);
> > > + (version & TPS65086_DEVICEID2_PART_MASK),
> > > + (char)((version & TPS65086_DEVICEID2_OTP_MASK) >> 4) + 'A',
> > > + (version & TPS65086_DEVICEID2_REV_MASK) >> 6);
> > >
> > > if (tps->irq > 0) {
> > > ret = regmap_add_irq_chip(tps->regmap, tps->irq, IRQF_ONESHOT, 0,
> > > diff --git a/include/linux/mfd/tps65086.h b/include/linux/mfd/tps65086.h
> > > index 16f87cccc003..88df344b38df 100644
> > > --- a/include/linux/mfd/tps65086.h
> > > +++ b/include/linux/mfd/tps65086.h
> > > @@ -13,8 +13,9 @@
> > > #include <linux/regmap.h>
> > >
> > > /* List of registers for TPS65086 */
> > > -#define TPS65086_DEVICEID 0x01
> > > -#define TPS65086_IRQ 0x02
> > > +#define TPS65086_DEVICEID1 0x00
> > > +#define TPS65086_DEVICEID2 0x01
> > > +#define TPS65086_IRQ 0x02
> > > #define TPS65086_IRQ_MASK 0x03
> > > #define TPS65086_PMICSTAT 0x04
> > > #define TPS65086_SHUTDNSRC 0x05
> > > @@ -75,16 +76,29 @@
> > > #define TPS65086_IRQ_SHUTDN_MASK BIT(3)
> > > #define TPS65086_IRQ_FAULT_MASK BIT(7)
> > >
> > > -/* DEVICEID Register field definitions */
> > > -#define TPS65086_DEVICEID_PART_MASK GENMASK(3, 0)
> > > -#define TPS65086_DEVICEID_OTP_MASK GENMASK(5, 4)
> > > -#define TPS65086_DEVICEID_REV_MASK GENMASK(7, 6)
> > > +/* DEVICEID1 Register field definitions */
> > > +#define TPS6508640_ID 0x00
> > > +#define TPS65086401_ID 0x01
> > > +#define TPS6508641_ID 0x10
> > > +#define TPS65086470_ID 0x70
> > > +
> > > +/* DEVICEID2 Register field definitions */
> > > +#define TPS65086_DEVICEID2_PART_MASK GENMASK(3, 0)
> > > +#define TPS65086_DEVICEID2_OTP_MASK GENMASK(5, 4)
> > > +#define TPS65086_DEVICEID2_REV_MASK GENMASK(7, 6)
> > >
> > > /* VID Masks */
> > > #define BUCK_VID_MASK GENMASK(7, 1)
> > > #define VDOA1_VID_MASK GENMASK(4, 1)
> > > #define VDOA23_VID_MASK GENMASK(3, 0)
> > >
> > > +enum tps65086_ids {
> > > + TPS6508640,
> > > + TPS65086401,
> > > + TPS6508641,
> > > + TPS65086470,
> > > +};
> > > +
> > > /* Define the TPS65086 IRQ numbers */
> > > enum tps65086_irqs {
> > > TPS65086_IRQ_DIETEMP,
> > > @@ -100,6 +114,7 @@ enum tps65086_irqs {
> > > struct tps65086 {
> > > struct device *dev;
> > > struct regmap *regmap;
> > > + unsigned int chip_id;
> > >
> > > /* IRQ Data */
> > > int irq;
> > > --
> > > 2.41.0
> > >
> >
> > --
> > Lee Jones [李琼斯]
> >
>
> Regards,
>
> André
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists