Re: [PATCH v2 2/2] hwmon: (pmbus/tda38640) Add workaround for bug in SVID mode

[Naresh Solanki]

Hi Guenter,

>
> On 8/1/23 05:34, Naresh Solanki wrote:
>
> [ ... ]
>
> >>> +     if (IS_ENABLED(CONFIG_SENSORS_TDA38640_REGULATOR) || svid) {
> >>
> >> If you hide this behind IS_ENABLED(CONFIG_SENSORS_TDA38640_REGULATOR), reading
> >> svid outside the if() statement has no value.
> > svid mode check is needed only when regulator is enabled for on/off
> > control later.
> > Will align the code such that if svid_mode check is done only when
> > REGULATOR config is enabled
> > & if it is in svid mode then apply the WA.
> >
> >>
> >>> +             /*
> >>> +              * Apply ON_OFF_CONFIG workaround as enabling the regulator using the
> >>> +              * OPERATION register doesn't work in SVID mode.
> >>> +              *
> >>> +              * One should configure PMBUS_ON_OFF_CONFIG here, but
> >>> +              * PB_ON_OFF_CONFIG_POWERUP_CONTROL and PB_ON_OFF_CONFIG_EN_PIN_REQ
> >>> +              * are ignored by the device.
> >>> +              * Only PB_ON_OFF_CONFIG_POLARITY_HIGH has an effect.
> >>
> >> Hmm, maybe I start to understand. This is really weird, since it changes
> >> the polarity of the EN input pin, effectively reverting its value.
> >> In other words, what really happens is that it is not possible to disable
> >> the chip with PMBUS_ON_OFF_CONFIG in SVID mode, and that reverting
> >> the EN pin polarity effectively simulates turning the chip on or off by
> >> software. Maybe software enable is disabled on purpose in VID mode.
> >> Is that really a bug or is it a feature, and is it really a good idea to
> >> override it ?
> > By design, SVID mode only has HW control enabled.
> > This was with the assumption that PGOOD will be used for controlling
> > Enable of another rail in Hardware.
> >
> > Since my use case needs the complete PMBUS based control,
> > EN pin polarity flipping can be used for controlling output.
> >
>
> So, effectively, this is not really a bug. It is working around chip functionality.
>
> That means we can not just enable this unconditionally in SVID mode after all.
> Sorry, but it has to be configurable after all, with appropriate explanation.
By 'configurable' you mean add a dt-property like 'en-svid-control' to have this
enabled ?

Regards,
Naresh
>
> Guenter
>
> >>
> >> AN_2203_PL12_2204_184108 might really help here.
> >>
> >> Guenter
> >>
> >>> +              */
> >>> +             data->info.read_byte_data = tda38640_read_byte_data;
> >>> +             data->info.write_byte_data = tda38640_write_byte_data;
> >>> +     }
> >>> +     return pmbus_do_probe(client, &data->info);
> >>>    }
> >>>
> >>>    static const struct i2c_device_id tda38640_id[] = {
> >>
>