Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support

Chris Packham
2024-09-19T05:07:01Z

On 19/09/24 09:44, Chris Packham wrote:
> Hi Andi, Rob,
>
> On 19/09/24 08:36, Andi Shyti wrote:
>> Hi Chris,
>>
>> ...
>>
>>> -module_platform_driver(rtl9300_i2c_driver);
>>> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32
>>> chan)
>>> +{
>>> +    struct i2c_adapter *adap = muxc->parent;
>>> +    struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
>>> +    int ret;
>>> +
>>> +    ret = rtl9300_i2c_config_io(i2c, chan);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return 0;
>> return "rtl9300_i2c_config_io()"?
>
> Ack.
>
>>> +}
>> ...
>>
>>> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux,
>>> struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct fwnode_handle *fwnode = dev_fwnode(dev);
>>> +    struct device_node *np = dev->of_node;
>>> +    struct device_node *adap_np;
>>> +    struct i2c_adapter *adap = NULL;
>>> +    struct fwnode_handle *child;
>>> +    unsigned int *chans;
>>> +    int i = 0;
>>> +
>>> +    if (!is_of_node(fwnode))
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if (!np)
>>> +        return -ENODEV;
>>> +
>>> +    adap_np = of_parse_phandle(np, "i2c-parent", 0);
>>> +    if (!adap_np) {
>>> +        dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
>>> +        return -ENODEV;
>> return dev_err_probe(...)?
>
> Ack.
>
>>> +    }
>>> +    adap = of_find_i2c_adapter_by_node(adap_np);
>>> +    of_node_put(adap_np);
>> ...
>>
>>> +static int __init rtl9300_i2c_init(void)
>>> +{
>>> +    return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
>>> +}
>>> +module_init(rtl9300_i2c_init);
>>> +
>>> +static void __exit rtl9300_i2c_exit(void)
>>> +{
>>> +    platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
>>> +}
>>> +module_exit(rtl9300_i2c_exit);
>> You could use module_platform_driver()
>
> Can I though? I want to support both the simple I2C controller and the
> MUX mode with the same driver. Which is why I've ended up with two
> drivers to register.
>
> On the binding patch, Rob made the suggestion that I just make the
> i2c-mux part of the parent. I did consider that but quickly got tied
> in knots because I couldn't figure out how to have a device that is
> both an adapter and a mux. The main problem was that any child nodes
> of an i2c adapter in the device tree are presumed to be I2C devices
> and get probed automatically by of_i2c_register_devices(). Equally I
> can't register a mux without having an adapter that the mux operates
> over.

OK I think I've got something working that has a dt binding like

i2c@36c {
compatible = "realtek,rtl9300-i2c";
reg = <0x36c 0x14>;
status = "okay";
#address-cells = <0x01>;
#size-cells = <0x00>;

i2c@0 {
reg = <0x00>;
#address-cells = <1>;
#size-cells = <1>;
gpio@20 {
...
};
};

i2c@2 {
reg = <0x02>;
};
};

In the probe() I can iterate over the child nodes and create an adapter
for each. The code is a bit fiddly but I think it's a net win if I can
do away with the rtl9300-i2c-mux part. It also happily means that I
don't have an extra I2C bus that is the same as the first mux channel.

I'll try an tidy things up and get another iteration out before my weekend.

>
>>
>> Thanks,
>> Andi
>>
>>>     MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>>>   MODULE_LICENSE("GPL");
>>> --
>>> 2.46.1
>>>