Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Laurent Pinchart]

Hi Dan,

On Wed, Aug 25, 2021 at 03:12:25PM +0100, Daniel Scally wrote:
> On 25/08/2021 14:59, Laurent Pinchart wrote:
> > Hello,
> >
> > CC'ing Sakari.
> >
> > On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote:
> >> On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote:
> >>> On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
> >>>> No, what was proposed for regulator was to duplicate all the the DT
> >>>> binding code in the regulator framework so it parses fwnodes then have
> >>>> an API for encoding fwnodes from C data structures at runtime.  The bit
> >>>> where the data gets joined up with the devices isn't the problem, it's
> >>>> the duplication and fragility introduced by encoding everything into
> >>>> an intermediate representation that has no purpose and passing that
> >>>> around which is the problem.
> >>> The whole exercise with swnode is to minimize the driver intrusion and
> >>> evolving a unified way for (some) of the device properties. V4L2 won't
> >> The practical implementation for regulators was to duplicate a
> >> substantial amount of code in the core in order to give us a less type
> >> safe and more indirect way of passing data from onen C file in the
> >> kernel to another.  This proposal is a lot better in that it uses the
> >> existing init_data and avoids the huge amounts of duplication, it's just
> >> not clear from the changelog why it's doing this in a regulator specific
> >> manner.
> >>
> >> *Please* stop trying to force swnodes in everywhere, take on board the
> >> feedback about why the swnode implementation is completely inappropriate
> >> for regulators.  I don't understand why you continue to push this so
> >> hard.  swnodes and fwnodes are a solution to a specific problem, they're
> >> not the answer to every problem out there and having to rehash this
> >> continually is getting in the way of actually discussing practical
> >> workarounds for these poorly implemented ACPI platforms.
> >>
> >>> like what you are suggesting exactly because they don't like the idea
> >>> of spreading the board code over the drivers. In some cases it might
> >>> even be not so straightforward and easy.
> >>> Laurent, do I understand correctly the v4l2 expectations?
> >> There will be some cases where swnodes make sense, for example where the
> >> data is going to be read through the fwnode API since the binding is
> >> firmware neutral which I think is the v4l case.  On the other hand
> >> having a direct C representation is a very common way of implementing
> >> DMI quirk tables, and we have things like the regulator API where
> >> there's off the shelf platform data support and we actively don't want
> >> to support fwnode.
> > From a camera sensor point of view, we want to avoid code duplication.
> > Having to look for regulators using OF lookups *and* platform data in
> > every single sensor driver is not a good solution. This means that, from
> > a camera sensor driver point of view, we want to call regulator_get()
> > (or the devm_ version) with a name, without caring about who establishes
> > the mapping and how the lookup is performed. I don't care much
> > personally if this would be implemented through swnode or a different
> > mechanism, as long as the implementation can be centralized.
> 
> I think rather than sensor drivers, the idea would be just to have the
> tps68470-regulator driver check platform data for the init_data instead,
> like the tps65023-regulator driver [1] does. I'm sure that'll work, but
> it's not particularly centralized from the regulator driver's point of
> view.

That would obviously be less of an issue from a V4L2 point of view :-)
Given that the situation we're facing doesn't affect (to our knowledge)
a very large number of regulators, it may not be too bad in practice. If
I were to maintain the regulator subsystem I'd probably want a
centralized implementation there, but that's certainly a personal
preference, at least partly.

On a side note, this RFC looks quite similar to what the GPIO subsystem
does, which I personally consider nice as differences between regulator
and GPIO in these areas are confusing for users.

> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/regulator/tps65023-regulator.c#L268

-- 
Regards,

Laurent Pinchart