On Wed, 14 Feb 2024 at 15:58, Jishnu Prakash <quic_jprakash@xxxxxxxxxxx> wrote:
>
> Hi Dmitry,
>
> On 12/31/2023 11:16 PM, Dmitry Baryshkov wrote:
> > On Sun, 31 Dec 2023 at 19:13, Jishnu Prakash <quic_jprakash@xxxxxxxxxxx> wrote:
> >> The ADC architecture on PMIC5 Gen3 is similar to that on PMIC5 Gen2,
> >> with all SW communication to ADC going through PMK8550 which
> >> communicates with other PMICs through PBS.
>
> >> +static int adc_tm_register_tzd(struct adc5_chip *adc)
> >> +{
> >> + unsigned int i, channel;
> >> + struct thermal_zone_device *tzd;
> >> +
> >> + for (i = 0; i < adc->nchannels; i++) {
> >> + channel = V_CHAN(adc->chan_props[i]);
> >> +
> >> + if (!adc->chan_props[i].adc_tm)
> >> + continue;
> >> + tzd = devm_thermal_of_zone_register(adc->dev, channel,
> >> + &adc->chan_props[i], &adc_tm_ops);
> > It is _very_ useful to register a hwmon too by calling
> > devm_thermal_add_hwmon_sysfs(). However this becomes tricky, as this
> > function is not defined in one of the global headers.
> >
> > This actually points out an issue. You have the ADC driver fused
> > together with the thermal driver. Can I suggest using the aux device
> > to split the thermal functionality to the separate driver?
> >
> > This way it would be possible to use the ADC without any thermal
> > monitoring in place.
>
>
> There are a couple of issues which may make it harder to split the
> thermal functionality from this driver into an auxiliary driver as you
> mentioned.
>
> For one, we use the same set of registers (offsets 0x4f-0x55) for both
> VADC function(requesting an immediate channel reading) and ADC_TM
> function (setting upper/lower thermal thresholds on a channel). To avoid
> any race conditions, we would need to share a mutex between the
> top-level ADC driver and the auxiliary ADC_TM thermal driver to avoid
> concurrently accessing these or any other shared registers.
Just export an API performing this access. No need to export data (aka mutex).
>
> In addition, the device has only one interrupt with one interrupt
> handler, and it gets triggered for both VADC and ADC_TM events (end of
> conversion and threshold violation, respectively). The handler checks
> for both types of event and handles it as required.
You can extend auxiliary drivers with the custom callbacks, see
drivers/base/auxiliary.c .
I think you can call a callback from ADC_TM driver from your ADC driver.
>
> For the shared interrupt, we may be able to keep the interrupt handler
> in the top-level driver and just notify the auxiliary TM driver if a
> threshold violation is detected. For the shared mutex, I think the
> auxiliary driver may be able to access the parent driver's mutex, but
> I'll need to check more for the implementation in both of these cases.
>
> Please let me know if you see any problems with this kind of
> implementation or if you have any additional comments.
>
> Thanks,
>
> Jishnu
>
> >> +
> >> + if (IS_ERR(tzd)) {
> >> + if (PTR_ERR(tzd) == -ENODEV) {
> >> + dev_warn(adc->dev, "thermal sensor on channel %d is not used\n",
> >> + channel);
> >> + continue;
> >> + }
> >> +
> >>
> >
--
With best wishes
Dmitry
