On Fri, 11 Nov 2022, Naresh Solanki wrote: > On 07-11-2022 03:06 pm, Lee Jones wrote: > > On Thu, 03 Nov 2022, Naresh Solanki wrote: > > > > > From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx> > > > > > > Implement a regulator driver with IRQ support for fault management. > > > > This is not a "regulator driver". > > > > > Written against documentation [1] and [2] and tested on real hardware. > > > > > > Every channel has its own regulator supplies nammed 'vss1-supply' and > > > 'vss2-supply'. The regulator supply is used to determine the output > > > voltage, as the smart switch provides no output regulation. > > > The driver requires the 'shunt-resistor-micro-ohms' property to be > > > present in Device Tree to properly calculate current related > > > values. > > > > > > Datasheet links: > > > 1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf > > > 2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf > > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx> > > > Signed-off-by: Marcello Sylvester Bauer <sylv@xxxxxxx> > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx> [...] > > > +static const struct of_device_id max597x_of_match[] = { > > > + { .compatible = "maxim,max5970", .data = (void *)MAX597x_TYPE_MAX5970 }, > > > + { .compatible = "maxim,max5978", .data = (void *)MAX597x_TYPE_MAX5978 }, > > > > Where is .data used? > The .data isn't used. Then why add it? [...] > > > +#include <linux/device.h> > > > +#include <linux/regmap.h> > > > + > > > +/* Number of switch based on chip variant */ > > > > This comment is superfluous. > You mean this comment should be removed ? I do. > > > +#define MAX5970_NUM_SWITCHES 2 > > > +#define MAX5978_NUM_SWITCHES 1 > > > +/* Both chip variant have 4 indication LEDs used by LED cell */ > > > > Here too I think. > > > > > +#define MAX597X_NUM_LEDS 4 > > > + > > > +enum max597x_chip_type { > > > + MAX597x_TYPE_MAX5978 = 1, > > > > Why 1? > MAX5978 & single power switch wheres MAX5970 has two. That's not what this enum means. You are just describing the type to be matched on. The value should be arbitrary, no? [...] > > > +#define OC_STATUS_WARN(ch) BIT(ch) > > > + > > > +#define MAX5970_REG_CHXEN 0x3b > > > +#define CHXEN(ch) (3 << (ch * 2)) > > > + > > > +#define MAX5970_REG_LED_FLASH 0x43 > > > > Do these all need to be shared? > > > > Or can they be isolated into, say, the Regulator driver? > > > Shared reg. Where else are they used? -- Lee Jones [李琼斯]
