On Mon, Oct 23, 2023 at 01:09:11PM +0100, Mark Brown wrote: > On Wed, Oct 04, 2023 at 04:17:17PM +0200, Stephan Gerhold wrote: > > > Instead of -EINVAL we could also use a different return code to indicate > > the initial status is unknown. Or maybe there is some other option that > > would be easier? This is working for me but I'm sending it as RFC to get > > more feedback. :) > > The more normal thing here would be -EBUSY I think - -EINVAL kind of > indicates that the operation will never work while in reality it could > possibly work in future. Though for the RPMH it's not really the case > that it ever supports readback, what it does is have it's own reference > counting in the driver. Rather than doing this we should probably have > logic in the core which sees that the driver has a write operation but > no read operation and implements appropriate behaviour. Yep, I agree that it would be nicer to handle this case in the core, rather than duplicating the logic in all the RPM-related drivers. I think it does not change much for this patch, though. Even when implemented in the core we still need to represent this situation somehow for regulator_is_enabled(). Simply returning 0 (disabled) or 1 (enabled) would be wrong. Do you think returning -EBUSY would be appropriate for that? The second challenge I see on a quick look is that both qcom_smd-regulator.c and qcom-rpmh-regulator.c use their reference counter internally in other function (e.g. to decide if a voltage change should be sent, see "vreg->enabled" checks). I think we would also need to add some rdev_is_enabled() function that would expose the core reference counter to the driver? Tracking the enable state in the driver (the way it is right now) is not that much code, so I'm not entirely sure if we might actually end up with more code/complexity when moving this to the core. Thanks, -- Stephan Gerhold <stephan.gerhold@xxxxxxxxxxxxxxx> Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth
