Messages in this thread | | | Date | Mon, 14 Nov 2022 09:11:35 +0000 | From | Lee Jones <> | Subject | Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978 |
| |
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@9elements.com> > > > > > > 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@9elements.com> > > > Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
[...]
> > > +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 [李琼斯]
| |