On Mon, 29 Sep 2014, Bjorn Andersson wrote:
> Driver for the Resource Power Manager (RPM) found in Qualcomm 8974 based
> devices.
> The driver exposes resources that child drivers can operate on; to
> implementing regulator, clock and bus frequency drivers.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Note that the qcom_rpm_smd_write is equivalent of qcom_rpm_write and there is a
> possibility of re-using at least the clock implementation on top of this. This
> would however require some logic for calling the right implementation so I have
> not done it at this time to keep things as clean as possible.
>
> An idea for improvement is that in qcom_rpm_smd_write we put the ack_status and
> completion on the stack and register this with idr using the message id, upon
> receiving the interrupt we would find the right client and complete this.
> Allowing for multiple requests to be in flight at any given time.
>
> I did not implement this because I haven't done any measurements on what kind
> of improvements this could give and it would be a clean iteration ontop of
> this.
>
> drivers/mfd/Kconfig | 14 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/qcom-smd-rpm.c | 299 ++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/qcom-smd-rpm.h | 9 ++
> 4 files changed, 323 insertions(+)
> create mode 100644 drivers/mfd/qcom-smd-rpm.c
> create mode 100644 include/linux/mfd/qcom-smd-rpm.h
> +#define RPM_ERR_INVALID_RESOURCE "resource does not exist"
> +
> +static bool qcom_rpm_msg_is_invalid_resource(struct qcom_rpm_message *msg)
> +{
> + size_t msg_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1;
> +
> + if (msg->length != msg_len)
> + return false;
> +
> + if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE, msg_len))
> + return false;
> +
> + return true;
> +}
You can save yourself a hell of a lot of code by just doing:
if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE,
min(msg_len, sizeof(RPM_ERR_INVALID_RESOURCE))))
... in qcom_smd_rpm_callback().
[...]
> +static int qcom_smd_rpm_probe(struct qcom_smd_device *sdev)
> +{
> + const struct of_device_id *match;
> + struct qcom_smd_rpm *rpm;
> +
> + rpm = devm_kzalloc(&sdev->dev, sizeof(*rpm), GFP_KERNEL);
> + if (!rpm)
> + return -ENOMEM;
> +
> + rpm->dev = &sdev->dev;
> + mutex_init(&rpm->lock);
> + init_completion(&rpm->ack);
> +
> + match = of_match_device(qcom_smd_rpm_of_match, &sdev->dev);
You need to check the return value here.
> + rpm->data = match->data;
> + rpm->rpm_channel = sdev->channel;
> +
> + dev_set_drvdata(&sdev->dev, rpm);
> +
> + dev_info(&sdev->dev, "Qualcomm SMD RPM driver probed\n");
Please remove this line.
> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> +}
> +
> +static void qcom_smd_rpm_remove(struct qcom_smd_device *sdev)
> +{
> + dev_set_drvdata(&sdev->dev, NULL);
If you use the proper platform device interface you don't have to do
this.
> + of_platform_depopulate(&sdev->dev);
> +}
> +
> +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> + .probe = qcom_smd_rpm_probe,
> + .remove = qcom_smd_rpm_remove,
> + .callback = qcom_smd_rpm_callback,
> + .driver = {
> + .name = "qcom_smd_rpm",
> + .owner = THIS_MODULE,
> + .of_match_table = qcom_smd_rpm_of_match,
> + },
> +};
> +
> +module_qcom_smd_driver(qcom_smd_rpm_driver);
I don't like this. What's wrong with the existing platform driver
code?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed 08 Oct 01:40 PDT 2014, Lee Jones wrote:
Hi Lee,
Thanks for your review.
> On Mon, 29 Sep 2014, Bjorn Andersson wrote:
>
[..]
> > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist"
> > +
> > +static bool qcom_rpm_msg_is_invalid_resource(struct qcom_rpm_message *msg)
> > +{
> > + size_t msg_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1;
> > +
> > + if (msg->length != msg_len)
> > + return false;
> > +
> > + if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE, msg_len))
> > + return false;
> > +
> > + return true;
> > +}
>
> You can save yourself a hell of a lot of code by just doing:
>
> if (memcmp(msg->message, RPM_ERR_INVALID_RESOURCE,
> min(msg_len, sizeof(RPM_ERR_INVALID_RESOURCE))))
>
> ... in qcom_smd_rpm_callback().
>
I can agree with you that there will be less code, but not "a hell of a lot". I
made the choise because I had something like the snippet you suggest and I
wanted to make it cleaner - I'll fold it back in.
> [...]
>
> > +static int qcom_smd_rpm_probe(struct qcom_smd_device *sdev)
> > +{
> > + const struct of_device_id *match;
> > + struct qcom_smd_rpm *rpm;
> > +
> > + rpm = devm_kzalloc(&sdev->dev, sizeof(*rpm), GFP_KERNEL);
> > + if (!rpm)
> > + return -ENOMEM;
> > +
> > + rpm->dev = &sdev->dev;
> > + mutex_init(&rpm->lock);
> > + init_completion(&rpm->ack);
> > +
> > + match = of_match_device(qcom_smd_rpm_of_match, &sdev->dev);
>
> You need to check the return value here.
>
As long as we only support device tree probing of this match will never return
NULL. I can add a check to fail on non-dt boards if someone chooses to ever
implement one of those.
> > + rpm->data = match->data;
> > + rpm->rpm_channel = sdev->channel;
> > +
> > + dev_set_drvdata(&sdev->dev, rpm);
> > +
> > + dev_info(&sdev->dev, "Qualcomm SMD RPM driver probed\n");
>
> Please remove this line.
>
Ok
> > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> > +}
> > +
> > +static void qcom_smd_rpm_remove(struct qcom_smd_device *sdev)
> > +{
> > + dev_set_drvdata(&sdev->dev, NULL);
>
> If you use the proper platform device interface you don't have to do
> this.
>
Ok
> > + of_platform_depopulate(&sdev->dev);
> > +}
> > +
> > +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> > + .probe = qcom_smd_rpm_probe,
> > + .remove = qcom_smd_rpm_remove,
> > + .callback = qcom_smd_rpm_callback,
> > + .driver = {
> > + .name = "qcom_smd_rpm",
> > + .owner = THIS_MODULE,
> > + .of_match_table = qcom_smd_rpm_of_match,
> > + },
> > +};
> > +
> > +module_qcom_smd_driver(qcom_smd_rpm_driver);
>
> I don't like this. What's wrong with the existing platform driver
> code?
>
I started off with having smd child devices as platform drivers and had some
accessor functions to find the open handles that triggered the probe() and
register the callback with those. But this didn't feel very sane, so I did
implemented a custom driver struct and probe prototype to simplify writing
drivers.
May I ask why you dislike this? This is how it's done in so many other places
in the kernel...
Regards,
Bjorn
On Fri, 17 Oct 2014, Bjorn Andersson wrote:
> On Wed 08 Oct 01:40 PDT 2014, Lee Jones wrote:
[...]
> > > +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> > > + .probe = qcom_smd_rpm_probe,
> > > + .remove = qcom_smd_rpm_remove,
> > > + .callback = qcom_smd_rpm_callback,
> > > + .driver = {
> > > + .name = "qcom_smd_rpm",
> > > + .owner = THIS_MODULE,
> > > + .of_match_table = qcom_smd_rpm_of_match,
> > > + },
> > > +};
> > > +
> > > +module_qcom_smd_driver(qcom_smd_rpm_driver);
> >
> > I don't like this. What's wrong with the existing platform driver
> > code?
> >
>
> I started off with having smd child devices as platform drivers and had some
> accessor functions to find the open handles that triggered the probe() and
> register the callback with those. But this didn't feel very sane, so I did
> implemented a custom driver struct and probe prototype to simplify writing
> drivers.
>
> May I ask why you dislike this? This is how it's done in so many other places
> in the kernel...
I don't believe that's the case. All owners of their own
module_*_driver() registration calls are busses (see below), whereas
'qcom_smd' is just a driver. Things would soon get out of control if
we allowed every driver in the kernel to supply their own driver
registration information variants.
$ git grep "^module_.*_driver(" | \
cut -d: -f2 | cut -d'(' -f1 | sort | uniq
module_acpi_driver
module_amba_driver
module_comedi_driver
module_comedi_pci_driver
module_comedi_pcmcia_driver
module_comedi_usb_driver
module_gameport_driver
module_hid_driver
module_i2c_driver
module_mcb_driver
module_mipi_dsi_driver
module_pci_driver
module_pcmcia_driver
module_platform_driver
module_serio_driver
module_spi_driver
module_spmi_driver
module_usb_composite_driver
module_usb_driver
module_usb_serial_driver
module_virtio_driver
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon 20 Oct 00:22 PDT 2014, Lee Jones wrote:
> On Fri, 17 Oct 2014, Bjorn Andersson wrote:
> > On Wed 08 Oct 01:40 PDT 2014, Lee Jones wrote:
>
> [...]
>
> > > > +static struct qcom_smd_driver qcom_smd_rpm_driver = {
> > > > + .probe = qcom_smd_rpm_probe,
> > > > + .remove = qcom_smd_rpm_remove,
> > > > + .callback = qcom_smd_rpm_callback,
> > > > + .driver = {
> > > > + .name = "qcom_smd_rpm",
> > > > + .owner = THIS_MODULE,
> > > > + .of_match_table = qcom_smd_rpm_of_match,
> > > > + },
> > > > +};
> > > > +
> > > > +module_qcom_smd_driver(qcom_smd_rpm_driver);
> > >
> > > I don't like this. What's wrong with the existing platform driver
> > > code?
> > >
> >
> > I started off with having smd child devices as platform drivers and had some
> > accessor functions to find the open handles that triggered the probe() and
> > register the callback with those. But this didn't feel very sane, so I did
> > implemented a custom driver struct and probe prototype to simplify writing
> > drivers.
> >
> > May I ask why you dislike this? This is how it's done in so many other places
> > in the kernel...
>
> I don't believe that's the case. All owners of their own
> module_*_driver() registration calls are busses (see below), whereas
> 'qcom_smd' is just a driver. Things would soon get out of control if
> we allowed every driver in the kernel to supply their own driver
> registration information variants.
>
I modelled this after rpmsg, with the intention of having qcom_smd provide a
"smd bus" and all client drivers sitting on that bus being probed and removed
as the remote services appear and disappear.
I'm afraid I don't understand what part I missed that makes my smd driver "just
a driver". I will reread the documentation and try to figure out what I might
have missed.
Regards,
Bjorn