On 19/01/2021 09:24, Andy Shevchenko wrote:
>>>>> +static struct i2c_driver int3472_tps68470 = {
>>>>> + .driver = {
>>>>> + .name = "int3472-tps68470",
>>>>> + .acpi_match_table = int3472_device_id,
>>>>> + },
>>>>> + .probe_new = skl_int3472_tps68470_probe,
>>>>> +};
>>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can
>>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it
>>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better...
>> Sorry, I'm a bit confused by this. The i2c device is already
>> present...we just want the driver to bind to them, so what role do those
>> functions have there?
> What I meant is something like
>
> *_i2c.c
> real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI
> involved (and it sounds like it should be mfd/tps one, in which you
> just cut out ACPI IDs and convert to pure I²C one, that what I had
> suggested in the first place)
Ahh; sorry - i misunderstood what you meant there. I understand now I
think, but there is one complication; the ACPI subsystem already creates
a client for that i2c adapter and address; i2c_new_client_device()
includes a check to see whether that adapter / address combination has
an i2c device already. So we would have to have the platform driver
with ACPI ID first find the existing i2c_client and unregister it before
registering the new one...the existing clients have a name matching the
ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an
i2c_device_id of course.
>
> *_proxy.c
> GPIO proxy as library
>
> *.c
> platform driver with ACPI ID, in which ->probe() we actually instantiate
> above via calling i2c_acpi_new_device(), *if needed*, along with GPIO
> proxy
>
> ...
>
>>>>> +struct int3472_gpio_clock {
>>>>> + struct clk *clk;
>>>>> + struct clk_hw clk_hw;
>>>>> + struct clk_lookup *cl;
>>>>> + struct gpio_desc *gpio;
>>>>> +};
>>>>> +static const struct clk_ops skl_int3472_clock_ops = {
>>>>> + .prepare = skl_int3472_clk_prepare,
>>>>> + .unprepare = skl_int3472_clk_unprepare,
>>>>> + .enable = skl_int3472_clk_enable,
>>>>> + .disable = skl_int3472_clk_disable,
>>>>> +};
>>> Wondering if this has some similarities with and actually can utilize clk-gpio
>>> driver.
>>> Yeah, sounds like reinventing clk-gpio.c.
>>>
>>> static const struct clk_ops clk_gpio_gate_ops = {
>>> .enable = clk_gpio_gate_enable,
>>> .disable = clk_gpio_gate_disable,
>>> .is_enabled = clk_gpio_gate_is_enabled,
>>> };
>>>
>>> Or is it mux? It has support there as well.
>>>
>> Hmm, yeah, this looks like it would work actually. So I think I'd need to:
>>
>>
>> 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver
>>
>> 2. Register a platform device to bind to the clk-gpio driver
>>
>> 3. Register a gpio lookup table so that the clk-gpio driver can find the
>> gpio in question using gpiod_get()
>>
>> And that looks like it will work; I'll try it.
> You need to modify clk-gpio.c to export
>
> clk_hw_register_gpio_gate()
> clk_hw_register_gpio_mux()
>
> (perhaps it will require to add *_unregister() counterparts) and call it from
> your code.
>
> See, for example, how clk_hw_unregister_fixed_rate() is being used. Another
> case is to add a helper directly into clk-gpio and call it instead of
> clk_hw_*() one, see how clk_register_fractional_divider() is implemented and
> used.
I'll take a look, thanks
> ...
>
>>>>> + /* Lenovo Miix 510-12ISK - OV5648, Rear */
>>>>> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL},
>>>>> + /* Surface Go 1&2 - OV5693, Front */
>>>>> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL},
>>> I'm wondering if you should use same I2C format macro and create this
>>> dynamically? Or rather find a corresponding ACPI device instance and
>>> copy it's name? ...
>> The supply name needs hard-coding really, but the device name I suppose
>> can come from int3472->sensor_name.
> To be strict in terms you are using "device instance name" in the
> REGULATOR_SUPPLY() second parameter. Because "device name" is generic and
> doesn't point to the actual *instance* of the device in the system.
>
> So, and "device name instance" we may get only by traversing through the (ACPI)
> bus and find corresponding struct device and derive name from it. Same way like
> you have done in previous patch series.
>
> Because there is no guarantee that, e.g., i2c-INT33BE:00 will be present on
> the system and moreover there is no guarantee that if two INT33BE or more
> devices are present you will get :00 always for the one you need!
Mm, good point, hadn't considered two identical sensors on the same
platform. Alright; I'll think about this in more detail, thank you.
>>>>> + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion",
>>>>> + &client->dev);
>>>>> + if (IS_ERR(opregion_dev)) {
>>>>> + ret = PTR_ERR(opregion_dev);
>>>>> + goto err_free_gpio;
>>>>> + }
>>>>> + }
>>>> I wonder if this could be simplified by using devm_mfd_add_devices. You
>>>> could have two arrays of mfd_cell, one for each case.
>>> Yeah, which effectively means that we should have some kind of mfd/tps68470 in
>>> place.
>> Can you expand on what you mean by that a little, please?
> The very first comment in this reply should hopefully shed a light on my idea.
>
It did, thanks
