Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230727091358.3274620-1-Naresh.Solanki@9elements.com>
 <20230727091358.3274620-2-Naresh.Solanki@9elements.com> <41a8ae4b-0f96-9f26-f25b-b1554b2695d6@roeck-us.net>
 <CABqG17jKoJ8FJdA-vpX8uda9yi_ir3f2FxFAiE7GTaVM7Mb2aA@mail.gmail.com> <b4097ed6-95d6-b11b-9c9e-edd6e8c51d00@roeck-us.net>
In-Reply-To: <b4097ed6-95d6-b11b-9c9e-edd6e8c51d00@roeck-us.net>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Wed, 2 Aug 2023 00:16:04 +0530
Message-ID: <CABqG17iAsx2nysBSX10PTCK=fTpvaz2456a-s6CBwQjuJduWQw@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] hwmon: (pmbus/tda38640) Add workaround for bug in
 SVID mode
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Jean Delvare <jdelvare@suse.com>,
        krzysztof.kozlowski+dt@linaro.org, linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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[] = {
> >>
>