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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <89b2d1a7-fcd5-408c-b632-7052013036f1@alliedtelesis.co.nz>
Date: Thu, 19 Sep 2024 17:06:54 +1200
From: Chris Packham <chris.packham@...iedtelesis.co.nz>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 tsbogend@...ha.franken.de, linux-i2c@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-mips@...r.kernel.org
Subject: Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support


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
>>>